Hi,
The Designware 10G MAC(dwxgmac) driver has lack of vlan support
in term of hardware, such as the hardware accelerated VLAN stripping.
The driver was not draft from scratch, however it was ported from the
Ethernet Quality-of-Service (dwmac4) driver, it was tested working on
ourside.
Refer to:
https://github.com/torvalds/linux/commit/750011e239a50873251c16207b0fe78eabf8577e
Boon Khai Ng (1):
net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
.../net/ethernet/stmicro/stmmac/dwxgmac2.h | 28 +++++++++++++++
.../ethernet/stmicro/stmmac/dwxgmac2_core.c | 34 +++++++++++++++++++
.../ethernet/stmicro/stmmac/dwxgmac2_descs.c | 18 ++++++++++
3 files changed, 80 insertions(+)
--
2.35.3
Currently, VLAN tag stripping is done by software driver in
stmmac_rx_vlan() for dwxgmac2 driver. This patch is to Add support
for VLAN tag stripping by the MAC hardware and MAC drivers to support it.
This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks
at stmmac_ops struct which are called from upper software layer.
The setting can be turn on and off at ethool by running the command
below:
ethtool -k eth0 rx-vlan-offload on
ethtool -k eth0 rx-vlan-offload off
Hence for XGMAC IP, it is supported with the hardware stripping and
the flag NETIF_F_HW_VLAN_CTAG_RX is used to
determine that the hardware stripping feature is selected.
This implementation was ported from the dwmac4 driver.
Signed-off-by: Boon Khai Ng <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwxgmac2.h | 28 +++++++++++++++
.../ethernet/stmicro/stmmac/dwxgmac2_core.c | 34 +++++++++++++++++++
.../ethernet/stmicro/stmmac/dwxgmac2_descs.c | 18 ++++++++++
3 files changed, 80 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 6a2c7d22df1e..05b0f210ad90 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -7,6 +7,7 @@
#ifndef __STMMAC_DWXGMAC2_H__
#define __STMMAC_DWXGMAC2_H__
+#include <linux/bitfield.h>
#include "common.h"
/* Misc */
@@ -455,6 +456,7 @@
#define XGMAC_TDES2_VTIR GENMASK(15, 14)
#define XGMAC_TDES2_VTIR_SHIFT 14
#define XGMAC_TDES2_B1L GENMASK(13, 0)
+#define XGMAC_TDES2_VLAN_TAG_MASK GENMASK(15, 14)
#define XGMAC_TDES3_OWN BIT(31)
#define XGMAC_TDES3_CTXT BIT(30)
#define XGMAC_TDES3_FD BIT(29)
@@ -486,6 +488,7 @@
#define XGMAC_RDES3_RSV BIT(26)
#define XGMAC_RDES3_L34T GENMASK(23, 20)
#define XGMAC_RDES3_L34T_SHIFT 20
+#define XGMAC_RDES3_ET_LT GENMASK(19, 16)
#define XGMAC_L34T_IP4TCP 0x1
#define XGMAC_L34T_IP4UDP 0x2
#define XGMAC_L34T_IP6TCP 0x9
@@ -495,4 +498,29 @@
#define XGMAC_RDES3_TSD BIT(6)
#define XGMAC_RDES3_TSA BIT(4)
+/* RDES0 (write back format) */
+#define XGMAC_RDES0_VLAN_TAG_MASK GENMASK(15, 0)
+
+/* MAC VLAN Tag Control */
+#define XGMAC_VLAN_TAG_CTRL_OB BIT(0)
+#define XGMAC_VLAN_TAG_CTRL_CT BIT(1)
+#define XGMAC_VLAN_TAG_CTRL_OFS_MASK GENMASK(6, 2)
+#define XGMAC_VLAN_TAG_CTRL_OFS_SHIFT 2
+#define XGMAC_VLAN_TAG_CTRL_EVLS_MASK GENMASK(22, 21)
+#define XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT 21
+#define XGMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
+
+#define XGMAC_VLAN_TAG_STRIP_NONE FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x0)
+#define XGMAC_VLAN_TAG_STRIP_PASS FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x1)
+#define XGMAC_VLAN_TAG_STRIP_FAIL FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x2)
+#define XGMAC_VLAN_TAG_STRIP_ALL FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x3)
+
+/* Error Type or L2 Type(ET/LT) Field Number */
+#define XGMAC_ET_LT_VLAN_STAG 8
+#define XGMAC_ET_LT_VLAN_CTAG 9
+#define XGMAC_ET_LT_DVLAN_CTAG_CTAG 10
+#define XGMAC_ET_LT_DVLAN_STAG_STAG 11
+#define XGMAC_ET_LT_DVLAN_CTAG_STAG 12
+#define XGMAC_ET_LT_DVLAN_STAG_CTAG 13
+
#endif /* __STMMAC_DWXGMAC2_H__ */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index f8e7775bb633..2870ee3d7104 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -6,6 +6,7 @@
#include <linux/bitrev.h>
#include <linux/crc32.h>
+#include <linux/if_vlan.h>
#include <linux/iopoll.h>
#include "stmmac.h"
#include "stmmac_ptp.h"
@@ -1301,6 +1302,35 @@ static void dwxgmac2_sarc_configure(void __iomem *ioaddr, int val)
writel(value, ioaddr + XGMAC_TX_CONFIG);
}
+static void dwxgmac2_rx_hw_vlan(struct mac_device_info *hw,
+ struct dma_desc *rx_desc, struct sk_buff *skb)
+{
+ if (hw->desc->get_rx_vlan_valid(rx_desc)) {
+ u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
+
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
+ }
+}
+
+static void dwxgmac2_set_hw_vlan_mode(struct mac_device_info *hw)
+{
+ void __iomem *ioaddr = hw->pcsr;
+ u32 val = readl(ioaddr + XGMAC_VLAN_TAG);
+
+ val &= ~XGMAC_VLAN_TAG_CTRL_EVLS_MASK;
+
+ if (hw->hw_vlan_en)
+ /* Always strip VLAN on Receive */
+ val |= XGMAC_VLAN_TAG_STRIP_ALL;
+ else
+ /* Do not strip VLAN on Receive */
+ val |= XGMAC_VLAN_TAG_STRIP_NONE;
+
+ /* Enable outer VLAN Tag in Rx DMA descriptro */
+ val |= XGMAC_VLAN_TAG_CTRL_EVLRXS;
+ writel(val, ioaddr + XGMAC_VLAN_TAG);
+}
+
static void dwxgmac2_enable_vlan(struct mac_device_info *hw, u32 type)
{
void __iomem *ioaddr = hw->pcsr;
@@ -1574,6 +1604,8 @@ const struct stmmac_ops dwxgmac210_ops = {
.config_l4_filter = dwxgmac2_config_l4_filter,
.set_arp_offload = dwxgmac2_set_arp_offload,
.fpe_configure = dwxgmac3_fpe_configure,
+ .rx_hw_vlan = dwxgmac2_rx_hw_vlan,
+ .set_hw_vlan_mode = dwxgmac2_set_hw_vlan_mode,
};
static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
@@ -1634,6 +1666,8 @@ const struct stmmac_ops dwxlgmac2_ops = {
.config_l4_filter = dwxgmac2_config_l4_filter,
.set_arp_offload = dwxgmac2_set_arp_offload,
.fpe_configure = dwxgmac3_fpe_configure,
+ .rx_hw_vlan = dwxgmac2_rx_hw_vlan,
+ .set_hw_vlan_mode = dwxgmac2_set_hw_vlan_mode,
};
int dwxgmac2_setup(struct stmmac_priv *priv)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
index fc82862a612c..284c0c840ed1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
@@ -4,6 +4,7 @@
* stmmac XGMAC support.
*/
+#include <linux/bitfield.h>
#include <linux/stmmac.h>
#include "common.h"
#include "dwxgmac2.h"
@@ -67,6 +68,21 @@ static int dwxgmac2_get_tx_ls(struct dma_desc *p)
return (le32_to_cpu(p->des3) & XGMAC_RDES3_LD) > 0;
}
+static inline u16 dwxgmac2_wrback_get_rx_vlan_tci(struct dma_desc *p)
+{
+ return le32_to_cpu(p->des0) & XGMAC_RDES0_VLAN_TAG_MASK;
+}
+
+static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc *p)
+{
+ u32 et_lt;
+
+ et_lt = FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));
+
+ return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
+ et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG;
+}
+
static int dwxgmac2_get_rx_frame_len(struct dma_desc *p, int rx_coe)
{
return (le32_to_cpu(p->des3) & XGMAC_RDES3_PL);
@@ -349,6 +365,8 @@ const struct stmmac_desc_ops dwxgmac210_desc_ops = {
.set_tx_owner = dwxgmac2_set_tx_owner,
.set_rx_owner = dwxgmac2_set_rx_owner,
.get_tx_ls = dwxgmac2_get_tx_ls,
+ .get_rx_vlan_tci = dwxgmac2_wrback_get_rx_vlan_tci,
+ .get_rx_vlan_valid = dwxgmac2_wrback_get_rx_vlan_valid,
.get_rx_frame_len = dwxgmac2_get_rx_frame_len,
.enable_tx_timestamp = dwxgmac2_enable_tx_timestamp,
.get_tx_timestamp_status = dwxgmac2_get_tx_timestamp_status,
--
2.35.3
> -----Original Message-----
> From: Boon Khai Ng <[email protected]>
> Sent: Monday, May 27, 2024 3:04 PM
> To: Alexandre Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; [email protected]; linux-stm32@st-
> md-mailman.stormreply.com; [email protected]; linux-
> [email protected]; Tien Sung Ang <[email protected]>; G Thomas
> Rohan <[email protected]>; Looi Hong Aun
> <[email protected]>; Andy Shevchenko
> <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Cc: Boon Khai Ng <[email protected]>
> Subject: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature v2
> 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> Stripping
>
New features should be submitted against 'net-next' instead of 'net'.
Also 'net-next' is currently closed.
Thanks,
Sunil.
> -----Original Message-----
> From: Sunil Kovvuri Goutham <[email protected]>
> Sent: Monday, May 27, 2024 6:41 PM
> To: Ng, Boon Khai <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> David S . Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Maxime Coquelin <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; Ang,
> Tien Sung <[email protected]>; G Thomas, Rohan
> <[email protected]>; Looi, Hong Aun <[email protected]>;
> Andy Shevchenko <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Subject: RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature
> v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> Stripping
>
>
>
> > -----Original Message-----
> > From: Boon Khai Ng <[email protected]>
> > Sent: Monday, May 27, 2024 3:04 PM
> > To: Alexandre Torgue <[email protected]>; Jose Abreu
> > <[email protected]>; David S . Miller <[email protected]>; Eric
> > Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo
> > Abeni <[email protected]>; Maxime Coquelin
> > <[email protected]>; [email protected]; linux-
> stm32@st-
> > md-mailman.stormreply.com; [email protected];
> > linux- [email protected]; Tien Sung Ang
> > <[email protected]>; G Thomas Rohan
> <[email protected]>;
> > Looi Hong Aun <[email protected]>; Andy Shevchenko
> > <[email protected]>; Ilpo Jarvinen
> > <[email protected]>
> > Cc: Boon Khai Ng <[email protected]>
> > Subject: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature
> v2
> > 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > Stripping
> >
>
> New features should be submitted against 'net-next' instead of 'net'.
Hi Sunil, I was cloning the repo from net-next, but how to choose the destination as 'net-next'?
> Also 'net-next' is currently closed.
I see, may I know when the next opening period is? Thanks
>
> Thanks,
> Sunil.
Regards,
Boon Khai
> -----Original Message-----
> From: Ng, Boon Khai <[email protected]>
> Sent: Monday, May 27, 2024 6:58 PM
> To: Sunil Kovvuri Goutham <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> David S . Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Maxime Coquelin <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Ang, Tien Sung
> <[email protected]>; G Thomas, Rohan <[email protected]>;
> Looi, Hong Aun <[email protected]>; Andy Shevchenko
> <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature
> v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> Stripping
>
.........
> > > 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > > Stripping
> > >
> >
> > New features should be submitted against 'net-next' instead of 'net'.
>
> Hi Sunil, I was cloning the repo from net-next, but how to choose the
> destination as 'net-next'?
While creating patch you can add appropriate prefix .. like below
git format-patch --subject-prefix="net-next PATCH"
git format-patch --subject-prefix="net PATCH"
>
> > Also 'net-next' is currently closed.
>
> I see, may I know when the next opening period is? Thanks
Please track
https://patchwork.hopto.org/net-next.html
> This implementation was ported from the dwmac4 driver.
How does it differ from dwmac4? Can the dwmac4 implementation just be
used, rather than duplicating all the code/bugs.
Andrew
> -----Original Message-----
> From: Sunil Kovvuri Goutham <[email protected]>
> Sent: Monday, May 27, 2024 11:36 PM
> To: Ng, Boon Khai <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> David S . Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Maxime Coquelin <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; Ang,
> Tien Sung <[email protected]>; G Thomas, Rohan
> <[email protected]>; Looi, Hong Aun <[email protected]>;
> Andy Shevchenko <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Subject: RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature
> v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> Stripping
>
>
>
> > -----Original Message-----
> > From: Ng, Boon Khai <[email protected]>
> > Sent: Monday, May 27, 2024 6:58 PM
> > To: Sunil Kovvuri Goutham <[email protected]>; Alexandre Torgue
> > <[email protected]>; Jose Abreu <[email protected]>;
> > David S . Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> > <[email protected]>; Maxime Coquelin
> <[email protected]>;
> > [email protected]; [email protected];
> > linux- [email protected]; [email protected];
> > Ang, Tien Sung <[email protected]>; G Thomas, Rohan
> > <[email protected]>; Looi, Hong Aun <[email protected]>;
> > Andy Shevchenko <[email protected]>; Ilpo Jarvinen
> > <[email protected]>
> > Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature
> > v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > Stripping
> >
> ..........
>
> > > > 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > > > Stripping
> > > >
> > >
> > > New features should be submitted against 'net-next' instead of 'net'.
> >
> > Hi Sunil, I was cloning the repo from net-next, but how to choose the
> > destination as 'net-next'?
>
> While creating patch you can add appropriate prefix .. like below git format-
> patch --subject-prefix="net-next PATCH"
> git format-patch --subject-prefix="net PATCH"
>
Okay will update that in the next version.
> >
> > > Also 'net-next' is currently closed.
> >
> > I see, may I know when the next opening period is? Thanks
>
> Please track
> https://patchwork.hopto.org/net-next.html
Checked the link it is just a photo saying "come in we're open" is that mean the net-next is currently open now?
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Tuesday, May 28, 2024 3:00 AM
> To: Ng, Boon Khai <[email protected]>
> Cc: Alexandre Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; [email protected]; linux-stm32@st-
> md-mailman.stormreply.com; [email protected]; linux-
> [email protected]; Ang, Tien Sung <[email protected]>; G
> Thomas, Rohan <[email protected]>; Looi, Hong Aun
> <[email protected]>; Andy Shevchenko
> <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
>
> > This implementation was ported from the dwmac4 driver.
>
> How does it differ from dwmac4? Can the dwmac4 implementation just be
> used, rather than duplicating all the code/bugs.
Hi Andrew, 5 years ago the driver was initially implemented separately, maybe need David S. Miller help to clarify this.
https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
If you take a look at the code, the register mapping looks different at their TX MAC Configuration register and RX MAC Configuration register.
https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
So, for this XGMAC VLAN patch, the idea of getting the VLAN id from the descriptor is the same, but
The register bit filed of getting the VLAN packet VALID is different. Thus, it need to be implemented separately.
>
> Andrew
> -----Original Message-----
> From: Ng, Boon Khai <[email protected]>
> Sent: Tuesday, May 28, 2024 7:37 AM
> To: Sunil Kovvuri Goutham <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> David S . Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Maxime Coquelin <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Ang, Tien Sung
> <[email protected]>; G Thomas, Rohan <[email protected]>;
> Looi, Hong Aun <[email protected]>; Andy Shevchenko
> <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature
> v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> Stripping
>
> > -----Original Message-----
> > From: Sunil Kovvuri Goutham <[email protected]>
> > Sent: Monday, May 27, 2024 11:36 PM
> > To: Ng, Boon Khai <[email protected]>; Alexandre Torgue
> > <[email protected]>; Jose Abreu <[email protected]>;
> > David S . Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> > <[email protected]>; Maxime Coquelin <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > Ang, Tien Sung <[email protected]>; G Thomas, Rohan
> > <[email protected]>; Looi, Hong Aun <[email protected]>;
> > Andy Shevchenko <[email protected]>; Ilpo Jarvinen
> > <[email protected]>
> > Subject: RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping
> > Feature
> > v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > Stripping
> >
> >
> >
> > > -----Original Message-----
> > > From: Ng, Boon Khai <[email protected]>
> > > Sent: Monday, May 27, 2024 6:58 PM
> > > To: Sunil Kovvuri Goutham <[email protected]>; Alexandre Torgue
> > > <[email protected]>; Jose Abreu <[email protected]>;
> > > David S . Miller <[email protected]>; Eric Dumazet
> > > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> > > <[email protected]>; Maxime Coquelin
> > <[email protected]>;
> > > [email protected]; [email protected];
> > > linux- [email protected]; [email protected];
> > > Ang, Tien Sung <[email protected]>; G Thomas, Rohan
> > > <[email protected]>; Looi, Hong Aun
> > > <[email protected]>; Andy Shevchenko
> > > <[email protected]>; Ilpo Jarvinen
> > > <[email protected]>
> > > Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature
> > > v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > > Stripping
> > >
> > ..........
> >
> > > > > 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > > > > Stripping
> > > > >
> > > >
> > > > New features should be submitted against 'net-next' instead of 'net'.
> > >
> > > Hi Sunil, I was cloning the repo from net-next, but how to choose
> > > the destination as 'net-next'?
> >
> > While creating patch you can add appropriate prefix .. like below git
> > format- patch --subject-prefix="net-next PATCH"
> > git format-patch --subject-prefix="net PATCH"
> >
>
> Okay will update that in the next version.
>
> > >
> > > > Also 'net-next' is currently closed.
> > >
> > > I see, may I know when the next opening period is? Thanks
> >
> > Please track
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patchwork.hopto.o
> > rg_net-
> 2Dnext.html&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_
> F
> >
> 01ggTzHuhwawxR1P9_tMCN2FODU4&m=a48jwcbUStFRUDMUfXcfGEXhkW
> 3Pe9T0oNLv7B3
> > myIrV1geS5aBPZyougPLQZ3vy&s=oO5Em8PF8w6U6a1xROdgg-
> C0TRXsRmdFWku-FZQpH1
> > E&e=
>
> Checked the link it is just a photo saying "come in we're open" is that mean the
> net-next is currently open now?
>
>
>
Yes, it's open now.
> -----Original Message-----
> From: Sunil Kovvuri Goutham <[email protected]>
> Sent: Tuesday, May 28, 2024 3:18 PM
> To: Ng, Boon Khai <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> David S . Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Maxime Coquelin <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; Ang,
> Tien Sung <[email protected]>; G Thomas, Rohan
> <[email protected]>; Looi, Hong Aun <[email protected]>;
> Andy Shevchenko <[email protected]>; Ilpo Jarvinen
> <[email protected]>
> Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
>
>
>
> > -----Original Message-----
> > From: Ng, Boon Khai <[email protected]>
> > Sent: Tuesday, May 28, 2024 7:37 AM
> > To: Sunil Kovvuri Goutham <[email protected]>; Alexandre Torgue
> > <[email protected]>; Jose Abreu <[email protected]>;
> > David S . Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> > <[email protected]>; Maxime Coquelin
> <[email protected]>;
> > [email protected]; [email protected];
> > linux- [email protected]; [email protected];
> > Ang, Tien Sung <[email protected]>; G Thomas, Rohan
> > <[email protected]>; Looi, Hong Aun <[email protected]>;
> > Andy Shevchenko <[email protected]>; Ilpo Jarvinen
> > <[email protected]>
> > Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature
> > v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > Stripping
> >
> > > -----Original Message-----
> > > From: Sunil Kovvuri Goutham <[email protected]>
> > > Sent: Monday, May 27, 2024 11:36 PM
> > > To: Ng, Boon Khai <[email protected]>; Alexandre Torgue
> > > <[email protected]>; Jose Abreu <[email protected]>;
> > > David S . Miller <[email protected]>; Eric Dumazet
> > > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni
> > > <[email protected]>; Maxime Coquelin
> <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > Ang, Tien Sung <[email protected]>; G Thomas, Rohan
> > > <[email protected]>; Looi, Hong Aun
> > > <[email protected]>; Andy Shevchenko
> > > <[email protected]>; Ilpo Jarvinen
> > > <[email protected]>
> > > Subject: RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping
> > > Feature
> > > v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN
> > > Stripping
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ng, Boon Khai <[email protected]>
> > > > Sent: Monday, May 27, 2024 6:58 PM
> > > > To: Sunil Kovvuri Goutham <[email protected]>; Alexandre
> Torgue
> > > > <[email protected]>; Jose Abreu
> <[email protected]>;
> > > > David S . Miller <[email protected]>; Eric Dumazet
> > > > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> > > > Abeni <[email protected]>; Maxime Coquelin
> > > <[email protected]>;
> > > > [email protected]; linux-stm32@st-md-
> mailman.stormreply.com;
> > > > linux- [email protected];
> > > > [email protected]; Ang, Tien Sung
> > > > <[email protected]>; G Thomas, Rohan
> > > > <[email protected]>; Looi, Hong Aun
> > > > <[email protected]>; Andy Shevchenko
> > > > <[email protected]>; Ilpo Jarvinen
> > > > <[email protected]>
> > > > Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature
> > > > v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated
> VLAN
> > > > Stripping
> > > >
> > > ..........
> > >
> > > > > > 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated
> > > > > > VLAN Stripping
> > > > > >
> > > > >
> > > > > New features should be submitted against 'net-next' instead of 'net'.
> > > >
> > > > Hi Sunil, I was cloning the repo from net-next, but how to choose
> > > > the destination as 'net-next'?
> > >
> > > While creating patch you can add appropriate prefix .. like below
> > > git
> > > format- patch --subject-prefix="net-next PATCH"
> > > git format-patch --subject-prefix="net PATCH"
> > >
> >
> > Okay will update that in the next version.
> >
> > > >
> > > > > Also 'net-next' is currently closed.
> > > >
> > > > I see, may I know when the next opening period is? Thanks
> > >
> > > Please track
> > > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__patchwork.hopto.o
> > > rg_net-
> >
> 2Dnext.html&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_
> > F
> > >
> >
> 01ggTzHuhwawxR1P9_tMCN2FODU4&m=a48jwcbUStFRUDMUfXcfGEXhkW
> > 3Pe9T0oNLv7B3
> > > myIrV1geS5aBPZyougPLQZ3vy&s=oO5Em8PF8w6U6a1xROdgg-
> > C0TRXsRmdFWku-FZQpH1
> > > E&e=
> >
> > Checked the link it is just a photo saying "come in we're open" is
> > that mean the net-next is currently open now?
> >
> >
> >
> Yes, it's open now.
Hi Sunil, thanks for confirming, should I straight away submit another change,
with the correct subject prefix on "net-next"? Or I should wait for others to
comments, and fix them all in v3?
> > > Checked the link it is just a photo saying "come in we're open" is
> > > that mean the net-next is currently open now?
> > >
> > >
> > >
> > Yes, it's open now.
>
> Hi Sunil, thanks for confirming, should I straight away submit another change,
> with the correct subject prefix on "net-next"? Or I should wait for others to
> comments, and fix them all in v3?
>
Please trim replies to what it just relevant.
You probably should read:
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
You might also want to read out to other Intel developers in Jesse
Brandeburg group and ask them to do an internal review before you post
to the list.
Andrew
> So, for this XGMAC VLAN patch, the idea of getting the VLAN id from the descriptor is the same, but
> The register bit filed of getting the VLAN packet VALID is different. Thus, it need to be implemented separately.
Please wrap your emails to around 78 characters.
It is well know this driver is a mess. I just wanted to check you are
not adding to be mess by simply cut/pasting rather than refactoring
code.
Lets look at the code. From your patch:
+static void dwxgmac2_rx_hw_vlan(struct mac_device_info *hw,
+ struct dma_desc *rx_desc, struct sk_buff *skb)
+{
+ if (hw->desc->get_rx_vlan_valid(rx_desc)) {
+ u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
+
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
+ }
+}
+
and
static void dwmac4_rx_hw_vlan(struct mac_device_info *hw,
struct dma_desc *rx_desc, struct sk_buff *skb)
{
if (hw->desc->get_rx_vlan_valid(rx_desc)) {
u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
}
}
Looks identical to me.
From your patch:
static void dwxgmac2_set_hw_vlan_mode(struct mac_device_info *hw)
+{
+ void __iomem *ioaddr = hw->pcsr;
+ u32 val = readl(ioaddr + XGMAC_VLAN_TAG);
+
+ val &= ~XGMAC_VLAN_TAG_CTRL_EVLS_MASK;
+
+ if (hw->hw_vlan_en)
+ /* Always strip VLAN on Receive */
+ val |= XGMAC_VLAN_TAG_STRIP_ALL;
+ else
+ /* Do not strip VLAN on Receive */
+ val |= XGMAC_VLAN_TAG_STRIP_NONE;
+
+ /* Enable outer VLAN Tag in Rx DMA descriptro */
+ val |= XGMAC_VLAN_TAG_CTRL_EVLRXS;
+ writel(val, ioaddr + XGMAC_VLAN_TAG);
+}
static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw)
{
void __iomem *ioaddr = hw->pcsr;
u32 value = readl(ioaddr + GMAC_VLAN_TAG);
value &= ~GMAC_VLAN_TAG_CTRL_EVLS_MASK;
if (hw->hw_vlan_en)
/* Always strip VLAN on Receive */
value |= GMAC_VLAN_TAG_STRIP_ALL;
else
/* Do not strip VLAN on Receive */
value |= GMAC_VLAN_TAG_STRIP_NONE;
/* Enable outer VLAN Tag in Rx DMA descriptor */
value |= GMAC_VLAN_TAG_CTRL_EVLRXS;
writel(value, ioaddr + GMAC_VLAN_TAG);
}
The basic flow is the same. Lets look at the #defines:
#define XGMAC_VLAN_TAG 0x00000050
#define GMAC_VLAN_TAG 0x00000050
#define GMAC_VLAN_TAG_CTRL_EVLS_MASK GENMASK(22, 21)
#define GMAC_VLAN_TAG_CTRL_EVLS_SHIFT 21
+#define XGMAC_VLAN_TAG_CTRL_EVLS_MASK GENMASK(22, 21)
+#define XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT 21
+#define XGMAC_VLAN_TAG_STRIP_NONE FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x0)
+#define XGMAC_VLAN_TAG_STRIP_PASS FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x1)
+#define XGMAC_VLAN_TAG_STRIP_FAIL FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x2)
+#define XGMAC_VLAN_TAG_STRIP_ALL FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x3)
#define GMAC_VLAN_TAG_STRIP_NONE (0x0 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
#define GMAC_VLAN_TAG_STRIP_PASS (0x1 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
#define GMAC_VLAN_TAG_STRIP_FAIL (0x2 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
#define GMAC_VLAN_TAG_STRIP_ALL (0x3 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
This is less obvious a straight cut/paste, but they are in fact
identical.
#define GMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
#define XGMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
So this also looks identical to me, but maybe i'm missing something
subtle.
+static inline u16 dwxgmac2_wrback_get_rx_vlan_tci(struct dma_desc *p)
+{
+ return le32_to_cpu(p->des0) & XGMAC_RDES0_VLAN_TAG_MASK;
+}
+
static u16 dwmac4_wrback_get_rx_vlan_tci(struct dma_desc *p)
{
return (le32_to_cpu(p->des0) & RDES0_VLAN_TAG_MASK);
}
#define RDES0_VLAN_TAG_MASK GENMASK(15, 0)
#define XGMAC_RDES0_VLAN_TAG_MASK GENMASK(15, 0)
More identical code.
+static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc *p)
+{
+ u32 et_lt;
+
+ et_lt = FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));
+
+ return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
+ et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG;
+}
static bool dwmac4_wrback_get_rx_vlan_valid(struct dma_desc *p)
{
return ((le32_to_cpu(p->des3) & RDES3_LAST_DESCRIPTOR) &&
(le32_to_cpu(p->des3) & RDES3_RDES0_VALID));
}
#define RDES3_RDES0_VALID BIT(25)
#define RDES3_LAST_DESCRIPTOR BIT(28)
#define XGMAC_RDES3_ET_LT GENMASK(19, 16)
+#define XGMAC_ET_LT_VLAN_STAG 8
+#define XGMAC_ET_LT_VLAN_CTAG 9
+#define XGMAC_ET_LT_DVLAN_CTAG_CTAG 10
This does actually look different.
Please take a step back and see if you can help clean up some of the
mess in this driver by refactoring bits of identical code, rather than
copy/pasting it.
Andrew
> Please trim replies to what it just relevant.
>
> You probably should read:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
>
Hi Andrew, Thanks for pointing that out, will take note on that.
> You might also want to read out to other Intel developers in Jesse Brandeburg
> group and ask them to do an internal review before you post to the list.
>
I have reached out to our internal intel reviewer, and this patch
was reviewed by Andy Shevchenko for V1 and
Ilpo Jarvinen for V2.
For the V1 I got a NACK for the reason new hw_vlan_en switch
is being introduced
https://lore.kernel.org/netdev/DM8PR11MB5751E5388AEFCFB80BCB483F
[email protected]/
So, after the internal review of V2 to detach the newly
Introduced hw_vlan_en switch, I have sent the v2 for
internal review and Ilpo helps to point out some of
the code that can be improved.
When I about to send out the v2, I found that the similar
implementation at dwmac4(which I got NACK) is
already accepted at dwmac4 driver
https://lore.kernel.org/lkml/20231121053842.719531-1-yi.fang.gan
@intel.com/T/
So, I have to revamp the v2 code again to match dwmac4
implementation. as we are using the same upper layer
code (stmmac_main.c). after sending this code out to
review internally again and no one else is entertaining the
review even after email and reminder also private message
were sent out several time and which leads me to publish
the review at the mailing list.
I hold no grievances toward the community or any individual,
I fully understand and appreciate the rigorous review process
necessary to maintain the high quality and integrity of the
linux codebase. My primary goal is to learn and grow as a
contributer and I believe that constructive feedback from
experienced members of the community will be invaluable
in this regard
With that said, I kindly request your assistance in providing
feedback on my code submissions. I am eager to understand
where improvements can be made and how I can align my
contributions more closely with the project's expectations.
By working together, I am confident that we can enhance
the quality of my code and ultimately contribute to the
success of the Linux project as a whole.
Please feel free to review my recent submissions and
share your insights, suggestions, and recommendations.
Your feedback is highly appreciated and will contribute
significantly to help with the linux codebase.
Thank you for your time and consideration.
I look forward to your valuable feedback and to continuing
my journey as a contributor to the Linux project.
Regards,
Boon Khai.
>
> It is well know this driver is a mess. I just wanted to check you are not adding
> to be mess by simply cut/pasting rather than refactoring code.
>
Okay sure Andrew, will take note on this.
>
> static void dwmac4_rx_hw_vlan(struct mac_device_info *hw,
> struct dma_desc *rx_desc, struct sk_buff *skb) {
> if (hw->desc->get_rx_vlan_valid(rx_desc)) {
> u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
>
> __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
> }
> }
>
> Looks identical to me.
Yes, some of the functions are identical, the driver has been divided
into dwmac4_core.c and dwxgmac2_core.c initially, so to enable the
rx_hw_vlan, it is porting from dwmac4_core to dwxgmac2_core.
> The basic flow is the same. Lets look at the #defines:
>
Right, the basic flow is direct copy and paste, and only the function
name is updated from dwmac4 to dwxgmac2.
> +#define XGMAC_VLAN_TAG_STRIP_NONE
> FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x0)
> +#define XGMAC_VLAN_TAG_STRIP_PASS
> FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x1)
> +#define XGMAC_VLAN_TAG_STRIP_FAIL
> FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x2)
> +#define XGMAC_VLAN_TAG_STRIP_ALL
> FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x3)
> #define GMAC_VLAN_TAG_STRIP_NONE (0x0 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
> #define GMAC_VLAN_TAG_STRIP_PASS (0x1 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
> #define GMAC_VLAN_TAG_STRIP_FAIL (0x2 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
> #define GMAC_VLAN_TAG_STRIP_ALL (0x3 <<
> GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
>
> This is less obvious a straight cut/paste, but they are in fact identical.
>
This was Ilpo suggestion to use Field prep and Field get instead.
> #define GMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
> #define XGMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
>
> So this also looks identical to me, but maybe i'm missing something subtle.
>
For VLAN register mapping, they don't have much different between
The dwmac4 and dwxgmac2, but the descriptor of getting the
VLAN id and VLAN packet is valid is a little bit different.
> +static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc
> +*p) {
> + u32 et_lt;
> +
> + et_lt = FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));
> +
> + return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
> + et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG; }
>
> static bool dwmac4_wrback_get_rx_vlan_valid(struct dma_desc *p) {
> return ((le32_to_cpu(p->des3) & RDES3_LAST_DESCRIPTOR) &&
> (le32_to_cpu(p->des3) & RDES3_RDES0_VALID)); }
>
> #define RDES3_RDES0_VALID BIT(25)
> #define RDES3_LAST_DESCRIPTOR BIT(28)
>
> #define XGMAC_RDES3_ET_LT GENMASK(19, 16)
> +#define XGMAC_ET_LT_VLAN_STAG 8
> +#define XGMAC_ET_LT_VLAN_CTAG 9
> +#define XGMAC_ET_LT_DVLAN_CTAG_CTAG 10
>
> This does actually look different.
>
Yes, this is the part in the descriptor where dwxgmac2 get the vlan Valid.
it is described in Designware cores xgmac 10G Ethernet MAC version 3.10a
page 1573.
> Please take a step back and see if you can help clean up some of the mess in
> this driver by refactoring bits of identical code, rather than copy/pasting it.
>
Appreciate if you could help to point out which part that I have to cleanup?
Do you mean I should combine the identical part between dwmac4_core.c
and dwxgmac2_core.c? or I should update the part that looks different and
make them the same?
Regards,
Boon Khai
On Thu, 30 May 2024, Ng, Boon Khai wrote:
> > It is well know this driver is a mess. I just wanted to check you are not adding
> > to be mess by simply cut/pasting rather than refactoring code.
>
> Okay sure Andrew, will take note on this.
>
> > static void dwmac4_rx_hw_vlan(struct mac_device_info *hw,
> > struct dma_desc *rx_desc, struct sk_buff *skb) {
> > if (hw->desc->get_rx_vlan_valid(rx_desc)) {
> > u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
> >
> > __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
> > }
> > }
> >
> > Looks identical to me.
>
> Yes, some of the functions are identical, the driver has been divided
> into dwmac4_core.c and dwxgmac2_core.c initially, so to enable the
> rx_hw_vlan, it is porting from dwmac4_core to dwxgmac2_core.
>
> > The basic flow is the same. Lets look at the #defines:
> >
> Right, the basic flow is direct copy and paste, and only the function
> name is updated from dwmac4 to dwxgmac2.
> > +static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc
> > +*p) {
> > + u32 et_lt;
> > +
> > + et_lt = FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));
> > +
> > + return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
> > + et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG; }
> >
> > static bool dwmac4_wrback_get_rx_vlan_valid(struct dma_desc *p) {
> > return ((le32_to_cpu(p->des3) & RDES3_LAST_DESCRIPTOR) &&
> > (le32_to_cpu(p->des3) & RDES3_RDES0_VALID)); }
> >
> > #define RDES3_RDES0_VALID BIT(25)
> > #define RDES3_LAST_DESCRIPTOR BIT(28)
> >
> > #define XGMAC_RDES3_ET_LT GENMASK(19, 16)
> > +#define XGMAC_ET_LT_VLAN_STAG 8
> > +#define XGMAC_ET_LT_VLAN_CTAG 9
> > +#define XGMAC_ET_LT_DVLAN_CTAG_CTAG 10
> >
> > This does actually look different.
> >
>
> Yes, this is the part in the descriptor where dwxgmac2 get the vlan Valid.
> it is described in Designware cores xgmac 10G Ethernet MAC version 3.10a
> page 1573.
>
> > Please take a step back and see if you can help clean up some of the mess in
> > this driver by refactoring bits of identical code, rather than copy/pasting it.
> >
>
> Appreciate if you could help to point out which part that I have to cleanup?
> Do you mean I should combine the identical part between dwmac4_core.c
> and dwxgmac2_core.c? or I should update the part that looks different and
> make them the same?
You should generalize the existing functions into some other file within
stmmac/ folder and call those functions from both dwmac4_core and dwxgmac2_core.
Do the rework of existing function & callers first and add the new bits
in another patch in the patch series.
Unfortunately, it's hard to catch copy-paste like this from other files
when not very familiar with the driver.
--
i.
> You should generalize the existing functions into some other file within
> stmmac/ folder and call those functions from both dwmac4_core and
> dwxgmac2_core.
> Do the rework of existing function & callers first and add the new bits in
> another patch in the patch series.
>
Hi Ilpo, do you mean I should create a new file for example,
stammc_vlan.c, and move the common vlan function inside?
so that it can be called either from dwmac4_core, dwxgmac2_core
or stmmac_main.c? or maybe I should just consolidate them into
stmmac_main.c?
> Unfortunately, it's hard to catch copy-paste like this from other files when not
> very familiar with the driver.
>
I totally understand that, while I think Andrew has already call them out
In the previous thread.
On Tue, Jun 04, 2024 at 06:05:35AM +0000, Ng, Boon Khai wrote:
>
> > You should generalize the existing functions into some other file within
> > stmmac/ folder and call those functions from both dwmac4_core and
> > dwxgmac2_core.
> > Do the rework of existing function & callers first and add the new bits in
> > another patch in the patch series.
> >
>
> Hi Ilpo, do you mean I should create a new file for example,
> stammc_vlan.c, and move the common vlan function inside?
> so that it can be called either from dwmac4_core, dwxgmac2_core
> or stmmac_main.c? or maybe I should just consolidate them into
> stmmac_main.c?
Do you have access to all the reference documentation for the IP
driven in dwmac4_core.c, dwxgmac2_core.c and stmmac_main.c? Is it just
VLAN which is the same, and everything else is different? Or are other
blocks of the hardware also identical and the code should be shared?
If VLAN is all that is identical, then stammc_vlan.c would make sense.
If there is more in common, you can start the cleanup of the mess this
driver is by moving the VLAN code into a shared file, but make the
naming of that file more generic so more shared code can be added with
later cleanups.
Andrew
>
> Do you have access to all the reference documentation for the IP driven in
> dwmac4_core.c, dwxgmac2_core.c and stmmac_main.c? Is it just VLAN which
> is the same, and everything else is different? Or are other blocks of the
> hardware also identical and the code should be shared?
> If VLAN is all that is identical, then stammc_vlan.c would make sense.
Hi Andrew, I only have access to the document for
dwmac4_core.c and dwxgmac2_core.c, I notice that in the linux mainline
https://github.com/torvalds/linux/tree/master/drivers/net/ethernet/stmicro/
stmmac
it does have stmmac_est.c and stmmac_ptp.c to that support for both
dwmac4 and dwxgmac2, with that I think it is suitable for introducing
another file called stmmac_vlan?
Regards,
Boon Khai
On Fri, Jun 07, 2024 at 04:09:37AM +0000, Ng, Boon Khai wrote:
> >
> > Do you have access to all the reference documentation for the IP driven in
> > dwmac4_core.c, dwxgmac2_core.c and stmmac_main.c? Is it just VLAN which
> > is the same, and everything else is different? Or are other blocks of the
> > hardware also identical and the code should be shared?
> > If VLAN is all that is identical, then stammc_vlan.c would make sense.
>
> Hi Andrew, I only have access to the document for
> dwmac4_core.c and dwxgmac2_core.c
O.K. So please do look at the VLAN code in other places and see if any
can be shared.
, I notice that in the linux mainline
> https://github.com/torvalds/linux/tree/master/drivers/net/ethernet/stmicro/
> stmmac
>
> it does have stmmac_est.c and stmmac_ptp.c to that support for both
> dwmac4 and dwxgmac2, with that I think it is suitable for introducing
> another file called stmmac_vlan?
Yes, stmmac_vlan.c is O.K.
Andrew
> > it does have stmmac_est.c and stmmac_ptp.c to that support for both
> > dwmac4 and dwxgmac2, with that I think it is suitable for introducing
> > another file called stmmac_vlan?
>
> Yes, stmmac_vlan.c is O.K.
Thanks Andrew, I'll make an effort to consolidate the code into the
stmmac_vlan.c, wonder the next submission I should go into net or
net next?
Regards,
Boon Khai
On Tue, 11 Jun 2024, Ng, Boon Khai wrote:
> > > it does have stmmac_est.c and stmmac_ptp.c to that support for both
> > > dwmac4 and dwxgmac2, with that I think it is suitable for introducing
> > > another file called stmmac_vlan?
> >
> > Yes, stmmac_vlan.c is O.K.
>
> Thanks Andrew, I'll make an effort to consolidate the code into the
> stmmac_vlan.c, wonder the next submission I should go into net or
> net next?
By default, everything goes to -next (in any subsystem). Only fixes and a
few other exceptions this is not go through the non-next trees.
--
i.