Hi Niklas,
On Mon, May 13, 2024 at 12:09 PM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > > * 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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
