Re: [PATCH 2/6] arm64: dts: rockchip: Fix the 1Ghz ethernet on Qnap TS433

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

Am Donnerstag, 25. September 2025, 19:11:29 Mitteleuropäische Normalzeit schrieb Andrew Lunn:
> On Thu, Sep 25, 2025 at 06:58:06PM +0200, Jonas Karlman wrote:
> > Hi Heiko,
> > 
> > On 9/25/2025 11:29 AM, Heiko Stuebner wrote:
> > > While I want to remember that the dwmac on the TS433 was working at some
> > > point, it seems I had my network always connected to the 2.5G nic after
> > > that "point". And testing now revealed that the gmac does not actually
> > > manages to transfer data.
> > > 
> > > Currently the gmac is set to rgmii-id with no rx-/tx-delay values set
> > > which makes the driver use default values. Setting the delays to 0
> > > also does not provide a working interface.
> > > 
> > > The vendor kernel is running with phy-mode set to rgmii and delays of
> > >     tx_delay = 0x3c, rx_delay = 0x2f
> > > 
> > > As Andrew points out often, those delay values "are magic" and rgmii-id
> > > should definitly be used "with small values" for delays, if really needed.
> > > 
> > > The Rockchip vendor-kernel actually contains additional code in the dwmac
> > > driver to use the loopback function of a phy to find a window of usable
> > > delay values. Code can be found for example on [0] and the process is
> > > described in a document called "Rockchip GMAC RGMII Delayline Guide"
> > > which has made its way onto the internet in a lot of places [1].
> > > 
> > > So I used this process, with the interface set to rgmii-id to get values
> > > for this mode, which are in face lower than the ones for rgmii with
> > >     tx_delay = 0x21, rx_delay = 0x15
> > > and results in a working interface on the dwmac.
> > > 
> > > [0] https://github.com/armbian/linux-rockchip/blob/rk-6.1-rkr6.1/drivers/net/ethernet/stmicro/stmmac/dwmac-rk-tool.c
> > > [1] https://gitlab.com/firefly-linux/docs/-/blob/rk356x/firefly/Common/GMAC/Rockchip_Developer_Guide_Linux_GMAC_RGMII_Delayline_EN.pdf
> > > 
> > > Cc: Andrew Lunn <andrew@lunn.ch>
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > >  arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> > > index 5656554ca284..e8af92a011d6 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> > > @@ -257,6 +257,8 @@ &gmac0_tx_bus2
> > >  		     &gmac0_rx_bus2
> > >  		     &gmac0_rgmii_clk
> > >  		     &gmac0_rgmii_bus>;
> > > +	rx_delay = <0x15>;
> > > +	tx_delay = <0x21>;
> > 
> > I do not understand why defining rx/tx_delay would change anything.
> 
> I was also wondering why these two properties are added but no change
> to phy-mode.
> 
> > 
> > Setting these should currently not have any effect on the driver code
> > when phy-mode=rgmii-id is used, see below (next-20250924, dwmac-rk.c):
> > 
> > 
> > 	switch (bsp_priv->phy_iface) {
> > 	case PHY_INTERFACE_MODE_RGMII:
> > 		dev_info(dev, "init for RGMII\n");
> > 		bsp_priv->ops->set_to_rgmii(bsp_priv, bsp_priv->tx_delay,
> > 					    bsp_priv->rx_delay);
> > 		break;
> > 	case PHY_INTERFACE_MODE_RGMII_ID:
> > 		dev_info(dev, "init for RGMII_ID\n");
> > 		bsp_priv->ops->set_to_rgmii(bsp_priv, 0, 0);
> > 		break;
> 
> And this explains it, thanks.
> 
> > I have played around with a few patches that changes this and apply the
> > rx/tx_delay for rgmii-id modes (both Linux and U-Boot), see top of [2]
> > for Linux patches. Will try to get them on ML in a few days.
> 
> We should not really be expanding the use of rx_delay and
> tx_delay. The standardized properties {tx|rx}-internal-delay-ps should
> be used.

as Jonas wrote the delays do nothing for rgmii-id. So the whole thing
must have been me doing something wonky, because on re-testing
everything, it is as expected - no delays necessary and ethernet just
works.

So I'll drop this patch altogether and adapt the TS233 one.

Heiko

Heiko