Messages in this thread Patch in this message | | | Date | Mon, 27 Nov 2023 13:20:53 +0300 | From | Serge Semin <> | Subject | Re: [PATCH v2] net: stmmac: fix FPE events losing |
| |
Hi Jianheng
On Mon, Nov 27, 2023 at 07:08:17AM +0000, Jianheng Zhang wrote: > > The status bits of register MAC_FPE_CTRL_STS are clear on read. Using > 32-bit read for MAC_FPE_CTRL_STS in dwmac5_fpe_configure() and > dwmac5_fpe_send_mpacket() clear the status bits. Then the stmmac interrupt > handler missing FPE event status and leads to FPE handshaking failure and > retries. > To avoid clear status bits of MAC_FPE_CTRL_STS in dwmac5_fpe_configure() > and dwmac5_fpe_send_mpacket(), add fpe_csr to stmmac_fpe_cfg structure to > cache the control bits of MAC_FPE_CTRL_STS and to avoid reading > MAC_FPE_CTRL_STS in those methods. > > Fixes: 5a5586112b92 ("net: stmmac: support FPE link partner hand-shaking procedure") > Signed-off-by: jianheng <jianheng@synopsys.com>
Thanks for the fix and taking all my notes and nitpicks into account. Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
-Serge(y)
> --- > drivers/net/ethernet/stmicro/stmmac/dwmac5.c | 45 +++++++++------------- > drivers/net/ethernet/stmicro/stmmac/dwmac5.h | 4 +- > .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 3 +- > drivers/net/ethernet/stmicro/stmmac/hwif.h | 4 +- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +++- > drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 1 + > include/linux/stmmac.h | 1 + > 7 files changed, 36 insertions(+), 30 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c > index e95d35f..8fd1675 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c > @@ -710,28 +710,22 @@ void dwmac5_est_irq_status(void __iomem *ioaddr, struct net_device *dev, > } > } > > -void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq, > +void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg, > + u32 num_txq, u32 num_rxq, > bool enable) > { > u32 value; > > - if (!enable) { > - value = readl(ioaddr + MAC_FPE_CTRL_STS); > - > - value &= ~EFPE; > - > - writel(value, ioaddr + MAC_FPE_CTRL_STS); > - return; > + if (enable) { > + cfg->fpe_csr = EFPE; > + value = readl(ioaddr + GMAC_RXQ_CTRL1); > + value &= ~GMAC_RXQCTRL_FPRQ; > + value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT; > + writel(value, ioaddr + GMAC_RXQ_CTRL1); > + } else { > + cfg->fpe_csr = 0; > } > - > - value = readl(ioaddr + GMAC_RXQ_CTRL1); > - value &= ~GMAC_RXQCTRL_FPRQ; > - value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT; > - writel(value, ioaddr + GMAC_RXQ_CTRL1); > - > - value = readl(ioaddr + MAC_FPE_CTRL_STS); > - value |= EFPE; > - writel(value, ioaddr + MAC_FPE_CTRL_STS); > + writel(cfg->fpe_csr, ioaddr + MAC_FPE_CTRL_STS); > } > > int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev) > @@ -741,6 +735,9 @@ int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev) > > status = FPE_EVENT_UNKNOWN; > > + /* Reads from the MAC_FPE_CTRL_STS register should only be performed > + * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read" > + */ > value = readl(ioaddr + MAC_FPE_CTRL_STS); > > if (value & TRSP) { > @@ -766,19 +763,15 @@ int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev) > return status; > } > > -void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, enum stmmac_mpacket_type type) > +void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg, > + enum stmmac_mpacket_type type) > { > - u32 value; > + u32 value = cfg->fpe_csr; > > - value = readl(ioaddr + MAC_FPE_CTRL_STS); > - > - if (type == MPACKET_VERIFY) { > - value &= ~SRSP; > + if (type == MPACKET_VERIFY) > value |= SVER; > - } else { > - value &= ~SVER; > + else if (type == MPACKET_RESPONSE) > value |= SRSP; > - } > > writel(value, ioaddr + MAC_FPE_CTRL_STS); > } > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h > index 53c138d..34e6207 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h > @@ -153,9 +153,11 @@ int dwmac5_est_configure(void __iomem *ioaddr, struct stmmac_est *cfg, > unsigned int ptp_rate); > void dwmac5_est_irq_status(void __iomem *ioaddr, struct net_device *dev, > struct stmmac_extra_stats *x, u32 txqcnt); > -void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq, > +void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg, > + u32 num_txq, u32 num_rxq, > bool enable); > void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, > + struct stmmac_fpe_cfg *cfg, > enum stmmac_mpacket_type type); > int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev); > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > index 453e88b..a74e71d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > @@ -1484,7 +1484,8 @@ static int dwxgmac3_est_configure(void __iomem *ioaddr, struct stmmac_est *cfg, > return 0; > } > > -static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq, > +static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg, > + u32 num_txq, > u32 num_rxq, bool enable) > { > u32 value; > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h > index b95d3e1..68aa2d5 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h > @@ -412,9 +412,11 @@ struct stmmac_ops { > unsigned int ptp_rate); > void (*est_irq_status)(void __iomem *ioaddr, struct net_device *dev, > struct stmmac_extra_stats *x, u32 txqcnt); > - void (*fpe_configure)(void __iomem *ioaddr, u32 num_txq, u32 num_rxq, > + void (*fpe_configure)(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg, > + u32 num_txq, u32 num_rxq, > bool enable); > void (*fpe_send_mpacket)(void __iomem *ioaddr, > + struct stmmac_fpe_cfg *cfg, > enum stmmac_mpacket_type type); > int (*fpe_irq_status)(void __iomem *ioaddr, struct net_device *dev); > }; > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 3e50fd5..7791e9b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -964,7 +964,8 @@ 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, MPACKET_VERIFY); > + stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, > + MPACKET_VERIFY); > } else { > *lo_state = FPE_STATE_OFF; > *lp_state = FPE_STATE_OFF; > @@ -5838,6 +5839,7 @@ static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status) > /* If user has requested FPE enable, quickly response */ > if (*hs_enable) > stmmac_fpe_send_mpacket(priv, priv->ioaddr, > + fpe_cfg, > MPACKET_RESPONSE); > } > > @@ -7262,6 +7264,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work) > if (*lo_state == FPE_STATE_ENTERING_ON && > *lp_state == FPE_STATE_ENTERING_ON) { > stmmac_fpe_configure(priv, priv->ioaddr, > + fpe_cfg, > priv->plat->tx_queues_to_use, > priv->plat->rx_queues_to_use, > *enable); > @@ -7280,6 +7283,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work) > netdev_info(priv->dev, SEND_VERIFY_MPAKCET_FMT, > *lo_state, *lp_state); > stmmac_fpe_send_mpacket(priv, priv->ioaddr, > + fpe_cfg, > MPACKET_VERIFY); > } > /* Sleep then retry */ > @@ -7294,6 +7298,7 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) > if (priv->plat->fpe_cfg->hs_enable != enable) { > if (enable) { > stmmac_fpe_send_mpacket(priv, priv->ioaddr, > + priv->plat->fpe_cfg, > MPACKET_VERIFY); > } else { > priv->plat->fpe_cfg->lo_fpe_state = FPE_STATE_OFF; > @@ -7754,6 +7759,7 @@ int stmmac_suspend(struct device *dev) > if (priv->dma_cap.fpesel) { > /* Disable FPE */ > stmmac_fpe_configure(priv, priv->ioaddr, > + priv->plat->fpe_cfg, > priv->plat->tx_queues_to_use, > priv->plat->rx_queues_to_use, false); > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > index ac41ef4..6ad3e0a 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > @@ -1079,6 +1079,7 @@ static int tc_setup_taprio(struct stmmac_priv *priv, > > priv->plat->fpe_cfg->enable = false; > stmmac_fpe_configure(priv, priv->ioaddr, > + priv->plat->fpe_cfg, > priv->plat->tx_queues_to_use, > priv->plat->rx_queues_to_use, > false); > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index 0b4658a..dee5ad6 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -175,6 +175,7 @@ struct stmmac_fpe_cfg { > bool hs_enable; /* FPE handshake enable */ > enum stmmac_fpe_state lp_fpe_state; /* Link Partner FPE state */ > enum stmmac_fpe_state lo_fpe_state; /* Local station FPE state */ > + u32 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */ > }; > > struct stmmac_safety_feature_cfg { > -- > 1.8.3.1 > >
| |