[PATCH] arm64: dts: rockchip: qnap-ts433: Simplify network PHY connection

[Dragan Simic]

Hello all,

On 2024-03-06 01:03, Diederik de Haas wrote:
> On Monday, 4 March 2024 23:44:48 CET Uwe Kleine-König wrote:
>> > That was because it's actually a bug report (wrt Quartz64 A and B), but
>> > especially your remark made all the pieces I found earlier fall into
>> > place.
>> > Therefor I 'abused' this thread/patch to report it.
>> >
>> > I'm happy to test patches, but I lack the knowledge to come up with one
>> > myself.
>> 
>> I guess that would be:
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
>> b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts index
>> 59843a7a199c..f4d1deba3110 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
>> @@ -269,7 +269,7 @@ &gmac1 {
>>         assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru
>> SCLK_GMAC1>, <&gmac1_clkin>; clock_in_out = "input";
>>         phy-supply = <&vcc_3v3>;
>> -       phy-mode = "rgmii";
>> +       phy-mode = "rgmii-id";
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&gmac1m0_miim
>>                      &gmac1m0_tx_bus2
>> @@ -281,8 +281,6 @@ &gmac1m0_clkinout
>>         snps,reset-active-low;
>>         /* Reset time is 20ms, 100ms for rtl8211f */
>>         snps,reset-delays-us = <0 20000 100000>;
>> -       tx_delay = <0x30>;
>> -       rx_delay = <0x10>;
>>         phy-handle = <&rgmii_phy1>;
>>         status = "okay";
>>  };
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts
>> b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts index
>> 2d92713be2a0..ec1351a171d4 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts
>> @@ -176,7 +176,7 @@ &gmac1 {
>>         assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru
>> SCLK_GMAC1_RGMII_SPEED>, <&cru SCLK_GMAC1>; assigned-clock-parents = 
>> <&cru
>> SCLK_GMAC1_RGMII_SPEED>, <&cru SCLK_GMAC1>, <&gmac1_clkin>; 
>> clock_in_out =
>> "input";
>> -       phy-mode = "rgmii";
>> +       phy-mode = "rgmii-id";
>>         phy-supply = <&vcc_3v3>;
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&gmac1m1_miim
>> @@ -189,8 +189,6 @@ &gmac1m1_clkinout
>>         snps,reset-active-low;
>>         /* Reset time is 20ms, 100ms for rtl8211f, also works well 
>> here */
>>         snps,reset-delays-us = <0 20000 100000>;
>> -       tx_delay = <0x4f>;
>> -       rx_delay = <0x24>;
>>         phy-handle = <&rgmii_phy1>;
>>         status = "okay";
>>  };
> 
> It turns out my research was incomplete. I already felt uneasy when I 
> realized
> that 'pgwipeout' had set it to rgmii and while I wasn't able to track 
> the
> conversation down, I did have a vague recollection of there being a 
> discussion
> wrt rgmii vs rgmii-id. IOW: he must have set it to rgmii deliberately.
> 
> And then I found this:
> https://lore.kernel.org/all/20220606163023.3677147-1-pgwipeout@gmail.com/
> 
> For Model B it was initially set to rgmii-id, but was later changed to 
> rgmii
> due to compatibility issues on the production Model B.
> I'm going to assume that it was (initially) set to rgmii on Model A for
> similar reasons.

I went through some of my old-ish notes and found the right excerpts 
from
my logs of the #quartz64 channel on the Pine64 IRC server.  Here they 
are,
for future reference, and sorry for a bit long lines:

   <megi2> the ethernet issue is resolved by phy-mode = "rgmii-id" -> 
phy-mode = "rgmii"?
   <megi2> disabling internal delays in the phy...

   <pgwipeout> I've been running rgmii-id for a while now, on several 
boards.
   <pgwipeout> The inner delays are programmed over the mii interface in 
the mac itself.
   <pgwipeout> The Motorcomm has insane default settings, rgmii mode 
zeros them (well as close to zero
               as we can get), rgmii-id sets them to the default values 
the rgmii spec calls for.
   <megi> pgwipeout: delays are hardcoded in the driver?
   <pgwipeout> Yeah, they are currently. Adjustable delays are available 
in the gmac driver and rgmii mode.

   <dsimic> @pgwipeout (re: TX and RX delays) if I got it right, 
"tx_delay" and "rx_delay" parameters
            in DT are for the GMAC itself, as described for the RK3399 on 
page 604 in
            
https://www.t-firefly.com/download/Firefly-RK3399/docs/TRM/Rockchip%20RK3399TRM%20V1.3%20Part2.pdf
   <dsimic> while the Motorcomm PHY has its own, separate delays, which 
are configured in the Motorcomm PHY driver
   <dsimic> and all that depends on the selected interface mode (RGMII, 
RGMII_ID, etc.)
   <dsimic> could you, please, tell me if my understanding is right?

   <pgwipeout> @dsimic Correct, the gmac parameters control the gmac's 
delays. RGMII spec requires the clock
               to be active for a specific period of time before the data 
is received. RGMII mode is supposed
               to be for when the correct delay is built into the 
hardware by making the data lines longer
               than the clock lines. RGMII-ID assumes all of the lines 
are exactly the same length and implements
               the delay in the MAC instead. Adjusting the delays from 
the default means neither of these are true.
   <pgwipeout> When RGMII-ID mode is active, the GMAC driver zeros out 
its internal delays, but it still complains
               if they aren't in the DT.

   <dsimic> @pgwipeout: thank you very much for the explanation;  yes, I 
saw that the "tx_delay" and "rx_delay"
            parameters in the DT are mandatory even when they end up 
unused, and there are even hardcoded defaults
            for those parameters in the MAC driver, which all looked 
strange

   <pgwipeout> More likely we just don't know the value translation for 
the Rx and Tx values and they aren't perfectly
               lining up. So we are right on the edge of the bell with 
ID, and variations in manufacturing put us on
               one side or the other.
   <pgwipeout> Either that or the rk356x gmac has different internal 
delays than previous chips. The default is based
               on the rk33 series of 0x10 but the ref boards for the rk35 
series uses 0x1f.