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

[PATCH v5 0/2] 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.

The pinctrl changes from v4 have been picked up and are in linux-next so I
haven't included them in this round. That leaves just the dts files and a minor
Kconfig update for arm64.

Chris Packham (2):
  arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board
  arm64: marvell: enable the 98DX2530 pinctrl driver

 arch/arm64/Kconfig.platforms                  |   2 +
 arch/arm64/boot/dts/marvell/Makefile          |   1 +
 .../boot/dts/marvell/armada-98dx2530.dtsi     | 310 ++++++++++++++++++
 arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
 4 files changed, 403 insertions(+)
 create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
 create mode 100644 arch/arm64/boot/dts/marvell/rd-ac5x.dts

-- 
2.36.0

[PATCH v5 1/2] 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---

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 two instances of
    the existing marvell,orion-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 v5:
    - add #{address,size}-cells property to i2c nodes
    - make i2c nodes disabled in the SoC and enable them in the board
    - add interrupt controller attributes to gpio nodes
    - Move fixed-clock nodes up a level and remove unnecessary @0
    Changes in v4:
    - use 'phy-handle' instead of 'phy'
    - move status="okay" on usb nodes to board dts
    - Add review from Andrew
    Changes in v3:
    - Move memory node to board
    - Use single digit reg value for phy address
    - Remove MMC node (driver needs work)
    - Remove syscon & simple-mfd for pinctrl
    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     | 310 ++++++++++++++++++
 arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
 3 files changed, 401 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..ec4cfb17a260
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
@@ -0,0 +1,310 @@
+// 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";
+	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@22004 {
+				#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>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				clocks = <&core_clock>;
+				clock-names = "core";
+				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+				clock-frequency=<100000>;
+				status="disabled";
+
+				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>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				clocks = <&core_clock>;
+				clock-names = "core";
+				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+				clock-frequency=<100000>;
+				status="disabled";
+
+				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>;
+			};
+
+			gpio0: gpio@18100 {
+				compatible = "marvell,orion-gpio";
+				reg = <0x18100 0x40>;
+				ngpios = <32>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				gpio-ranges = <&pinctrl0 0 0 32>;
+				marvell,pwm-offset = <0x1f0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+			};
+
+			gpio1: gpio@18140 {
+				reg = <0x18140 0x40>;
+				compatible = "marvell,orion-gpio";
+				ngpios = <14>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				gpio-ranges = <&pinctrl0 0 32 14>;
+				marvell,pwm-offset = <0x1f0>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+			};
+		};
+
+		/*
+		 * 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 = "disabled";
+			};
+
+			/* USB1 is a peripheral USB */
+			usb1: usb@a0000 {
+				reg = <0x0 0xa0000 0x0 0x500>;
+				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+				status = "disabled";
+			};
+		};
+
+		pinctrl0: pinctrl@80020100 {
+			compatible = "marvell,ac5-pinctrl";
+			reg = <0 0x80020100 0 0x20>;
+
+			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";
+			};
+		};
+
+		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";
+		};
+
+		spi1: 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";
+		};
+	};
+
+	core_clock: core_clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <400000000>;
+	};
+
+	axi_clock: axi_clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <325000000>;
+	};
+
+	spi_clock: spi_clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <200000000>;
+	};
+
+	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";
+		};
+	};
+};
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..2655e1658750
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/rd-ac5x.dts
@@ -0,0 +1,90 @@
+// 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
+ */
+
+/dts-v1/;
+
+#include "armada-98dx2530.dtsi"
+
+/ {
+	model = "Marvell RD-AC5X Board";
+	compatible = "marvell,ac5x", "marvell,ac5";
+
+	memory@0 {
+		device_type = "memory";
+		reg = <0x2 0x00000000 0x0 0x40000000>;
+	};
+};
+
+&mdio {
+	phy0: ethernet-phy@0 {
+		reg = <0>;
+	};
+};
+
+&i2c0 {
+	status = "okay";
+};
+
+&i2c1 {
+	status = "okay";
+};
+
+&eth0 {
+	status = "okay";
+	phy-handle = <&phy0>;
+};
+
+&usb0 {
+	status = "okay";
+};
+
+&usb1 {
+	status = "okay";
+};
+
+&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.36.0

[PATCH v5 2/2] 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---

Notes:
    Changes in v5:
    - None
    Changes in v4:
    - None
    Changes in v3:
    - Add review from Andrew
    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 30b123cde02c..229571d57496 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.36.0

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Krzysztof Kozlowski]

On 04/05/2022 06:46, Chris Packham 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).

Thank you for your patch. There is something to discuss/improve.

>  arch/arm64/boot/dts/marvell/Makefile          |   1 +
>  .../boot/dts/marvell/armada-98dx2530.dtsi     | 310 ++++++++++++++++++
>  arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
>  3 files changed, 401 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..ec4cfb17a260
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> @@ -0,0 +1,310 @@
> +// 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";

Missing board bindings patch.

> +	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@22004 {
> +				#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>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				clocks = <&core_clock>;
> +				clock-names = "core";
> +				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
> +				clock-frequency=<100000>;
> +				status="disabled";
> +
> +				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>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				clocks = <&core_clock>;
> +				clock-names = "core";
> +				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
> +				clock-frequency=<100000>;
> +				status="disabled";

Spaces around '='. Also, put status as the last property. Here and in
every other node.

> +
> +				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>;
> +			};
> +
> +			gpio0: gpio@18100 {
> +				compatible = "marvell,orion-gpio";
> +				reg = <0x18100 0x40>;
> +				ngpios = <32>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				gpio-ranges = <&pinctrl0 0 0 32>;
> +				marvell,pwm-offset = <0x1f0>;
> +				interrupt-controller;
> +				#interrupt-cells = <2>;
> +			};
> +
> +			gpio1: gpio@18140 {
> +				reg = <0x18140 0x40>;
> +				compatible = "marvell,orion-gpio";
> +				ngpios = <14>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				gpio-ranges = <&pinctrl0 0 32 14>;
> +				marvell,pwm-offset = <0x1f0>;
> +				interrupt-controller;
> +				#interrupt-cells = <2>;
> +			};
> +		};
> +
> +		/*
> +		 * 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 {

Node name: usb-phy or phy

> +				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 = "disabled";
> +			};
> +
> +			/* USB1 is a peripheral USB */
> +			usb1: usb@a0000 {
> +				reg = <0x0 0xa0000 0x0 0x500>;
> +				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +		};
> +
> +		pinctrl0: pinctrl@80020100 {
> +			compatible = "marvell,ac5-pinctrl";
> +			reg = <0 0x80020100 0 0x20>;
> +
> +			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";
> +			};
> +		};
> +
> +		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";
> +		};
> +
> +		spi1: 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";
> +		};
> +	};
> +
> +	core_clock: core_clock {

No underscores in node names.

All these clocks do not look like real clocks. Where are they if outside
of SoC? These should be fed from clock controllers, shouldn't they? If
they are provided by boards, don't put them into SoC...

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <400000000>;
> +	};
> +
> +	axi_clock: axi_clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <325000000>;
> +	};
> +
> +	spi_clock: spi_clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <200000000>;
> +	};
> +
> +	gic: interrupt-controller@80600000 {

Outside of SoC?

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

cpus go before soc.

> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&CPU0>;
> +				};
> +				core1 {
> +					cpu = <&CPU1>;
> +				};
> +			};
> +		};
> +
> +		CPU0:cpu@0 {

space after :

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

l2-cache? or you expect more of l2 caches... ?

> +			compatible = "cache";
> +		};
> +	};
> +};
> 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..2655e1658750
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/rd-ac5x.dts
> @@ -0,0 +1,90 @@
> +// 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
> + */
> +
> +/dts-v1/;
> +
> +#include "armada-98dx2530.dtsi"
> +
> +/ {
> +	model = "Marvell RD-AC5X Board";
> +	compatible = "marvell,ac5x", "marvell,ac5";

Not documented.

> +
> +	memory@0 {
> +		device_type = "memory";
> +		reg = <0x2 0x00000000 0x0 0x40000000>;
> +	};
> +};
> +
> +&mdio {
> +	phy0: ethernet-phy@0 {
> +		reg = <0>;
> +	};
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +};
> +
> +&eth0 {
> +	status = "okay";
> +	phy-handle = <&phy0>;
> +};
> +
> +&usb0 {
> +	status = "okay";
> +};
> +
> +&usb1 {
> +	status = "okay";
> +};
> +
> +&spi0 {
> +	status = "okay";
> +
> +	spiflash0: spi-flash@0 {

flash

Please run `make dtbs_check`, to use automated tools instead of
reviewers time.



Best regards,
Krzysztof

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

(resend without the html)


On 5/05/22 21:19, Krzysztof Kozlowski wrote:
>> +	core_clock: core_clock {
> No underscores in node names.
>
> All these clocks do not look like real clocks. Where are they if outside
> of SoC? These should be fed from clock controllers, shouldn't they? If
> they are provided by boards, don't put them into SoC...
>
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <400000000>;
>> +	};
>> +
>> +	axi_clock: axi_clock {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <325000000>;
>> +	};
>> +
>> +	spi_clock: spi_clock {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <200000000>;
>> +	};

Based on the information I have (which isn't much) there is a ref_clk 
input that is connected to a 25MHz oscillator and then I'm assuming 
these are all generated from that with various dividers. 25MHz is the 
only documented option.

There doesn't appear to be any documented register where I can read out 
the divider ratios. It might be nice I could have the fixed osc node and 
have these 3 clocks derived with fixed divisors but I don't see any what 
of achieving that.

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Krzysztof Kozlowski]

On 10/05/2022 06:14, Chris Packham wrote:
> 
> Based on the information I have (which isn't much) there is a ref_clk 
> input that is connected to a 25MHz oscillator and then I'm assuming 
> these are all generated from that with various dividers. 25MHz is the 
> only documented option.
> 
> There doesn't appear to be any documented register where I can read out 
> the divider ratios. It might be nice I could have the fixed osc node and 
> have these 3 clocks derived with fixed divisors but I don't see any what 
> of achieving that.


OK, but where are the dividers? The ref_clk is outside of SoC, so should
be defined in board DTS (at least its rate). If the rest is in the SoC,
they are usually part of clock controller, because usually they belong
to some power domain or have some clock gating.


Best regards,
Krzysztof

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

On Tue, May 10, 2022 at 09:08:08AM +0200, Krzysztof Kozlowski wrote:
> On 10/05/2022 06:14, Chris Packham wrote:
> > 
> > Based on the information I have (which isn't much) there is a ref_clk 
> > input that is connected to a 25MHz oscillator and then I'm assuming 
> > these are all generated from that with various dividers. 25MHz is the 
> > only documented option.
> > 
> > There doesn't appear to be any documented register where I can read out 
> > the divider ratios. It might be nice I could have the fixed osc node and 
> > have these 3 clocks derived with fixed divisors but I don't see any what 
> > of achieving that.
> 
> 
> OK, but where are the dividers? The ref_clk is outside of SoC, so should
> be defined in board DTS (at least its rate). If the rest is in the SoC,
> they are usually part of clock controller, because usually they belong
> to some power domain or have some clock gating.

25MHz is a 'magic value' in Ethernet, nearly everything is based
around it. And remember this SoC is basically an Ethernet switch with
a small CPU glued on one side. If you gated clocks derived from the
25MHz reference clock, probably part of your Ethernet switch stops
working, which is the whole point of this SoC. So i doubt there are
gates on the derived clocks. If there is any gating and power domains,
it is generally at a different level. You can power down individual
ports of the Ethernet switch. But generally, there is one bit in a
register somewhere to do that, and you don't have direct control over
clocks and regulators etc.

       Andrew

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Krzysztof Kozlowski]

On 10/05/2022 14:37, Andrew Lunn wrote:
> On Tue, May 10, 2022 at 09:08:08AM +0200, Krzysztof Kozlowski wrote:
>> On 10/05/2022 06:14, Chris Packham wrote:
>>>
>>> Based on the information I have (which isn't much) there is a ref_clk 
>>> input that is connected to a 25MHz oscillator and then I'm assuming 
>>> these are all generated from that with various dividers. 25MHz is the 
>>> only documented option.
>>>
>>> There doesn't appear to be any documented register where I can read out 
>>> the divider ratios. It might be nice I could have the fixed osc node and 
>>> have these 3 clocks derived with fixed divisors but I don't see any what 
>>> of achieving that.
>>
>>
>> OK, but where are the dividers? The ref_clk is outside of SoC, so should
>> be defined in board DTS (at least its rate). If the rest is in the SoC,
>> they are usually part of clock controller, because usually they belong
>> to some power domain or have some clock gating.
> 
> 25MHz is a 'magic value' in Ethernet, nearly everything is based
> around it. And remember this SoC is basically an Ethernet switch with
> a small CPU glued on one side. If you gated clocks derived from the
> 25MHz reference clock, probably part of your Ethernet switch stops
> working, which is the whole point of this SoC. So i doubt there are
> gates on the derived clocks. If there is any gating and power domains,
> it is generally at a different level. You can power down individual
> ports of the Ethernet switch. But generally, there is one bit in a
> register somewhere to do that, and you don't have direct control over
> clocks and regulators etc.

The 25 MHz input clock I understand, it was about other clocks, like
spi, axi and core. These clearly look like part of SoC, so defining them
with a "stubs" (uncontrollable fixed-clock) is not the best way of
modelling an SoC. Although maybe this SoC does not have a proper clock
controller and even SPI and AXI clocks are always on?


Best regards,
Krzysztof

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

On 5/05/22 21:19, Krzysztof Kozlowski wrote:
>> +/ {
>> +	model = "Marvell AC5 SoC";
>> +	compatible = "marvell,ac5";
> Missing board bindings patch.
>
Where do these usually end up? I can't see any of the other Marvell 
boards having bindings (which might be why you're telling me not to add 
new boards/SoCs without it).  There's one entry in 
Documentation/devicetree/bindings/board but that seems more related to 
some board specific misc devices.

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

On 11/05/22 09:51, Chris Packham wrote:
>
> On 5/05/22 21:19, Krzysztof Kozlowski wrote:
>>> +/ {
>>> +    model = "Marvell AC5 SoC";
>>> +    compatible = "marvell,ac5";
>> Missing board bindings patch.
>>
> Where do these usually end up? I can't see any of the other Marvell 
> boards having bindings (which might be why you're telling me not to 
> add new boards/SoCs without it).  There's one entry in 
> Documentation/devicetree/bindings/board but that seems more related to 
> some board specific misc devices.
Ah I just found 
Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml. I can 
put something next to that.

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Vadym Kochan]

Hi Chris,

> arch/arm64/boot/dts/marvell/Makefile          |   1 +
> .../boot/dts/marvell/armada-98dx2530.dtsi     | 310 ++++++++++++++++++
> arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
> 3 files changed, 401 insertions(+)

Marvell is going to start the upstreaming of AC5X boards support, we have also patches with similar .dts(i) files
but with different naming:

    ac5.dtsi
    ac5_rd.dts
    ac5_db.dts
    ac5x_db.dts

What do you think about to use these naming scheme ?

Regards,
Vadym Kochan

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote:
> Hi Chris,
> 
> > arch/arm64/boot/dts/marvell/Makefile          |   1 +
> > .../boot/dts/marvell/armada-98dx2530.dtsi     | 310 ++++++++++++++++++
> > arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
> > 3 files changed, 401 insertions(+)
> 
> Marvell is going to start the upstreaming of AC5X boards support, we have also patches with similar .dts(i) files
> but with different naming:
> 
>     ac5.dtsi
>     ac5_rd.dts
>     ac5_db.dts
>     ac5x_db.dts
> 
> What do you think about to use these naming scheme ?

Chris has done all the hard work, he gets to pick the naming. And get
his files merged first.

However, now that i come to look in arch/arm64/boot/dts/marvell, i
think most of the current filenames proposed don't match the current names.

armada-98dx2530.dtsi fits the current pattern.

However, Chris's board files should probably be

armada-98dx2530-rd.dts

and the other files should be

armada-98dx2530-db.dts

armada-98dx2530-x-db.dts

What does the x in x_db mean? Does that refer to the board or the SoC?

	Andrew

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

On 12/05/22 04:20, Andrew Lunn wrote:
> On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote:
>> Hi Chris,
>>
>>> arch/arm64/boot/dts/marvell/Makefile          |   1 +
>>> .../boot/dts/marvell/armada-98dx2530.dtsi     | 310 ++++++++++++++++++
>>> arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
>>> 3 files changed, 401 insertions(+)
>> Marvell is going to start the upstreaming of AC5X boards support,
That's good news. I'm probably the customer that's been nagging the 
Marvell support team. But I'm also impatient hence I started working on 
this series. The pinctrl and mvneta changes have already been accepted.
>>   we have also patches with similar .dts(i) files
>> but with different naming:
>>
>>      ac5.dtsi
>>      ac5_rd.dts
>>      ac5_db.dts
>>      ac5x_db.dts
>>
>> What do you think about to use these naming scheme ?

Personally I thought they'd be rejected upstream as being too vague and 
generic. I settled on armada-98dx2530 as I saw the 98dx2530 name used on 
the Marvell Portal to refer to the CnM block for the AC5/AC5X. I was 
going to call the board file "rd-ac5x-32g16hvg6hlg.dts" as that's what 
the silkscreen on the board I have says but I shortened it to "rd-ac5x" 
as the switch port configuration is largely irrelevant to the board 
support I'm trying to get landed.

> Chris has done all the hard work, he gets to pick the naming. And get
> his files merged first.

I'm not against changing if there is a consensus. On another thread the 
idea of armada-98dx25xx/armada-98dx35xx was mentioned. That might be a 
reasonable compromise (although technically there's no difference in the 
CPU block between the 25xx and 35xx).

> However, now that i come to look in arch/arm64/boot/dts/marvell, i
> think most of the current filenames proposed don't match the current names.
>
> armada-98dx2530.dtsi fits the current pattern.
>
> However, Chris's board files should probably be
>
> armada-98dx2530-rd.dts
>
> and the other files should be
>
> armada-98dx2530-db.dts
>
> armada-98dx2530-x-db.dts
>
> What does the x in x_db mean? Does that refer to the board or the SoC?

The x is from AC5X so we'd actually have armada-98dx25xx-db.dts and 
armada-98dx35xx-db.dts. My board would be called armada-98dx35xx-rd.dts 
or perhaps armada-98dx3550-rd.dts.

Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

On Wed, May 11, 2022 at 10:59:37PM +0000, Chris Packham wrote:
> 
> On 12/05/22 04:20, Andrew Lunn wrote:
> > On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote:
> >> Hi Chris,
> >>
> >>> arch/arm64/boot/dts/marvell/Makefile          |   1 +
> >>> .../boot/dts/marvell/armada-98dx2530.dtsi     | 310 ++++++++++++++++++
> >>> arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
> >>> 3 files changed, 401 insertions(+)
> >> Marvell is going to start the upstreaming of AC5X boards support,
> That's good news. I'm probably the customer that's been nagging the 
> Marvell support team. But I'm also impatient hence I started working on 
> this series. The pinctrl and mvneta changes have already been accepted.
> >>   we have also patches with similar .dts(i) files
> >> but with different naming:
> >>
> >>      ac5.dtsi
> >>      ac5_rd.dts
> >>      ac5_db.dts
> >>      ac5x_db.dts
> >>
> >> What do you think about to use these naming scheme ?
> 
> Personally I thought they'd be rejected upstream as being too vague and 
> generic.

Agreed.

> I'm not against changing if there is a consensus. On another thread the 
> idea of armada-98dx25xx/armada-98dx35xx was mentioned. That might be a 
> reasonable compromise (although technically there's no difference in the 
> CPU block between the 25xx and 35xx).

Until we find there is a difference. Marvell, can you confirm that the
switch really is the only difference?

This might also come down to ID registers. The DT could be enough to
find the ID of the device, the rest is then done in the drivers, not
DT.

> > However, now that i come to look in arch/arm64/boot/dts/marvell, i
> > think most of the current filenames proposed don't match the current names.
> >
> > armada-98dx2530.dtsi fits the current pattern.
> >
> > However, Chris's board files should probably be
> >
> > armada-98dx2530-rd.dts
> >
> > and the other files should be
> >
> > armada-98dx2530-db.dts
> >
> > armada-98dx2530-x-db.dts
> >
> > What does the x in x_db mean? Does that refer to the board or the SoC?
> 
> The x is from AC5X so we'd actually have armada-98dx25xx-db.dts and 
> armada-98dx35xx-db.dts. My board would be called armada-98dx35xx-rd.dts 
> or perhaps armada-98dx3550-rd.dts.

armada-98dx25xx and armada-98dx35xx does help with the naming.  So it
probably is worth having armada-98dx35xx which simply includes
armada-98d25xx.

	Andrew

RE: [EXT] Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Elad Nachman]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, May 12, 2022 3:40 AM
> To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
> Cc: Vadym Kochan <vadym.kochan@plvision.eu>;
> catalin.marinas@arm.com; will@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; gregory.clement@bootlin.com;
> sebastian.hesselbarth@gmail.com; Kostya Porotchkin
> <kostap@marvell.com>; robert.marko@sartura.hr; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Elad Nachman <enachman@marvell.com>
> Subject: [EXT] Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530
> SoC and RD-AC5X board
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, May 11, 2022 at 10:59:37PM +0000, Chris Packham wrote:
> >
> > On 12/05/22 04:20, Andrew Lunn wrote:
> > > On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote:
> > >> Hi Chris,
> > >>
> > >>> arch/arm64/boot/dts/marvell/Makefile          |   1 +
> > >>> .../boot/dts/marvell/armada-98dx2530.dtsi     | 310
> ++++++++++++++++++
> > >>> arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
> > >>> 3 files changed, 401 insertions(+)
> > >> Marvell is going to start the upstreaming of AC5X boards support,
> > That's good news. I'm probably the customer that's been nagging the
> > Marvell support team. But I'm also impatient hence I started working
> > on this series. The pinctrl and mvneta changes have already been accepted.
> > >>   we have also patches with similar .dts(i) files but with
> > >> different naming:
> > >>
> > >>      ac5.dtsi
> > >>      ac5_rd.dts
> > >>      ac5_db.dts
> > >>      ac5x_db.dts
> > >>
> > >> What do you think about to use these naming scheme ?
> >
> > Personally I thought they'd be rejected upstream as being too vague
> > and generic.
> 
> Agreed.
> 
> > I'm not against changing if there is a consensus. On another thread
> > the idea of armada-98dx25xx/armada-98dx35xx was mentioned. That
> might
> > be a reasonable compromise (although technically there's no difference
> > in the CPU block between the 25xx and 35xx).
> 
> Until we find there is a difference. Marvell, can you confirm that the switch
> really is the only difference?

Basically, the cpu-subsystems of Prestera 98DX25xx (AC5) and Prestera 98DX35xx (AC5X) are the same.
There is a very small difference in how the default memory map starts after boot, which is reconfigurable in u-boot.
This affects the switch (not part of the device tree anyway) and the PCIe.
The PCIe window still overlap partially between AC5 and AC5X, however.
The original Marvell DTSI overcomes this by defining only the PCIe overlapping part in the device tree, resulting in a single device tree which works on both AC5 and AC5X.
The DTSI Chris proposed had the PCIe portion removed.
We have PCIe support for AC5/AC5X so we would obviously like to include this portion in both the DTSI and as a patch to the Armada8K PCIe driver.

> 
> This might also come down to ID registers. The DT could be enough to find
> the ID of the device, the rest is then done in the drivers, not DT.
> 
> > > However, now that i come to look in arch/arm64/boot/dts/marvell, i
> > > think most of the current filenames proposed don't match the current
> names.
> > >
> > > armada-98dx2530.dtsi fits the current pattern.
> > >
> > > However, Chris's board files should probably be
> > >
> > > armada-98dx2530-rd.dts
> > >
> > > and the other files should be
> > >
> > > armada-98dx2530-db.dts
> > >
> > > armada-98dx2530-x-db.dts
> > >
> > > What does the x in x_db mean? Does that refer to the board or the SoC?
> >
> > The x is from AC5X so we'd actually have armada-98dx25xx-db.dts and
> > armada-98dx35xx-db.dts. My board would be called
> > armada-98dx35xx-rd.dts or perhaps armada-98dx3550-rd.dts.
> 
> armada-98dx25xx and armada-98dx35xx does help with the naming.  So it
> probably is worth having armada-98dx35xx which simply includes armada-
> 98d25xx.
> 
> 	Andrew

Elad.

Re: [EXT] Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

> Basically, the cpu-subsystems of Prestera 98DX25xx (AC5) and Prestera 98DX35xx (AC5X) are the same.

Great, thanks for the conformation.

> The DTSI Chris proposed had the PCIe portion removed.

> We have PCIe support for AC5/AC5X so we would obviously like to
> include this portion in both the DTSI and as a patch to the Armada8K
> PCIe driver.

So you can add the needed node to the .dtsi as part of the patch to
the pci-aardvark.c driver. That sounds O.K.

    Andrew

Re: [EXT] Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Chris Packham]

On 13/05/22 00:47, Andrew Lunn wrote:
>> Basically, the cpu-subsystems of Prestera 98DX25xx (AC5) and Prestera 98DX35xx (AC5X) are the same.
> Great, thanks for the conformation.
>
>> The DTSI Chris proposed had the PCIe portion removed.
>> We have PCIe support for AC5/AC5X so we would obviously like to
>> include this portion in both the DTSI and as a patch to the Armada8K
>> PCIe driver.
> So you can add the needed node to the .dtsi as part of the patch to
> the pci-aardvark.c driver. That sounds O.K.

I deliberately left the pci stuff out of my submission to keep things 
simple and because I have no way of testing it. Eventually we (allied 
telesis) are planning a product variant that will need a 2nd switch 
connected via pci so I'll be revisiting the pci stuff then (unless 
someone else beats me to it).