Re: [PATCH net-next v4 4/7] net: stmmac: configure FPE via ethtool-mm

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Furong Xu <0x1207@gmail.com>
Date: Tue, 20 Aug 2024 17:38:32 +0800

> Implement ethtool --show-mm and --set-mm callbacks.
> 
> NIC up/down, link up/down, suspend/resume, kselftest-ethtool_mm,
> all tested okay.
> 
> Signed-off-by: Furong Xu <0x1207@gmail.com>

[...]

> @@ -589,6 +589,21 @@ void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
>  		cfg->fpe_csr = 0;
>  	}
>  	writel(cfg->fpe_csr, ioaddr + MAC_FPE_CTRL_STS);
> +
> +	value = readl(ioaddr + GMAC_INT_EN);
> +
> +	if (pmac_enable) {
> +		if (!(value & GMAC_INT_FPE_EN)) {
> +			/* Dummy read to clear any pending masked interrupts */
> +			(void)readl(ioaddr + MAC_FPE_CTRL_STS);

Are you sure this cast to void is needed? Have you seen readl() with
__must_check anywhere?

> +
> +			value |= GMAC_INT_FPE_EN;
> +		}
> +	} else {
> +		value &= ~GMAC_INT_FPE_EN;
> +	}
> +
> +	writel(value, ioaddr + GMAC_INT_EN);
>  }
>  
>  int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> @@ -638,3 +653,20 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
>  
>  	writel(value, ioaddr + MAC_FPE_CTRL_STS);
>  }
> +
> +int dwmac5_fpe_get_add_frag_size(void __iomem *ioaddr)

@ioaddr can be const.

> +{
> +	return FIELD_GET(AFSZ, readl(ioaddr + MTL_FPE_CTRL_STS));
> +}
> +
> +void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size)
> +{
> +	u32 value;
> +
> +	value = readl(ioaddr + MTL_FPE_CTRL_STS);
> +
> +	value &= ~AFSZ;
> +	value |= FIELD_PREP(AFSZ, add_frag_size);
> +
> +	writel(value, ioaddr + MTL_FPE_CTRL_STS);

	value = readl(ioaddr + MTL_FPE_CTRL_STS);
	writel(u32_replace_bits(value, add_frag_size, AFSZ),
	       ioaddr + MTL_FPE_CTRL_STS);


> +}
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> index bf33a51d229e..e369e65920fc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> @@ -39,6 +39,9 @@
>  #define MAC_PPSx_INTERVAL(x)		(0x00000b88 + ((x) * 0x10))
>  #define MAC_PPSx_WIDTH(x)		(0x00000b8c + ((x) * 0x10))
>  
> +#define MTL_FPE_CTRL_STS		0x00000c90
> +#define AFSZ				GENMASK(1, 0)

Can you leave comments next to definitions explaining what this is?
I guessed AFSZ is "added frag size", but meh...
Also, I'd prefix every definition with some vendor prefix (STMMAC_ or
so), otherwise it may conflict one day with some generic one.

> +
>  #define MTL_RXP_CONTROL_STATUS		0x00000ca0
>  #define RXPI				BIT(31)
>  #define NPE				GENMASK(23, 16)

[...]

> @@ -1270,6 +1271,112 @@ static int stmmac_set_tunable(struct net_device *dev,
>  	return ret;
>  }
>  
> +static int stmmac_get_mm(struct net_device *ndev,
> +			 struct ethtool_mm_state *state)
> +{
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	unsigned long flags;
> +	u32 add_frag_size;
> +
> +	if (!priv->dma_cap.fpesel)
> +		return -EOPNOTSUPP;
> +
> +	spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
> +
> +	state->pmac_enabled = priv->fpe_cfg.pmac_enabled;
> +	state->verify_time = priv->fpe_cfg.verify_time;
> +	state->verify_enabled = priv->fpe_cfg.verify_enabled;
> +	state->verify_status = priv->fpe_cfg.status;

See, you could embed &ethtool_mm_state into &fpe_cfg as I wrote under
the previous patch, so that you could do a direct assignment here :D

> +	state->rx_min_frag_size = ETH_ZLEN;
> +
> +	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
> +	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
> +	 */

Also make it a definition.

> +	state->max_verify_time = 128;
> +
> +	/* Cannot read MAC_FPE_CTRL_STS register here, or FPE interrupt events
> +	 * can be lost.
> +	 *
> +	 * See commit 37e4b8df27bc ("net: stmmac: fix FPE events losing")

I think it's not needed to leave commit references in the code?

> +	 */
> +	state->tx_enabled = !!(priv->fpe_cfg.fpe_csr == EFPE);

tx_enabled is bool, you don't need to add a double negation here (the
parenthesis are redundant as well).

> +
> +	/* FPE active if common tx_enabled and verification success or disabled (forced) */
> +	state->tx_active = state->tx_enabled &&
> +			   (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
> +			    state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED);
> +
> +	add_frag_size = stmmac_fpe_get_add_frag_size(priv, priv->ioaddr);
> +	state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(add_frag_size);
> +
> +	spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
> +
> +	return 0;
> +}

[...]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6ae95f20b24f..00ed0543f5cf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3537,8 +3537,21 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
>  
>  	stmmac_set_hw_vlan_mode(priv, priv->hw);
>  
> -	if (priv->dma_cap.fpesel)
> +	if (priv->dma_cap.fpesel) {
> +		/* A SW reset just happened in stmmac_init_dma_engine(),
> +		 * we should restore fpe_cfg to HW, or FPE will stop working
> +		 * from suspend/resume.
> +		 */
> +		spin_lock(&priv->fpe_cfg.lock);

I don't think this happens in the interrupt context, so you need
_irqsave() version here?

> +		stmmac_fpe_configure(priv, priv->ioaddr,
> +				     &priv->fpe_cfg,
> +				     priv->plat->tx_queues_to_use,
> +				     priv->plat->rx_queues_to_use,
> +				     false, priv->fpe_cfg.pmac_enabled);
> +		spin_unlock(&priv->fpe_cfg.lock);
> +
>  		stmmac_fpe_start_wq(priv);
> +	}
>  
>  	return 0;
>  }
> @@ -7417,7 +7430,7 @@ static void stmmac_fpe_verify_task(struct work_struct *work)
>  			stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg,
>  					     priv->plat->tx_queues_to_use,
>  					     priv->plat->rx_queues_to_use,
> -					     false);
> +					     false, fpe_cfg->pmac_enabled);
>  			spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
>  			break;
>  		}

[...]

Thanks,
Olek