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

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

[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.

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-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Heiko Stübner]

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
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Uwe Kleine-König]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Heiko Stübner]

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



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Uwe Kleine-König]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Chen-Yu Tsai]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Andrew Lunn]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Diederik de Haas]

[-- Attachment #1.1: Type: text/plain, Size: 1979 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Jonas Karlman]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Diederik de Haas]

[-- Attachment #1.1: Type: text/plain, Size: 490 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Heiko Stübner]

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




_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Diederik de Haas]

[-- Attachment #1.1: Type: text/plain, Size: 1316 bytes --]

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/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Andrew Lunn]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Diederik de Haas]

[-- Attachment #1.1: Type: text/plain, Size: 3372 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 5644 bytes --]

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

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Diederik de Haas]

[-- Attachment #1.1: Type: text/plain, Size: 3119 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [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.


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Heiko Stuebner]

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,
-- 
Heiko Stuebner <heiko@sntech.de>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip