Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
cores.
Signed-off-by: Jianheng Zhang <[email protected]>
---
v2:
- updating the FPE debug message to use netdev_dbg()
v1: https://lore.kernel.org/all/CY5PR12MB63726FED738099761A9B81E7BF8FA@CY5PR12MB6372.namprd12.prod.outlook.com/
---
drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h | 6 ++
.../net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 66 ++++++++++++++++++----
2 files changed, 61 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 207ff17..306d15b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -194,6 +194,12 @@
#define XGMAC_MDIO_DATA 0x00000204
#define XGMAC_MDIO_C22P 0x00000220
#define XGMAC_FPE_CTRL_STS 0x00000280
+#define XGMAC_TRSP BIT(19)
+#define XGMAC_TVER BIT(18)
+#define XGMAC_RRSP BIT(17)
+#define XGMAC_RVER BIT(16)
+#define XGMAC_SRSP BIT(2)
+#define XGMAC_SVER BIT(1)
#define XGMAC_EFPE BIT(0)
#define XGMAC_ADDRx_HIGH(x) (0x00000300 + (x) * 0x8)
#define XGMAC_ADDR_MAX 32
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index eb48211..2f14f9b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -1439,22 +1439,64 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
{
u32 value;
- if (!enable) {
- value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
+ if (enable) {
+ cfg->fpe_csr = XGMAC_EFPE;
+ value = readl(ioaddr + XGMAC_RXQ_CTRL1);
+ value &= ~XGMAC_RQ;
+ value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
+ writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+ } else {
+ cfg->fpe_csr = 0;
+ }
+ writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
+}
- value &= ~XGMAC_EFPE;
+static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+{
+ u32 value;
+ int status;
- writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
- return;
+ status = FPE_EVENT_UNKNOWN;
+
+ /* Reads from the XGMAC_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 + XGMAC_FPE_CTRL_STS);
+
+ if (value & XGMAC_TRSP) {
+ status |= FPE_EVENT_TRSP;
+ netdev_dbg(dev, "FPE: Respond mPacket is transmitted\n");
}
- value = readl(ioaddr + XGMAC_RXQ_CTRL1);
- value &= ~XGMAC_RQ;
- value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
- writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+ if (value & XGMAC_TVER) {
+ status |= FPE_EVENT_TVER;
+ netdev_dbg(dev, "FPE: Verify mPacket is transmitted\n");
+ }
+
+ if (value & XGMAC_RRSP) {
+ status |= FPE_EVENT_RRSP;
+ netdev_dbg(dev, "FPE: Respond mPacket is received\n");
+ }
+
+ if (value & XGMAC_RVER) {
+ status |= FPE_EVENT_RVER;
+ netdev_dbg(dev, "FPE: Verify mPacket is received\n");
+ }
+
+ return status;
+}
+
+static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr,
+ struct stmmac_fpe_cfg *cfg,
+ enum stmmac_mpacket_type type)
+{
+ u32 value = cfg->fpe_csr;
+
+ if (type == MPACKET_VERIFY)
+ value |= XGMAC_SVER;
+ else if (type == MPACKET_RESPONSE)
+ value |= XGMAC_SRSP;
- value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
- value |= XGMAC_EFPE;
writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
}
@@ -1503,6 +1545,8 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
.config_l4_filter = dwxgmac2_config_l4_filter,
.set_arp_offload = dwxgmac2_set_arp_offload,
.fpe_configure = dwxgmac3_fpe_configure,
+ .fpe_send_mpacket = dwxgmac3_fpe_send_mpacket,
+ .fpe_irq_status = dwxgmac3_fpe_irq_status,
};
static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
--
1.8.3.1
On Tue, 12 Dec 2023 14:05:02 +0000 Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.
>
> Signed-off-by: Jianheng Zhang <[email protected]>
I defer to Vladimir on whether to trust that the follow up with
the ethtool API support will come later (and not require rewrite
of existing code); or we should request that it's part of the same
series.
--
pw-bot: needs-ack
On Tue, Dec 12, 2023 at 03:23:47PM -0800, Jakub Kicinski wrote:
> On Tue, 12 Dec 2023 14:05:02 +0000 Jianheng Zhang wrote:
> > Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> > cores.
> >
> > Signed-off-by: Jianheng Zhang <[email protected]>
>
> I defer to Vladimir on whether to trust that the follow up with
> the ethtool API support will come later (and not require rewrite
> of existing code); or we should request that it's part of the same
> series.
> --
> pw-bot: needs-ack
>
Here's the thing - my understanding of what Synopsys is doing is that
they use TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE
as implicit hints to the stmmac driver that FPE should be enabled.
Hold/Release is merely one use case for frame preemption. The "fp"
argument in the tc framework gives you access to the rest: preemption
without scheduling, preemption with scheduling but without hold/release.
And the ethtool-mm framework gives you compatibility with LLDP, so you
can adjust the minimum fragment sizes according to the link partner.
Roger Quadros is adding am65-cpsw support for FPE using the generic
framework, and the TI device has an erratum that the minimum fragment
size that can be received cannot be lower than 124 bytes. LLDP allows
link partners to discover that and still interoperate - which is very
important, because if first-gen TSN hardware, with all its pre-standard
quirks, does not deliver, there may not be a second-gen.
By using LLDP, you can also enable the FPE handshake based on user space
input - when the LLDP daemon gets an LLDPDU with an Additional Ethernet
Capabilities TLV, rather than during each and every stmmac_mac_link_up(),
and hoping for someone to respond. Depending on your hardware design,
this can even lead to power savings, because you can power on your pMAC
only when LLDP says the link partner is also capable, and it will be required.
Ethtool also gives you the ability to report standardized stats per eMAC
and per pMAC, and standardized stats for the MAC Merge layer itself.
Also, the FPE state machine from the stmmac driver is so chatty and
spams the kernel log so bad, because it has nowhere else to report its
current (fragile) state. The ethtool MAC Merge layer gives the driver
a way to report its verification state ("FPE Handshake" as Synopsys
calls it) in a standardized enum.
A small note that the mainline iproute2 does not even expose the
TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE netlink
attribute values. To quote from the manpage (which is not out of date
with the code; I checked - again): 'The only supported <command> is "S",
which means "SetGateStates"'.
So I can only guess that Synopsys and anyone else who wants to turn on
FPE on stmmac has to patch their iproute2. A Github code search made me
land here:
https://github.com/altera-opensource/meta-intel-fpga-refdes/blob/7b050ca9968f5ff71598e86bb3a10bb8e011439c/recipes-connectivity/iproute2/iproute2/0003-taprio-Add-support-for-the-SetAndHold-and-SetAndRele.patch
In principle there's nothing wrong with not sharing patches on community
software with the rest of the world. But I cannot help but get the
impression that stmmac support for FPE is abandonware / extremely low
priority / code written to tick the boxes. It's not in the best state
even in the "good" case where the XGMAC3+ support gets accepted.
Jianheng, please contradict me by showing what testing is currently
conducted with this implementation. If none or close to that, let's get
it to a more "observable" state, where at least others' tests could be
reused.
Even this very patch is slightly strange - it is not brand new hardware
support, but it fills in some more FPE ops in dwxlgmac2_ops - when
dwxgmac3_fpe_configure() was already there. So this suggests the
existing support was incomplete. How complete is it now? No way to tell.
There is a selftest to tell, but we can't run it because the driver
doesn't integrate with those kernel APIs.
There are long periods of radio silence from Synopsys engineers in upstream,
and as maintainers we simply don't know what stmmac's FPE implementation
does and what it doesn't do. If a future user gets into trouble, having
a "known good" bisection point, by means of a selftest that passes, is
going to allow even non-expert maintainers like us provide some help,
even if Synopsys engineers go radio silent again.
It may be that Jianheng just needs a little nudge to help the management
prioritize, by getting a NACK. It's a simple "help us help you" situation:
the framework is there and it is a gateway for better Linux user space
support for your platform, you just need to use it. And what better time
to integrate with new API than with new hardware... :) Because it's not
as if FPE on XGMAC3+ ever worked in mainline given my reading of the code.
So why would users not start learning to use it with what is becoming
the common tool set for everybody else.
Allow me to change the "needs-ack" into:
pw-bot: cr
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Wednesday, December 13, 2023 8:43 AM
> To: Jianheng Zhang <[email protected]>; Jakub Kicinski <[email protected]>
> Cc: Alexandre Torgue <[email protected]>; Jose Abreu <[email protected]>; David S.
> Miller <[email protected]>; Eric Dumazet <[email protected]>; Paolo Abeni
> <[email protected]>; Maxime Coquelin <[email protected]>; James Li
> <[email protected]>; Martin McKenny <[email protected]>; open list:STMMAC ETHERNET
> DRIVER <[email protected]>; moderated list:ARM/STM32 ARCHITECTURE
> <[email protected]>; moderated list:ARM/STM32 ARCHITECTURE
> <[email protected]>; open list <[email protected]>
> Subject: Re: [PATCH v2 net-next] net: stmmac: xgmac3+: add FPE handshaking support
>
> On Tue, Dec 12, 2023 at 03:23:47PM -0800, Jakub Kicinski wrote:
> > On Tue, 12 Dec 2023 14:05:02 +0000 Jianheng Zhang wrote:
> > > Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> > > cores.
> > >
> > > Signed-off-by: Jianheng Zhang <[email protected]>
> >
> > I defer to Vladimir on whether to trust that the follow up with
> > the ethtool API support will come later (and not require rewrite
> > of existing code); or we should request that it's part of the same
> > series.
> > --
> > pw-bot: needs-ack
> >
>
> Here's the thing - my understanding of what Synopsys is doing is that
> they use TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE
> as implicit hints to the stmmac driver that FPE should be enabled.
>
> Hold/Release is merely one use case for frame preemption. The "fp"
> argument in the tc framework gives you access to the rest: preemption
> without scheduling, preemption with scheduling but without hold/release.
>
> And the ethtool-mm framework gives you compatibility with LLDP, so you
> can adjust the minimum fragment sizes according to the link partner.
> Roger Quadros is adding am65-cpsw support for FPE using the generic
> framework, and the TI device has an erratum that the minimum fragment
> size that can be received cannot be lower than 124 bytes. LLDP allows
> link partners to discover that and still interoperate - which is very
> important, because if first-gen TSN hardware, with all its pre-standard
> quirks, does not deliver, there may not be a second-gen.
>
> By using LLDP, you can also enable the FPE handshake based on user space
> input - when the LLDP daemon gets an LLDPDU with an Additional Ethernet
> Capabilities TLV, rather than during each and every stmmac_mac_link_up(),
> and hoping for someone to respond. Depending on your hardware design,
> this can even lead to power savings, because you can power on your pMAC
> only when LLDP says the link partner is also capable, and it will be required.
>
> Ethtool also gives you the ability to report standardized stats per eMAC
> and per pMAC, and standardized stats for the MAC Merge layer itself.
>
> Also, the FPE state machine from the stmmac driver is so chatty and
> spams the kernel log so bad, because it has nowhere else to report its
> current (fragile) state. The ethtool MAC Merge layer gives the driver
> a way to report its verification state ("FPE Handshake" as Synopsys
> calls it) in a standardized enum.
>
> A small note that the mainline iproute2 does not even expose the
> TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE netlink
> attribute values. To quote from the manpage (which is not out of date
> with the code; I checked - again): 'The only supported <command> is "S",
> which means "SetGateStates"'.
>
> So I can only guess that Synopsys and anyone else who wants to turn on
> FPE on stmmac has to patch their iproute2. A Github code search made me
> land here:
> https://urldefense.com/v3/__https://github.com/altera-opensource/meta-intel-fpga-refdes/blob/7b050c
> a9968f5ff71598e86bb3a10bb8e011439c/recipes-connectivity/iproute2/iproute2/0003-taprio-Add-suppo
> rt-for-the-SetAndHold-and-SetAndRele.patch__;!!A4F2R9G_pg!cj_LK6iKkOj8RlYyKdNK6wv23ddmwT3_3
> ebNC0gb97xaKddBhm2B0uAZMIffG5vxyRHCcbyez2aY-JaDt-tNTg$
>
> In principle there's nothing wrong with not sharing patches on community
> software with the rest of the world. But I cannot help but get the
> impression that stmmac support for FPE is abandonware / extremely low
> priority / code written to tick the boxes. It's not in the best state
> even in the "good" case where the XGMAC3+ support gets accepted.
> Jianheng, please contradict me by showing what testing is currently
> conducted with this implementation. If none or close to that, let's get
> it to a more "observable" state, where at least others' tests could be
> reused.
>
> Even this very patch is slightly strange - it is not brand new hardware
> support, but it fills in some more FPE ops in dwxlgmac2_ops - when
> dwxgmac3_fpe_configure() was already there. So this suggests the
> existing support was incomplete. How complete is it now? No way to tell.
> There is a selftest to tell, but we can't run it because the driver
> doesn't integrate with those kernel APIs.
>
> There are long periods of radio silence from Synopsys engineers in upstream,
> and as maintainers we simply don't know what stmmac's FPE implementation
> does and what it doesn't do. If a future user gets into trouble, having
> a "known good" bisection point, by means of a selftest that passes, is
> going to allow even non-expert maintainers like us provide some help,
> even if Synopsys engineers go radio silent again.
>
> It may be that Jianheng just needs a little nudge to help the management
> prioritize, by getting a NACK. It's a simple "help us help you" situation:
> the framework is there and it is a gateway for better Linux user space
> support for your platform, you just need to use it. And what better time
> to integrate with new API than with new hardware... :) Because it's not
> as if FPE on XGMAC3+ ever worked in mainline given my reading of the code.
> So why would users not start learning to use it with what is becoming
> the common tool set for everybody else.
>
> Allow me to change the "needs-ack" into:
>
> pw-bot: cr
Hi Vladimir,
Your insights into the current state of FPE support in the stmmac driver are
indeed on point. It's accurate that the existing FPE support of stmmac relies
on the mentioned patch. Before the emergence of the ethtool-mm framework, some
users might have been able to use the FP function in this way. But with the
introduction of the common ethtool-mm framework, it is more important to update
the existing stmmac FPE implementation to support ethtool. And I now fully
understand that it would not be appropriate to add xgmac support for FPE based
on the current implementation. I appreciate you pointing out the problems with
the current implementation and providing suggestions, they are incredibly
helpful for the subsequent refactoring of stmmac's FPE functionality.
Jianheng