netdev - Re: [net-next,v5] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240513100931.GA3015543@ragnatech.se>
Date: Mon, 13 May 2024 12:09:31 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Andrew Lunn <andrew@...n.ch>,
	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,

Thanks for your feedback.

On 2024-05-13 11:39:54 +0200, Geert Uytterhoeven wrote:
> Hi Niklas, Andrew,
> 
> 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? 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.

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