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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/05/2022 01:49, Chris Packham wrote:
> 
>>> +		spi_clock: spi-clock {
>>> +			compatible = "fixed-clock";
>>> +			#clock-cells = <0>;
>>> +			clock-frequency = <200000000>;
>>> +		};
>> My questions about these clocks are still unanswered. Why do you define
>> fixed-clocks. Aren't these part of clock controller?
> 
> Not one that I have any information on. Marvell don't put it in their 
> customer facing documentation so I can only speculate. The 25MHz 
> oscillator goes into the chip, from there I guess that it is fed in some 
> fashion to both the CPU block (CnM in Marvell speak) and to the switch 
> block. Where exactly it gets divided into these individual peripheral 
> clocks I don't really know.

Hm, so it seems you do not have a proper clock-controller (or cannot
create one). OK then, but these are silly stubs, you know. :)

> 
>>> +	};
>>> +};
>>> 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..7ac87413e023
>>> --- /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";
>>  From the bindings I understood ac5x is a SoC, not board. If ac5x is a
>> board, not a SoC, then compatible should be rather "marvell,rd-ac5x".
> 
> So If I understand the convention the full compatible would be:
> 
> compatible = "marvell,rd-ac5x", "marvell,ac5x", "marvell,ac5";

Yes, this looks correct.

> 
>>
>>> +
>>> +	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: flash@0 {
>>> +		compatible = "jedec,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";
>> Why compatible is defined per board?
> 
> That came from the Marvell SDK. On some boards this is used as a 
> device/OTG interface. But regardless it should have one in the SoC dtsi. 

Yes, please.

> As for why they used the "chipidea,usb2" compatible I have no idea. I'll 
> remove this and add the correct compatible to the SoC.

They could reuse some other block. Pretty often for such cases there is
a dedicated compatible and fallback, e.g.:
Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml

Best regards,
Krzysztof