From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Andrew Lunn <andrew@lunn.ch>,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Paolo Abeni <pabeni@redhat.com>, netdev <netdev@vger.kernel.org>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [net-next,v5] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN
Date: Tue, 14 May 2024 14:25:25 +0200 [thread overview]
Message-ID: <CAMuHMdXATsobm4rjkCV8SECNXQhuD1Cqr7ChyvtcaiLtLFtetw@mail.gmail.com> (raw)
In-Reply-To: <20240513125658.GB3015543@ragnatech.se>
Hi Niklas,
On Mon, May 13, 2024 at 2:57 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2024-05-13 13:44:22 +0200, Geert Uytterhoeven wrote:
> > On Mon, May 13, 2024 at 12:09 PM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.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@ragnatech.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@ragnatech.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 ?
Indeed, both should be aligned, to avoid confusion.
The implementation of the ravb driver was based on the feedback I
received at that time:
1. The *ID phy mode delays must be applied by the PHY, not by the MAC,
2. Any additional MAC delays should be specified using
{rx,tx}-internal-delay-ps properties.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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
next prev parent reply other threads:[~2024-05-14 12:25 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 21:09 [net-next,v5] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN Niklas Söderlund
2024-05-13 9:39 ` Geert Uytterhoeven
2024-05-13 10:09 ` Niklas Söderlund
2024-05-13 11:44 ` Geert Uytterhoeven
2024-05-13 12:56 ` Niklas Söderlund
2024-05-14 12:25 ` Geert Uytterhoeven [this message]
2024-05-13 13:38 ` Andrew Lunn
2024-05-14 0:24 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMuHMdXATsobm4rjkCV8SECNXQhuD1Cqr7ChyvtcaiLtLFtetw@mail.gmail.com \
--to=geert@linux-m68k.org \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=pabeni@redhat.com \
--cc=yoshihiro.shimoda.uh@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).