Re: [PATCH v1 27/30] RISC-V: Add initial StarFive JH7110 device tree

[Krzysztof Kozlowski]

On 01/10/2022 12:52, Conor Dooley wrote:
> On Fri, Sep 30, 2022 at 03:49:14PM +0800, Hal Feng wrote:
>> From: Emil Renner Berthing <kernel@xxxxxxxx>
>>
>> Add initial device tree for the JH7110 RISC-V SoC by
>> StarFive Technology Ltd.
>>
>> Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx>
>> Signed-off-by: Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx>
> 
> There's little point reviewing this dt since there's a load of issues
> that you can trivially find by running dtbs_check/dt_binding_check, but

Yep...

> this SoB change is wrong - if Emil wrote the patch, then Jianlong's SoB
> is either redundant or should be accompanied by a Co-developed-by tag.

Depends. Jianlong might have just rebased the patch.

> 
> Ditto for patch 28/30 "RISC-V: Add StarFive JH7110 VisionFive2 board
> device tree".
> 
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 449 +++++++++++++++++++++++
>>  1 file changed, 449 insertions(+)
>>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> new file mode 100644
>> index 000000000000..46f418d4198a
>> --- /dev/null
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> 
>> +
>> +	osc: osc {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	clk_rtc: clk_rtc {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	gmac0_rmii_refin: gmac0_rmii_refin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <50000000>;
> 
> I assume, given osc has it's frequency set in the board dts, that these
> are all oscillators on the SoC?
> 
>> +	};
>> +
>> +	gmac0_rgmii_rxin: gmac0_rgmii_rxin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <125000000>;
>> +	};
>> +
>> +	gmac1_rmii_refin: gmac1_rmii_refin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <50000000>;
>> +	};
>> +
>> +	gmac1_rgmii_rxin: gmac1_rgmii_rxin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <125000000>;
>> +	};
>> +
>> +	i2stx_bclk_ext: i2stx_bclk_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <12288000>;
>> +	};
>> +
>> +	i2stx_lrck_ext: i2stx_lrck_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <192000>;
>> +	};
>> +
>> +	i2srx_bclk_ext: i2srx_bclk_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <12288000>;
>> +	};
>> +
>> +	i2srx_lrck_ext: i2srx_lrck_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <192000>;
>> +	};
>> +
>> +	tdm_ext: tdm_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <49152000>;
>> +	};
>> +
>> +	mclk_ext: mclk_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <49152000>;
>> +	};
> 
>> +		syscrg: syscrg@13020000 {
> 
> The generic node name for syscons is just "syscon" afaik.

Yes.

> 
>> +			compatible = "syscon", "simple-mfd";

And this is not allowed. Needs specific compatible.


>> +			reg = <0x0 0x13020000 0x0 0x10000>;
>> +
> 
>> +		aoncrg: aoncrg@17000000 {
> 
> Again, syscon as the node name?

Yes.

> 
>> +			compatible = "syscon", "simple-mfd";

And this is a NAK.

>> +			reg = <0x0 0x17000000 0x0 0x10000>;
>> +
>> +		gpio: gpio@13040000 {
> 
> Someone else (Krzysztof maybe?) should comment, but is "pinctrl" not the
> genric node name for pinctrl nodes?

Yes, for pin controller nodes, this should be "pinctrl" and schema
requires it. The problem was that his driver did not use generic pinctrl
bindings, which is no-go on its own.

This could be a gpio controller (so "gpio" would be fine), although
compatible suggests otherwise.


Best regards,
Krzysztof