Changes in V2:
- Call fix_mac_speed() with new mode parameter added.
- reorg the function of imx_dwmac_fix_speed_mx93 by using the
mode parameter.
Shenwei Wang (2):
net: stmmac: add new mode parameter for fix_mac_speed
net: stmmac: dwmac-imx: pause the TXC clock in fixed-link
.../net/ethernet/stmicro/stmmac/dwmac-imx.c | 47 ++++++++++++++++++-
.../ethernet/stmicro/stmmac/dwmac-ipq806x.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-meson.c | 2 +-
.../stmicro/stmmac/dwmac-qcom-ethqos.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-rk.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-socfpga.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-visconti.c | 2 +-
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
include/linux/stmmac.h | 2 +-
10 files changed, 55 insertions(+), 10 deletions(-)
--
2.34.1
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 <[email protected]>
Reviewed-by: Frank Li <[email protected]>
---
.../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)
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)
--
2.34.1
A mode parameter has been added to the callback function of fix_mac_speed
to indicate the physical layer type.
The mode can be one the following:
MLO_AN_PHY - Conventional PHY
MLO_AN_FIXED - Fixed-link mode
MLO_AN_INBAND - In-band protocol
Also use short version of 'uint' to replace the 'unsigned int' in the
function definitions.
Signed-off-by: Shenwei Wang <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
include/linux/stmmac.h | 2 +-
10 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index b9378a63f0e8..53ee5a42c071 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -178,7 +178,7 @@ static void imx_dwmac_exit(struct platform_device *pdev, void *priv)
/* nothing to do now */
}
-static void imx_dwmac_fix_speed(void *priv, unsigned int speed)
+static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
{
struct plat_stmmacenet_data *plat_dat;
struct imx_priv_data *dwmac = priv;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
index e39406df8516..8070352844e3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
@@ -257,7 +257,7 @@ static int ipq806x_gmac_of_parse(struct ipq806x_gmac *gmac)
return PTR_ERR_OR_ZERO(gmac->qsgmii_csr);
}
-static void ipq806x_gmac_fix_mac_speed(void *priv, unsigned int speed)
+static void ipq806x_gmac_fix_mac_speed(void *priv, uint speed, uint mode)
{
struct ipq806x_gmac *gmac = priv;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
index 7aa5e6bc04eb..612551c09ad9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
@@ -22,7 +22,7 @@ struct meson_dwmac {
void __iomem *reg;
};
-static void meson6_dwmac_fix_mac_speed(void *priv, unsigned int speed)
+static void meson6_dwmac_fix_mac_speed(void *priv, uint speed, uint mode)
{
struct meson_dwmac *dwmac = priv;
unsigned int val;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 735525ba8b93..c32549d2fc5a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -631,7 +631,7 @@ static int ethqos_configure(struct qcom_ethqos *ethqos)
return ethqos->configure_func(ethqos);
}
-static void ethqos_fix_mac_speed(void *priv, unsigned int speed)
+static void ethqos_fix_mac_speed(void *priv, uint speed, uint mode)
{
struct qcom_ethqos *ethqos = priv;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index d81591b470a2..2fb24c7e1b44 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1785,7 +1785,7 @@ static void rk_gmac_powerdown(struct rk_priv_data *gmac)
gmac_clk_enable(gmac, false);
}
-static void rk_fix_speed(void *priv, unsigned int speed)
+static void rk_fix_speed(void *priv, uint speed, uint mode)
{
struct rk_priv_data *bsp_priv = priv;
struct device *dev = &bsp_priv->pdev->dev;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 6267bcb60206..ef3be5a3e7b5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -61,7 +61,7 @@ struct socfpga_dwmac {
struct mdio_device *pcs_mdiodev;
};
-static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
+static void socfpga_dwmac_fix_mac_speed(void *priv, uint speed, uint mode)
{
struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
void __iomem *splitter_base = dwmac->splitter_base;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
index 50963e91c347..4bbc9d6888f1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
@@ -72,7 +72,7 @@ static void sun7i_gmac_exit(struct platform_device *pdev, void *priv)
regulator_disable(gmac->regulator);
}
-static void sun7i_fix_speed(void *priv, unsigned int speed)
+static void sun7i_fix_speed(void *priv, uint speed, uint mode)
{
struct sunxi_priv_data *gmac = priv;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
index acbb284be174..5c50cebe9a17 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
@@ -53,7 +53,7 @@ struct visconti_eth {
spinlock_t lock; /* lock to protect register update */
};
-static void visconti_eth_fix_mac_speed(void *priv, unsigned int speed)
+static void visconti_eth_fix_mac_speed(void *priv, uint speed, uint mode)
{
struct visconti_eth *dwmac = priv;
struct net_device *netdev = dev_get_drvdata(dwmac->dev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e1f1c034d325..1c26d60886be 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1060,7 +1060,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
priv->speed = speed;
if (priv->plat->fix_mac_speed)
- priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed);
+ priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed, mode);
if (!duplex)
ctrl &= ~priv->hw->link.duplex;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index ef67dba775d0..7d5e178574be 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -253,7 +253,7 @@ struct plat_stmmacenet_data {
u8 tx_sched_algorithm;
struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES];
struct stmmac_txq_cfg tx_queues_cfg[MTL_MAX_TX_QUEUES];
- void (*fix_mac_speed)(void *priv, unsigned int speed);
+ void (*fix_mac_speed)(void *priv, uint speed, uint mode);
int (*fix_soc_reset)(void *priv, void __iomem *ioaddr);
int (*serdes_powerup)(struct net_device *ndev, void *priv);
void (*serdes_powerdown)(struct net_device *ndev, void *priv);
--
2.34.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 <[email protected]>
> Reviewed-by: Frank Li <[email protected]>
> ---
> .../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?
On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <[email protected]> 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."
> -----Original Message-----
> From: Fabio Estevam <[email protected]>
> Sent: Friday, July 28, 2023 6:01 AM
> To: Shenwei Wang <[email protected]>; Andrew Lunn <[email protected]>
> Cc: Russell King <[email protected]>; David S. Miller
> <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Maxime
> Coquelin <[email protected]>; Shawn Guo <[email protected]>;
> Sascha Hauer <[email protected]>; Neil Armstrong
> <[email protected]>; Kevin Hilman <[email protected]>; Vinod
> Koul <[email protected]>; Chen-Yu Tsai <[email protected]>; Jernej Skrabec
> <[email protected]>; Samuel Holland <[email protected]>;
> Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> Pengutronix Kernel Team <[email protected]>; dl-linux-imx <linux-
>
>
> On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <[email protected]>
> 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
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
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> -----Original Message-----
> From: Andrew Halaney <[email protected]>
> Sent: Thursday, July 27, 2023 1:37 PM
> To: Shenwei Wang <[email protected]>
> Cc: Russell King <[email protected]>; David S. Miller
> <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Maxime
> Coquelin <[email protected]>; Shawn Guo <[email protected]>;
> Sascha Hauer <[email protected]>; Neil Armstrong
> <[email protected]>; Kevin Hilman <[email protected]>; Vinod
> Koul <[email protected]>; Chen-Yu Tsai <[email protected]>; 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 <[email protected]>
> > Reviewed-by: Frank Li <[email protected]>
> > ---
> > .../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?
> -----Original Message-----
> From: Will Deacon <[email protected]>
> Sent: Friday, July 28, 2023 10:36 AM
> To: Russell King (Oracle) <[email protected]>
> Cc: Andrew Halaney <[email protected]>; Shenwei Wang
> <[email protected]>; David S. Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; Shawn Guo <[email protected]>; Sascha
> Hauer <[email protected]>; Neil Armstrong <[email protected]>;
> Kevin Hilman <[email protected]>; Vinod Koul <[email protected]>; Chen-
> Yu Tsai <[email protected]>; Jernej Skrabec <[email protected]>; Samuel
> Holland <[email protected]>; Giuseppe Cavallaro
> <[email protected]>; Alexandre Torgue <[email protected]>;
> Jose Abreu <[email protected]>; Pengutronix Kernel Team
> <[email protected]>; Fabio Estevam <[email protected]>; dl-linux-imx
> <[email protected]>; Jerome Brunet <[email protected]>; Martin
> Blumenstingl <[email protected]>; Bhupesh Sharma
> <[email protected]>; Nobuhiro Iwamatsu
> <[email protected]>; Simon Horman
> <[email protected]>; Bartosz Golaszewski
> <[email protected]>; Wong Vee Khee <[email protected]>;
> Revanth Kumar Uppala <[email protected]>; Jochen Henneberg
> <[email protected]>; [email protected]; linux-stm32@st-
> md-mailman.stormreply.com; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Frank Li <[email protected]>
> 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
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 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
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Friday, July 28, 2023 11:56 AM
> To: Shenwei Wang <[email protected]>
> Cc: Fabio Estevam <[email protected]>; Russell King
> <[email protected]>; David S. Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; Shawn Guo <[email protected]>; Sascha
> Hauer <[email protected]>; Neil Armstrong <[email protected]>;
> Kevin Hilman <[email protected]>; Vinod Koul <[email protected]>; Chen-
> Yu Tsai <[email protected]>; Jernej Skrabec <[email protected]>; Samuel
> Holland <[email protected]>; Giuseppe Cavallaro
> <[email protected]>; Alexandre Torgue <[email protected]>;
> Jose Abreu <[email protected]>; Pengutronix Kernel Team
> <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
> <[email protected]>; Martin Blumenstingl
> <[email protected]>; Bhupesh Sharma
> <[email protected]>; Nobuhiro Iwamatsu
> <[email protected]>; Simon Horman
> <[email protected]>; Andrew Halaney <[email protected]>;
> Bartosz Golaszewski <[email protected]>; Wong Vee Khee
> <[email protected]>; Revanth Kumar Uppala <[email protected]>; Jochen
> Henneberg <[email protected]>; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Frank Li <[email protected]>
> 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
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!