[PATCH v5 1/2] net: stmmac: add support for platform specific reset

[PATCH v5 1/2] net: stmmac: add support for platform specific reset

[Shenwei Wang]

This patch adds support for platform-specific reset logic in the
stmmac driver. Some SoCs require a different reset mechanism than
the standard dwmac IP reset. To support these platforms, a new function
pointer 'fix_soc_reset' is added to the plat_stmmacenet_data structure.
The stmmac_reset in hwif.h is modified to call the 'fix_soc_reset'
function if it exists. This enables the driver to use the platform-specific
reset logic when necessary.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 v5:
  - add the missing __iomem tag in the stmmac_reset definition.

 drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
 include/linux/stmmac.h                     |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
index bb7114f970f8..0eefa697ffe8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
@@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv)
 	return 0;
 }

+int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr)
+{
+	struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;
+
+	if (plat && plat->fix_soc_reset)
+		return plat->fix_soc_reset(plat, ioaddr);
+
+	return stmmac_do_callback(priv, dma, reset, ioaddr);
+}
+
 static const struct stmmac_hwif_entry {
 	bool gmac;
 	bool gmac4;
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 16a7421715cb..1cc286b000b6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -214,8 +214,6 @@ struct stmmac_dma_ops {
 	int (*enable_tbs)(void __iomem *ioaddr, bool en, u32 chan);
 };

-#define stmmac_reset(__priv, __args...) \
-	stmmac_do_callback(__priv, dma, reset, __args)
 #define stmmac_dma_init(__priv, __args...) \
 	stmmac_do_void_callback(__priv, dma, init, __args)
 #define stmmac_init_chan(__priv, __args...) \
@@ -640,6 +638,7 @@ extern const struct stmmac_mmc_ops dwxgmac_mmc_ops;
 #define GMAC_VERSION		0x00000020	/* GMAC CORE Version */
 #define GMAC4_VERSION		0x00000110	/* GMAC4+ CORE Version */

+int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr);
 int stmmac_hwif_init(struct stmmac_priv *priv);

 #endif /* __STMMAC_HWIF_H__ */
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index a2414c187483..dafa001e9e7a 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -223,6 +223,7 @@ struct plat_stmmacenet_data {
 	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);
+	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);
 	void (*speed_mode_2500)(struct net_device *ndev, void *priv);
--
2.34.1

[PATCH v5 2/2] net: stmmac: dwmac-imx: use platform specific reset for imx93 SoCs

[Shenwei Wang]

The patch addresses an issue with the reset logic on the i.MX93 SoC, which
requires configuration of the correct interface speed under RMII mode to
complete the reset. The patch implements a fix_soc_reset function and uses
it specifically for the i.MX93 SoCs.

Reviewed-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 2a2be65d65a0..465de3392e4e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -37,10 +37,15 @@
 #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
 #define MX93_GPR_ENET_QOS_CLK_GEN_EN		(0x1 << 0)
 
+#define DMA_BUS_MODE			0x00001000
+#define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
+#define RMII_RESET_SPEED		(0x3 << 14)
+
 struct imx_dwmac_ops {
 	u32 addr_width;
 	bool mac_rgmii_txclk_auto_adj;
 
+	int (*fix_soc_reset)(void *priv, void __iomem *ioaddr);
 	int (*set_intf_mode)(struct plat_stmmacenet_data *plat_dat);
 };
 
@@ -207,6 +212,25 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed)
 		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
 }
 
+static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
+{
+	u32 value = readl(ioaddr + DMA_BUS_MODE);
+	struct plat_stmmacenet_data *plat_dat = priv;
+
+	/* DMA SW reset */
+	value |= DMA_BUS_MODE_SFT_RESET;
+	writel(value, ioaddr + DMA_BUS_MODE);
+
+	if (plat_dat->interface == PHY_INTERFACE_MODE_RMII) {
+		usleep_range(100, 200);
+		writel(RMII_RESET_SPEED, ioaddr + MAC_CTRL_REG);
+	}
+
+	return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
+				 !(value & DMA_BUS_MODE_SFT_RESET),
+				 10000, 1000000);
+}
+
 static int
 imx_dwmac_parse_dt(struct imx_priv_data *dwmac, struct device *dev)
 {
@@ -304,6 +328,8 @@ static int imx_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_dwmac_init;
 
+	dwmac->plat_dat->fix_soc_reset = dwmac->ops->fix_soc_reset;
+
 	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
 	if (ret)
 		goto err_drv_probe;
@@ -337,6 +363,7 @@ static struct imx_dwmac_ops imx93_dwmac_data = {
 	.addr_width = 32,
 	.mac_rgmii_txclk_auto_adj = true,
 	.set_intf_mode = imx93_set_intf_mode,
+	.fix_soc_reset = imx_dwmac_mx93_reset,
 };
 
 static const struct of_device_id imx_dwmac_match[] = {
-- 
2.34.1

Re: [PATCH v5 2/2] net: stmmac: dwmac-imx: use platform specific reset for imx93 SoCs

[Simon Horman]

On Mon, Apr 03, 2023 at 10:24:08AM -0500, Shenwei Wang wrote:
> The patch addresses an issue with the reset logic on the i.MX93 SoC, which
> requires configuration of the correct interface speed under RMII mode to
> complete the reset. The patch implements a fix_soc_reset function and uses
> it specifically for the i.MX93 SoCs.
> 
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 2a2be65d65a0..465de3392e4e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -37,10 +37,15 @@
>  #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
>  #define MX93_GPR_ENET_QOS_CLK_GEN_EN		(0x1 << 0)
>  
> +#define DMA_BUS_MODE			0x00001000
> +#define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
> +#define RMII_RESET_SPEED		(0x3 << 14)
> +
>  struct imx_dwmac_ops {
>  	u32 addr_width;
>  	bool mac_rgmii_txclk_auto_adj;
>  
> +	int (*fix_soc_reset)(void *priv, void __iomem *ioaddr);
>  	int (*set_intf_mode)(struct plat_stmmacenet_data *plat_dat);
>  };
>  
> @@ -207,6 +212,25 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed)
>  		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
>  }
>  
> +static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
> +{
> +	u32 value = readl(ioaddr + DMA_BUS_MODE);
> +	struct plat_stmmacenet_data *plat_dat = priv;
> +

nit: reverse xmas tree - longest line to shortest - for local variable
     declarations.

...

Re: [PATCH v5 1/2] net: stmmac: add support for platform specific reset

[Simon Horman]

On Mon, Apr 03, 2023 at 10:24:07AM -0500, Shenwei Wang wrote:
> This patch adds support for platform-specific reset logic in the
> stmmac driver. Some SoCs require a different reset mechanism than
> the standard dwmac IP reset. To support these platforms, a new function
> pointer 'fix_soc_reset' is added to the plat_stmmacenet_data structure.
> The stmmac_reset in hwif.h is modified to call the 'fix_soc_reset'
> function if it exists. This enables the driver to use the platform-specific
> reset logic when necessary.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  v5:
>   - add the missing __iomem tag in the stmmac_reset definition.
> 
>  drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
>  drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
>  include/linux/stmmac.h                     |  1 +
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index bb7114f970f8..0eefa697ffe8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv)
>  	return 0;
>  }
> 
> +int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr)
> +{
> +	struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;

Here the case where priv is NULL is handled.

> +
> +	if (plat && plat->fix_soc_reset)
> +		return plat->fix_soc_reset(plat, ioaddr);
> +
> +	return stmmac_do_callback(priv, dma, reset, ioaddr);

But this will dereference priv unconditionally.

I think perhaps this is code that I suggested.
If so, sorry about not noticing this then.

> +}
> +
>  static const struct stmmac_hwif_entry {
>  	bool gmac;
>  	bool gmac4;

...

RE: [EXT] Re: [PATCH v5 1/2] net: stmmac: add support for platform specific reset

[Shenwei Wang]

> -----Original Message-----
> From: Simon Horman <simon.horman@corigine.com>
> Sent: Monday, April 3, 2023 2:50 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Pengutronix Kernel Team <kernel@pengutronix.de>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Fabio
> Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Wong Vee Khee
> <veekhee@apple.com>; Kurt Kanzenbach <kurt@linutronix.de>; Mohammad
> Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Andrey Konovalov
> <andrey.konovalov@linaro.org>; Jochen Henneberg <jh@henneberg-
> systemdesign.com>; Tan Tee Min <tee.min.tan@linux.intel.com>;
> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-stm32@st-
> md-mailman.stormreply.com; imx@lists.linux.dev
> Subject: [EXT] Re: [PATCH v5 1/2] net: stmmac: add support for platform specific
> reset
> 
> Caution: EXT Email
> 
> On Mon, Apr 03, 2023 at 10:24:07AM -0500, Shenwei Wang wrote:
> > This patch adds support for platform-specific reset logic in the
> > stmmac driver. Some SoCs require a different reset mechanism than the
> > standard dwmac IP reset. To support these platforms, a new function
> > pointer 'fix_soc_reset' is added to the plat_stmmacenet_data structure.
> > The stmmac_reset in hwif.h is modified to call the 'fix_soc_reset'
> > function if it exists. This enables the driver to use the
> > platform-specific reset logic when necessary.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > ---
> >  v5:
> >   - add the missing __iomem tag in the stmmac_reset definition.
> >
> >  drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
> > drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
> >  include/linux/stmmac.h                     |  1 +
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > index bb7114f970f8..0eefa697ffe8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > @@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv
> *priv)
> >       return 0;
> >  }
> >
> > +int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr) {
> > +     struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;
> 
> Here the case where priv is NULL is handled.
> 
> > +
> > +     if (plat && plat->fix_soc_reset)
> > +             return plat->fix_soc_reset(plat, ioaddr);
> > +
> > +     return stmmac_do_callback(priv, dma, reset, ioaddr);
> 
> But this will dereference priv unconditionally.
> 

The original macro implementation assumes that the priv pointer will not be NULL. However, adding 
an extra condition check for priv in the stmmac_reset() function can ensure that the code is more 
robust and secure.

Thanks,
Shenwei

> I think perhaps this is code that I suggested.
> If so, sorry about not noticing this then.
> 
> > +}
> > +
> >  static const struct stmmac_hwif_entry {
> >       bool gmac;
> >       bool gmac4;
> 
> ...

Re: [EXT] Re: [PATCH v5 1/2] net: stmmac: add support for platform specific reset

[Simon Horman]

On Mon, Apr 03, 2023 at 10:16:46PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <simon.horman@corigine.com>
> > Sent: Monday, April 3, 2023 2:50 PM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> > <pabeni@redhat.com>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> > <s.hauer@pengutronix.de>; Pengutronix Kernel Team <kernel@pengutronix.de>;
> > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Fabio
> > Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Maxime
> > Coquelin <mcoquelin.stm32@gmail.com>; Wong Vee Khee
> > <veekhee@apple.com>; Kurt Kanzenbach <kurt@linutronix.de>; Mohammad
> > Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Andrey Konovalov
> > <andrey.konovalov@linaro.org>; Jochen Henneberg <jh@henneberg-
> > systemdesign.com>; Tan Tee Min <tee.min.tan@linux.intel.com>;
> > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-stm32@st-
> > md-mailman.stormreply.com; imx@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v5 1/2] net: stmmac: add support for platform specific
> > reset
> > 
> > Caution: EXT Email
> > 
> > On Mon, Apr 03, 2023 at 10:24:07AM -0500, Shenwei Wang wrote:
> > > This patch adds support for platform-specific reset logic in the
> > > stmmac driver. Some SoCs require a different reset mechanism than the
> > > standard dwmac IP reset. To support these platforms, a new function
> > > pointer 'fix_soc_reset' is added to the plat_stmmacenet_data structure.
> > > The stmmac_reset in hwif.h is modified to call the 'fix_soc_reset'
> > > function if it exists. This enables the driver to use the
> > > platform-specific reset logic when necessary.
> > >
> > > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > > ---
> > >  v5:
> > >   - add the missing __iomem tag in the stmmac_reset definition.
> > >
> > >  drivers/net/ethernet/stmicro/stmmac/hwif.c | 10 ++++++++++
> > > drivers/net/ethernet/stmicro/stmmac/hwif.h |  3 +--
> > >  include/linux/stmmac.h                     |  1 +
> > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > index bb7114f970f8..0eefa697ffe8 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > @@ -87,6 +87,16 @@ static int stmmac_dwxlgmac_quirks(struct stmmac_priv
> > *priv)
> > >       return 0;
> > >  }
> > >
> > > +int stmmac_reset(struct stmmac_priv *priv, void __iomem *ioaddr) {
> > > +     struct plat_stmmacenet_data *plat = priv ? priv->plat : NULL;
> > 
> > Here the case where priv is NULL is handled.
> > 
> > > +
> > > +     if (plat && plat->fix_soc_reset)
> > > +             return plat->fix_soc_reset(plat, ioaddr);
> > > +
> > > +     return stmmac_do_callback(priv, dma, reset, ioaddr);
> > 
> > But this will dereference priv unconditionally.
> > 
> 
> The original macro implementation assumes that the priv pointer will not be NULL. However, adding 
> an extra condition check for priv in the stmmac_reset() function can ensure that the code is more 
> robust and secure.

But it seems to me that it is not safe because stmmac_do_callback
will dereference priv even if it is NULL.

So I think either the NULL case should be handled in a safe way.
Or there is no point in checking for it at all.