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

Commit Message

Shenwei Wang July 27, 2023, 3:25 p.m. UTC

When using a fixed-link setup, certain devices like the SJA1105 require a
small pause in the TXC clock line to enable their internal tunable
delay line (TDL).

To satisfy this requirement, this patch temporarily disables the TX clock,
and restarts it after a required period. This provides the 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.

Signed-off-by: Shenwei Wang <[e-mail]>
Reviewed-by: Frank Li <[e-mail]>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Andrew Halaney July 27, 2023, 6:36 p.m. UTC | #1

On Thu, Jul 27, 2023 at 10:25:03AM -0500, Shenwei Wang wrote:
> When using a fixed-link setup, certain devices like the SJA1105 require a
> small pause in the TXC clock line to enable their internal tunable
> delay line (TDL).
> 
> To satisfy this requirement, this patch temporarily disables the TX clock,
> and restarts it after a required period. This provides the 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.

> Signed-off-by: Shenwei Wang <[e-mail]>
> Reviewed-by: Frank Li <[e-mail]>
> ---
>  .../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

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

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

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

Fabio Estevam July 28, 2023, 11 a.m. UTC | #2

On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <[e-mail]> wrote:

>         struct plat_stmmacenet_data *plat_dat = priv;
> @@ -317,8 +359,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
>         plat_dat->exit = imx_dwmac_exit;
>         plat_dat->clks_config = imx_dwmac_clks_config;
>         plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
> +       if (of_machine_is_compatible("fsl,imx93"))
> +               plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;

So you are forcing this on all imx93 boards, even if they don't use a SJA1105.

Andrew Lunn gave the following feedback in v1:

"The SJA1105 has the problem, so i would expect it to be involved in
the solution. Otherwise, how is this going to work for other MAC
drivers?

Maybe you need to expose a common clock framework clock for the TXC
clock line, which the SJA1105 can disable/enable? That then makes it
clear what other MAC drivers need to do."

Shenwei Wang July 28, 2023, 2:59 p.m. UTC | #3

> -----Original Message-----
> From: Andrew Halaney <[e-mail]>
> Sent: Thursday, July 27, 2023 1:37 PM
> To: Shenwei Wang <[e-mail]>
> Cc: Russell King <[e-mail]>; David S. Miller
> <[e-mail]>; Eric Dumazet <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>; Maxime
> Coquelin <[e-mail]>; Shawn Guo <[e-mail]>;
> Sascha Hauer <[e-mail]>; Neil Armstrong
> <[e-mail]>; Kevin Hilman <[e-mail]>; Vinod
> Koul <[e-mail]>; Chen-Yu Tsai <[e-mail]>; 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 <[e-mail]>
> > Reviewed-by: Frank Li <[e-mail]>
> > ---
> >  .../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?

Shenwei Wang July 28, 2023, 3:09 p.m. UTC | #4

> -----Original Message-----
> From: Fabio Estevam <[e-mail]>
> Sent: Friday, July 28, 2023 6:01 AM
> To: Shenwei Wang <[e-mail]>; Andrew Lunn <[e-mail]>
> Cc: Russell King <[e-mail]>; David S. Miller
> <[e-mail]>; Eric Dumazet <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>; Maxime
> Coquelin <[e-mail]>; Shawn Guo <[e-mail]>;
> Sascha Hauer <[e-mail]>; Neil Armstrong
> <[e-mail]>; Kevin Hilman <[e-mail]>; Vinod
> Koul <[e-mail]>; Chen-Yu Tsai <[e-mail]>; Jernej Skrabec
> <[e-mail]>; Samuel Holland <[e-mail]>;
> Giuseppe Cavallaro <[e-mail]>; Alexandre Torgue
> <[e-mail]>; Jose Abreu <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx <linux-
> 
> 
> On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <[e-mail]>
> wrote:
> 
> >         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
> > +359,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
> >         plat_dat->exit = imx_dwmac_exit;
> >         plat_dat->clks_config = imx_dwmac_clks_config;
> >         plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
> > +       if (of_machine_is_compatible("fsl,imx93"))
> > +               plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
> 
> So you are forcing this on all imx93 boards, even if they don't use a SJA1105.
> 

Yes, that's the purpose because it won't hurt even the other side is not SJA1105.

> Andrew Lunn gave the following feedback in v1:
> 
> "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> Otherwise, how is this going to work for other MAC drivers?
> 
> Maybe you need to expose a common clock framework clock for the TXC clock
> line, which the SJA1105 can disable/enable? That then makes it clear what other
> MAC drivers need to do."

I have been considering this plan for some time. The idea should be implemented 
across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
and it will take some time to implement it.

Thanks,
Shenwei

Russell King (Oracle) July 28, 2023, 3:22 p.m. UTC | #5

On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > 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

BIT() is only useful for single-bit items, not for use with bitfields,
and their use with bitfields just makes the whole thing perverse.

#define CTRL_SPEED_MASK		GENMASK(15, 14)
#define CTRL_SPEED_RGMII_RESET	0
#define CTRL_SPEED_MII_RESET	2
#define CTRL_SPEED_RMII_RESET	3

and then its use:

	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RGMII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_MII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RMII_RESET)

alternatively:

        if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
                speed = CTRL_SPEED_RMII_RESET;
        else (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
                speed = CTRL_SPEED_MII_RESET;
	else
		speed = CTRL_SPEED_RGMII_RESET;

	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
	ctrl |= FIELD_PREP(CTRL_SPEED_MASK, speed);
	writel(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

I wonder about whether that wmb() is required. If the mapping is
device-like rather than memory-like, the write should be committed
before the read that regmap_update_bits() does according to the ARM
memory model. Maybe a bit of information about where this barrier
has come from would be good, and maybe getting it reviewed by the
arm64 barrier specialist, Will Deacon. :)

wmb() is normally required to be paired with a rmb(), but we're not
talking about system memory here, so I also wonder whether wmb() is
the correct barrier to use.

Adding Will.

Will Deacon July 28, 2023, 3:36 p.m. UTC | #6

On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > 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
> 
> I wonder about whether that wmb() is required. If the mapping is
> device-like rather than memory-like, the write should be committed
> before the read that regmap_update_bits() does according to the ARM
> memory model. Maybe a bit of information about where this barrier
> has come from would be good, and maybe getting it reviewed by the
> arm64 barrier specialist, Will Deacon. :)
> 
> wmb() is normally required to be paired with a rmb(), but we're not
> talking about system memory here, so I also wonder whether wmb() is
> the correct barrier to use.

Yes, I don't think wmb() is the right thing here. If you need to ensure
that the write to MAC_CTRL_REG has taken effect, then you'll need to go
through some device-specific sequence which probably involves reading
something back. If you just need things to arrive in order eventually,
the memory type already gives you that.

It's also worth pointing out that udelay() isn't necessarily ordered wrt
MMIO writes, so that usleep_range() might need some help as well.
Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
udelay(), so if you add the readback then this might all work out.

I gave a (slightly dated) talk about some of this at ELC a while back:

https://www.youtube.com/watch?v=i6DayghhA8Q

which might help.

Will

Shenwei Wang July 28, 2023, 4:09 p.m. UTC | #7

> -----Original Message-----
> From: Will Deacon <[e-mail]>
> Sent: Friday, July 28, 2023 10:36 AM
> To: Russell King (Oracle) <[e-mail]>
> Cc: Andrew Halaney <[e-mail]>; Shenwei Wang
> <[e-mail]>; David S. Miller <[e-mail]>; Eric
> Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
> Abeni <[e-mail]>; Maxime Coquelin
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Neil Armstrong <[e-mail]>;
> Kevin Hilman <[e-mail]>; Vinod Koul <[e-mail]>; Chen-
> Yu Tsai <[e-mail]>; Jernej Skrabec <[e-mail]>; Samuel
> Holland <[e-mail]>; Giuseppe Cavallaro
> <[e-mail]>; Alexandre Torgue <[e-mail]>;
> Jose Abreu <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>; dl-linux-imx
> <[e-mail]>; Jerome Brunet <[e-mail]>; Martin
> Blumenstingl <[e-mail]>; Bhupesh Sharma
> <[e-mail]>; Nobuhiro Iwamatsu
> <[e-mail]>; Simon Horman
> <[e-mail]>; Bartosz Golaszewski
> <[e-mail]>; Wong Vee Khee <[e-mail]>;
> Revanth Kumar Uppala <[e-mail]>; Jochen Henneberg
> <[e-mail]>; [e-mail]; linux-stm32@st-
> md-mailman.stormreply.com; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> Frank Li <[e-mail]>
> Subject: [EXT] Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC
> clock in fixed-link
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > 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
> >
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier has
> > come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> >
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
>
> Yes, I don't think wmb() is the right thing here. If you need to ensure that the
> write to MAC_CTRL_REG has taken effect, then you'll need to go through some
> device-specific sequence which probably involves reading something back. If you
> just need things to arrive in order eventually, the memory type already gives you
> that.
>
> It's also worth pointing out that udelay() isn't necessarily ordered wrt MMIO
> writes, so that usleep_range() might need some help as well.
> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
>

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. usleep_range()
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

In the above example, if a readback after step #2 could ensure sufficient time has passed
before step #4, the wmb() here should be abandoned.

Thanks,
Shenwei

> I gave a (slightly dated) talk about some of this at ELC a while back:
>
> https://www.yo/
> utube.com%2Fwatch%3Fv%3Di6DayghhA8Q&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C32396fd0396e4e46975f08db8f806680%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638261553857503588%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=X5CQrQEVmUjYafYJ%2BzcnGXI9mhDT%
> 2BMzDazGHOcoomas%3D&reserved=0
>
> which might help.
>
> Will

Frank Li July 28, 2023, 4:29 p.m. UTC | #8

On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > 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
> > 
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier
> > has come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> > 
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
> 
> Yes, I don't think wmb() is the right thing here. If you need to ensure
> that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> through some device-specific sequence which probably involves reading
> something back. If you just need things to arrive in order eventually,
> the memory type already gives you that.
> 
> It's also worth pointing out that udelay() isn't necessarily ordered wrt
> MMIO writes, so that usleep_range() might need some help as well.

Hi Deacon:

Does it means below pattern will be problem?

1.writel()
2.udelay()
3.writel()

It may not wait enough time between 1 and 3. I think the above pattern
is quite common in driver code.  I am not sure if usleep_range involve
MMIO to get current counter, ARM may use cp15 to get local timer counter.

In our system, readl() is quite slow because cross some bus bridge.
even readl() can work, we don't know it is because delay by readl() itself.
Or it works logically. Suppose readl() and writel() just guarantee memory
access order. 

Frank

> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
> 
> I gave a (slightly dated) talk about some of this at ELC a while back:
> 
> https://www.youtube.com/watch?v=i6DayghhA8Q
> 
> which might help.
> 
> Will

Andrew Lunn July 28, 2023, 4:55 p.m. UTC | #9

> > Andrew Lunn gave the following feedback in v1:
> > 
> > "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> > Otherwise, how is this going to work for other MAC drivers?
> > 
> > Maybe you need to expose a common clock framework clock for the TXC clock
> > line, which the SJA1105 can disable/enable? That then makes it clear what other
> > MAC drivers need to do."
> 
> I have been considering this plan for some time. The idea should be implemented 
> across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
> and it will take some time to implement it.

So you don't think anybody will use anything else for driving this
switch? Vybrid?

It does not really matter what you implement it for, so long is at is
a clear example for others to follow who might be using the switch
with other SoCs.

	Andrew

Shenwei Wang July 28, 2023, 5:01 p.m. UTC | #10

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: Friday, July 28, 2023 11:56 AM
> To: Shenwei Wang <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>; Russell King
> <[e-mail]>; David S. Miller <[e-mail]>; Eric
> Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
> Abeni <[e-mail]>; Maxime Coquelin
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Neil Armstrong <[e-mail]>;
> Kevin Hilman <[e-mail]>; Vinod Koul <[e-mail]>; Chen-
> Yu Tsai <[e-mail]>; Jernej Skrabec <[e-mail]>; Samuel
> Holland <[e-mail]>; Giuseppe Cavallaro
> <[e-mail]>; Alexandre Torgue <[e-mail]>;
> Jose Abreu <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; dl-linux-imx <[e-mail]>; Jerome Brunet
> <[e-mail]>; Martin Blumenstingl
> <[e-mail]>; Bhupesh Sharma
> <[e-mail]>; Nobuhiro Iwamatsu
> <[e-mail]>; Simon Horman
> <[e-mail]>; Andrew Halaney <[e-mail]>;
> Bartosz Golaszewski <[e-mail]>; Wong Vee Khee
> <[e-mail]>; Revanth Kumar Uppala <[e-mail]>; Jochen
> Henneberg <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Frank Li <[e-mail]>
> Subject: Re: [EXT] Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the
> TXC clock in fixed-link
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> > > Andrew Lunn gave the following feedback in v1:
> > >
> > > "The SJA1105 has the problem, so i would expect it to be involved in the
> solution.
> > > Otherwise, how is this going to work for other MAC drivers?
> > >
> > > Maybe you need to expose a common clock framework clock for the TXC
> > > clock line, which the SJA1105 can disable/enable? That then makes it
> > > clear what other MAC drivers need to do."
> >
> > I have been considering this plan for some time. The idea should be
> > implemented across all i.mx8/9 platforms. I am going to start to work
> > on it in the following month, and it will take some time to implement it.
> 
> So you don't think anybody will use anything else for driving this switch? Vybrid?
> 

Vybrid or i.MX6 don't have such kind of problem because they are not using dwmac.
Those are FEC MACs, and the MAC itself can provide delay to the switch.

Thanks,
Shenwei

> It does not really matter what you implement it for, so long is at is a clear
> example for others to follow who might be using the switch with other SoCs.
> 
>         Andrew

Russell King (Oracle) July 28, 2023, 7:33 p.m. UTC | #11

On Fri, Jul 28, 2023 at 12:29:46PM -0400, Frank Li wrote:
> On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> > Yes, I don't think wmb() is the right thing here. If you need to ensure
> > that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> > through some device-specific sequence which probably involves reading
> > something back. If you just need things to arrive in order eventually,
> > the memory type already gives you that.
> > 
> > It's also worth pointing out that udelay() isn't necessarily ordered wrt
> > MMIO writes, so that usleep_range() might need some help as well.
> 
> Hi Deacon:
> 
> Does it means below pattern will be problem?
> 
> 1.writel()
> 2.udelay()
> 3.writel()

Yes, it can be a problem - because the first write may take a while
to hit the hardware. It's been this way ever since PCI became a thing,
even on x86 hardware.

PCI posting rules are that writes can be posted into the various
bridges in the bus structure and forwarded on at some point later.
However, reads are not allowed to bypass writes - which means that if
one reads from a PCI device, the preceeding writes need to be flushed
out of the bridges _in the path to the device being read_.

So, if we take an example and apply it to PCI:

	writel()
	udelay(100)
	writel()
	readl()

The device could well see nothing for a while, and then two consecutive
writes and a read in quick succession.

> It may not wait enough time between 1 and 3. I think the above pattern
> is quite common in driver code.  I am not sure if usleep_range involve
> MMIO to get current counter, ARM may use cp15 to get local timer counter.

There are no guarantees, even on x86, that udelay() offers anything to
space device writes apart.

If this pattern is popular in drivers, and it's critical to the
drivers operation, then it's technically buggy - and it's been that way
for at least a couple of decades! One might get away with it (maybe the
hardware isn't delaying the writes?) but the kernel has never
guaranteed that writel(), udelay(), writel() will space the two writes
apart by the specified delay.

Patch

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)
 
 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;
+
+	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
+
+	/* Ensure the settings for CTRL are applied */
+	wmb();
+
+	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);
+}
+
 static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
 {
 	struct plat_stmmacenet_data *plat_dat = priv;
@@ -317,8 +359,11 @@  static int imx_dwmac_probe(struct platform_device *pdev)
 	plat_dat->exit = imx_dwmac_exit;
 	plat_dat->clks_config = imx_dwmac_clks_config;
 	plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
+	if (of_machine_is_compatible("fsl,imx93"))
+		plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
 	plat_dat->bsp_priv = dwmac;
 	dwmac->plat_dat = plat_dat;
+	dwmac->base_addr = stmmac_res.addr;
 
 	ret = imx_dwmac_clks_config(dwmac, true);
 	if (ret)