| Message ID | 20230321190921.3113971-1-shenwei.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] net: stmmac: add support for platform specific reset | expand |
On Tue, Mar 21, 2023 at 02:09:20PM -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 macro 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> > --- > drivers/net/ethernet/stmicro/stmmac/hwif.h | 10 +++++++++- > include/linux/stmmac.h | 1 + > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h > index 16a7421715cb..e24ce870690e 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h > @@ -215,7 +215,15 @@ struct stmmac_dma_ops { > }; > > #define stmmac_reset(__priv, __args...) \ > - stmmac_do_callback(__priv, dma, reset, __args) > +({ \ > + int __result = -EINVAL; \ > + if ((__priv) && (__priv)->plat && (__priv)->plat->fix_soc_reset) { \ > + __result = (__priv)->plat->fix_soc_reset((__priv)->plat, ##__args); \ > + } else { \ > + __result = stmmac_do_callback(__priv, dma, reset, __args); \ > + } \ > + __result; \ > +}) Hi Shenwei Wang, I am wondering if any consideration was given to an approach that has a bit better type safety. Something like this (*compile tested only!*): static inline 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); } In which case the first parameter of the fix_soc_reset field of struct plat_stmmacenet_data can become struct plat_stmmacenet_data *. diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index 9044477fad61..b26ade7e4be8 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -223,6 +223,8 @@ 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)(struct plat_stmmacenet_data *, + 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); I do see that the approach you have is in keeping with the existing structure of stmmac_do_callback(). But I guess my question there is: why is that model used? And could there be a plan to move away from that model?
> -----Original Message----- > From: Simon Horman <simon.horman@corigine.com> > Sent: Wednesday, March 22, 2023 11:33 AM > 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>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; > Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Sascha > Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; Wong Vee > Khee <veekhee@apple.com>; Jon Hunter <jonathanh@nvidia.com>; > Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Andrey > Konovalov <andrey.konovalov@linaro.org>; Revanth Kumar Uppala > <ruppala@nvidia.com>; Tan Tee Min <tee.min.tan@linux.intel.com>; > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux- > arm-kernel@lists.infradead.org; imx@lists.linux.dev > Subject: [EXT] Re: [PATCH 1/1] net: stmmac: add support for platform specific > reset > > Caution: EXT Email > > On Tue, Mar 21, 2023 at 02:09:20PM -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 macro 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> > > --- > > drivers/net/ethernet/stmicro/stmmac/hwif.h | 10 +++++++++- > > include/linux/stmmac.h | 1 + > > 2 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h > > b/drivers/net/ethernet/stmicro/stmmac/hwif.h > > index 16a7421715cb..e24ce870690e 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h > > @@ -215,7 +215,15 @@ struct stmmac_dma_ops { }; > > > > #define stmmac_reset(__priv, __args...) \ > > - stmmac_do_callback(__priv, dma, reset, __args) > > +({ \ > > + int __result = -EINVAL; \ > > + if ((__priv) && (__priv)->plat && (__priv)->plat->fix_soc_reset) { \ > > + __result = (__priv)->plat->fix_soc_reset((__priv)->plat, ##__args); \ > > + } else { \ > > + __result = stmmac_do_callback(__priv, dma, reset, __args); \ > > + } \ > > + __result; \ > > +}) > > Hi Shenwei Wang, > > I am wondering if any consideration was given to an approach that has a bit > better type safety. > The patch aims to add the missing feature to the original source file in a consistent coding style. I am also okay to use the function prototype if everyone agrees. Thanks, Shenwei > Something like this (*compile tested only!*): > > static inline 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); } > > In which case the first parameter of the fix_soc_reset field of struct > plat_stmmacenet_data can become struct plat_stmmacenet_data *. > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index > 9044477fad61..b26ade7e4be8 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -223,6 +223,8 @@ 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)(struct plat_stmmacenet_data *, > + 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); > > I do see that the approach you have is > in keeping with the existing structure of stmmac_do_callback(). > But I guess my question there is: why is that model used? > And could there be a plan to move away from that model?
> -----Original Message----- > From: Shenwei Wang <shenwei.wang@nxp.com> > Sent: Tuesday, March 21, 2023 2:09 PM > To: 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>; dl-linux-imx <linux-imx@nxp.com> > Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Sascha > Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; Wong Vee > Khee <veekhee@apple.com>; Jon Hunter <jonathanh@nvidia.com>; > Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Andrey > Konovalov <andrey.konovalov@linaro.org>; Revanth Kumar Uppala > <ruppala@nvidia.com>; Tan Tee Min <tee.min.tan@linux.intel.com>; Shenwei > Wang <shenwei.wang@nxp.com>; netdev@vger.kernel.org; linux-stm32@st- > md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; > imx@lists.linux.dev > Subject: [PATCH 1/1] net: stmmac: add support for platform specific reset > > 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 macro 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. > A soft ping.
On Thu, 30 Mar 2023 14:55:02 +0000 Shenwei Wang wrote:
> A soft ping.
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index 16a7421715cb..e24ce870690e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -215,7 +215,15 @@ struct stmmac_dma_ops { }; #define stmmac_reset(__priv, __args...) \ - stmmac_do_callback(__priv, dma, reset, __args) +({ \ + int __result = -EINVAL; \ + if ((__priv) && (__priv)->plat && (__priv)->plat->fix_soc_reset) { \ + __result = (__priv)->plat->fix_soc_reset((__priv)->plat, ##__args); \ + } else { \ + __result = stmmac_do_callback(__priv, dma, reset, __args); \ + } \ + __result; \ +}) #define stmmac_dma_init(__priv, __args...) \ stmmac_do_void_callback(__priv, dma, init, __args) #define stmmac_init_chan(__priv, __args...) \ diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index a152678b82b7..9044477fad61 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);
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 macro 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> --- drivers/net/ethernet/stmicro/stmmac/hwif.h | 10 +++++++++- include/linux/stmmac.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-)