Messages in this thread | | | From | Shenwei Wang <> | Subject | RE: [EXT] Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link | Date | Fri, 28 Jul 2023 14:59:09 +0000 |
| |
> -----Original Message----- > From: Andrew Halaney <ahalaney@redhat.com> > Sent: Thursday, July 27, 2023 1:37 PM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Russell King <linux@armlinux.org.uk>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; > Sascha Hauer <s.hauer@pengutronix.de>; Neil Armstrong > <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Vinod > Koul <vkoul@kernel.org>; Chen-Yu Tsai <wens@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@nxp.com> > > Reviewed-by: Frank Li <frank.li@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?
| |