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

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

[Chris Packham]

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

-- 
2.35.1

[PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Chris Packham]

Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
SoC.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - Remove syscon and simple-mfd compatibles

 .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
new file mode 100644
index 000000000000..65af1d5f5fe0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/marvell,ac5-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell AC5 pin controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description:
+  Bindings for Marvell's AC5 memory-mapped pin controller.
+
+properties:
+  compatible:
+    const: marvell,ac5-pinctrl
+
+patternProperties:
+  '-pins$':
+    type: object
+    $ref: pinmux-node.yaml#
+
+    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
+        items:
+          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
+                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
+                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
+                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
+                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
+
+allOf:
+  - $ref: "pinctrl.yaml#"
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    system-controller@80020100 {
+      compatible = "syscon", "simple-mfd";
+      reg = <0x80020000 0x20>;
+
+      pinctrl0: pinctrl {
+        compatible = "marvell,ac5-pinctrl";
+
+        i2c0_pins: i2c0-pins {
+          marvell,pins = "mpp26", "mpp27";
+          marvell,function = "i2c0";
+        };
+
+        i2c0_gpio: i2c0-gpio-pins {
+          marvell,pins = "mpp26", "mpp27";
+          marvell,function = "gpio";
+        };
+      };
+    };
-- 
2.35.1

[PATCH v2 2/8] dt-bindings: net: mvneta: Add marvell,armada-ac5-neta

[Chris Packham]

The out of band port on the 98DX2530 SoC is similar to the armada-3700
except it requires a slightly different MBUS window configuration. Add a
new compatible string so this difference can be accounted for.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - New

 .../devicetree/bindings/net/marvell-armada-370-neta.txt          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 691f886cfc4a..2bf31572b08d 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -5,6 +5,7 @@ Required properties:
 	"marvell,armada-370-neta"
 	"marvell,armada-xp-neta"
 	"marvell,armada-3700-neta"
+	"marvell,armada-ac5-neta"
 - reg: address and length of the register set for the device.
 - interrupts: interrupt for the device
 - phy: See ethernet.txt file in the same directory.
-- 
2.35.1

[PATCH v2 3/8] dt-bindings: mmc: xenon: add AC5 compatible string

[Chris Packham]

Import binding documentation from the Marvell SDK which adds
marvell,ac5-sdhci compatible string and documents the requirements for
the for the Xenon SDHCI controller on the 98DX2530.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - New

 .../bindings/mmc/marvell,xenon-sdhci.txt      | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
index c51a62d751dc..43df466f0cb3 100644
--- a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
@@ -14,6 +14,7 @@ Required Properties:
   - "marvell,armada-ap806-sdhci": For controllers on Armada AP806.
   - "marvell,armada-ap807-sdhci": For controllers on Armada AP807.
   - "marvell,armada-cp110-sdhci": For controllers on Armada CP110.
+  - "marvell,ac5-sdhci": For CnM on AC5, AC5X and derived.
 
 - clocks:
   Array of clocks required for SDHC.
@@ -33,6 +34,13 @@ Required Properties:
     in below.
     Please also check property marvell,pad-type in below.
 
+  * For "marvell,ac5-sdhci", one or two register areas.
+    (reg-names "ctrl" & "decoder").
+    The first one is mandatory for the Xenon IP registers.
+    The second one is for systems where DMA mapping is required and is the
+    related address decoder register (the value to configure is derived from
+    the parent "dma-ranges").
+
   * For other compatible strings, one register area for Xenon IP.
 
 Optional Properties:
@@ -171,3 +179,47 @@ Example:
 
 		marvell,pad-type = "sd";
 	};
+
+
+- For eMMC with compatible "marvell,ac5-sdhci" with one reg range (no dma):
+	sdhci0: sdhci@805c0000 {
+		compatible = "marvell,ac5-sdhci";
+		reg = <0x0 0x805c0000 0x0 0x300>;
+		reg-names = "ctrl", "decoder";
+		interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&core_clock>;
+		clock-names = "core";
+		status = "okay";
+		bus-width = <8>;
+		/*marvell,xenon-phy-slow-mode;*/
+		non-removable;
+		mmc-ddr-1_8v;
+		mmc-hs200-1_8v;
+		mmc-hs400-1_8v;
+	};
+
+- For eMMC with compatible "marvell,ac5-sdhci" with two reg ranges (with dma):
+	mmc_dma: mmc-dma-peripherals@80500000 {
+		compatible = "simple-bus";
+		#address-cells = <0x2>;
+		#size-cells = <0x2>;
+		ranges;
+		dma-ranges = <0x2 0x0 0x2 0x80000000 0x1 0x0>;
+		dma-coherent;
+
+		sdhci0: sdhci@805c0000 {
+			compatible = "marvell,ac5-sdhci", "marvell,armada-ap806-sdhci";
+			reg = <0x0 0x805c0000 0x0 0x300>, <0x0 0x80440230 0x0 0x4>;
+			reg-names = "ctrl", "decoder";
+			interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&core_clock>;
+			clock-names = "core";
+			status = "okay";
+			bus-width = <8>;
+			/*marvell,xenon-phy-slow-mode;*/
+			non-removable;
+			mmc-ddr-1_8v;
+			mmc-hs200-1_8v;
+			mmc-hs400-1_8v;
+		};
+	};
-- 
2.35.1

[PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC

[Chris Packham]

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;
+
+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);
+
+	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),
+	},
+	.probe = ac5_pinctrl_probe,
+};
+
+builtin_platform_driver(ac5_pinctrl_driver);
-- 
2.35.1

[PATCH v2 5/8] net: mvneta: Add support for 98DX2530 Ethernet port

[Chris Packham]

The 98DX2530 SoC is similar to the Armada 3700 except it needs a
different MBUS window configuration. Add a new compatible string to
identify this device and the required MBUS window configuration.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - New

 drivers/net/ethernet/marvell/mvneta.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 83c8908f0cc7..000929794266 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -76,6 +76,8 @@
 #define MVNETA_WIN_SIZE(w)                      (0x2204 + ((w) << 3))
 #define MVNETA_WIN_REMAP(w)                     (0x2280 + ((w) << 2))
 #define MVNETA_BASE_ADDR_ENABLE                 0x2290
+#define      MVNETA_AC5_CNM_DDR_TARGET		0x2
+#define      MVNETA_AC5_CNM_DDR_ATTR		0xb
 #define MVNETA_ACCESS_PROTECT_ENABLE            0x2294
 #define MVNETA_PORT_CONFIG                      0x2400
 #define      MVNETA_UNI_PROMISC_MODE            BIT(0)
@@ -544,6 +546,7 @@ struct mvneta_port {
 
 	/* Flags for special SoC configurations */
 	bool neta_armada3700;
+	bool neta_ac5;
 	u16 rx_offset_correction;
 	const struct mbus_dram_target_info *dram_target_info;
 };
@@ -5272,6 +5275,10 @@ static void mvneta_conf_mbus_windows(struct mvneta_port *pp,
 			win_protect |= 3 << (2 * i);
 		}
 	} else {
+		if (pp->neta_ac5)
+			mvreg_write(pp, MVNETA_WIN_BASE(0),
+				    (MVNETA_AC5_CNM_DDR_ATTR << 8) |
+				    MVNETA_AC5_CNM_DDR_TARGET);
 		/* For Armada3700 open default 4GB Mbus window, leaving
 		 * arbitration of target/attribute to a different layer
 		 * of configuration.
@@ -5397,6 +5404,11 @@ static int mvneta_probe(struct platform_device *pdev)
 	/* Get special SoC configurations */
 	if (of_device_is_compatible(dn, "marvell,armada-3700-neta"))
 		pp->neta_armada3700 = true;
+	if (of_device_is_compatible(dn, "marvell,armada-ac5-neta")) {
+		pp->neta_armada3700 = true;
+		pp->neta_ac5 = true;
+	}
+
 
 	pp->clk = devm_clk_get(&pdev->dev, "core");
 	if (IS_ERR(pp->clk))
@@ -5720,6 +5732,7 @@ static const struct of_device_id mvneta_match[] = {
 	{ .compatible = "marvell,armada-370-neta" },
 	{ .compatible = "marvell,armada-xp-neta" },
 	{ .compatible = "marvell,armada-3700-neta" },
+	{ .compatible = "marvell,armada-ac5-neta" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mvneta_match);
-- 
2.35.1

[PATCH v2 6/8] mmc: xenon: add AC5 compatible string

[Chris Packham]

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>
---

Notes:
    Changes in v2:
    - New

 drivers/mmc/host/sdhci-xenon.c | 1 +
 drivers/mmc/host/sdhci-xenon.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
index 666cee4c7f7c..ac95d16809c5 100644
--- a/drivers/mmc/host/sdhci-xenon.c
+++ b/drivers/mmc/host/sdhci-xenon.c
@@ -692,6 +692,7 @@ static const struct of_device_id sdhci_xenon_dt_ids[] = {
 	{ .compatible = "marvell,armada-ap807-sdhci", .data = (void *)XENON_AP807},
 	{ .compatible = "marvell,armada-cp110-sdhci", .data =  (void *)XENON_CP110},
 	{ .compatible = "marvell,armada-3700-sdhci", .data =  (void *)XENON_A3700},
+	{ .compatible = "marvell,ac5-sdhci", .data = (void *)XENON_AC5},
 	{}
 };
 MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
index 3e9c6c908a79..451b41dd3447 100644
--- a/drivers/mmc/host/sdhci-xenon.h
+++ b/drivers/mmc/host/sdhci-xenon.h
@@ -57,7 +57,8 @@ enum xenon_variant {
 	XENON_A3700,
 	XENON_AP806,
 	XENON_AP807,
-	XENON_CP110
+	XENON_CP110,
+	XENON_AC5,
 };
 
 struct xenon_priv {
-- 
2.35.1

[PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

The 98DX2530 SoC is the Control and Management CPU integrated into
the Marvell 98DX25xx and 98DX35xx series of switch chip (internally
referred to as AlleyCat5 and AlleyCat5X).

These files have been taken from the Marvell SDK and lightly cleaned
up with the License and copyright retained.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    The Marvell SDK has a number of new compatible strings. I've brought
    through some of the drivers or where possible used an in-tree
    alternative (e.g. there is SDK code for a ac5-gpio but the existing
    marvell,armada-8k-gpio seems to cover what is needed if you use an
    appropriate binding). I expect that there will a new series of patches
    when I get some different hardware (or additions to this series
    depending on if/when it lands).
    
    Changes in v2:
    - Make pinctrl a child node of a syscon node
    - Use marvell,armada-8k-gpio instead of orion-gpio
    - Remove nand peripheral. The Marvell SDK does have some changes for the
      ac5-nand-controller but I currently lack hardware with NAND fitted so
      I can't test it right now. I've therefore chosen to omit the node and
      not attempted to bring in the driver or binding.
    - Remove pcie peripheral. Again there are changes in the SDK and I have
      no way of testing them.
    - Remove prestera node.
    - Remove "marvell,ac5-ehci" compatible from USB node as
      "marvell,orion-ehci" is sufficient
    - Remove watchdog node. There is a buggy driver for the ac5 watchdog in
      the SDK but it needs some work so I've dropped the node for now.

 arch/arm64/boot/dts/marvell/Makefile          |   1 +
 .../boot/dts/marvell/armada-98dx2530.dtsi     | 343 ++++++++++++++++++
 arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  62 ++++
 3 files changed, 406 insertions(+)
 create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
 create mode 100644 arch/arm64/boot/dts/marvell/rd-ac5x.dts

diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
index 1c794cdcb8e6..3905dee558b4 100644
--- a/arch/arm64/boot/dts/marvell/Makefile
+++ b/arch/arm64/boot/dts/marvell/Makefile
@@ -24,3 +24,4 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db.dtb
 dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db-B.dtb
 dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
 dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
+dtb-$(CONFIG_ARCH_MVEBU) += rd-ac5x.dtb
diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
new file mode 100644
index 000000000000..ebe464b9ebd2
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Device Tree For AC5.
+ *
+ * Copyright (C) 2021 Marvell
+ *
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	model = "Marvell AC5 SoC";
+	compatible = "marvell,ac5", "marvell,armada3700";
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	aliases {
+		serial0 = &uart0;
+		spiflash0 = &spiflash0;
+		gpio0 = &gpio0;
+		gpio1 = &gpio1;
+		ethernet0 = &eth0;
+		ethernet1 = &eth1;
+	};
+
+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>,
+				 <GIC_PPI 8 IRQ_TYPE_LEVEL_HIGH>,
+				 <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>,
+				 <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
+		clock-frequency = <25000000>;
+	};
+
+	pmu {
+		compatible = "arm,armv8-pmuv3";
+		interrupts = <GIC_PPI 12 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	soc {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		dma-ranges;
+
+		internal-regs@7f000000 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "simple-bus";
+			/* 16M internal register @ 0x7f00_0000 */
+			ranges = <0x0 0x0 0x7f000000 0x1000000>;
+			dma-coherent;
+
+			uart0: serial@12000 {
+				compatible = "snps,dw-apb-uart";
+				reg = <0x12000 0x100>;
+				reg-shift = <2>;
+				interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+				reg-io-width = <1>;
+				clock-frequency = <328000000>;
+				status = "okay";
+			};
+
+			mdio: mdio@20000 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				compatible = "marvell,orion-mdio";
+				reg = <0x22004 0x4>;
+				clocks = <&core_clock>;
+			};
+
+			i2c0: i2c@11000{
+				compatible = "marvell,mv78230-i2c";
+				reg = <0x11000 0x20>;
+
+				clocks = <&core_clock>;
+				clock-names = "core";
+				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+				clock-frequency=<100000>;
+				status="okay";
+
+				pinctrl-names = "default", "gpio";
+				pinctrl-0 = <&i2c0_pins>;
+				pinctrl-1 = <&i2c0_gpio>;
+				scl_gpio = <&gpio0 26 GPIO_ACTIVE_HIGH>;
+				sda_gpio = <&gpio0 27 GPIO_ACTIVE_HIGH>;
+			};
+
+			i2c1: i2c@11100{
+				compatible = "marvell,mv78230-i2c";
+				reg = <0x11100 0x20>;
+
+				clocks = <&core_clock>;
+				clock-names = "core";
+				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+				clock-frequency=<100000>;
+				status="okay";
+
+				pinctrl-names = "default", "gpio";
+				pinctrl-0 = <&i2c1_pins>;
+				pinctrl-1 = <&i2c1_gpio>;
+				scl_gpio = <&gpio0 20 GPIO_ACTIVE_HIGH>;
+				sda_gpio = <&gpio0 21 GPIO_ACTIVE_HIGH>;
+			};
+
+			system-controller@18000 {
+				compatible = "syscon", "simple-mfd";
+				reg = <0x18000 0x200>;
+
+				gpio0: gpio@100 {
+					compatible = "marvell,armada-8k-gpio";
+					offset = <0x100>;
+					ngpios = <32>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					gpio-ranges = <&pinctrl0 0 0 32>;
+					#pwm-cells = <2>;
+				};
+
+				gpio1: gpio@140 {
+					compatible = "marvell,armada-8k-gpio";
+					offset = <0x140>;
+					ngpios = <14>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					gpio-ranges = <&pinctrl0 0 32 14>;
+					#pwm-cells = <2>;
+				};
+			};
+		};
+
+		mmc_dma: mmc-dma-peripherals@80500000 {
+				compatible = "simple-bus";
+				#address-cells = <0x2>;
+				#size-cells = <0x2>;
+				ranges;
+				dma-coherent;
+
+				sdhci0: sdhci@805c0000 {
+					compatible = "marvell,ac5-sdhci",
+						     "marvell,armada-ap806-sdhci";
+					reg = <0x0 0x805c0000 0x0 0x300>;
+					reg-names = "ctrl", "decoder";
+					interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
+					clocks = <&core_clock>;
+					clock-names = "core";
+					status = "okay";
+					bus-width = <8>;
+					/*marvell,xenon-phy-slow-mode;*/
+					non-removable;
+					mmc-ddr-1_8v;
+					mmc-hs200-1_8v;
+					mmc-hs400-1_8v;
+				};
+		};
+
+		/*
+		 * Dedicated section for devices behind 32bit controllers so we
+		 * can configure specific DMA mapping for them
+		 */
+		behind-32bit-controller@7f000000 {
+			compatible = "simple-bus";
+			#address-cells = <0x2>;
+			#size-cells = <0x2>;
+			ranges = <0x0 0x0 0x0 0x7f000000 0x0 0x1000000>;
+			/* Host phy ram starts at 0x200M */
+			dma-ranges = <0x0 0x0 0x2 0x0 0x1 0x0>;
+			dma-coherent;
+
+			eth0: ethernet@20000 {
+				compatible = "marvell,armada-ac5-neta";
+				reg = <0x0 0x20000 0x0 0x4000>;
+				interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&core_clock>;
+				status = "disabled";
+				phy-mode = "sgmii";
+			};
+
+			eth1: ethernet@24000 {
+				compatible = "marvell,armada-ac5-neta";
+				reg = <0x0 0x24000 0x0 0x4000>;
+				interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&core_clock>;
+				status = "disabled";
+				phy-mode = "sgmii";
+			};
+
+			/* A dummy entry used for chipidea phy init */
+			usb1phy: usbphy {
+				compatible = "usb-nop-xceiv";
+				#phy-cells = <0>;
+			};
+
+			/* USB0 is a host USB */
+			usb0: usb@80000 {
+				compatible = "marvell,orion-ehci";
+				reg = <0x0 0x80000 0x0 0x500>;
+				interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
+				status = "okay";
+			};
+
+			/* USB1 is a peripheral USB */
+			usb1: usb@a0000 {
+				reg = <0x0 0xa0000 0x0 0x500>;
+				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+				status = "okay";
+			};
+		};
+
+		system-controller@80020100 {
+			compatible = "syscon", "simple-mfd";
+			reg = <0 0x80020100 0 0x20>;
+
+			pinctrl0: pinctrl@80020100 {
+				compatible = "marvell,ac5-pinctrl";
+
+				i2c0_pins: i2c0-pins {
+					marvell,pins = "mpp26", "mpp27";
+					marvell,function = "i2c0";
+				};
+
+				i2c0_gpio: i2c0-gpio-pins {
+					marvell,pins = "mpp26", "mpp27";
+					marvell,function = "gpio";
+				};
+
+				i2c1_pins: i2c1-pins {
+					marvell,pins = "mpp20", "mpp21";
+					marvell,function = "i2c1";
+				};
+
+				i2c1_gpio: i2c1-gpio-pins {
+					marvell,pins = "mpp20", "mpp21";
+					marvell,function = "i2c1";
+				};
+			};
+		};
+
+		core_clock: core_clock@0 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <400000000>;
+		};
+
+		axi_clock: axi_clock@0 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <325000000>;
+		};
+
+		spi_clock: spi_clock@0 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <200000000>;
+		};
+
+		spi0: spi@805a0000 {
+			compatible = "marvell,armada-3700-spi";
+			reg = <0x0 0x805a0000 0x0 0x50>;
+			#address-cells = <0x1>;
+			#size-cells = <0x0>;
+			clocks = <&spi_clock>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			num-cs = <1>;
+			status = "disabled";
+		};
+
+		spi@805a8000 {
+			compatible = "marvell,armada-3700-spi";
+			reg = <0x0 0x805a8000 0x0 0x50>;
+			#address-cells = <0x1>;
+			#size-cells = <0x0>;
+			clocks = <&spi_clock>;
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			num-cs = <1>;
+			status = "disabled";
+		};
+	};
+
+	gic: interrupt-controller@80600000 {
+		compatible = "arm,gic-v3";
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		/*#redistributor-regions = <1>;*/
+		redistributor-stride = <0x0 0x20000>;	// 128kB stride
+		reg = <0x0 0x80600000 0x0 0x10000>, /* GICD */
+			  <0x0 0x80660000 0x0 0x40000>; /* GICR */
+		interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&CPU0>;
+				};
+				core1 {
+					cpu = <&CPU1>;
+				};
+			};
+		};
+
+		CPU0:cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,armv8";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+			next-level-cache = <&L2_0>;
+		};
+
+		CPU1:cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,armv8";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+			next-level-cache = <&L2_0>;
+		};
+
+		L2_0: l2-cache0 {
+			compatible = "cache";
+		};
+	};
+
+	memory@0 {
+		device_type = "memory";
+		reg = <0x2 0x00000000 0x0 0x40000000>;
+		// linux,usable-memory = <0x2 0x00000000 0x0 0x80000000>;
+	};
+
+};
diff --git a/arch/arm64/boot/dts/marvell/rd-ac5x.dts b/arch/arm64/boot/dts/marvell/rd-ac5x.dts
new file mode 100644
index 000000000000..013cf7bd913a
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/rd-ac5x.dts
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Device Tree For AC5X.
+ *
+ * Copyright (C) 2021 Marvell
+ *
+ */
+/*
+ * Device Tree file for Marvell Alleycat 5X development board
+ * This board file supports the B configuration of the board
+ */
+
+#include "armada-98dx2530.dtsi"
+
+&mdio {
+	phy0: ethernet-phy@0 {
+		reg = <0 0>;
+	};
+};
+
+&eth0 {
+	status = "okay";
+	phy = <&phy0>;
+};
+
+&spi0 {
+	status = "okay";
+
+	spiflash0: spi-flash@0 {
+		compatible = "spi-nor";
+		spi-max-frequency = <50000000>;
+		spi-tx-bus-width = <1>; /* 1-single, 2-dual, 4-quad */
+		spi-rx-bus-width = <1>; /* 1-single, 2-dual, 4-quad */
+		reg = <0>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partition@0 {
+			label = "spi_flash_part0";
+			reg = <0x0 0x800000>;
+		};
+
+		parition@1 {
+			label = "spi_flash_part1";
+			reg = <0x800000 0x700000>;
+		};
+
+		parition@2 {
+			label = "spi_flash_part2";
+			reg = <0xF00000 0x100000>;
+		};
+	};
+};
+
+&usb1 {
+	compatible = "chipidea,usb2";
+	phys = <&usb1phy>;
+	phy-names = "usb-phy";
+	dr_mode = "peripheral";
+};
+
-- 
2.35.1

[PATCH v2 8/8] arm64: marvell: enable the 98DX2530 pinctrl driver

[Chris Packham]

This commit makes sure the drivers for the 98DX2530 pin controller is
enabled.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - None

 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 21697449d762..6bbb56901794 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -183,11 +183,13 @@ config ARCH_MVEBU
 	select PINCTRL_ARMADA_37XX
 	select PINCTRL_ARMADA_AP806
 	select PINCTRL_ARMADA_CP110
+	select PINCTRL_AC5
 	help
 	  This enables support for Marvell EBU familly, including:
 	   - Armada 3700 SoC Family
 	   - Armada 7K SoC Family
 	   - Armada 8K SoC Family
+	   - 98DX2530 SoC Family
 
 config ARCH_MXC
 	bool "ARMv8 based NXP i.MX SoC family"
-- 
2.35.1

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Andrew Lunn]

> +    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

Re: [PATCH v2 2/8] dt-bindings: net: mvneta: Add marvell,armada-ac5-neta

[Andrew Lunn]

On Tue, Mar 15, 2022 at 10:31:37AM +1300, Chris Packham wrote:
> The out of band port on the 98DX2530 SoC is similar to the armada-3700
> except it requires a slightly different MBUS window configuration. Add a
> new compatible string so this difference can be accounted for.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

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

    Andrew

Re: [PATCH v2 5/8] net: mvneta: Add support for 98DX2530 Ethernet port

[Andrew Lunn]

On Tue, Mar 15, 2022 at 10:31:40AM +1300, Chris Packham wrote:
> The 98DX2530 SoC is similar to the Armada 3700 except it needs a
> different MBUS window configuration. Add a new compatible string to
> identify this device and the required MBUS window configuration.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

I suggest you separate the two mvneta patches and send them to the
netdev list. They are likely to get merged before the merge window
opens next weekend. The other patches are less likely to be merged so
fast.

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

    Andrew

Re: [PATCH v2 3/8] dt-bindings: mmc: xenon: add AC5 compatible string

[Andrew Lunn]

On Tue, Mar 15, 2022 at 10:31:38AM +1300, Chris Packham wrote:
> Import binding documentation from the Marvell SDK which adds
> marvell,ac5-sdhci compatible string and documents the requirements for
> the for the Xenon SDHCI controller on the 98DX2530.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

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

    Andrew

Re: [PATCH v2 6/8] mmc: xenon: add AC5 compatible string

[Andrew Lunn]

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

Re: [PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC

[Andrew Lunn]

On Tue, Mar 15, 2022 at 10:31:39AM +1300, 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>

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

    Andrew

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Chris Packham]

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

Re: [PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> new file mode 100644
> index 000000000000..ebe464b9ebd2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Device Tree For AC5.
> + *
> + * Copyright (C) 2021 Marvell
> + *
> + */
> +
> +/dts-v1/;

> +	memory@0 {
> +		device_type = "memory";
> +		reg = <0x2 0x00000000 0x0 0x40000000>;
> +		// linux,usable-memory = <0x2 0x00000000 0x0 0x80000000>;
> +	};

Is the memory part of the SoC, or is it on the board? Normally it is
on the board, so should be in the .dts file. But Apple M1 etc...

> +&mdio {
> +	phy0: ethernet-phy@0 {
> +		reg = <0 0>;

phy reg values are a single number, the address on the bus, in the
range 0 to 31.

      Andrew

Re: [PATCH v2 8/8] arm64: marvell: enable the 98DX2530 pinctrl driver

[Andrew Lunn]

On Tue, Mar 15, 2022 at 10:31:43AM +1300, Chris Packham wrote:
> This commit makes sure the drivers for the 98DX2530 pin controller is
> enabled.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

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

    Andrew

Re: [PATCH v2 5/8] net: mvneta: Add support for 98DX2530 Ethernet port

[Chris Packham]

On 15/03/22 13:12, Andrew Lunn wrote:
> On Tue, Mar 15, 2022 at 10:31:40AM +1300, Chris Packham wrote:
>> The 98DX2530 SoC is similar to the Armada 3700 except it needs a
>> different MBUS window configuration. Add a new compatible string to
>> identify this device and the required MBUS window configuration.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> I suggest you separate the two mvneta patches and send them to the
> netdev list. They are likely to get merged before the merge window
> opens next weekend. The other patches are less likely to be merged so
> fast.

Thanks for the suggestion. I did wonder about doing that for this patch 
and the sdhci one. It'd also cut down the Cc spam.

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

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Andrew Lunn]

> 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

Re: [PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

On 15/03/22 13:24, Andrew Lunn wrote:
>> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>> new file mode 100644
>> index 000000000000..ebe464b9ebd2
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>> @@ -0,0 +1,343 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Device Tree For AC5.
>> + *
>> + * Copyright (C) 2021 Marvell
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +	memory@0 {
>> +		device_type = "memory";
>> +		reg = <0x2 0x00000000 0x0 0x40000000>;
>> +		// linux,usable-memory = <0x2 0x00000000 0x0 0x80000000>;
>> +	};
> Is the memory part of the SoC, or is it on the board? Normally it is
> on the board, so should be in the .dts file. But Apple M1 etc...

It's on the board. Marvell's SDK conflates the SoC and the common 
elements between their board designs (hence the SPI and Ethernet stuff 
in v1). I'll move it for the next round.

I also wasn't sure about linux,usable-memory. It's commented out so it's 
obviously doing nothing but should it? No other in-tree dts files have it.

>> +&mdio {
>> +	phy0: ethernet-phy@0 {
>> +		reg = <0 0>;
> phy reg values are a single number, the address on the bus, in the
> range 0 to 31.
Will fix.
>        Andrew

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Krzysztof Kozlowski]

On 14/03/2022 22:31, Chris Packham wrote:
> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
> SoC.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Remove syscon and simple-mfd compatibles
> 
>  .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
> new file mode 100644
> index 000000000000..65af1d5f5fe0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/marvell,ac5-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell AC5 pin controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description:
> +  Bindings for Marvell's AC5 memory-mapped pin controller.
> +
> +properties:
> +  compatible:
> +    const: marvell,ac5-pinctrl
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    $ref: pinmux-node.yaml#
> +
> +    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
> +        items:
> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
> +
> +allOf:
> +  - $ref: "pinctrl.yaml#"
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    system-controller@80020100 {
> +      compatible = "syscon", "simple-mfd";
> +      reg = <0x80020000 0x20>;

This is unusual. Usually the pinctrl should be a device @80020100, not
child of syscon node. Why do you need it? In v1 you mentioned that
vendor sources do like this, but it's not correct to copy wrong DTS. :)



Best regards,
Krzysztof

Re: [PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC

[Krzysztof Kozlowski]

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

Re: [PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

On Tue, Mar 15, 2022 at 02:11:56AM +0000, Chris Packham wrote:
> 
> On 15/03/22 13:24, Andrew Lunn wrote:
> >> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> >> new file mode 100644
> >> index 000000000000..ebe464b9ebd2
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> >> @@ -0,0 +1,343 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> +/*
> >> + * Device Tree For AC5.
> >> + *
> >> + * Copyright (C) 2021 Marvell
> >> + *
> >> + */
> >> +
> >> +/dts-v1/;
> >> +	memory@0 {
> >> +		device_type = "memory";
> >> +		reg = <0x2 0x00000000 0x0 0x40000000>;
> >> +		// linux,usable-memory = <0x2 0x00000000 0x0 0x80000000>;
> >> +	};
> > Is the memory part of the SoC, or is it on the board? Normally it is
> > on the board, so should be in the .dts file. But Apple M1 etc...

> I also wasn't sure about linux,usable-memory. It's commented out so it's 
> obviously doing nothing but should it? No other in-tree dts files have it.

Hi Chris

I've no idea what it means. Maybe search the SDK and see if they have
some code and what it does. But i would drop it.

     Andrew

Re: [PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC

[Andrew Lunn]

> > +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

Re: [PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC

[Krzysztof Kozlowski]

On 15/03/2022 15:33, Andrew Lunn wrote:
>>> +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.

What I wanted to say - current setting should cause warnings. Therefore
choose:
1. For ACPI && !OF this should be still without of_match_ptr, to allow
ACPI matching by OF (PRP0001).
2. For !OF with of_match_ptr() (weird setup... how to match then?) the
ac5_pinctrl_of_match should be marked as maybe unused.


Best regards,
Krzysztof

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Chris Packham]

(trimmed cc list to the arm, pinctrl and dt people)

On 15/03/22 23:46, Krzysztof Kozlowski wrote:
> On 14/03/2022 22:31, Chris Packham wrote:
>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>> SoC.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      - Remove syscon and simple-mfd compatibles
>>
>>   .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..65af1d5f5fe0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>> @@ -0,0 +1,70 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19ce8eGP0QA&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19cPrfjTyTg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell AC5 pin controller
>> +
>> +maintainers:
>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +
>> +description:
>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>> +
>> +properties:
>> +  compatible:
>> +    const: marvell,ac5-pinctrl
>> +
>> +patternProperties:
>> +  '-pins$':
>> +    type: object
>> +    $ref: pinmux-node.yaml#
>> +
>> +    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
>> +        items:
>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>> +
>> +allOf:
>> +  - $ref: "pinctrl.yaml#"
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    system-controller@80020100 {
>> +      compatible = "syscon", "simple-mfd";
>> +      reg = <0x80020000 0x20>;
> This is unusual. Usually the pinctrl should be a device @80020100, not
> child of syscon node. Why do you need it? In v1 you mentioned that
> vendor sources do like this, but it's not correct to copy wrong DTS. :)

The vendor dts has this

         pinctrl0: pinctrl@80020100 {
             compatible = "marvell,ac5-pinctrl",
                      "syscon", "simple-mfd";
             reg = <0 0x80020100 0 0x20>;
             i2c_mpps: i2c-mpps {
                 marvell,pins = "mpp26", "mpp27";
                 marvell,function = "i2c0-opt";
             };
      };

Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking 
and found marvell,armada-7k-pinctrl which has the pinctrl as a child of 
a syscon node and what you see in v2 is the result.

I probably went a bit too far off the deep end and should have just 
dropped the "syscon", "simple-mfd" compatibles. I even wrote that 
version but decided to add some gold plating before I submitted it.

>
>
> Best regards,
> Krzysztof

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Krzysztof Kozlowski]

On 15/03/2022 22:12, Chris Packham wrote:
> (trimmed cc list to the arm, pinctrl and dt people)
> 
> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>> On 14/03/2022 22:31, Chris Packham wrote:
>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>> SoC.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>>      Changes in v2:
>>>      - Remove syscon and simple-mfd compatibles
>>>
>>>   .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>   1 file changed, 70 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>> new file mode 100644
>>> index 000000000000..65af1d5f5fe0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>> @@ -0,0 +1,70 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19ce8eGP0QA&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19cPrfjTyTg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>> +
>>> +title: Marvell AC5 pin controller
>>> +
>>> +maintainers:
>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> +
>>> +description:
>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: marvell,ac5-pinctrl
>>> +
>>> +patternProperties:
>>> +  '-pins$':
>>> +    type: object
>>> +    $ref: pinmux-node.yaml#
>>> +
>>> +    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
>>> +        items:
>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>> +
>>> +allOf:
>>> +  - $ref: "pinctrl.yaml#"
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    system-controller@80020100 {
>>> +      compatible = "syscon", "simple-mfd";
>>> +      reg = <0x80020000 0x20>;
>> This is unusual. Usually the pinctrl should be a device @80020100, not
>> child of syscon node. Why do you need it? In v1 you mentioned that
>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
> 
> The vendor dts has this
> 
>          pinctrl0: pinctrl@80020100 {
>              compatible = "marvell,ac5-pinctrl",
>                       "syscon", "simple-mfd";
>              reg = <0 0x80020100 0 0x20>;
>              i2c_mpps: i2c-mpps {
>                  marvell,pins = "mpp26", "mpp27";
>                  marvell,function = "i2c0-opt";
>              };
>       };
> 
> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking 
> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of 
> a syscon node and what you see in v2 is the result.
> 
> I probably went a bit too far off the deep end and should have just 
> dropped the "syscon", "simple-mfd" compatibles. I even wrote that 
> version but decided to add some gold plating before I submitted it.

More or less it is explained in
Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
armada-7k uses it that way. The pinctrl is part of system registers
which apparently has to be shared with others (on shared SFR range).

It depends on your case, your SFR ranges for pinctrl and other blocks.


Best regards,
Krzysztof

Re: [PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Marc Zyngier]

On Mon, 14 Mar 2022 21:31:42 +0000,
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
> The 98DX2530 SoC is the Control and Management CPU integrated into
> the Marvell 98DX25xx and 98DX35xx series of switch chip (internally
> referred to as AlleyCat5 and AlleyCat5X).
> 
> These files have been taken from the Marvell SDK and lightly cleaned
> up with the License and copyright retained.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     The Marvell SDK has a number of new compatible strings. I've brought
>     through some of the drivers or where possible used an in-tree
>     alternative (e.g. there is SDK code for a ac5-gpio but the existing
>     marvell,armada-8k-gpio seems to cover what is needed if you use an
>     appropriate binding). I expect that there will a new series of patches
>     when I get some different hardware (or additions to this series
>     depending on if/when it lands).
>     
>     Changes in v2:
>     - Make pinctrl a child node of a syscon node
>     - Use marvell,armada-8k-gpio instead of orion-gpio
>     - Remove nand peripheral. The Marvell SDK does have some changes for the
>       ac5-nand-controller but I currently lack hardware with NAND fitted so
>       I can't test it right now. I've therefore chosen to omit the node and
>       not attempted to bring in the driver or binding.
>     - Remove pcie peripheral. Again there are changes in the SDK and I have
>       no way of testing them.
>     - Remove prestera node.
>     - Remove "marvell,ac5-ehci" compatible from USB node as
>       "marvell,orion-ehci" is sufficient
>     - Remove watchdog node. There is a buggy driver for the ac5 watchdog in
>       the SDK but it needs some work so I've dropped the node for now.
> 
>  arch/arm64/boot/dts/marvell/Makefile          |   1 +
>  .../boot/dts/marvell/armada-98dx2530.dtsi     | 343 ++++++++++++++++++
>  arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  62 ++++
>  3 files changed, 406 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>  create mode 100644 arch/arm64/boot/dts/marvell/rd-ac5x.dts
> 
> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
> index 1c794cdcb8e6..3905dee558b4 100644
> --- a/arch/arm64/boot/dts/marvell/Makefile
> +++ b/arch/arm64/boot/dts/marvell/Makefile
> @@ -24,3 +24,4 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db-B.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
> +dtb-$(CONFIG_ARCH_MVEBU) += rd-ac5x.dtb
> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> new file mode 100644
> index 000000000000..ebe464b9ebd2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Device Tree For AC5.
> + *
> + * Copyright (C) 2021 Marvell
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	model = "Marvell AC5 SoC";
> +	compatible = "marvell,ac5", "marvell,armada3700";

Are you sure about this compatibility with 3700? If so, why not reuse
the existing file? But the rest of the file tends to show that the SoC
is somehow different (the PPIs are all over the place -- someone got
pointlessly creative here...). I'd really drop this string.

> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		serial0 = &uart0;
> +		spiflash0 = &spiflash0;
> +		gpio0 = &gpio0;
> +		gpio1 = &gpio1;
> +		ethernet0 = &eth0;
> +		ethernet1 = &eth1;
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-0.2";
> +		method = "smc";
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>,
> +				 <GIC_PPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +				 <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>,
> +				 <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;

If you have an A55, you're missing an interrupt for the EL2 virtual
timer.

> +		clock-frequency = <25000000>;

Please drop this. The firmware should do the right thing, and if not,
that's an opportunity to fix it.

> +	};
> +

[...]

> +	gic: interrupt-controller@80600000 {
> +		compatible = "arm,gic-v3";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		/*#redistributor-regions = <1>;*/
> +		redistributor-stride = <0x0 0x20000>;	// 128kB stride

Please drop these two lines. They are totally pointless as they are
only expressing the default values.

> +		reg = <0x0 0x80600000 0x0 0x10000>, /* GICD */
> +			  <0x0 0x80660000 0x0 0x40000>; /* GICR */
> +		interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +	};

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Chris Packham]

On 16/03/22 21:16, Krzysztof Kozlowski wrote:
> On 15/03/2022 22:12, Chris Packham wrote:
>> (trimmed cc list to the arm, pinctrl and dt people)
>>
>> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>>> On 14/03/2022 22:31, Chris Packham wrote:
>>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>>> SoC.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> ---
>>>>
>>>> Notes:
>>>>       Changes in v2:
>>>>       - Remove syscon and simple-mfd compatibles
>>>>
>>>>    .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>>    1 file changed, 70 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>> new file mode 100644
>>>> index 000000000000..65af1d5f5fe0
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>> @@ -0,0 +1,70 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6CXu-YC1w&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6TAvbEE2Q&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>>> +
>>>> +title: Marvell AC5 pin controller
>>>> +
>>>> +maintainers:
>>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> +
>>>> +description:
>>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: marvell,ac5-pinctrl
>>>> +
>>>> +patternProperties:
>>>> +  '-pins$':
>>>> +    type: object
>>>> +    $ref: pinmux-node.yaml#
>>>> +
>>>> +    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
>>>> +        items:
>>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>>> +
>>>> +allOf:
>>>> +  - $ref: "pinctrl.yaml#"
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    system-controller@80020100 {
>>>> +      compatible = "syscon", "simple-mfd";
>>>> +      reg = <0x80020000 0x20>;
>>> This is unusual. Usually the pinctrl should be a device @80020100, not
>>> child of syscon node. Why do you need it? In v1 you mentioned that
>>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
>> The vendor dts has this
>>
>>           pinctrl0: pinctrl@80020100 {
>>               compatible = "marvell,ac5-pinctrl",
>>                        "syscon", "simple-mfd";
>>               reg = <0 0x80020100 0 0x20>;
>>               i2c_mpps: i2c-mpps {
>>                   marvell,pins = "mpp26", "mpp27";
>>                   marvell,function = "i2c0-opt";
>>               };
>>        };
>>
>> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking
>> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of
>> a syscon node and what you see in v2 is the result.
>>
>> I probably went a bit too far off the deep end and should have just
>> dropped the "syscon", "simple-mfd" compatibles. I even wrote that
>> version but decided to add some gold plating before I submitted it.
> More or less it is explained in
> Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
> armada-7k uses it that way. The pinctrl is part of system registers
> which apparently has to be shared with others (on shared SFR range).
>
> It depends on your case, your SFR ranges for pinctrl and other blocks.
>
I can tell you that without a syscon node in the mix somewhere the 
driver will fail to load. And when I switch to 
mvebu_pinctrl_simple_mmio_probe() the driver loads but then kernel 
panics when something tries to use one of the pin functions.

So I think the syscon is needed. I just need to come up with a better 
justification than "because it's needed".

> Best regards,
> Krzysztof

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Krzysztof Kozlowski]

On 16/03/2022 21:21, Chris Packham wrote:
> 
> On 16/03/22 21:16, Krzysztof Kozlowski wrote:
>> On 15/03/2022 22:12, Chris Packham wrote:
>>> (trimmed cc list to the arm, pinctrl and dt people)
>>>
>>> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>>>> On 14/03/2022 22:31, Chris Packham wrote:
>>>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>>>> SoC.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>       Changes in v2:
>>>>>       - Remove syscon and simple-mfd compatibles
>>>>>
>>>>>    .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>>>    1 file changed, 70 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..65af1d5f5fe0
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>> @@ -0,0 +1,70 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6CXu-YC1w&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6TAvbEE2Q&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>>>> +
>>>>> +title: Marvell AC5 pin controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> +
>>>>> +description:
>>>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: marvell,ac5-pinctrl
>>>>> +
>>>>> +patternProperties:
>>>>> +  '-pins$':
>>>>> +    type: object
>>>>> +    $ref: pinmux-node.yaml#
>>>>> +
>>>>> +    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
>>>>> +        items:
>>>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: "pinctrl.yaml#"
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    system-controller@80020100 {
>>>>> +      compatible = "syscon", "simple-mfd";
>>>>> +      reg = <0x80020000 0x20>;
>>>> This is unusual. Usually the pinctrl should be a device @80020100, not
>>>> child of syscon node. Why do you need it? In v1 you mentioned that
>>>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
>>> The vendor dts has this
>>>
>>>           pinctrl0: pinctrl@80020100 {
>>>               compatible = "marvell,ac5-pinctrl",
>>>                        "syscon", "simple-mfd";
>>>               reg = <0 0x80020100 0 0x20>;
>>>               i2c_mpps: i2c-mpps {
>>>                   marvell,pins = "mpp26", "mpp27";
>>>                   marvell,function = "i2c0-opt";
>>>               };
>>>        };
>>>
>>> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking
>>> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of
>>> a syscon node and what you see in v2 is the result.
>>>
>>> I probably went a bit too far off the deep end and should have just
>>> dropped the "syscon", "simple-mfd" compatibles. I even wrote that
>>> version but decided to add some gold plating before I submitted it.
>> More or less it is explained in
>> Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
>> armada-7k uses it that way. The pinctrl is part of system registers
>> which apparently has to be shared with others (on shared SFR range).
>>
>> It depends on your case, your SFR ranges for pinctrl and other blocks.
>>
> I can tell you that without a syscon node in the mix somewhere the 
> driver will fail to load. And when I switch to 
> mvebu_pinctrl_simple_mmio_probe() the driver loads but then kernel 
> panics when something tries to use one of the pin functions.
> 
> So I think the syscon is needed. I just need to come up with a better 
> justification than "because it's needed".

What do you mean "driver fails to load"? You control the driver, don't
you? You wrote it? If you write a driver which is not compatible with
bindings, it won't work obviously, so after changing bindings you need
to revisit the driver.

There is no need for syscon because driver "fails to load". You need to
fix your driver. Currently the driver code is definitely not a proper
platform driver.

Different question is whether something else requires here syscon
because it accesses these registers but this requires knowledge of
architecture and other components.


Best regards,
Krzysztof

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Andrew Lunn]

> What do you mean "driver fails to load"? You control the driver, don't
> you?

It is a thin wrapper around the mvebu driver, which does all the real
work. So no, Chris does not really control what the core of the driver
does.

The existing binding documentation says:

    * Marvell Armada 37xx SoC pin and gpio controller

    Each Armada 37xx SoC come with two pin and gpio controller one for
    the south bridge and the other for the north bridge.

    Inside this set of register the gpio latch allows exposing some
    configuration of the SoC and especially the clock frequency of the
    xtal. Hence, this node is a represent as syscon allowing sharing
    the register between multiple hardware block.


So the syscon is there to allow the clock driver to share the register
space.

	Andrew

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Krzysztof Kozlowski]

On 17/03/2022 15:14, Andrew Lunn wrote:
>> What do you mean "driver fails to load"? You control the driver, don't
>> you?
> 
> It is a thin wrapper around the mvebu driver, which does all the real
> work. So no, Chris does not really control what the core of the driver
> does.

This this design still require a pinctrl to be a child of some node?

> 
> The existing binding documentation says:
> 
>     * Marvell Armada 37xx SoC pin and gpio controller
> 
>     Each Armada 37xx SoC come with two pin and gpio controller one for
>     the south bridge and the other for the north bridge.
> 
>     Inside this set of register the gpio latch allows exposing some
>     configuration of the SoC and especially the clock frequency of the
>     xtal. Hence, this node is a represent as syscon allowing sharing
>     the register between multiple hardware block.
> 
> 
> So the syscon is there to allow the clock driver to share the register
> space.


This makes sense. Solution here would be to add syscon compatible to
pinctrl node. This parent simple-mfd+syscon node looks like a workaround
to share some registers in a highly flexible way. However isn't it
better to have more obvious owner of the register space (e.g. pinctrl)?
IOW, if there is only one child of syscon+simple-mfd node, why not
getting rid of it and making pinctrl owner of this address space? It's
also simpler code.


Best regards,
Krzysztof

Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Rob Herring]

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