Re: [PATCH net-next v1] net: stmmac: Refactor Frame Preemption(FPE) implementation

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Jul 08, 2024 at 04:22:20PM +0800, Furong Xu wrote:
> Refactor FPE implementation by moving common code for DWMAC4 and
> DWXGMAC into a separate FPE module.
> 
> FPE implementation for DWMAC4 and DWXGMAC differs only for:
> 1) Offset address of MAC_FPE_CTRL_STS
> 2) FPRQ(Frame Preemption Residue Queue) field in MAC_RxQ_Ctrl1

This quite a big patch. Could you split it up a bit please.

> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -84,8 +84,8 @@
>  #define XGMAC_MCBCQEN			BIT(15)
>  #define XGMAC_MCBCQ			GENMASK(11, 8)
>  #define XGMAC_MCBCQ_SHIFT		8
> -#define XGMAC_RQ			GENMASK(7, 4)
> -#define XGMAC_RQ_SHIFT			4
> +#define XGMAC_FPRQ			GENMASK(7, 4)
> +#define XGMAC_FPRQ_SHIFT		4

You could for example put renames like this into a patch. It should
then be obviously correct, so quick and easy to review.

> @@ -96,10 +96,11 @@
>  #define XGMAC_LPIIS			BIT(5)
>  #define XGMAC_PMTIS			BIT(4)
>  #define XGMAC_INT_EN			0x000000b4
> +#define XGMAC_FPEIE			BIT(15)
>  #define XGMAC_TSIE			BIT(12)
>  #define XGMAC_LPIIE			BIT(5)
>  #define XGMAC_PMTIE			BIT(4)
> -#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE)
> +#define XGMAC_INT_DEFAULT_EN		(XGMAC_FPEIE | XGMAC_LPIIE | XGMAC_PMTIE)

This change is not obvious. First off, it would of been easier to read
if you put XGMAC_LPIIE at the end. But i would also suggest you make
this a separate patch, and have the commit message explain why this is
needed.

> +static void fpe_configure(struct stmmac_priv *priv, struct stmmac_fpe_cfg *cfg,
> +			  u32 num_txq, u32 num_rxq, bool enable)
> +{
> +	u32 value;
> +
> +	if (enable) {
> +		cfg->fpe_csr = FPE_CTRL_STS_EFPE;
> +		if (priv->plat->has_xgmac) {
> +			value = readl(priv->ioaddr + XGMAC_RXQ_CTRL1);
> +			value &= ~XGMAC_FPRQ;
> +			value |= (num_rxq - 1) << XGMAC_FPRQ_SHIFT;
> +			writel(value, priv->ioaddr + XGMAC_RXQ_CTRL1);
> +		} else if (priv->plat->has_gmac4) {
> +			value = readl(priv->ioaddr + GMAC_RXQ_CTRL1);
> +			value &= ~GMAC_RXQCTRL_FPRQ;
> +			value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
> +			writel(value, priv->ioaddr + GMAC_RXQ_CTRL1);
> +		}

Since you are using a structure of function pointers, it would seem
more logical to have a fpe_xgmac_configure() and a
fpe_gmac4_configure(), and then xgmac_fpe_ops and gmac4_fpe_ops.

> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -974,8 +974,7 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  	bool *hs_enable = &fpe_cfg->hs_enable;
>  
>  	if (is_up && *hs_enable) {
> -		stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
> -					MPACKET_VERIFY);
> +		stmmac_fpe_send_mpacket(priv, priv, fpe_cfg, MPACKET_VERIFY);

passing priv twice looks very odd! It makes me think the API is
designed wrong. This could be because of the refactoring changes you
made? Maybe add another patch cleaning this up?

    Andrew

---
pw-bot: cr