| Message ID | CY5PR12MB6372BF02C49FC9E628D0EC02BFBCA@CY5PR12MB6372.namprd12.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net: stmmac: fix FPE events losing | expand |
On Tue, 2023-11-28 at 05:56 +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") > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Jianheng Zhang <jianheng@synopsys.com> > --- > 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); > } It's unclear to me why it's not necessary to preserve the SVER/SRSP bits across MAC_FPE_CTRL_STS writes. I guess they are not part of the status bits? perhaps an explicit comment somewhere will help? Thanks Paolo
Hi Paolo On Thu, Nov 30, 2023 at 10:55:34AM +0100, Paolo Abeni wrote: > On Tue, 2023-11-28 at 05:56 +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") > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > Signed-off-by: Jianheng Zhang <jianheng@synopsys.com> > > --- > > 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); > > } > > It's unclear to me why it's not necessary to preserve the SVER/SRSP > bits across MAC_FPE_CTRL_STS writes. I guess they are not part of the > status bits? perhaps an explicit comment somewhere will help? The SRSP and SVER are self-cleared flags with no effect on zero writing. Their responsibility is to emit the Respond and Verify mPackets respectively. As soon as the packets are sent, the flags will be reset by hardware automatically. So no, they aren't a part of the status bits. Note since 'value' now isn't read from the MAC_FPE_CTRL_STS register, there is no point in clearing up these flags in the local variable because 'value' has now them cleared by default. Not sure whether a comment about that is required, since the described behavior is well documented in the Synopsys HW-manual. -Serge(y) > > Thanks > > Paolo > >
On Thu, 2023-11-30 at 16:09 +0300, Serge Semin wrote: > Hi Paolo > > On Thu, Nov 30, 2023 at 10:55:34AM +0100, Paolo Abeni wrote: > > On Tue, 2023-11-28 at 05:56 +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") > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > Signed-off-by: Jianheng Zhang <jianheng@synopsys.com> > > > --- > > > 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); > > > } > > > > > It's unclear to me why it's not necessary to preserve the SVER/SRSP > > bits across MAC_FPE_CTRL_STS writes. I guess they are not part of the > > status bits? perhaps an explicit comment somewhere will help? > > The SRSP and SVER are self-cleared flags with no effect on zero > writing. Their responsibility is to emit the Respond and Verify > mPackets respectively. As soon as the packets are sent, the flags will > be reset by hardware automatically. So no, they aren't a part of the > status bits. > > Note since 'value' now isn't read from the MAC_FPE_CTRL_STS register, > there is no point in clearing up these flags in the local variable > because 'value' has now them cleared by default. > > Not sure whether a comment about that is required, since the described > behavior is well documented in the Synopsys HW-manual. Thanks for the explanation, it clarifies the things to me. I agree there is no need for a patch change. Cheers, Paolo
On Thu, 2023-11-30 at 15:09 +0100, Paolo Abeni wrote: > On Thu, 2023-11-30 at 16:09 +0300, Serge Semin wrote: > > Hi Paolo > > > > On Thu, Nov 30, 2023 at 10:55:34AM +0100, Paolo Abeni wrote: > > > On Tue, 2023-11-28 at 05:56 +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") > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > Signed-off-by: Jianheng Zhang <jianheng@synopsys.com> > > > > --- > > > > 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); > > > > } > > > > > > > > It's unclear to me why it's not necessary to preserve the SVER/SRSP > > > bits across MAC_FPE_CTRL_STS writes. I guess they are not part of the > > > status bits? perhaps an explicit comment somewhere will help? > > > > The SRSP and SVER are self-cleared flags with no effect on zero > > writing. Their responsibility is to emit the Respond and Verify > > mPackets respectively. As soon as the packets are sent, the flags will > > be reset by hardware automatically. So no, they aren't a part of the > > status bits. > > > > Note since 'value' now isn't read from the MAC_FPE_CTRL_STS register, > > there is no point in clearing up these flags in the local variable > > because 'value' has now them cleared by default. > > > > Not sure whether a comment about that is required, since the described > > behavior is well documented in the Synopsys HW-manual. > > Thanks for the explanation, it clarifies the things to me. I agree > there is no need for a patch change. I'm sorry, I have to take back the last sentence: the submitter and SoB email address still don't match. @Jianheng: please fix it for good at the next iteration. Cheers, Paolo
> -----Original Message----- > From: Paolo Abeni <pabeni@redhat.com> > Sent: Thursday, November 30, 2023 10:14 PM > To: Serge Semin <fancer.lancer@gmail.com> > Cc: Jianheng Zhang <jianheng@synopsys.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose > Abreu <joabreu@synopsys.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Maxime Coquelin > <mcoquelin.stm32@gmail.com>; Simon Horman <horms@kernel.org>; Andrew Halaney > <ahalaney@redhat.com>; Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Shenwei Wang > <shenwei.wang@nxp.com>; Johannes Zink <j.zink@pengutronix.de>; Russell King (Oracle > <rmk+kernel@armlinux.org.uk>; Jochen Henneberg <jh@henneberg-systemdesign.com>; Voon Weifeng > <weifeng.voon@intel.com>; Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>; Ong > Boon Leong <boon.leong.ong@intel.com>; Tan Tee Min <tee.min.tan@intel.com>; open list:STMMAC > ETHERNET DRIVER <netdev@vger.kernel.org>; moderated list:ARM/STM32 ARCHITECTURE > <linux-stm32@st-md-mailman.stormreply.com>; moderated list:ARM/STM32 ARCHITECTURE > <linux-arm-kernel@lists.infradead.org>; open list <linux-kernel@vger.kernel.org>; James Li > <lijames@synopsys.com>; Martin McKenny <mmckenny@synopsys.com> > Subject: Re: [PATCH v3] net: stmmac: fix FPE events losing > > On Thu, 2023-11-30 at 15:09 +0100, Paolo Abeni wrote: > > On Thu, 2023-11-30 at 16:09 +0300, Serge Semin wrote: > > > Hi Paolo > > > > > > On Thu, Nov 30, 2023 at 10:55:34AM +0100, Paolo Abeni wrote: > > > > On Tue, 2023-11-28 at 05:56 +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") > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > Signed-off-by: Jianheng Zhang <jianheng@synopsys.com> > > > > > --- > > > > > 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); > > > > > } > > > > > > > > > > > It's unclear to me why it's not necessary to preserve the SVER/SRSP > > > > bits across MAC_FPE_CTRL_STS writes. I guess they are not part of the > > > > status bits? perhaps an explicit comment somewhere will help? > > > > > > The SRSP and SVER are self-cleared flags with no effect on zero > > > writing. Their responsibility is to emit the Respond and Verify > > > mPackets respectively. As soon as the packets are sent, the flags will > > > be reset by hardware automatically. So no, they aren't a part of the > > > status bits. > > > > > > Note since 'value' now isn't read from the MAC_FPE_CTRL_STS register, > > > there is no point in clearing up these flags in the local variable > > > because 'value' has now them cleared by default. > > > > > > Not sure whether a comment about that is required, since the described > > > behavior is well documented in the Synopsys HW-manual. Thanks for Serge’s explanation. > > > > Thanks for the explanation, it clarifies the things to me. I agree > > there is no need for a patch change. > > I'm sorry, I have to take back the last sentence: the submitter and SoB > email address still don't match. @Jianheng: please fix it for good at > the next iteration. OH, sorry~ I mixed up my internet email address with the internal one. I will fix it and resend the patch. Best Regards, Jianheng > > Cheers, > > Paolo
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 {
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") Reviewed-by: Serge Semin <fancer.lancer@gmail.com> Signed-off-by: Jianheng Zhang <jianheng@synopsys.com> --- 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(-)