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

Alexander Lobakin
2024-08-20T14:52:27Z

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