Message-ID: <20240513125658.GB3015543@ragnatech.se>
Date: Mon, 13 May 2024 14:56:58 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Andrew Lunn <andrew@...n.ch>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev <netdev@...r.kernel.org>,
	Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [net-next,v5] net: ethernet: rtsn: Add support for Renesas
 Ethernet-TSN

Hi Geert, Andrew,

On 2024-05-13 13:44:22 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Mon, May 13, 2024 at 12:09 PM Niklas Söderlund
> <niklas.soderlund+renesas@...natech.se> wrote:
> > On 2024-05-13 11:39:54 +0200, Geert Uytterhoeven wrote:
> > > On Thu, May 9, 2024 at 11:10 PM Niklas Söderlund
> > > <niklas.soderlund+renesas@...natech.se> wrote:
> > > > Add initial support for Renesas Ethernet-TSN End-station device of R-Car
> > > > V4H. The Ethernet End-station can connect to an Ethernet network using a
> > > > 10 Mbps, 100 Mbps, or 1 Gbps full-duplex link via MII/GMII/RMII/RGMII.
> > > > Depending on the connected PHY.
> > > >
> > > > The driver supports Rx checksum and offload and hardware timestamps.
> > > >
> > > > While full power management and suspend/resume is not yet supported the
> > > > driver enables runtime PM in order to enable the module clock. While
> > > > explicit clock management using clk_enable() would suffice for the
> > > > supported SoC, the module could be reused on SoCs where the module is
> > > > part of a power domain.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> > > > ---
> > > > * Changes since v4
> > > > - Enable GPOUT_RDM and GPOUT_TDM delays depending on phy-mode.
> > >
> > > Thanks for the update!
> > >
> > > > +static void rtsn_set_delay_mode(struct rtsn_private *priv)
> > > > +{
> > > > +       u32 val = 0;
> > > > +
> > > > +       /* The MAC is capable of applying a delay on both Rx and Tx. Each
> > > > +        * delay can either be on or off, there is no way to set its length.
> > > > +        *
> > > > +        * The exact delay applied depends on electric characteristics of the
> > > > +        * board. The datasheet describes a typical Rx delay of 1800 ps and a
> > > > +        * typical Tx delay of 2000 ps.
> > > > +        *
> > > > +        * There are boards where the RTSN device is used together with PHYs
> > > > +        * who do not support a large enough internal delays to function. These
> > > > +        * boards depends on the MAC applying these inexact delays.
> > > > +        */
> > > > +
> > > > +       /* If the phy-mode is rgmii or rgmii-rxid apply Rx delay on the MAC */
> > > > +       if (priv->iface == PHY_INTERFACE_MODE_RGMII_ID ||
> > > > +           priv->iface == PHY_INTERFACE_MODE_RGMII_RXID)
> > > > +               val |= GPOUT_RDM;
> > > > +
> > > > +       /* If the phy-mode is rgmii or rgmii-txid apply Tx delay on the MAC */
> > > > +       if (priv->iface == PHY_INTERFACE_MODE_RGMII_ID ||
> > > > +           priv->iface == PHY_INTERFACE_MODE_RGMII_TXID)
> > > > +               val |= GPOUT_TDM;
> > > > +
> > > > +       rtsn_write(priv, GPOUT, val);
> > > > +}
> > >
> > > > +static int rtsn_phy_init(struct rtsn_private *priv)
> > > > +{
> > > > +       struct device_node *np = priv->ndev->dev.parent->of_node;
> > > > +       struct phy_device *phydev;
> > > > +       struct device_node *phy;
> > > > +       phy_interface_t iface;
> > > > +
> > > > +       /* Delays, if any, are applied by the MAC. Mask RGMII mode passed to the
> > > > +        * PHY to avoid it also adding the delay.
> > > > +        */
> > > > +       switch (priv->iface) {
> > > > +       case PHY_INTERFACE_MODE_RGMII:
> > > > +       case PHY_INTERFACE_MODE_RGMII_ID:
> > > > +       case PHY_INTERFACE_MODE_RGMII_RXID:
> > > > +       case PHY_INTERFACE_MODE_RGMII_TXID:
> > > > +               iface = PHY_INTERFACE_MODE_RGMII;
> > > > +               break;
> > > > +       default:
> > > > +               iface = priv->iface;
> > > > +               break;
> > > > +       }
> > >
> > > This introduces the same issues (the "workaround" state below) we had
> > > with ravb before.
> > > 9b23203c32ee02cd ("ravb: Mask PHY mode to avoid inserting delays twice")
> > > was the workaround,
> > > a6f51f2efa742df0 ("ravb: Add support for explicit internal clock delay
> > > configuration")
> > > was the final fix.
> > >
> > > Do we really want to repeat that mistake?
> >
> > Is it the same issue?
> 
> Sort of: you end up in a state similar to between the two commits fixing
> the issue on ravb.
> 
> > The RAVB issue is around PHY drivers adjusting
> > delays based on [rt]xc-skew-ps properties. The RTSN bindings only deal
> > with {rx,tx}-internal-delay-ps properties.
> >
> > After a discussion with Andrew my understanding is that the PHY shall
> > not attempt to add any delays from {rx,tx}-internal-delay-ps properties
> > if the phy-mode used in of_phy_connect() is PHY_INTERFACE_MODE_RGMII. As
> > we mask the phy-mode here the PHY shall never attempt to add delays as
> > we deal with that in the MAC.
> >
> > It feels like I missed something? Sorry if I'm confused.
> 
> The PHY never applies the {rx,tx}-internal-delay-ps properties, as
> these are meant for the MAC (cfr. "internal").
> The PHY does apply the "*-skew-ps" properties.
> 
> If you mask any *ID part from the phy_interface_t, you lose the ability
> to let the PHY apply any additional delay.
> 
> We have several boards that use both the internal MAC delay and
> the PHY skew properties.

I understand and this make sens to me. But it is in direct contrast to 
what Andrew and I have iterated in previous versions of this driver.

If I understand you correctly Geert, you suggest I should modify the 
driver to

1. Have the MAC (RTSN driver) apply Rx and/or Tx delays based on the 
   {rx,tx}-internal-delay-ps properties.

2. Not mask the phy-mode and pass it as is to of_phy_connect() as the 
   PHY driver will act only on [rt]xc-skew-ps properties and always 
   ignore any {rx,tx}-internal-delay-ps properties.

This allows both the MAC and PHY driver to apply delays independently of 
each other.

While if I understand Andrew correctly (and this is what the RTSN driver 
tries to do in this version) is

1. Have the MAC (RTSN driver) apply Rx and/or Tx delays based on the 
   phy-mode. Add Rx+Rx if rgmii-id, Rx if rgmii-rxid, Tx if rgmii-txid.

2. Mask the phy-mode passed to of_phy_connect() to always be 
   PHY_INTERFACE_MODE_RGMII if a rgmii phy-mode is in use. This is done 
   as the MAC driver will always apply the delay and the PHY should 
   never add a delay on its own.

3. The [rt]xc-skew-ps properties are not consider at all in this 
   solution.

It is not possible to implement both proposed solutions I'm afraid. I 
prefers Geert's solution and this is what was done in earlier versions 
and this would align the behavior of the Renesas Ethernet driver if 
nothing else. But in conversation with Andrew in earlier versions of 
this series have moved to solution 2 as this seemed like the correct way 
of doing things.

At this point I'm confused on which approach to use. @Andrew and @Geert 
how do you guys propose we align the two ?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Kind Regards,
Niklas Söderlund

Powered by blists - more mailing lists