mbox series

[v2,0/8] arm64: mvebu: Support for Marvell 98DX2530 (and variants)

Message ID 20220314213143.2404162-1-chris.packham@alliedtelesis.co.nz
Headers show
Series arm64: mvebu: Support for Marvell 98DX2530 (and variants) | expand

Message

Chris Packham March 14, 2022, 9:31 p.m. UTC 
This series adds support for the Marvell 98DX2530 SoC which is the Control and
Management CPU integrated into the AlleyCat5/AlleyCat5X series of Marvell
switches.

The CPU core is an ARM Cortex-A55 with neon, simd and crypto extensions.

This is fairly similar to the Armada-3700 SoC so most of the required
peripherals are already supported. This series adds a devicetree and pinctrl
driver for the SoC and the RD-AC5X-32G16HVG6HLG reference board.

Chris Packham (8):
  dt-bindings: pinctrl: mvebu: Document bindings for AC5
  dt-bindings: net: mvneta: Add marvell,armada-ac5-neta
  dt-bindings: mmc: xenon: add AC5 compatible string
  pinctrl: mvebu: pinctrl driver for 98DX2530 SoC
  net: mvneta: Add support for 98DX2530 Ethernet port
  mmc: xenon: add AC5 compatible string
  arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board
  arm64: marvell: enable the 98DX2530 pinctrl driver

 .../bindings/mmc/marvell,xenon-sdhci.txt      |  52 +++
 .../bindings/net/marvell-armada-370-neta.txt  |   1 +
 .../bindings/pinctrl/marvell,ac5-pinctrl.yaml |  70 ++++
 arch/arm64/Kconfig.platforms                  |   2 +
 arch/arm64/boot/dts/marvell/Makefile          |   1 +
 .../boot/dts/marvell/armada-98dx2530.dtsi     | 343 ++++++++++++++++++
 arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  62 ++++
 drivers/mmc/host/sdhci-xenon.c                |   1 +
 drivers/mmc/host/sdhci-xenon.h                |   3 +-
 drivers/net/ethernet/marvell/mvneta.c         |  13 +
 drivers/pinctrl/mvebu/Kconfig                 |   4 +
 drivers/pinctrl/mvebu/Makefile                |   1 +
 drivers/pinctrl/mvebu/pinctrl-ac5.c           | 226 ++++++++++++
 13 files changed, 778 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
 create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
 create mode 100644 arch/arm64/boot/dts/marvell/rd-ac5x.dts
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-ac5.c

Comments

Andrew Lunn March 15, 2022, 12:07 a.m. UTC | #1 
> +    properties:
> +      marvell,function:
> +        $ref: "/schemas/types.yaml#/definitions/string"
> +        description:
> +          Indicates the function to select.
> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
> +
> +      marvell,pins:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        description:
> +          Array of MPP pins to be used for the given function.
> +        minItems: 1

Now that i've looked at the .txt files, i'm wondering if this should
be split into a marvell,mvebu-pinctrl.yaml and
marvell,ac5-pinctrl.yaml?

I don't know yaml well enough to know if this is possible. All the
mvebu pinctrl drivers have marvell,function and marvell,pins. The enum
will differ, this ethernet switch SoC does not have sata, audio etc,
where as the general purpose Socs do. Can that be represented in yaml?

      Andrew
Andrew Lunn March 15, 2022, 12:14 a.m. UTC | #2 
On Tue, Mar 15, 2022 at 10:31:41AM +1300, Chris Packham wrote:
> Add marvell,ac5-sdhci to the list of compatible strings for the Xenon
> SDHCI controller. Currently this is functionally no different to the
> ap806 but having the compatible string will allow handling any
> differences that arise from the controller being integrated in the
> 98DX2530 switch chips.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Chris Packham March 15, 2022, 12:22 a.m. UTC | #3 
On 15/03/22 13:07, Andrew Lunn wrote:
>> +    properties:
>> +      marvell,function:
>> +        $ref: "/schemas/types.yaml#/definitions/string"
>> +        description:
>> +          Indicates the function to select.
>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>> +
>> +      marvell,pins:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        description:
>> +          Array of MPP pins to be used for the given function.
>> +        minItems: 1
> Now that i've looked at the .txt files, i'm wondering if this should
> be split into a marvell,mvebu-pinctrl.yaml and
> marvell,ac5-pinctrl.yaml?
>
> I don't know yaml well enough to know if this is possible. All the
> mvebu pinctrl drivers have marvell,function and marvell,pins. The enum
> will differ, this ethernet switch SoC does not have sata, audio etc,
> where as the general purpose Socs do. Can that be represented in yaml?

I think it can. I vaguely remember seeing conditional clauses based on 
compatible strings in other yaml bindings.

I started a new binding document because I expected adding significant 
additions to the existing .txt files would be rejected. If I get some 
cycles I could look at converting the existing docs from txt to yaml.

I'm not sure that there will be much in the way of a common 
mvebu-pinctrl.yaml as you'd end up repeating most of the common stuff to 
make things conditional anyway.

>
>        Andrew
Andrew Lunn March 15, 2022, 12:27 a.m. UTC | #4 
> I think it can. I vaguely remember seeing conditional clauses based on 
> compatible strings in other yaml bindings.
> 
> I started a new binding document because I expected adding significant 
> additions to the existing .txt files would be rejected. If I get some 
> cycles I could look at converting the existing docs from txt to yaml.
> 
> I'm not sure that there will be much in the way of a common 
> mvebu-pinctrl.yaml as you'd end up repeating most of the common stuff to 
> make things conditional anyway.

We should wait for Rob to comment. But is suspect you are right, there
will not be much savings.

     Andrew
Krzysztof Kozlowski March 15, 2022, 10:49 a.m. UTC | #5 
On 14/03/2022 22:31, Chris Packham wrote:
> This pinctrl driver supports the 98DX25xx and 98DX35xx family of chips
> from Marvell. It is based on the Marvell SDK with additions for various
> (non-gpio) pin configurations based on the datasheet.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Make pinctrl a child of a syscon node like the armada-7k-pinctrl
> 
>  drivers/pinctrl/mvebu/Kconfig       |   4 +
>  drivers/pinctrl/mvebu/Makefile      |   1 +
>  drivers/pinctrl/mvebu/pinctrl-ac5.c | 226 ++++++++++++++++++++++++++++
>  3 files changed, 231 insertions(+)
>  create mode 100644 drivers/pinctrl/mvebu/pinctrl-ac5.c
> 
> diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig
> index 0d12894d3ee1..aa5883f09d7b 100644
> --- a/drivers/pinctrl/mvebu/Kconfig
> +++ b/drivers/pinctrl/mvebu/Kconfig
> @@ -45,6 +45,10 @@ config PINCTRL_ORION
>  	bool
>  	select PINCTRL_MVEBU
>  
> +config PINCTRL_AC5
> +	bool
> +	select PINCTRL_MVEBU
> +
>  config PINCTRL_ARMADA_37XX
>  	bool
>  	select GENERIC_PINCONF
> diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile
> index cd082dca4482..23458ab17c53 100644
> --- a/drivers/pinctrl/mvebu/Makefile
> +++ b/drivers/pinctrl/mvebu/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_PINCTRL_ARMADA_CP110) += pinctrl-armada-cp110.o
>  obj-$(CONFIG_PINCTRL_ARMADA_XP)  += pinctrl-armada-xp.o
>  obj-$(CONFIG_PINCTRL_ARMADA_37XX)  += pinctrl-armada-37xx.o
>  obj-$(CONFIG_PINCTRL_ORION)  += pinctrl-orion.o
> +obj-$(CONFIG_PINCTRL_AC5) += pinctrl-ac5.o
> diff --git a/drivers/pinctrl/mvebu/pinctrl-ac5.c b/drivers/pinctrl/mvebu/pinctrl-ac5.c
> new file mode 100644
> index 000000000000..8bc0bbff7c1b
> --- /dev/null
> +++ b/drivers/pinctrl/mvebu/pinctrl-ac5.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell ac5 pinctrl driver based on mvebu pinctrl core
> + *
> + * Copyright (C) 2021 Marvell
> + *
> + * Noam Liron <lnoam@marvell.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +
> +#include "pinctrl-mvebu.h"
> +
> +static struct mvebu_mpp_mode ac5_mpp_modes[] = {
> +	MPP_MODE(0,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d0"),
> +		 MPP_FUNCTION(2, "nand",  "io4")),
> +	MPP_MODE(1,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d1"),
> +		 MPP_FUNCTION(2, "nand",  "io3")),
> +	MPP_MODE(2,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d2"),
> +		 MPP_FUNCTION(2, "nand",  "io2")),
> +	MPP_MODE(3,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d3"),
> +		 MPP_FUNCTION(2, "nand",  "io7")),
> +	MPP_MODE(4,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d4"),
> +		 MPP_FUNCTION(2, "nand",  "io6"),
> +		 MPP_FUNCTION(3, "uart3", "txd"),
> +		 MPP_FUNCTION(4, "uart2", "txd")),
> +	MPP_MODE(5,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d5"),
> +		 MPP_FUNCTION(2, "nand",  "io5"),
> +		 MPP_FUNCTION(3, "uart3", "rxd"),
> +		 MPP_FUNCTION(4, "uart2", "rxd")),
> +	MPP_MODE(6,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d6"),
> +		 MPP_FUNCTION(2, "nand",  "io0"),
> +		 MPP_FUNCTION(3, "i2c1",  "sck")),
> +	MPP_MODE(7,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d7"),
> +		 MPP_FUNCTION(2, "nand",  "io1"),
> +		 MPP_FUNCTION(3, "i2c1",  "sda")),
> +	MPP_MODE(8,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "clk"),
> +		 MPP_FUNCTION(2, "nand",  "wen")),
> +	MPP_MODE(9,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "cmd"),
> +		 MPP_FUNCTION(2, "nand",  "ale")),
> +	MPP_MODE(10,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "ds"),
> +		 MPP_FUNCTION(2, "nand",  "cle")),
> +	MPP_MODE(11,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "rst"),
> +		 MPP_FUNCTION(2, "nand",  "cen")),
> +	MPP_MODE(12,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "clk")),
> +	MPP_MODE(13,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "csn")),
> +	MPP_MODE(14,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "mosi")),
> +	MPP_MODE(15,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "miso")),
> +	MPP_MODE(16,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "wpn"),
> +		 MPP_FUNCTION(2, "nand",  "ren"),
> +		 MPP_FUNCTION(3, "uart1", "txd")),
> +	MPP_MODE(17,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "hold"),
> +		 MPP_FUNCTION(2, "nand",  "rb"),
> +		 MPP_FUNCTION(3, "uart1", "rxd")),
> +	MPP_MODE(18,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "rxd")),
> +	MPP_MODE(19,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "txd")),
> +	MPP_MODE(20,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "i2c1",  "sck"),
> +		 MPP_FUNCTION(3, "spi1",  "clk"),
> +		 MPP_FUNCTION(4, "uart3", "txd")),
> +	MPP_MODE(21,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "i2c1",  "sda"),
> +		 MPP_FUNCTION(3, "spi1",  "csn"),
> +		 MPP_FUNCTION(4, "uart3", "rxd")),
> +	MPP_MODE(22,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "spi1",  "mosi")),
> +	MPP_MODE(23,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "spi1",  "miso")),
> +	MPP_MODE(24,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "txd")),
> +	MPP_MODE(25,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "rxd")),
> +	MPP_MODE(26,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "i2c0",  "sck"),
> +		 MPP_FUNCTION(3, "uart3", "txd")),
> +	MPP_MODE(27,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "i2c0",  "sda"),
> +		 MPP_FUNCTION(3, "uart3", "rxd")),
> +	MPP_MODE(28,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "uart3", "txd")),
> +	MPP_MODE(29,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "uart3", "rxd")),
> +	MPP_MODE(30,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(31,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(32,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "uart0", "txd")),
> +	MPP_MODE(33,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "uart0", "rxd")),
> +	MPP_MODE(34,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart3", "rxd")),
> +	MPP_MODE(35,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart3", "txd")),
> +	MPP_MODE(36,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(37,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(38,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(39,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(40,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(41,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(4, "uart2", "txd"),
> +		 MPP_FUNCTION(5, "i2c1",  "sck")),
> +	MPP_MODE(42,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(4, "uart2", "rxd"),
> +		 MPP_FUNCTION(5, "i2c1",  "sda")),
> +	MPP_MODE(43,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(44,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(45,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +};
> +
> +static struct mvebu_pinctrl_soc_info ac5_pinctrl_info;

You should not have static/file-scope variables, especially that it is
not actually used in that way.

> +
> +static const struct of_device_id ac5_pinctrl_of_match[] = {
> +	{
> +		.compatible = "marvell,ac5-pinctrl",
> +	},
> +	{ },
> +};
> +
> +static const struct mvebu_mpp_ctrl ac5_mpp_controls[] = {
> +	MPP_FUNC_CTRL(0, 45, NULL, mvebu_regmap_mpp_ctrl), };
> +
> +static struct pinctrl_gpio_range ac5_mpp_gpio_ranges[] = {
> +	MPP_GPIO_RANGE(0,   0,  0, 46), };
> +
> +static int ac5_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct mvebu_pinctrl_soc_info *soc = &ac5_pinctrl_info;
> +	const struct of_device_id *match =
> +		of_match_device(ac5_pinctrl_of_match, &pdev->dev);

Why is this needed? Unusual, dead-code.

> +
> +	if (!match || !pdev->dev.parent)
> +		return -ENODEV;
> +
> +	soc->variant = 0; /* no variants for ac5 */
> +	soc->controls = ac5_mpp_controls;
> +	soc->ncontrols = ARRAY_SIZE(ac5_mpp_controls);
> +	soc->gpioranges = ac5_mpp_gpio_ranges;
> +	soc->ngpioranges = ARRAY_SIZE(ac5_mpp_gpio_ranges);
> +	soc->modes = ac5_mpp_modes;
> +	soc->nmodes = ac5_mpp_controls[0].npins;
> +
> +	pdev->dev.platform_data = soc;
> +
> +	return mvebu_pinctrl_simple_regmap_probe(pdev, pdev->dev.parent, 0);
> +}
> +
> +static struct platform_driver ac5_pinctrl_driver = {
> +	.driver = {
> +		.name = "ac5-pinctrl",
> +		.of_match_table = of_match_ptr(ac5_pinctrl_of_match),

of_match_ptr() does not look correct for OF-only platform. This should
complain in W=1 compile tests on !OF config.

Best regards,
Krzysztof
Andrew Lunn March 15, 2022, 2:33 p.m. UTC | #6 
> > +static struct platform_driver ac5_pinctrl_driver = {
> > +	.driver = {
> > +		.name = "ac5-pinctrl",
> > +		.of_match_table = of_match_ptr(ac5_pinctrl_of_match),
> 
> of_match_ptr() does not look correct for OF-only platform. This should
> complain in W=1 compile tests on !OF config.

The Marvell family of SoC which this embedded SoC borrows HW blocks
from can boot using ACPI. I doubt anybody would boot this particularly
SoC using ACPI, but the drivers Chris copied probably do build !OF for
when ACPI is in us.

     Andrew
Rob Herring (Arm) March 23, 2022, 6:34 p.m. UTC | #7 
On Tue, Mar 15, 2022 at 01:27:48AM +0100, Andrew Lunn wrote:
> > I think it can. I vaguely remember seeing conditional clauses based on 
> > compatible strings in other yaml bindings.
> > 
> > I started a new binding document because I expected adding significant 
> > additions to the existing .txt files would be rejected. If I get some 
> > cycles I could look at converting the existing docs from txt to yaml.
> > 
> > I'm not sure that there will be much in the way of a common 
> > mvebu-pinctrl.yaml as you'd end up repeating most of the common stuff to 
> > make things conditional anyway.
> 
> We should wait for Rob to comment. But is suspect you are right, there
> will not be much savings.

It's always a judgement call of amount of if/then schema vs. duplicating 
the common parts. If it's the function/pin parts that vary, then that's 
probably best as separate schema for each case. Otherwise, I'm not sure 
without seeing something.

Rob