| Message ID | 20240304084612.711678-2-ukleinek@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rockchip: qnap-ts433: Simplify network PHY connection | expand |
Hi Uwe, Am Montag, 4. März 2024, 09:46:11 CET schrieb Uwe Kleine-König: > While it requires to have the right phy driver loaded (i.e. motorcomm) > to make the phy asserting the right delays, this is generally the > preferred way to define the MAC <-> PHY connection. > > Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> > --- > Hello, > > 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. trying to understand how the (not) loaded module fits into this :-) The mdio-bus is supposed to probe the phy and load the appropriate module. From your description it sounds like the correct phy module needs to be actually loaded? Or was that meant to be a "requires to have the right phy driver compiled" instead? Heiko > Best regards > Uwe > > arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts > index 6a998166003c..36ad48d46bc1 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts > @@ -20,15 +20,13 @@ &gmac0 { > assigned-clock-rates = <0>, <125000000>; > clock_in_out = "output"; > phy-handle = <&rgmii_phy0>; > - phy-mode = "rgmii"; > + phy-mode = "rgmii-id"; > pinctrl-names = "default"; > pinctrl-0 = <&gmac0_miim > &gmac0_tx_bus2 > &gmac0_rx_bus2 > &gmac0_rgmii_clk > &gmac0_rgmii_bus>; > - rx_delay = <0x2f>; > - tx_delay = <0x3c>; > status = "okay"; > }; > > > base-commit: 67908bf6954b7635d33760ff6dfc189fc26ccc89 >
Guten Morgen Heiko, On 04.03.24 10:07, Heiko Stübner wrote: > Hi Uwe, > > Am Montag, 4. März 2024, 09:46:11 CET schrieb Uwe Kleine-König: >> While it requires to have the right phy driver loaded (i.e. motorcomm) >> to make the phy asserting the right delays, this is generally the >> preferred way to define the MAC <-> PHY connection. >> >> Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> >> --- >> Hello, >> >> 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. > > trying to understand how the (not) loaded module fits into this :-) > The mdio-bus is supposed to probe the phy and load the appropriate module. > > From your description it sounds like the correct phy module needs to be > actually loaded? Or was that meant to be a "requires to have the right phy > driver compiled" instead? The latter. i.e. with MOTORCOMM_PHY=n it's broken, but works fine with MOTORCOMM_PHY=y or =m. Best regards Uwe
Hi Uwe, Am Montag, 4. März 2024, 10:15:57 CET schrieb Uwe Kleine-König: > On 04.03.24 10:07, Heiko Stübner wrote: > > Am Montag, 4. März 2024, 09:46:11 CET schrieb Uwe Kleine-König: > >> While it requires to have the right phy driver loaded (i.e. motorcomm) > >> to make the phy asserting the right delays, this is generally the > >> preferred way to define the MAC <-> PHY connection. > >> > >> Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> > >> --- > >> Hello, > >> > >> 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. > > > > trying to understand how the (not) loaded module fits into this :-) > > The mdio-bus is supposed to probe the phy and load the appropriate module. > > > > From your description it sounds like the correct phy module needs to be > > actually loaded? Or was that meant to be a "requires to have the right phy > > driver compiled" instead? > > The latter. i.e. with MOTORCOMM_PHY=n it's broken, but works fine with > MOTORCOMM_PHY=y or =m. ah great, then it's really fine. If it's ok with you I'll change the "loaded" to "available" then (for compiled in or as module) Heiko
On Mon, Mar 4, 2024 at 4:47 PM Uwe Kleine-König <ukleinek@debian.org> wrote: > > While it requires to have the right phy driver loaded (i.e. motorcomm) > to make the phy asserting the right delays, this is generally the > preferred way to define the MAC <-> PHY connection. > > Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> > --- > Hello, > > 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. ChenYu > Best regards > Uwe > > arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts > index 6a998166003c..36ad48d46bc1 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts > @@ -20,15 +20,13 @@ &gmac0 { > assigned-clock-rates = <0>, <125000000>; > clock_in_out = "output"; > phy-handle = <&rgmii_phy0>; > - phy-mode = "rgmii"; > + phy-mode = "rgmii-id"; > pinctrl-names = "default"; > pinctrl-0 = <&gmac0_miim > &gmac0_tx_bus2 > &gmac0_rx_bus2 > &gmac0_rgmii_clk > &gmac0_rgmii_bus>; > - rx_delay = <0x2f>; > - tx_delay = <0x3c>; > status = "okay"; > }; > > > base-commit: 67908bf6954b7635d33760ff6dfc189fc26ccc89 > -- > 2.43.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Mar 04, 2024 at 06:07:54PM +0800, Chen-Yu Tsai wrote: > On Mon, Mar 4, 2024 at 4:47 PM Uwe Kleine-König <ukleinek@debian.org> wrote: > > > > While it requires to have the right phy driver loaded (i.e. motorcomm) > > to make the phy asserting the right delays, this is generally the > > preferred way to define the MAC <-> PHY connection. > > > > Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> > > --- > > Hello, > > > > 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. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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@kwiboo.se> > 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? Cheers, Diederik
On 2024-03-04 16:32, 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@kwiboo.se> >> 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? Sorry for not getting back to you sooner, but yes I would check with phy-mode = "rgmii-id". There is also a possible issue with rk gmac driver in both U-Boot and linux, it always set enable tx/rx delay bit. Please, try with following in U-Boot: diff --git a/drivers/net/dwc_eth_qos_rockchip.c b/drivers/net/dwc_eth_qos_rockchip.c index fa9e513faea3..e5c320c36741 100644 --- a/drivers/net/dwc_eth_qos_rockchip.c +++ b/drivers/net/dwc_eth_qos_rockchip.c @@ -73,7 +73,7 @@ static int rk3568_set_to_rgmii(struct udevice *dev, { struct eth_pdata *pdata = dev_get_plat(dev); struct rockchip_platform_data *data = pdata->priv_pdata; - u32 con0, con1; + u32 con0, con1, val; con0 = (data->id == 1) ? RK3568_GRF_GMAC1_CON0 : RK3568_GRF_GMAC0_CON0; @@ -84,10 +84,12 @@ static int rk3568_set_to_rgmii(struct udevice *dev, RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) | RK3568_GMAC_CLK_TX_DL_CFG(tx_delay)); - regmap_write(data->grf, con1, - RK3568_GMAC_PHY_INTF_SEL_RGMII | - RK3568_GMAC_RXCLK_DLY_ENABLE | - RK3568_GMAC_TXCLK_DLY_ENABLE); + val = RK3568_GMAC_PHY_INTF_SEL_RGMII; + if (rx_delay > 0) + val |= RK3568_GMAC_RXCLK_DLY_ENABLE; + if (tx_delay > 0) + val |= RK3568_GMAC_TXCLK_DLY_ENABLE; + regmap_write(data->grf, con1, val); return 0; } Also check with the series "phy: rockchip-inno-usb2: Write to correct GRF" [1]. Trying to configure bits intended for USBGRF in GRF is probably not that good :-) [1] https://patchwork.ozlabs.org/cover/1903987/ Regards, Jonas > > Cheers, > Diederik
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@kwiboo.se> > > 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. 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. Andrew
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@kwiboo.se> > > > 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. Cheers, Diederik
On Monday, 4 March 2024 16:46:59 CET Jonas Karlman wrote: > Sorry for not getting back to you sooner, but yes I would check with > phy-mode = "rgmii-id". There is also a possible issue with rk gmac > driver in both U-Boot and linux, it always set enable tx/rx delay bit. > > Please, try with following in U-Boot: No worries :) Will try your suggestions soon, but it's probably better to take that discussion somewhere else to not further clutter up this patch/thread. Cheers, Diederik
Hello, [I already replied to this mail earlier today, but I just noticed I only did that to Heiko. So here comes the mail again.] On 04.03.24 10:24, Heiko Stübner wrote: > Am Montag, 4. März 2024, 10:15:57 CET schrieb Uwe Kleine-König: >> On 04.03.24 10:07, Heiko Stübner wrote: >>> Am Montag, 4. März 2024, 09:46:11 CET schrieb Uwe Kleine-König: >>>> While it requires to have the right phy driver loaded (i.e. motorcomm) >>>> to make the phy asserting the right delays, this is generally the >>>> preferred way to define the MAC <-> PHY connection. >>>> >>>> Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> >>>> --- >>>> Hello, >>>> >>>> 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. >>> >>> trying to understand how the (not) loaded module fits into this :-) >>> The mdio-bus is supposed to probe the phy and load the appropriate module. >>> >>> From your description it sounds like the correct phy module needs to be >>> actually loaded? Or was that meant to be a "requires to have the right phy >>> driver compiled" instead? >> >> The latter. i.e. with MOTORCOMM_PHY=n it's broken, but works fine with >> MOTORCOMM_PHY=y or =m. > > ah great, then it's really fine. If it's ok with you I'll change the "loaded" > to "available" then (for compiled in or as module) ack! Best regards Uwe
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@kwiboo.se> > > > > 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
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. Cheers, Diederik
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.
On Mon, 4 Mar 2024 09:46:11 +0100, Uwe Kleine-König wrote: > While it requires to have the right phy driver loaded (i.e. motorcomm) > to make the phy asserting the right delays, this is generally the > preferred way to define the MAC <-> PHY connection. > > Applied, thanks! [1/1] arm64: dts: rockchip: qnap-ts433: Simplify network PHY connection commit: e8d45544f806f3b55c30345de84262cbb9504902 Works nicely on my TS433 :-) Best regards,
Am Montag, 4. März 2024, 17:47:22 CEST schrieb Diederik de Haas: > On Monday, 4 March 2024 16:46:59 CET Jonas Karlman wrote: > > Sorry for not getting back to you sooner, but yes I would check with > > phy-mode = "rgmii-id". There is also a possible issue with rk gmac > > driver in both U-Boot and linux, it always set enable tx/rx delay bit. > > > > Please, try with following in U-Boot: > > No worries :) > Will try your suggestions soon, but it's probably better to take that > discussion somewhere else to not further clutter up this patch/thread. did you get somewhere with your testing? Just as a datapoint, I also had a lot of "fun" with the gmac0 on my qnap ts433 today. Cable connection was detected but dhcp did not get an IP address. In the end I realized that the vccio4 supply was 1.8V but the pmu-io-domains were not enabled, so the soc thought it got 3.3V After enabling pmu-io-domains [0], I now get networking on that port. Heiko [0] https://lore.kernel.org/linux-rockchip/20240805162052.3345768-1-heiko@sntech.de/T/#u
On Monday, 5 August 2024 18:33:52 CEST Heiko Stübner wrote: > Am Montag, 4. März 2024, 17:47:22 CEST schrieb Diederik de Haas: > > On Monday, 4 March 2024 16:46:59 CET Jonas Karlman wrote: > > > Sorry for not getting back to you sooner, but yes I would check with > > > phy-mode = "rgmii-id". There is also a possible issue with rk gmac > > > driver in both U-Boot and linux, it always set enable tx/rx delay bit. > > > > > Please, try with following in U-Boot: > > No worries :) > > Will try your suggestions soon, but it's probably better to take that > > discussion somewhere else to not further clutter up this patch/thread. > > did you get somewhere with your testing? Yeah, none of the patches made a difference: On zaterdag 9 maart 2024 15:51:50 CEST Diederik de Haas wrote: > I did the following tests based on 2024.01 on my Q64-B: > - conditional delay: FAIL > - grf fix: FAIL > - conditional delay + grf fix: FAIL > > Then I rebased upon 2024.04-rc4: > - conditional delay + grf fix: FAIL > I skipped the other variations, but could do that if that could be useful. After the 2024.07 release I checked again and it still failed and my workaround (partial revert) still worked, so I reported it to the u-boot ML: https://lore.kernel.org/u-boot/2086393.9F9pDXStbY@bagend/
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts index 6a998166003c..36ad48d46bc1 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts @@ -20,15 +20,13 @@ &gmac0 { assigned-clock-rates = <0>, <125000000>; clock_in_out = "output"; phy-handle = <&rgmii_phy0>; - phy-mode = "rgmii"; + phy-mode = "rgmii-id"; pinctrl-names = "default"; pinctrl-0 = <&gmac0_miim &gmac0_tx_bus2 &gmac0_rx_bus2 &gmac0_rgmii_clk &gmac0_rgmii_bus>; - rx_delay = <0x2f>; - tx_delay = <0x3c>; status = "okay"; };
While it requires to have the right phy driver loaded (i.e. motorcomm) to make the phy asserting the right delays, this is generally the preferred way to define the MAC <-> PHY connection. Signed-off-by: Uwe Kleine-König <ukleinek@debian.org> --- Hello, 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. Best regards Uwe arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) base-commit: 67908bf6954b7635d33760ff6dfc189fc26ccc89