Re: [PATCH net-next v1 5/5] net: stmmac: xgmac: Complete FPE support

[Simon Horman <horms@kernel.org>]

On Tue, Oct 15, 2024 at 05:09:26PM +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
> 
> Refactor stmmac_fpe_ops callback functions to avoid code duplication
> between gmac4 and xgmac.
> 
> Signed-off-by: Furong Xu <0x1207@gmail.com>

Hi Furong Xu,

I think it would be best to split this patch so that the refactor of dwmac4
code is in one patch, and adding xgmac code is in another.

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> index 6060a1d702c6..80f12b6e84e6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
> @@ -160,41 +160,54 @@ void stmmac_fpe_apply(struct stmmac_priv *priv)
>  	}
>  }
>  
> -static void dwmac5_fpe_configure(void __iomem *ioaddr,
> -				 struct stmmac_fpe_cfg *cfg,
> -				 u32 num_txq, u32 num_rxq,
> -				 bool tx_enable, bool pmac_enable)
> +static void common_fpe_configure(void __iomem *ioaddr,
> +				 struct stmmac_fpe_cfg *cfg, u32 rxq,
> +				 bool tx_enable, bool pmac_enable,
> +				 u32 rxq_addr, u32 fprq_mask, u32 fprq_shift,
> +				 u32 mac_fpe_addr, u32 int_en_addr,
> +				 u32 int_en_bit)

This function now has a lot of parameters. Could we consider another way?
One idea I had was that describes the addresses for different chips.

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 75ad2da1a37f..6a79e6a111ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -1290,8 +1290,8 @@ const struct stmmac_tc_ops dwxgmac_tc_ops = {
>  	.setup_cls_u32 = tc_setup_cls_u32,
>  	.setup_cbs = tc_setup_cbs,
>  	.setup_cls = tc_setup_cls,
> -	.setup_taprio = tc_setup_taprio_without_fpe,
> +	.setup_taprio = tc_setup_taprio,
>  	.setup_etf = tc_setup_etf,
>  	.query_caps = tc_query_caps,
> -	.setup_mqprio = tc_setup_mqprio_unimplemented,
> +	.setup_mqprio = tc_setup_dwmac510_mqprio,
>  };

It is not clear to me how this hunk relates to the rest of the patch.