Re: [PATCH net-next v6 3/6] net: stmmac: Refactor FPE functions to generic version

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Oct 30, 2024 at 01:36:12PM +0800, Furong Xu wrote:
> FPE implementation for DWMAC4 and DWXGMAC differs only for:
> 1) Offset address of MAC_FPE_CTRL_STS and MTL_FPE_CTRL_STS
> 2) FPRQ(Frame Preemption Residue Queue) field in MAC_RxQ_Ctrl1
> 3) Bit offset of Frame Preemption Interrupt Enable
> 
> Refactor FPE functions to avoid code duplication.

I would add "and to simplify the code flow by avoiding the use of
function pointers".

> 
> Signed-off-by: Furong Xu <0x1207@gmail.com>
> ---
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> index 70ea475046f0..ee86658f77b4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> @@ -27,6 +27,20 @@
>  #define STMMAC_MAC_FPE_CTRL_STS_SVER	BIT(1)
>  #define STMMAC_MAC_FPE_CTRL_STS_EFPE	BIT(0)
>  
> +struct stmmac_fpe_reg {
> +	const u32 mac_fpe_reg;		/* offset of MAC_FPE_CTRL_STS */
> +	const u32 mtl_fpe_reg;		/* offset of MTL_FPE_CTRL_STS */
> +	const u32 rxq_ctrl1_reg;	/* offset of MAC_RxQ_Ctrl1 */
> +	const u32 fprq_mask;		/* Frame Preemption Residue Queue */
> +	const u32 int_en_reg;		/* offset of MAC_Interrupt_Enable */
> +	const u32 int_en_bit;		/* Frame Preemption Interrupt Enable */
> +};
> +
> +bool stmmac_fpe_supported(struct stmmac_priv *priv)
> +{
> +	return (priv->dma_cap.fpesel && priv->fpe_cfg.reg);
> +}

This is a separate logical change from just refactoring. Refactoring
changes (which are noisy) should not have functional changes. Could
the introduction and use of stmmac_fpe_supported() please be a separate
patch?

Also, parentheses are not necessary.

> +
>  void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  {
>  	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> @@ -38,25 +52,19 @@ void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  
>  	if (is_up && fpe_cfg->pmac_enabled) {
>  		/* VERIFY process requires pmac enabled when NIC comes up */
> -		stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> -				     priv->plat->tx_queues_to_use,
> -				     priv->plat->rx_queues_to_use,
> -				     false, true);
> +		stmmac_fpe_configure(priv, false, true);
>  
>  		/* New link => maybe new partner => new verification process */
>  		stmmac_fpe_apply(priv);
>  	} else {
>  		/* No link => turn off EFPE */
> -		stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> -				     priv->plat->tx_queues_to_use,
> -				     priv->plat->rx_queues_to_use,
> -				     false, false);
> +		stmmac_fpe_configure(priv, false, false);
>  	}
>  
>  	spin_unlock_irqrestore(&fpe_cfg->lock, flags);
>  }
>  
> -void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
> +static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
>  {
>  	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
>  
> @@ -68,8 +76,7 @@ void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
>  
>  	/* LP has sent verify mPacket */
>  	if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER)
> -		stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
> -					MPACKET_RESPONSE);
> +		stmmac_fpe_send_mpacket(priv, MPACKET_RESPONSE);
>  
>  	/* Local has sent verify mPacket */
>  	if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER &&
> @@ -107,8 +114,7 @@ static void stmmac_fpe_verify_timer(struct timer_list *t)
>  	case ETHTOOL_MM_VERIFY_STATUS_INITIAL:
>  	case ETHTOOL_MM_VERIFY_STATUS_VERIFYING:
>  		if (fpe_cfg->verify_retries != 0) {
> -			stmmac_fpe_send_mpacket(priv, priv->ioaddr,
> -						fpe_cfg, MPACKET_VERIFY);
> +			stmmac_fpe_send_mpacket(priv, MPACKET_VERIFY);
>  			rearm = true;
>  		} else {
>  			fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> @@ -118,10 +124,7 @@ static void stmmac_fpe_verify_timer(struct timer_list *t)
>  		break;
>  
>  	case ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED:
> -		stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
> -				     priv->plat->tx_queues_to_use,
> -				     priv->plat->rx_queues_to_use,
> -				     true, true);
> +		stmmac_fpe_configure(priv, true, true);
>  		break;
>  
>  	default:
> @@ -154,6 +157,9 @@ void stmmac_fpe_init(struct stmmac_priv *priv)
>  	priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
>  	timer_setup(&priv->fpe_cfg.verify_timer, stmmac_fpe_verify_timer, 0);
>  	spin_lock_init(&priv->fpe_cfg.lock);
> +
> +	if (priv->dma_cap.fpesel && !priv->fpe_cfg.reg)
> +		dev_info(priv->device, "FPE on this MAC is not supported by driver.\n");

This as well.

>  }
>  
>  void stmmac_fpe_apply(struct stmmac_priv *priv)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> index 25725fd5182f..00e616d7cbf1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> @@ -22,24 +22,21 @@ struct stmmac_priv;
>  struct stmmac_fpe_cfg;
>  
>  void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up);
> -void stmmac_fpe_event_status(struct stmmac_priv *priv, int status);
> +bool stmmac_fpe_supported(struct stmmac_priv *priv);
>  void stmmac_fpe_init(struct stmmac_priv *priv);
>  void stmmac_fpe_apply(struct stmmac_priv *priv);
> -
> -void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> -			  u32 num_txq, u32 num_rxq,
> -			  bool tx_enable, bool pmac_enable);
> -void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
> -			     struct stmmac_fpe_cfg *cfg,
> +void stmmac_fpe_configure(struct stmmac_priv *priv, bool tx_enable,
> +			  bool pmac_enable);
> +void stmmac_fpe_send_mpacket(struct stmmac_priv *priv,
>  			     enum stmmac_mpacket_type type);

Sorry I noticed this just now. After the refactoring, stmmac_fpe_send_mpacket()
is only used from stmmac_fpe.c, and thus can be unexported and made static.
Same goes for enum stmmac_mpacket_type.

> -int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev);
> -int dwmac5_fpe_get_add_frag_size(const void __iomem *ioaddr);
> -void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size);
> +void stmmac_fpe_irq_status(struct stmmac_priv *priv);
> +int stmmac_fpe_get_add_frag_size(struct stmmac_priv *priv);
> +void stmmac_fpe_set_add_frag_size(struct stmmac_priv *priv, u32 add_frag_size);
> +
>  int dwmac5_fpe_map_preemption_class(struct net_device *ndev,
>  				    struct netlink_ext_ack *extack, u32 pclass);
>  
> -void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> -			    u32 num_txq, u32 num_rxq,
> -			    bool tx_enable, bool pmac_enable);
> +extern const struct stmmac_fpe_reg dwmac5_fpe_reg;
> +extern const struct stmmac_fpe_reg dwxgmac3_fpe_reg;
>  
>  #endif