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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx/

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.





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux