linux-kernel - Re: [PATCH] arm64: dts: rockchip: ROCK3B: Correct clock rates for Ethernet PHYs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9a210e3d-90eb-42d4-83fd-7bbad7b00e86@9elements.com>
Date: Fri, 20 Dec 2024 18:29:42 +0100
From: Alicja Michalska <alicja.michalska@...ements.com>
To: Jonas Karlman <jonas@...boo.se>
Cc: heiko@...ech.de, linux-rockchip@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: rockchip: ROCK3B: Correct clock rates for
 Ethernet PHYs

Hello Jonas,


On 20/12/2024 18:09, Jonas Karlman wrote:
> Have you used mainline or vendor u-boot when testing?
>
> There is an existing issue with RTL8211F PHYs that they must be reset
> before Linux can identify them, see [1] for more details on the issue.
>
> Mainline U-Boot will reset the Ethernet PHYs on this board and that
> should allow for Linux to identify and use the Ethernet PHYs.
>
> [1] https://lore.kernel.org/r/47d55aca-bee6-810f-379f-9431649fefa6@kwiboo.se/

Thank you for the link. I've tested with vendor-provided U-Boot build:

U-Boot latest-2023.10-11-cc60ff40-gcc60ff40 (Aug 14 2024 - 03:56:35 +0000)

I can test with upstream in upcoming days.

> The snps,reset props is deprecated and reset- props should be used in
> the PHY node.
>
> This also changes to use 20/100 ms delay instead of current 20/50ms
> delay. If anything it could be changed to 20/80ms, I have only seen
> datasheets for rtl8211f mention minimum 10 ms and minimum 30-76 ms.
>
>> +
>> +	tx_delay = <0x36>;
>> +	rx_delay = <0x2d>;
> Are you having issue with use of rgmii-id on your board?

Interesting insight, thank you!

I have to admit that I haven't read the current documentation, just seen that other RK356X boards in the tree used it. My apologies.

I've ran into issues with rgmii-id, but that's with vendor U-Boot (as mentioned above), will try with upstream next and see a follow-up if I will still experience issues.

>> +
>>  	status = "okay";
>>  };
>>  
>>  &gmac1 {
>>  	assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
>>  	assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru CLK_MAC1_2TOP>;
>> -	clock_in_out = "input";
>> -	phy-handle = <&rgmii_phy1>;
>> -	phy-mode = "rgmii-id";
>> +	assigned-clock-rates = <0>, <125000000>;
>> +	clock_in_out = "output";
>>  	phy-supply = <&vcc_3v3>;
>> +	phy-mode = "rgmii";
>> +	phy-handle = <&rgmii_phy1>;
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&gmac1m1_miim
>>  		     &gmac1m1_tx_bus2
>>  		     &gmac1m1_rx_bus2
>>  		     &gmac1m1_rgmii_clk
>> -		     &gmac1m1_rgmii_bus
>> -		     &gmac1m1_clkinout>;
>> +		     &gmac1m1_rgmii_bus>;
>> +	snps,reset-gpio = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
>> +	snps,reset-active-low;
>> +	snps,reset-delays-us = <0 50000 150000>;
> Same here, but now this is 50/150 ms?
I wasn't sure about those delays, as I've taken them from vendor's sources. They did seem awfully high, though.
>> +
>> +	tx_delay = <0x47>;
>> +	rx_delay = <0x28>;
> Same here, is use of rgmii-id causing issues on your boards?
Answer above :)
>> +
>>  	status = "okay";
>>  };
>>  
>> +&mdio0 {
>> +	rgmii_phy0: ethernet-phy@0 {
>> +		compatible = "ethernet-phy-ieee802.3-c22";
>> +		reg = <0>;
> The phy addr used for these boards is 0x1.
>
>> +	};
>> +};
>> +
>> +&mdio1 {
>> +	rgmii_phy1: ethernet-phy@0 {
>> +		compatible = "ethernet-phy-ieee802.3-c22";
>> +		reg = <0>;
> Same here.
Makes sense, other boards in the tree had it configured the same way.
>> +	};
>> +};
> There should be no need to move these nodes, they should already be in
> alphabetical order?
There's no reason to, just thought moving them closer to PHYs would look cleaner and make a bit more sense. It's just a nitpick though.
>> +
>>  &gpu {
>>  	mali-supply = <&vdd_gpu>;
>>  	status = "okay";
>> @@ -512,26 +540,6 @@ &i2s1m0_sdi0
>>  	status = "okay";
>>  };
>>  
>> -&mdio0 {
>> -	rgmii_phy0: ethernet-phy@1 {
>> -		compatible = "ethernet-phy-ieee802.3-c22";
> If you want to use vendor u-boot you can probably change this to
> "ethernet-phy-id001c.c916".
>
>> -		reg = <1>;
>> -		reset-assert-us = <20000>;
>> -		reset-deassert-us = <50000>;
>> -		reset-gpios = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
>> -	};
>> -};
>> -
>> -&mdio1 {
>> -	rgmii_phy1: ethernet-phy@1 {
>> -		compatible = "ethernet-phy-ieee802.3-c22";
> Same here.

True, but in this case it likely wouldn't be accepted in upstream, right?

I really would prefer devicetrees to be passed to the OS by bootloader, just like ACPI does on x86 and some (i.e Ampere) platforms.

This way we wouldn't need to maintain per-board DTs in the tree, and would keep functionality between kernel versions/BSDs... but one can dream, huh? :)

Thank you,

Alicja