Re: [PATCH v1 3/4] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Andrew Lunn]

On Thu, Mar 10, 2022 at 04:00:38PM +1300, 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).
> 
> 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@xxxxxxxxxxxxxxxxxxx>
> ---
> 
> Notes:
>     This has a number of undocumented compatible strings. I've got the SDK
>     source so I'll either bring through whatever drivers are needed or look
>     at for an in-tree alternative (e.g. there is SDK code for a ac5-gpio but
>     the existing marvell,orion-gpio seems to cover what is needed if you use
>     an appropriate binding).

Hi Chris

My understand is, you don't add nodes for which there is no
driver. The driver and its binding needs to be reviewed and accepted
before users of it are added.

> +	mvDma {
> +		compatible = "marvell,mv_dma";
> +		memory-region = <&prestera_rsvd>;
> +		status = "okay";
> +	};

Is this different to the existing Marvell XOR engine?

> +			mdio: mdio@20000 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				compatible = "marvell,orion-mdio";
> +				reg = <0x22004 0x4>;
> +				clocks = <&core_clock>;
> +				phy0: ethernet-phy@0 {
> +					reg = < 0 0 >;
> +				};

This embedded PHY looks wrong. reg should be a single value.

Is the PHY internal? Generally, the PHY is put in the .dts file, but
if it is internal, that the .dtsi would be correct.

> +				sdhci0: sdhci@805c0000 {
> +					compatible = "marvell,ac5-sdhci", "marvell,armada-ap806-sdhci";

This additional compatible should be added to the existing binding
document.

> +			eth0: ethernet@20000 {
> +				compatible = "marvell,armada-ac5-neta";

So it is not compatible with plain nata?

> +				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";
> +				fixed-link {
> +					speed = <100>;
> +					full-duplex;
> +				};

Fast Ethernet? Yet SGMII?

> +			/* USB0 is a host USB */
> +			usb0: usb@80000 {
> +				compatible = "marvell,ac5-ehci", "marvell,orion-ehci";

Please add this compatible to the binding.

> +		pcie0: pcie@800a0000 {
> +			compatible = "marvell,ac5-pcie", "snps,dw-pcie";

Please add this ...

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

The partitioning of the flash i would expect to be board specific, so
belongs on the .dts file.

> +		nand: nand@805b0000 {
> +			compatible = "marvell,ac5-nand-controller";

The current NAND driver does not work?

> +		prestera {
> +			compatible = "marvell,armada-ac5-switch";
> +			interrupts = <GIC_SPI 0x23 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "okay";
> +		};

Here we have to be careful with naming. I assume Marvell in kernel
Pretera driver does not actually work on the prestera hardware? That
driver assumes you are running Marvell firmware on this SoC, and have
a host running that driver which talks to the SoC firmware?

The name perstera seems O.K, and the compatible
marvell,armada-ac5-switch makes it clear the prestera driver cannot be
used. However, until we do actually have a driver, i don't think this
node should be added.

> +		watchdog@80216000 {
> +			compatible = "marvell,ac5-wd";

Not compatible with the existing watchdog driver?

    Andrew