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

["Heiko Stübner" <heiko@sntech.de>]

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
>