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

  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 <shenwei.wang@nxp.com>
Reviewed-by: Frank Li <frank.li@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
  

On 31.07.2023 11:19:29, 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.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..2e4173d099f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -32,6 +32,7 @@
>  #define GPR_ENET_QOS_RGMII_EN		(0x1 << 21)
>  
>  #define MX93_GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(3, 0)
> +#define MX93_GPR_ENET_QOS_INTF_MASK		GENMASK(3, 1)
>  #define MX93_GPR_ENET_QOS_INTF_SEL_MII		(0x0 << 1)
>  #define MX93_GPR_ENET_QOS_INTF_SEL_RMII		(0x4 << 1)
>  #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
> @@ -40,6 +41,7 @@
>  #define DMA_BUS_MODE			0x00001000
>  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>  #define RMII_RESET_SPEED		(0x3 << 14)
> +#define CTRL_SPEED_MASK			GENMASK(15, 14)
>  
>  struct imx_dwmac_ops {
>  	u32 addr_width;
> @@ -56,6 +58,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 +215,42 @@ 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;

regmap_read() wants a pointer to an "unsigned int".

> +
> +	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_MASK;
> +	if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> +		return;
> +
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> +	 * reordering.
> +	 */
> +	wmb();
> +
> +	usleep_range(10, 20);
> +	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);
> +}

Marc
  

Hi Shenwei,

thanks for your patch.

On 7/31/23 18:19, 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).

If this is only required for some devices, is it safe to enforce this behaviour 
unconditionally for any kind of fixed link devices connected to the MX93 EQOS 
or could this possibly break for other devices?

Best regards
Johannes

> 
> 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 <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..2e4173d099f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -32,6 +32,7 @@
>   #define GPR_ENET_QOS_RGMII_EN		(0x1 << 21)
>   
>   #define MX93_GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(3, 0)
> +#define MX93_GPR_ENET_QOS_INTF_MASK		GENMASK(3, 1)
>   #define MX93_GPR_ENET_QOS_INTF_SEL_MII		(0x0 << 1)
>   #define MX93_GPR_ENET_QOS_INTF_SEL_RMII		(0x4 << 1)
>   #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
> @@ -40,6 +41,7 @@
>   #define DMA_BUS_MODE			0x00001000
>   #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>   #define RMII_RESET_SPEED		(0x3 << 14)
> +#define CTRL_SPEED_MASK			GENMASK(15, 14)
>   
>   struct imx_dwmac_ops {
>   	u32 addr_width;
> @@ -56,6 +58,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 +215,42 @@ 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_MASK;
> +	if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> +		return;
> +
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> +	 * reordering.
> +	 */
> +	wmb();
> +
> +	usleep_range(10, 20);
> +	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 +356,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)
  

On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> Hi Shenwei,
> 
> thanks for your patch.
> 
> On 7/31/23 18:19, 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).
> 
> If this is only required for some devices, is it safe to enforce this
> behaviour unconditionally for any kind of fixed link devices connected to
> the MX93 EQOS or could this possibly break for other devices?

This same point has been raised by Andrew Halaney in message-id
 <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
and Fabio Estevam in message-id
 <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.com>
but we don't seem to have any answer for it.

Also, the patch still uses wmb() between the write and the delay, and as
Will Deacon pointed out in his message, message-id
 <20230728153611.GH21718@willie-the-truck>
this is not safe, yet still a new version was sent.

It seems the author of these patches is pretty resistant to comments,
and has shown that when I was requesting changes - it was an awful
struggle to get changes made. I'm now of the opinion that I really
can't be bothered to review these patches, precisely because feedback
is clearly not welcome or if welcome, apparently acted upon.
  

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Tuesday, August 1, 2023 7:57 AM
> To: Johannes Zink <j.zink@pengutronix.de>
> Cc: Shenwei Wang <shenwei.wang@nxp.com>; 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
> <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: [EXT] Re: [PATCH v3 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 Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> > Hi Shenwei,
> >
> > thanks for your patch.
> >
> > On 7/31/23 18:19, 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).
> >
> > If this is only required for some devices, is it safe to enforce this
> > behaviour unconditionally for any kind of fixed link devices connected
> > to the MX93 EQOS or could this possibly break for other devices?
>
> This same point has been raised by Andrew Halaney in message-id
> <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
> and Fabio Estevam in message-id
>
> <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
> com>
> but we don't seem to have any answer for it.
>
Hi Russell,

I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
I'm happy to clarify further, but kindly avoid unsubstantiated comments.

https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
" Yes, that's the purpose because it won't hurt even the other side is not SJA1105."

> Also, the patch still uses wmb() between the write and the delay, and as Will
> Deacon pointed out in his message, message-id
> <20230728153611.GH21718@willie-the-truck>
> this is not safe, yet still a new version was sent.
>

Can we conclude that even without the wmb() here, the desired delay time between
operations can still be ensured?

Thanks,
Shenwei

> It seems the author of these patches is pretty resistant to comments, and has
> shown that when I was requesting changes - it was an awful struggle to get
> changes made. I'm now of the opinion that I really can't be bothered to review
> these patches, precisely because feedback is clearly not welcome or if welcome,
> apparently acted upon.
>
> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7Ce65ab380ff5b4748da5308db928ec751%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638264914150592989%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=%2FzSqRqJFRQljX6ky3XJvfkMH9PwgOstb
> w8HpEppYOIM%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
  

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Tuesday, August 1, 2023 7:48 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: [EXT] Re: [PATCH v3 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
>
>
> Hi Shenwei,
>
> thanks for your patch.
>
> On 7/31/23 18:19, 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).
>
> If this is only required for some devices, is it safe to enforce this behaviour
> unconditionally for any kind of fixed link devices connected to the MX93 EQOS
> or could this possibly break for other devices?
>

It won't impact normal devices. The link layer hasn't built up yet.

Thanks,
Shenwei

> Best regards
> Johannes
>
> >
> > 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 <shenwei.wang@nxp.com>
> > Reviewed-by: Frank Li <frank.li@nxp.com>
> > ---
> >   .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..2e4173d099f3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -32,6 +32,7 @@
> >   #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
> >
> >   #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
> > +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
> >   #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
> >   #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
> >   #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
> > @@ -40,6 +41,7 @@
> >   #define DMA_BUS_MODE                        0x00001000
> >   #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
> >   #define RMII_RESET_SPEED            (0x3 << 14)
> > +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
> >
> >   struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +58,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 +215,42 @@
> > 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_MASK;
> > +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> > +             return;
> > +
> > +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> > +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> > +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> > +
> > +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> > +      * reordering.
> > +      */
> > +     wmb();
> > +
> > +     usleep_range(10, 20);
> > +     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 +356,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)
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
> eserved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
  

On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Tuesday, August 1, 2023 7:57 AM
> > To: Johannes Zink <j.zink@pengutronix.de>
> > Cc: Shenwei Wang <shenwei.wang@nxp.com>; 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
> > <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>;
> > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> > <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> > <jbrunet@baylibre.com>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> > <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> > <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> > <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> > Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> > <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> > Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> > stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> > imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> > Subject: [EXT] Re: [PATCH v3 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 Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> > > Hi Shenwei,
> > >
> > > thanks for your patch.
> > >
> > > On 7/31/23 18:19, 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).
> > >
> > > If this is only required for some devices, is it safe to enforce this
> > > behaviour unconditionally for any kind of fixed link devices connected
> > > to the MX93 EQOS or could this possibly break for other devices?
> >
> > This same point has been raised by Andrew Halaney in message-id
> > <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
> > and Fabio Estevam in message-id
> >
> > <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
> > com>
> > but we don't seem to have any answer for it.
> >
> Hi Russell,
> 
> I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
> I'm happy to clarify further, but kindly avoid unsubstantiated comments.
> 
> https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
> " Yes, that's the purpose because it won't hurt even the other side is not SJA1105."
> 
> > Also, the patch still uses wmb() between the write and the delay, and as Will
> > Deacon pointed out in his message, message-id
> > <20230728153611.GH21718@willie-the-truck>
> > this is not safe, yet still a new version was sent.
> >
> 
> Can we conclude that even without the wmb() here, the desired delay time between
> operations can still be ensured?

Will's talk[0] he linked has the sequence you've done here (writel's
followed by wmb() followed by a udelay), and he states it is wrong if
the goal is for the device to see the writes prior to the udelay. That's
discussed at around 28:00 and followed up by (thankfully, cuz I too
didn't understand it) a question at 34:10 to discuss why mb() isn't
sufficient (it completes the write, but the device *may not* see it
yet, the read forces that).

He mentioned that over at [1] in the review here, and suggested reading
from the device again prior to the udelay() instead to force the writes
to take affect on the device prior to the udelay.

I found a quick example in the ufs-qcom.c driver that I'll copy paste
here too from upstream that follows this advice:

		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);

		/*
		 * Make sure the write to ref_clk reaches the destination and
		 * not stored in a Write Buffer (WB).
		 */
		readl(host->dev_ref_clk_ctrl_mmio);

		/*
		 * If we call hibern8 exit after this, we need to make sure that
		 * device ref_clk is stable for at least 1us before the hibern8
		 * exit command.
		 */
		if (enable)
			udelay(1);


[0] https://www.youtube.com/watch?v=i6DayghhA8Q
[1] https://lore.kernel.org/netdev/20230728153611.GH21718@willie-the-truck/

I hope that helps,
Andrew
  

On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote:
> > On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> > > Hi Shenwei,
> > >
> > > thanks for your patch.
> > >
> > > On 7/31/23 18:19, 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).
> > >
> > > If this is only required for some devices, is it safe to enforce this
> > > behaviour unconditionally for any kind of fixed link devices connected
> > > to the MX93 EQOS or could this possibly break for other devices?
> >
> > This same point has been raised by Andrew Halaney in message-id
> > <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
> > and Fabio Estevam in message-id
> >
> > <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
> > com>
> > but we don't seem to have any answer for it.
> >
> Hi Russell,
> 
> I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
> I'm happy to clarify further, but kindly avoid unsubstantiated comments.
> 
> https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
> " Yes, that's the purpose because it won't hurt even the other side is not SJA1105."

So, why not include the answer in the commit message given that you've
had to answer it several times already?

> > Also, the patch still uses wmb() between the write and the delay, and as Will
> > Deacon pointed out in his message, message-id
> > <20230728153611.GH21718@willie-the-truck>
> > this is not safe, yet still a new version was sent.
> >
> 
> Can we conclude that even without the wmb() here, the desired delay time between
> operations can still be ensured?

How did you come to that conclusion? I see no further discussion after
I raised this, Will replied, and you suggested a read-back. However,
that isn't what you've implemented on v3, you've gone back to what
looks like the original code in v2 which brought up this question - and
Will indicated it was _unsafe_.
  

Hi Shenwei,

On 8/1/23 19:10, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Johannes Zink <j.zink@pengutronix.de>
>> Sent: Tuesday, August 1, 2023 7:48 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel
>> Holland <samuel@sholland.org>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>> <jbrunet@baylibre.com>; Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
>> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
>> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>> Subject: [EXT] Re: [PATCH v3 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
>>
>>
>> Hi Shenwei,
>>
>> thanks for your patch.
>>
>> On 7/31/23 18:19, 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).
>>
>> If this is only required for some devices, is it safe to enforce this behaviour
>> unconditionally for any kind of fixed link devices connected to the MX93 EQOS
>> or could this possibly break for other devices?
>>
> 
> It won't impact normal devices. The link layer hasn't built up yet.
> 

As Russel suggested in [1] - maybe you could rephrase your commit message for 
your v4 to point this out to future reviewers (apparently multiple people have 
had questions about this...)  and have this fact also recorded in the git log 
later on.

Also: does this only apply to i.MX93, or would we have to test and enable it on 
e.g. i.MX8MP as well?

Thanks
Johannes

[1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk


> Thanks,
> Shenwei
> 
>> Best regards
>> Johannes
>>
>>>
>>> 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 <shenwei.wang@nxp.com>
>>> Reviewed-by: Frank Li <frank.li@nxp.com>
>>> ---
>>>    .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
>>>    1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> index 53ee5a42c071..2e4173d099f3 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> @@ -32,6 +32,7 @@
>>>    #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
>>>
>>>    #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
>>>    #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
>>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
>>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
>>> @@ -40,6 +41,7 @@
>>>    #define DMA_BUS_MODE                        0x00001000
>>>    #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
>>>    #define RMII_RESET_SPEED            (0x3 << 14)
>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
>>>
>>>    struct imx_dwmac_ops {
>>>        u32 addr_width;
>>> @@ -56,6 +58,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 +215,42 @@
>>> 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_MASK;
>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
>>> +             return;
>>> +
>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
>>> +
>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
>>> +      * reordering.
>>> +      */
>>> +     wmb();
>>> +
>>> +     usleep_range(10, 20);
>>> +     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 +356,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)
>>
>> --
>> Pengutronix e.K.                | Johannes Zink                  |
>> Steuerwalder Str. 21            |
>> https://www.pe/
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
>> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
>> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
>> eserved=0    |
>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 
>
  

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Wednesday, August 2, 2023 1:26 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v3 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
>
>
> Hi Shenwei,
>
> On 8/1/23 19:10, Shenwei Wang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Johannes Zink <j.zink@pengutronix.de>
> >> Sent: Tuesday, August 1, 2023 7:48 AM
> >> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel Holland
> >> <samuel@sholland.org>
> >> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> >> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> >> <jbrunet@baylibre.com>; Martin Blumenstingl
> >> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> >> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> >> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> >> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> >> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> >> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
> >> Jochen Henneberg <jh@henneberg-systemdesign.com>;
> >> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> >> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> >> Subject: [EXT] Re: [PATCH v3 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
> >>
> >>
> >> Hi Shenwei,
> >>
> >> thanks for your patch.
> >>
> >> On 7/31/23 18:19, 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).
> >>
> >> If this is only required for some devices, is it safe to enforce this
> >> behaviour unconditionally for any kind of fixed link devices
> >> connected to the MX93 EQOS or could this possibly break for other devices?
> >>
> >
> > It won't impact normal devices. The link layer hasn't built up yet.
> >
>
> As Russel suggested in [1] - maybe you could rephrase your commit message for
> your v4 to point this out to future reviewers (apparently multiple people have
> had questions about this...)  and have this fact also recorded in the git log later
> on.
>

Okay.

> Also: does this only apply to i.MX93, or would we have to test and enable it on
> e.g. i.MX8MP as well?
>

Yes, it is required when the EQOS MAC is selected. However, this patch just enables
The feature on i.MX93.

Thanks,
Shenwei

> Thanks
> Johannes
>
> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
>
>
> > Thanks,
> > Shenwei
> >
> >> Best regards
> >> Johannes
> >>
> >>>
> >>> 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 <shenwei.wang@nxp.com>
> >>> Reviewed-by: Frank Li <frank.li@nxp.com>
> >>> ---
> >>>    .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
> +++++++++++++++++++
> >>>    1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> index 53ee5a42c071..2e4173d099f3 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> @@ -32,6 +32,7 @@
> >>>    #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
> >>>
> >>>    #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
> >>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
> >>>    #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
> >>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
> >>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
> >>> @@ -40,6 +41,7 @@
> >>>    #define DMA_BUS_MODE                        0x00001000
> >>>    #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
> >>>    #define RMII_RESET_SPEED            (0x3 << 14)
> >>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
> >>>
> >>>    struct imx_dwmac_ops {
> >>>        u32 addr_width;
> >>> @@ -56,6 +58,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 +215,42 @@
> >>> 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_MASK;
> >>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> >>> +             return;
> >>> +
> >>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> >>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> >>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> >>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> >>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> >>> +
> >>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> >>> +      * reordering.
> >>> +      */
> >>> +     wmb();
> >>> +
> >>> +     usleep_range(10, 20);
> >>> +     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
> >>> +356,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)
> >>
> >> --
> >> Pengutronix e.K.                | Johannes Zink                  |
> >> Steuerwalder Str. 21            |
> >> https://www/
> >> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a461
> ee01408
> >>
> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
> 36335
> >>
> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJ
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
> DPOaH5C
> >> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
> >>
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
> >>
> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> >>
> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> >>
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> >>
> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
> >> eserved=0    |
> >> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> >> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >
> >
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3D&r
> eserved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
  

Hi Shenwei,

On 8/2/23 16:27, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Johannes Zink <j.zink@pengutronix.de>
>> Sent: Wednesday, August 2, 2023 1:26 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel
>> Holland <samuel@sholland.org>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>> <jbrunet@baylibre.com>; Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
>> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
>> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>> Subject: Re: [EXT] Re: [PATCH v3 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
>>
>>
>> Hi Shenwei,
>>
>> On 8/1/23 19:10, Shenwei Wang wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Johannes Zink <j.zink@pengutronix.de>
>>>> Sent: Tuesday, August 1, 2023 7:48 AM
>>>> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel Holland
>>>> <samuel@sholland.org>
>>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>>>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>>>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>>>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>>>> <jbrunet@baylibre.com>; Martin Blumenstingl
>>>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>>>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>>>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>>>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>>>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>>>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
>>>> Jochen Henneberg <jh@henneberg-systemdesign.com>;
>>>> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
>>>> linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>>>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>>>> Subject: [EXT] Re: [PATCH v3 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
>>>>
>>>>
>>>> Hi Shenwei,
>>>>
>>>> thanks for your patch.
>>>>
>>>> On 7/31/23 18:19, 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).
>>>>
>>>> If this is only required for some devices, is it safe to enforce this
>>>> behaviour unconditionally for any kind of fixed link devices
>>>> connected to the MX93 EQOS or could this possibly break for other devices?
>>>>
>>>
>>> It won't impact normal devices. The link layer hasn't built up yet.
>>>
>>
>> As Russel suggested in [1] - maybe you could rephrase your commit message for
>> your v4 to point this out to future reviewers (apparently multiple people have
>> had questions about this...)  and have this fact also recorded in the git log later
>> on.
>>
> 
> Okay.
> 
>> Also: does this only apply to i.MX93, or would we have to test and enable it on
>> e.g. i.MX8MP as well?
>>
> 
> Yes, it is required when the EQOS MAC is selected. However, this patch just enables
> The feature on i.MX93.

If this behaviour is required on all EQOS, I think the name 
imx_dwmac_fix_speed_mx93() is misleading. It should either be 
imx_dwmac_fix_speed() if applicable to all imx implementations, or 
dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all 
implementations in general.

You can then add a second patch for enabling it for the i.mx93 in the gluecode 
driver.

Johannes


> 
> Thanks,
> Shenwei
> 
>> Thanks
>> Johannes
>>
>> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
>>
>>
>>> Thanks,
>>> Shenwei
>>>
>>>> Best regards
>>>> Johannes
>>>>
>>>>>
>>>>> 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 <shenwei.wang@nxp.com>
>>>>> Reviewed-by: Frank Li <frank.li@nxp.com>
>>>>> ---
>>>>>     .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
>> +++++++++++++++++++
>>>>>     1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> index 53ee5a42c071..2e4173d099f3 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> @@ -32,6 +32,7 @@
>>>>>     #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
>>>>>
>>>>>     #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
>>>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
>>>>> @@ -40,6 +41,7 @@
>>>>>     #define DMA_BUS_MODE                        0x00001000
>>>>>     #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
>>>>>     #define RMII_RESET_SPEED            (0x3 << 14)
>>>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
>>>>>
>>>>>     struct imx_dwmac_ops {
>>>>>         u32 addr_width;
>>>>> @@ -56,6 +58,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 +215,42 @@
>>>>> 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_MASK;
>>>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
>>>>> +             return;
>>>>> +
>>>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
>>>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
>>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
>>>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
>>>>> +
>>>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
>>>>> +      * reordering.
>>>>> +      */
>>>>> +     wmb();
>>>>> +
>>>>> +     usleep_range(10, 20);
>>>>> +     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
>>>>> +356,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)
>>>>
>>>> --
>>>> Pengutronix e.K.                | Johannes Zink                  |
>>>> Steuerwalder Str. 21            |
>>>> https://www/
>>>> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a461
>> ee01408
>>>>
>> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
>> 36335
>>>>
>> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJ
>>>>
>> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
>> DPOaH5C
>>>> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
>>>>
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
>>>>
>> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>>>
>> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>>>>
>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
>>>>
>> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
>>>> eserved=0    |
>>>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>>>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
>>>
>>>
>>
>> --
>> Pengutronix e.K.                | Johannes Zink                  |
>> Steuerwalder Str. 21            |
>> https://www.pe/
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
>> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
>> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
>> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3D&r
>> eserved=0    |
>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 
>
  

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Wednesday, August 2, 2023 9:40 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v3 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
>
>
> Hi Shenwei,
>
> On 8/2/23 16:27, Shenwei Wang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Johannes Zink <j.zink@pengutronix.de>
> >> Sent: Wednesday, August 2, 2023 1:26 AM
> >> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel Holland
> >> <samuel@sholland.org>
> >> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> >> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> >> <jbrunet@baylibre.com>; Martin Blumenstingl
> >> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> >> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> >> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> >> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> >> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> >> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
> >> Jochen Henneberg <jh@henneberg-systemdesign.com>;
> >> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> >> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> >> Subject: Re: [EXT] Re: [PATCH v3 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
> >>
> >>
> >> Hi Shenwei,
> >>
> >> On 8/1/23 19:10, Shenwei Wang wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Johannes Zink <j.zink@pengutronix.de>
> >>>> Sent: Tuesday, August 1, 2023 7:48 AM
> >>>> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel Holland
> >>>> <samuel@sholland.org>
> >>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >>>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >>>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> >>>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome
> >>>> Brunet <jbrunet@baylibre.com>; Martin Blumenstingl
> >>>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> >>>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> >>>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> >>>> <simon.horman@corigine.com>; Andrew Halaney
> <ahalaney@redhat.com>;
> >>>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> >>>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
> >>>> Jochen Henneberg <jh@henneberg-systemdesign.com>;
> >>>> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
> >>>> linux-arm-kernel@lists.infradead.org;
> >>>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> >>>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> >>>> Subject: [EXT] Re: [PATCH v3 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
> >>>>
> >>>>
> >>>> Hi Shenwei,
> >>>>
> >>>> thanks for your patch.
> >>>>
> >>>> On 7/31/23 18:19, 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).
> >>>>
> >>>> If this is only required for some devices, is it safe to enforce
> >>>> this behaviour unconditionally for any kind of fixed link devices
> >>>> connected to the MX93 EQOS or could this possibly break for other devices?
> >>>>
> >>>
> >>> It won't impact normal devices. The link layer hasn't built up yet.
> >>>
> >>
> >> As Russel suggested in [1] - maybe you could rephrase your commit
> >> message for your v4 to point this out to future reviewers (apparently
> >> multiple people have had questions about this...)  and have this fact
> >> also recorded in the git log later on.
> >>
> >
> > Okay.
> >
> >> Also: does this only apply to i.MX93, or would we have to test and
> >> enable it on e.g. i.MX8MP as well?
> >>
> >
> > Yes, it is required when the EQOS MAC is selected. However, this patch
> > just enables The feature on i.MX93.
>
> If this behaviour is required on all EQOS, I think the name
> imx_dwmac_fix_speed_mx93() is misleading. It should either be
> imx_dwmac_fix_speed() if applicable to all imx implementations, or
> dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all
> implementations in general.
>

It has the general fix_speed function there named imx_dwmac_fix_speed.
This one is the special for this mx93 fix.

Thanks,
Shenwei


> You can then add a second patch for enabling it for the i.mx93 in the gluecode
> driver.
>
> Johannes
>
>
> >
> > Thanks,
> > Shenwei
> >
> >> Thanks
> >> Johannes
> >>
> >> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
> >>
> >>
> >>> Thanks,
> >>> Shenwei
> >>>
> >>>> Best regards
> >>>> Johannes
> >>>>
> >>>>>
> >>>>> 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 <shenwei.wang@nxp.com>
> >>>>> Reviewed-by: Frank Li <frank.li@nxp.com>
> >>>>> ---
> >>>>>     .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
> >> +++++++++++++++++++
> >>>>>     1 file changed, 42 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> index 53ee5a42c071..2e4173d099f3 100644
> >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> @@ -32,6 +32,7 @@
> >>>>>     #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
> >>>>>
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
> >>>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
> >>>>> @@ -40,6 +41,7 @@
> >>>>>     #define DMA_BUS_MODE                        0x00001000
> >>>>>     #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
> >>>>>     #define RMII_RESET_SPEED            (0x3 << 14)
> >>>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
> >>>>>
> >>>>>     struct imx_dwmac_ops {
> >>>>>         u32 addr_width;
> >>>>> @@ -56,6 +58,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 +215,42
> >>>>> @@ 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_MASK;
> >>>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> >>>>> +             return;
> >>>>> +
> >>>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> >>>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> >>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> >>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> >>>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> >>>>> +
> >>>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> >>>>> +      * reordering.
> >>>>> +      */
> >>>>> +     wmb();
> >>>>> +
> >>>>> +     usleep_range(10, 20);
> >>>>> +     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
> >>>>> +356,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)
> >>>>
> >>>> --
> >>>> Pengutronix e.K.                | Johannes Zink                  |
> >>>> Steuerwalder Str. 21            |
> >>>> https://www/
> >>>> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a4
> 61
> >> ee01408
> >>>>
> >>
> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
> >> 36335
> >>>>
> >>
> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> >> zIiLCJ
> >>>>
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
> >> DPOaH5C
> >>>> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
> >>>>
> >>
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
> >>>>
> >>
> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> >>>>
> >>
> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> >>>>
> >>
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> >>>>
> >>
> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
> >>>> eserved=0    |
> >>>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> >>>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >>>
> >>>
> >>
> >> --
> >> Pengutronix e.K.                | Johannes Zink                  |
> >> Steuerwalder Str. 21            |
> >> https://www/
> >> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cdc64404f8c2c4e
> b87a7808
> >>
> db93666ec9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826584
> 03801
> >>
> 74614%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJ
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oxLnb3ppqjhMti
> cQH7P
> >> lfRbIlYJ2R1Z8Tg7Bz2vC%2F%2Bc%3D&reserved=0
> >>
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
> >>
> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> >>
> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> >>
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> >> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3
> D&r
> >> eserved=0    |
> >> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> >> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >
> >
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cdc64404f8
> c2c4eb87a7808db93666ec9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638265840380174614%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=r8tFe0Ts3ev2c7lg3MK0Qc40101d7W%2BEwnpmvMDwjho%3D&res
> erved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
  

Hi Shenwei,

[snip]
>>>
>>>> Also: does this only apply to i.MX93, or would we have to test and
>>>> enable it on e.g. i.MX8MP as well?
>>>>
>>>
>>> Yes, it is required when the EQOS MAC is selected. However, this patch
>>> just enables The feature on i.MX93.
>>
>> If this behaviour is required on all EQOS, I think the name
>> imx_dwmac_fix_speed_mx93() is misleading. It should either be
>> imx_dwmac_fix_speed() if applicable to all imx implementations, or
>> dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all
>> implementations in general.
>>
> 
> It has the general fix_speed function there named imx_dwmac_fix_speed.
> This one is the special for this mx93 fix.

I think I might have misunderstood your last statement or I failed to express 
my point. If you need to replace the dwmac_fix_speed() on mx93, because this 
SoC implementation requires doing so (the usual reason for doing something like 
this is something like reset quirks because of screwed up IP Core integration), 
then your approach is imho valid.

But if I got your last comment right, your changes should apply to EQOS MAC in 
general (but you want to only enable it for mx93 at the moment). In this case 
this quirk will later be as the fix_mac_speed function for other hardware as 
well, in which case the name ..._mx93 is misleading, and imho rather a 
descriptive name should be used (i.e. have the name describe what it does 
rather than for what hardware it is implemented).

Except if the maintainers have a strong opinion that the ..._mx93 suffix 
version is exactly how you should proceed...

Best regards
Johannes

> 
> Thanks,
> Shenwei
> [snip]
  

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Thursday, August 3, 2023 1:36 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; 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 <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> > It has the general fix_speed function there named imx_dwmac_fix_speed.
> > This one is the special for this mx93 fix.
>
> I think I might have misunderstood your last statement or I failed to express my
> point. If you need to replace the dwmac_fix_speed() on mx93, because this SoC
> implementation requires doing so (the usual reason for doing something like this
> is something like reset quirks because of screwed up IP Core integration), then
> your approach is imho valid.
>
> But if I got your last comment right, your changes should apply to EQOS MAC in
> general (but you want to only enable it for mx93 at the moment). In this case
> this quirk will later be as the fix_mac_speed function for other hardware as well,
> in which case the name ..._mx93 is misleading, and imho rather a descriptive
> name should be used (i.e. have the name describe what it does rather than for
> what hardware it is implemented).
>

The idea of supporting the SJA1105 is general, but the implementation depends on the
specific SoC. This patch provides the implementation for the MX93 SoC. To support the
MX8x SoCs, the implementation would need to be rewritten based on the current state
of the dwmac-imx driver.

I agree the function name is somewhat ugly. Maybe a better name could be:
mx93_dwmac_fix_speed()

The assumption is that the process of pausing the clock can still be considered part of fixing the speed.

Thanks,
Shenwei

> Except if the maintainers have a strong opinion that the ..._mx93 suffix version
> is exactly how you should proceed...
>
> Best regards
> Johannes
>
> >
> > Thanks,
> > Shenwei
> > [snip]
>
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cd328d89d
> 14e847270d5a08db93ebff01%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638266414027048795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=dkE9Es7kl3uNYx8zJYZt2r6933dSNtYDpQzCEagAV3M%3D&reserved
> =0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |