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

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On 11/03/22 03:26, Andrew Lunn wrote:
> 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.

I thought that might be the case. I'll be limited in what I can test on 
the reference board I have. I'll work to bring in whatever bindings and 
drivers I can test and probably remove anything that I can't.

I might be able to do another round of patches when we get our boards.

>
>> +	mvDma {
>> +		compatible = "marvell,mv_dma";
>> +		memory-region = <&prestera_rsvd>;
>> +		status = "okay";
>> +	};
> Is this different to the existing Marvell XOR engine?

Yes it has something to do with the DMA memory for the integrated L3 
switch. Because that driver doesn't exist I'll probably remove this node 
(and the other prestera node below) in v2.


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

Looks like an external 88E1512 PHY so I'll move that to the board dts.

>
>> +				sdhci0: sdhci@805c0000 {
>> +					compatible = "marvell,ac5-sdhci", "marvell,armada-ap806-sdhci";
> This additional compatible should be added to the existing binding
> document.
I'll see what differences there are with the ap806-sdhci. I might be 
able to remove it.
>
>> +			eth0: ethernet@20000 {
>> +				compatible = "marvell,armada-ac5-neta";
> So it is not compatible with plain nata?

There is some odd muxing setup where the serdes are either SGMII or PCIe 
or can even be connected to the internal switch. Whether the Ethernet 
driver needs to care about it I'm not sure.

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

I think the reference code might be trying to connect this to the 
internal switch. I'll remove the fixed-link portion.

>> +			/* USB0 is a host USB */
>> +			usb0: usb@80000 {
>> +				compatible = "marvell,ac5-ehci", "marvell,orion-ehci";
> Please add this compatible to the binding.
Will do (or delete).
>
>> +		pcie0: pcie@800a0000 {
>> +			compatible = "marvell,ac5-pcie", "snps,dw-pcie";
> Please add this ...
Will do (or delete).
>
>> +			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.
Will move.
>> +		nand: nand@805b0000 {
>> +			compatible = "marvell,ac5-nand-controller";
> The current NAND driver does not work?

This is one of the things I can't test on the board I have (uses eMMC 
instead of NAND). Should I put "marvell,armada-8k-nand-controller" in as 
a placeholder or leave the node out entirely?

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

On other SoCs I did put in specific prestera compatibles with 
documentation. I've even got out of tree code that uses those 
compatibles although Marvell's SDK hasn't caught up and is still using 
of_find_note_by_path("/soc/prestera").

>> +		watchdog@80216000 {
>> +			compatible = "marvell,ac5-wd";
> Not compatible with the existing watchdog driver?
Will check and either add binding or use a different compatible.
>
>      Andrew