* [PATCH v3] net: stmmac: fix FPE events losing
@ 2023-11-28 5:56 Jianheng Zhang
2023-11-30 9:55 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Jianheng Zhang @ 2023-11-28 5:56 UTC (permalink / raw)
To: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Simon Horman,
Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
Russell King (Oracle, Jochen Henneberg, Voon Weifeng,
Mohammad Athari Bin Ismail, Ong Boon Leong, Tan Tee Min
Cc: open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
Martin McKenny
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);
}
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] net: stmmac: fix FPE events losing
2023-11-28 5:56 [PATCH v3] net: stmmac: fix FPE events losing Jianheng Zhang
@ 2023-11-30 9:55 ` Paolo Abeni
2023-11-30 13:09 ` Serge Semin
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2023-11-30 9:55 UTC (permalink / raw)
To: Jianheng Zhang, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Maxime Coquelin, Simon Horman,
Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
Russell King (Oracle, Jochen Henneberg, Voon Weifeng,
Mohammad Athari Bin Ismail, Ong Boon Leong, Tan Tee Min
Cc: open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
Martin McKenny
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] net: stmmac: fix FPE events losing
2023-11-30 9:55 ` Paolo Abeni
@ 2023-11-30 13:09 ` Serge Semin
2023-11-30 14:09 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Serge Semin @ 2023-11-30 13:09 UTC (permalink / raw)
To: Paolo Abeni
Cc: Jianheng Zhang, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Maxime Coquelin, Simon Horman,
Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
Russell King (Oracle, Jochen Henneberg, Voon Weifeng,
Mohammad Athari Bin Ismail, Ong Boon Leong, Tan Tee Min,
open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
Martin McKenny
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
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] net: stmmac: fix FPE events losing
2023-11-30 13:09 ` Serge Semin
@ 2023-11-30 14:09 ` Paolo Abeni
2023-11-30 14:13 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2023-11-30 14:09 UTC (permalink / raw)
To: Serge Semin
Cc: Jianheng Zhang, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Maxime Coquelin, Simon Horman,
Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
Russell King (Oracle, Jochen Henneberg, Voon Weifeng,
Mohammad Athari Bin Ismail, Ong Boon Leong, Tan Tee Min,
open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
Martin McKenny
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] net: stmmac: fix FPE events losing
2023-11-30 14:09 ` Paolo Abeni
@ 2023-11-30 14:13 ` Paolo Abeni
2023-12-01 2:49 ` Jianheng Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2023-11-30 14:13 UTC (permalink / raw)
To: Serge Semin
Cc: Jianheng Zhang, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Maxime Coquelin, Simon Horman,
Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
Russell King (Oracle, Jochen Henneberg, Voon Weifeng,
Mohammad Athari Bin Ismail, Ong Boon Leong, Tan Tee Min,
open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
Martin McKenny
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH v3] net: stmmac: fix FPE events losing
2023-11-30 14:13 ` Paolo Abeni
@ 2023-12-01 2:49 ` Jianheng Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Jianheng Zhang @ 2023-12-01 2:49 UTC (permalink / raw)
To: Paolo Abeni, Serge Semin
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Maxime Coquelin, Simon Horman, Andrew Halaney,
Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
Russell King (Oracle, Jochen Henneberg, Voon Weifeng,
Mohammad Athari Bin Ismail, Ong Boon Leong, Tan Tee Min,
open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
Martin McKenny
> -----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-01 2:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-28 5:56 [PATCH v3] net: stmmac: fix FPE events losing Jianheng Zhang
2023-11-30 9:55 ` Paolo Abeni
2023-11-30 13:09 ` Serge Semin
2023-11-30 14:09 ` Paolo Abeni
2023-11-30 14:13 ` Paolo Abeni
2023-12-01 2:49 ` Jianheng Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).