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

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

 



Hello,

On Mon, Mar 04, 2024 at 05:43:08PM +0100, Diederik de Haas wrote:
> On Monday, 4 March 2024 16:59:38 CET Andrew Lunn wrote:
> > On Mon, Mar 04, 2024 at 04:32:03PM +0100, Diederik de Haas wrote:
> > > On Monday, 4 March 2024 14:09:15 CET Andrew Lunn wrote:
> > > > > > Andrew already pointed out when I posted the patch introducing the
> > > > > > gmac0 node that rgmii-id would be the preferred way to setup things.
> > > > > > Back then this didn't happen because this change broke reception of
> > > > > > network packets. However this only happend because I didn't have the
> > > > > > right phy driver loaded.
> > > > > 
> > > > > It could be that the PHY is strapped to not use its internal RX delay.
> > > > > And the PHY has some weird default TX delay, so having the driver
> > > > > put some sensible values in is probably better.
> > > > 
> > > > It could also be the bootloader putting odd values into the PHY.
> > > > 
> > > > Anyway, it will work better with the correct PHY, and enable WoL
> > > > support.
> > > 
> > > Not sure if this is the right place or way, but here we go...
> > > 
> > > A few days ago on #debian-kernel@OFTC:
> > > [28.02 16:35] <ukleinek> u-boot should be out of the game
> > > [28.02 16:36] <diederik> I'm not so sure anymore. On Quartz64 Model A and
> > > B
> > > (rk3566) I had massive packet loss and tracked it down to a change in
> > > u-boot [28.02 16:37] <ukleinek> diederik: sounds like the Linux network
> > > driver on that machine could do something better
> > > [28.02 16:38] <diederik> yeah, probably
> > > 
> > > I reported this about a month ago to Jonas Karlman as I bisected the
> > > problem> 
> > > to a change in u-boot:
> > > > diederik@bagend:~/dev/u-boot/u-boot$ git bisect bad
> > > > 25f56459aebced8e4bb7d01061dcb1b765b197e2 is the first bad commit
> > > > commit 25f56459aebced8e4bb7d01061dcb1b765b197e2
> > > > Author: Jonas Karlman <jonas@xxxxxxxxx>
> > > > Date:   Sun Oct 1 19:17:21 2023 +0000
> > > > 
> > > >     configs: rockchip: Enable ethernet driver on RK356x boards
> > > >     
> > > >     Enable DWC_ETH_QOS_ROCKCHIP and related PHY driver on RK356x boards
> > > >     that
> > > >     have an enabled gmac node.
> > > 
> > > I just checked and both Quartz64 Model A and B have `phy-mode = "rgmii";`
> > > and set `tx_delay` and `rx_delay` to some (other) values.
> > > Without knowing nor understanding the details, this seem very much
> > > related?
> > 
> > If you don't have a specific Linux PHY driver, you are at the mercies
> > of how the bootloader, or strapping set up the PHY. So it is always
> > best to use the correct PHY driver. 
> 
> This part is a bit over my head (that's ok, no need to explain it).
> 
> > The Linux PHY driver should assume
> > nothing and setup the hardware from scratch, removing anything odd the
> > bootloader did. However, the fall back generic PHY driver has no chip
> > specific knowledge, so it cannot undo whatever the bootloader did.
> > 
> > So, in an ideal world, we don't care about what the bootloader did,
> > just use the correct MAC and PHY driver and it should work. And if it
> > does not work, it is a Linux bug, which needs to be found and fixed.
> 
> I agree.
> 
> > > Not sure if this is the right place or way, but here we go...
> 
> 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";
 };

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature


[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