[EXT] Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Halaney <ahalaney at redhat.com>
> Sent: Thursday, July 27, 2023 1:37 PM
> To: Shenwei Wang <shenwei.wang at nxp.com>
> Cc: Russell King <linux at armlinux.org.uk>; David S. Miller
> <davem at davemloft.net>; Eric Dumazet <edumazet at google.com>; Jakub
> Kicinski <kuba at kernel.org>; Paolo Abeni <pabeni at redhat.com>; Maxime
> Coquelin <mcoquelin.stm32 at gmail.com>; Shawn Guo <shawnguo at kernel.org>;
> Sascha Hauer <s.hauer at pengutronix.de>; Neil Armstrong
> <neil.armstrong at linaro.org>; Kevin Hilman <khilman at baylibre.com>; Vinod
> Koul <vkoul at kernel.org>; Chen-Yu Tsai <wens at csie.org>; Jernej Skrabec
> > required silent interval on the clock line for SJA1105 to complete the
> > frequency transition and enable the internal TDLs.
> >
> > So far we have only enabled this feature on the i.MX93 platform.
> >
> 
> I'd just like to highlight that because of a quirk (I think this is not
> standard) in the particular connected switch on a board you're making the whole
> "fsl,imx93" platform (compatible) implement said switch quirk.
> 
> If you don't think there's any harm in doing that for other fixed-link scenarios,
> that's fine I suppose... but just highlighting that.
> 
> I have no idea at a higher level how else you'd tackle this. You could add a dt
> property for this, but I also don't love that you'd probably encode it in the MAC
> (maybe in the fixed-link description it would be more attractive). At least as a dt
> property it isn't unconditional.
> 

This change won't impact the function of any normal cases, introducing a dt property
is not necessary IMO.

> > Signed-off-by: Shenwei Wang <shenwei.wang at nxp.com>
> > Reviewed-by: Frank Li <frank.li at nxp.com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE                 0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET               (0x1 << 0)
> >  #define RMII_RESET_SPEED             (0x3 << 14)
> > +#define MII_RESET_SPEED                      (0x2 << 14)
> > +#define RGMII_RESET_SPEED            (0x0 << 14)
> > +#define CTRL_SPEED_MASK                      (0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see the driver
> currently does shifts.. so /me shrugs
> 

Okay.

> >
> >  struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +59,7 @@ struct imx_priv_data {
> >       struct regmap *intf_regmap;
> >       u32 intf_reg_off;
> >       bool rmii_refclk_ext;
> > +     void __iomem *base_addr;
> >
> >       const struct imx_dwmac_ops *ops;
> >       struct plat_stmmacenet_data *plat_dat; @@ -212,6 +216,44 @@
> > static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
> >               dev_err(dwmac->dev, "failed to set tx rate %lu\n",
> > rate);  }
> >
> > +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
> > +mode) {
> > +     struct imx_priv_data *dwmac = priv;
> > +     int ctrl, old_ctrl, iface;
> > +
> > +     imx_dwmac_fix_speed(priv, speed, mode);
> > +
> > +     if (!dwmac || mode != MLO_AN_FIXED)
> > +             return;
> > +
> > +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
> > +             return;
> > +
> > +     iface &= MX93_GPR_ENET_QOS_INTF_MODE_MASK;
> > +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> > +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> > +
> > +     /* by default ctrl will be RGMII */
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> > +             ctrl |= RMII_RESET_SPEED;
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> > +             ctrl |= MII_RESET_SPEED;
> 
> I see that ctrl right now would select RGMII, but I think it would read more
> clearly if you handled it and made the above an if/else if/else statement (since
> they're exclusive of eachother) vs two independent if's.
> 

I think I did too much here. The other two cases should be removed as only 
RGMII requires to add delays on the clock line.

> > +
> > +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> > +
> > +     /* Ensure the settings for CTRL are applied */
> > +     wmb();
> 
> I saw this and recently have been wondering about this sort of pattern (not an
> expert on this). From what I can tell it seems reading the register back is the
> preferred pattern to force the write out. The above works, but it feels to me
> personally akin to how local_lock() in the kernel is a more fine grained
> mechanism than using preempt_disable(). But that's pretty opinionated. See
> device-io.rst and io_ordering.rst for how I came to that conclusion.
> 

wmb is necessary here as we want to delay such a period after the registers are
written. But the location should be moved to before the usleep_range() line, so
that it could avoid the scenario #2 that you pointed out below.

Thanks,
Shenwei

> > +
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> > +     usleep_range(50, 100);
> > +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
> > +
> > +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
> 
> I don't have any documentation for the registers here, and as you can see I'm an
> amateur with respect to memory ordering based on my prior comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through
>     4. Read intf_reg_off (regmap_update_bits())
>     5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
>     6. Sleep for 50-100 us
>     7. Read intf_reg_off (regmap_update_bits())
>     8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
>        MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
> 
> I don't know what those bits do, but your description sounds like you are trying
> to stop the clock for 50-100 us. In your code, if I understand the memory
> ordering correctly, both of the following could
> occur:
> 
>     1. Write RESET_SPEED
>     2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     3. sleep
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
>     or
> 
>     1. Write RESET_SPEED
>     2. sleep
>     3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
> is the latter acceptable to you, or does that wmb() (or alternative) need to move?
> It seems to me only the first situation would stop the clock before sleeping, but
> that's going off the names in this driver only.
> 
> In either case, shouldn't regmap_update_bits() force a read of said bits, which
> would remove the need for that wmb() altogether to synchronize the two writes?