[EXT] Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link
Shenwei Wang
shenwei.wang at nxp.com
Fri Jul 28 07:59:09 PDT 2023
> -----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?
More information about the linux-amlogic
mailing list