[PATCH] arm64: dts: rockchip: increase gmac rx_delay to 0x11 on rk3399-puma

[PATCH] arm64: dts: rockchip: increase gmac rx_delay to 0x11 on rk3399-puma

[Jakob Unterwurzacher]

During mass manufacturing, we noticed the mmc_rx_crc_error counter,
as reported by "ethtool -S eth0 | grep mmc_rx_crc_error" to increase
above zero during nuttcp speedtests.

Cycling through the rx_delay range on two boards shows that is a large
"good" region from 0x11 to 0x35 (see below for details).

This commit increases rx_delay to 0x11, which is the smallest
possible change that fixes the issue we are seeing on the KSZ9031 PHY.
This also matches what most other rk3399 boards do.

Tests for Puma PCBA S/N TT0069903:

	rx_delay mmc_rx_crc_error
	-------- ----------------
	0x09 (dhcp broken)
	0x10 897
	0x11 0
	0x20 0
	0x30 0
	0x35 0
	0x3a 745
	0x3b 11375
	0x3c 36680
	0x40 (dhcp broken)
	0x7f (dhcp broken)

Tests for Puma PCBA S/N TT0157733:

	rx_delay mmc_rx_crc_error
	-------- ----------------
	0x10 59
	0x11 0
	0x35 0

Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
index 9efcdce0f593..13d0c511046b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
@@ -181,7 +181,7 @@ &gmac {
 	snps,reset-active-low;
 	snps,reset-delays-us = <0 10000 50000>;
 	tx_delay = <0x10>;
-	rx_delay = <0x10>;
+	rx_delay = <0x11>;
 	status = "okay";
 };
 
-- 
2.39.5

Re: [PATCH] arm64: dts: rockchip: increase gmac rx_delay to 0x11 on rk3399-puma

[Quentin Schulz]

Hi Jakob,

On 12/2/24 10:04 AM, Jakob Unterwurzacher wrote:
> During mass manufacturing, we noticed the mmc_rx_crc_error counter,
> as reported by "ethtool -S eth0 | grep mmc_rx_crc_error" to increase
> above zero during nuttcp speedtests.
> 
> Cycling through the rx_delay range on two boards shows that is a large
> "good" region from 0x11 to 0x35 (see below for details).
> 

Is this missing a "there" after that? "that there is a large good region"?

> This commit increases rx_delay to 0x11, which is the smallest
> possible change that fixes the issue we are seeing on the KSZ9031 PHY.
> This also matches what most other rk3399 boards do.
> 
> Tests for Puma PCBA S/N TT0069903:
> 
> 	rx_delay mmc_rx_crc_error
> 	-------- ----------------
> 	0x09 (dhcp broken)
> 	0x10 897
> 	0x11 0
> 	0x20 0
> 	0x30 0
> 	0x35 0
> 	0x3a 745
> 	0x3b 11375
> 	0x3c 36680
> 	0x40 (dhcp broken)
> 	0x7f (dhcp broken)
> 
> Tests for Puma PCBA S/N TT0157733:
> 
> 	rx_delay mmc_rx_crc_error
> 	-------- ----------------
> 	0x10 59
> 	0x11 0
> 	0x35 0
> 
> Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>

This would be a candidate for backporting I believe.

Therefore, a

Cc: <stable@vger.kernel.org>

here would have been nice (in the commit log), c.f. 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch

> ---
>   arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> index 9efcdce0f593..13d0c511046b 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> @@ -181,7 +181,7 @@ &gmac {
>   	snps,reset-active-low;
>   	snps,reset-delays-us = <0 10000 50000>;
>   	tx_delay = <0x10>;
> -	rx_delay = <0x10>;
> +	rx_delay = <0x11>;

While at it, we could reorder this alphabetically and move rx_delay 
between pinctrl-0 and snps,reset-gpio? c.f. 
https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node 
rx_delay and tx_delay seem to be vendor-specific but without the vendor 
prefix, but so is snps,reset-gpio so that should be fine to reorder this 
way.

Considering we have an option for KSZ9031 on RK3588 Jaguar and RK3588 
Tiger and the "same" MAC IP is used and that we use the same TXD and RXD 
delay than on RK3399 Puma right now, I guess we would want to check 
those don't need a change as well? (funnily enough, all RK3588-based 
boards in 6.12 actually have 0x00 for rx_delay and 0x43/0x44 for 
tx_delay, except ours which are at 0x10). Not a blocker for this patch 
though, so:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH] arm64: dts: rockchip: increase gmac rx_delay to 0x11 on rk3399-puma

[Heiko Stübner]

Am Montag, 2. Dezember 2024, 10:52:06 CET schrieb Quentin Schulz:
> Hi Jakob,
> 
> On 12/2/24 10:04 AM, Jakob Unterwurzacher wrote:
> > During mass manufacturing, we noticed the mmc_rx_crc_error counter,
> > as reported by "ethtool -S eth0 | grep mmc_rx_crc_error" to increase
> > above zero during nuttcp speedtests.
> > 
> > Cycling through the rx_delay range on two boards shows that is a large
> > "good" region from 0x11 to 0x35 (see below for details).
> > 
> 
> Is this missing a "there" after that? "that there is a large good region"?
> 
> > This commit increases rx_delay to 0x11, which is the smallest
> > possible change that fixes the issue we are seeing on the KSZ9031 PHY.
> > This also matches what most other rk3399 boards do.
> > 
> > Tests for Puma PCBA S/N TT0069903:
> > 
> > 	rx_delay mmc_rx_crc_error
> > 	-------- ----------------
> > 	0x09 (dhcp broken)
> > 	0x10 897
> > 	0x11 0
> > 	0x20 0
> > 	0x30 0
> > 	0x35 0
> > 	0x3a 745
> > 	0x3b 11375
> > 	0x3c 36680
> > 	0x40 (dhcp broken)
> > 	0x7f (dhcp broken)
> > 
> > Tests for Puma PCBA S/N TT0157733:
> > 
> > 	rx_delay mmc_rx_crc_error
> > 	-------- ----------------
> > 	0x10 59
> > 	0x11 0
> > 	0x35 0
> > 
> > Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>
> 
> This would be a candidate for backporting I believe.
> 
> Therefore, a

also please include a

Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) SoM")

> Cc: <stable@vger.kernel.org>
> 
> here would have been nice (in the commit log), c.f. 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch
> 
> > ---
> >   arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> > index 9efcdce0f593..13d0c511046b 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> > @@ -181,7 +181,7 @@ &gmac {
> >   	snps,reset-active-low;
> >   	snps,reset-delays-us = <0 10000 50000>;
> >   	tx_delay = <0x10>;
> > -	rx_delay = <0x10>;
> > +	rx_delay = <0x11>;
> 
> While at it, we could reorder this alphabetically and move rx_delay 

I would disagree. This is a "fix", so should ideally only do the minimal
changes to make life of the stable people easier.

Doing this one-line change is way easier to understand than stuff also
moving around.

Heiko

> between pinctrl-0 and snps,reset-gpio? c.f. 
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node 
> rx_delay and tx_delay seem to be vendor-specific but without the vendor 
> prefix, but so is snps,reset-gpio so that should be fine to reorder this 
> way.
> 
> Considering we have an option for KSZ9031 on RK3588 Jaguar and RK3588 
> Tiger and the "same" MAC IP is used and that we use the same TXD and RXD 
> delay than on RK3399 Puma right now, I guess we would want to check 
> those don't need a change as well? (funnily enough, all RK3588-based 
> boards in 6.12 actually have 0x00 for rx_delay and 0x43/0x44 for 
> tx_delay, except ours which are at 0x10). Not a blocker for this patch 
> though, so:
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin
> 

Re: [PATCH] arm64: dts: rockchip: increase gmac rx_delay to 0x11 on rk3399-puma

[Peter Geis]

On Mon, Dec 2, 2024 at 4:58 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Montag, 2. Dezember 2024, 10:52:06 CET schrieb Quentin Schulz:
> > Hi Jakob,
> >
> > On 12/2/24 10:04 AM, Jakob Unterwurzacher wrote:
> > > During mass manufacturing, we noticed the mmc_rx_crc_error counter,
> > > as reported by "ethtool -S eth0 | grep mmc_rx_crc_error" to increase
> > > above zero during nuttcp speedtests.
> > >
> > > Cycling through the rx_delay range on two boards shows that is a large
> > > "good" region from 0x11 to 0x35 (see below for details).
> > >
> >
> > Is this missing a "there" after that? "that there is a large good region"?

That large good region is actually an eye that you are aligning to the
clock signal. The board is on the tail end of the eye where it is
barely working. This value is supposed to be tuned to be in the middle
of that eye. You may want to test the old boards against the new
boards, because if the original board was tuned correctly something
may have changed in hardware that caused a significant shift in the
eye location. Examples of this would be changing to a new phy,
enabling phy delays, or changes in the trace length. If this is the
case, you'll probably want to make a new variant of the dts to cover
this.

> >
> > > This commit increases rx_delay to 0x11, which is the smallest
> > > possible change that fixes the issue we are seeing on the KSZ9031 PHY.
> > > This also matches what most other rk3399 boards do.
> > >
> > > Tests for Puma PCBA S/N TT0069903:
> > >
> > >     rx_delay mmc_rx_crc_error
> > >     -------- ----------------
> > >     0x09 (dhcp broken)
> > >     0x10 897
> > >     0x11 0
> > >     0x20 0
> > >     0x30 0
> > >     0x35 0
> > >     0x3a 745
> > >     0x3b 11375
> > >     0x3c 36680
> > >     0x40 (dhcp broken)
> > >     0x7f (dhcp broken)
> > >
> > > Tests for Puma PCBA S/N TT0157733:
> > >
> > >     rx_delay mmc_rx_crc_error
> > >     -------- ----------------
> > >     0x10 59
> > >     0x11 0
> > >     0x35 0
> > >
> > > Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>
> >
> > This would be a candidate for backporting I believe.
> >
> > Therefore, a
>
> also please include a
>
> Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) SoM")
>
> > Cc: <stable@vger.kernel.org>
> >
> > here would have been nice (in the commit log), c.f.
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch
> >
> > > ---
> > >   arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> > > index 9efcdce0f593..13d0c511046b 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> > > @@ -181,7 +181,7 @@ &gmac {
> > >     snps,reset-active-low;
> > >     snps,reset-delays-us = <0 10000 50000>;
> > >     tx_delay = <0x10>;
> > > -   rx_delay = <0x10>;
> > > +   rx_delay = <0x11>;
> >
> > While at it, we could reorder this alphabetically and move rx_delay
>
> I would disagree. This is a "fix", so should ideally only do the minimal
> changes to make life of the stable people easier.
>
> Doing this one-line change is way easier to understand than stuff also
> moving around.
>
> Heiko
>
> > between pinctrl-0 and snps,reset-gpio? c.f.
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node
> > rx_delay and tx_delay seem to be vendor-specific but without the vendor
> > prefix, but so is snps,reset-gpio so that should be fine to reorder this
> > way.
> >
> > Considering we have an option for KSZ9031 on RK3588 Jaguar and RK3588
> > Tiger and the "same" MAC IP is used and that we use the same TXD and RXD
> > delay than on RK3399 Puma right now, I guess we would want to check
> > those don't need a change as well? (funnily enough, all RK3588-based
> > boards in 6.12 actually have 0x00 for rx_delay and 0x43/0x44 for
> > tx_delay, except ours which are at 0x10). Not a blocker for this patch
> > though, so:
> >
> > Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> >
> > Thanks!
> > Quentin
> >
>
>
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] arm64: dts: rockchip: increase gmac rx_delay to 0x11 on rk3399-puma

[Jakob Unterwurzacher]

On Thu, Dec 5, 2024 at 2:33 AM Peter Geis <pgwipeout@gmail.com> wrote:
> > > > Cycling through the rx_delay range on two boards shows that is a large
> > > > "good" region from 0x11 to 0x35 (see below for details).
> > > >
> > >
> > > Is this missing a "there" after that? "that there is a large good region"?
>
> That large good region is actually an eye that you are aligning to the
> clock signal. The board is on the tail end of the eye where it is
> barely working. This value is supposed to be tuned to be in the middle
> of that eye. You may want to test the old boards against the new
> boards, because if the original board was tuned correctly something
> may have changed in hardware that caused a significant shift in the
> eye location. Examples of this would be changing to a new phy,
> enabling phy delays, or changes in the trace length. If this is the
> case, you'll probably want to make a new variant of the dts to cover
> this.

Thanks for the comment.

Nothing should have changed on the board, and I dug out a really old
one to verify. It behaves the same.
0x10 seems to be the lower edge of things still working.

I will put rx_delay in the center of the eye with the v3 patch.

I also checked tx_delay, and we already seem to be in the middle of
the eye, so I won't touch it.

Best regards,
Jakob