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

[Simon Horman <simon.horman@corigine.com>]

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?