[Enable Designware XGMAC VLAN Stripping Feature v2 0/1]

[Enable Designware XGMAC VLAN Stripping Feature v2 0/1]

[Boon Khai Ng]

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

[Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Boon Khai Ng]

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 <boon.khai.ng@intel.com>
---
 .../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

RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Sunil Kovvuri Goutham]

> -----Original Message-----
> From: Boon Khai Ng <boon.khai.ng@intel.com>
> Sent: Monday, May 27, 2024 3:04 PM
> To: 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>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Tien Sung Ang <tien.sung.ang@intel.com>; G Thomas
> Rohan <rohan.g.thomas@intel.com>; Looi Hong Aun
> <hong.aun.looi@intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> Cc: Boon Khai Ng <boon.khai.ng@intel.com>
> 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.

RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> Sent: Monday, May 27, 2024 6:41 PM
> To: Ng, Boon Khai <boon.khai.ng@intel.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>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Ang,
> Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Looi, Hong Aun <hong.aun.looi@intel.com>;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> 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 <boon.khai.ng@intel.com>
> > Sent: Monday, May 27, 2024 3:04 PM
> > To: 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>;
> Paolo
> > Abeni <pabeni@redhat.com>; Maxime Coquelin
> > <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-
> stm32@st-
> > md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> > linux- kernel@vger.kernel.org; Tien Sung Ang
> > <tien.sung.ang@intel.com>; G Thomas Rohan
> <rohan.g.thomas@intel.com>;
> > Looi Hong Aun <hong.aun.looi@intel.com>; Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > <ilpo.jarvinen@linux.intel.com>
> > Cc: Boon Khai Ng <boon.khai.ng@intel.com>
> > 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

RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Sunil Kovvuri Goutham]

> -----Original Message-----
> From: Ng, Boon Khai <boon.khai.ng@intel.com>
> Sent: Monday, May 27, 2024 6:58 PM
> To: Sunil Kovvuri Goutham <sgoutham@marvell.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>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Ang, Tien Sung
> <tien.sung.ang@intel.com>; G Thomas, Rohan <rohan.g.thomas@intel.com>;
> Looi, Hong Aun <hong.aun.looi@intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> 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

RE: [EXTERNAL] [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> Sent: Monday, May 27, 2024 11:36 PM
> To: Ng, Boon Khai <boon.khai.ng@intel.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>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Ang,
> Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Looi, Hong Aun <hong.aun.looi@intel.com>;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> 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 <boon.khai.ng@intel.com>
> > Sent: Monday, May 27, 2024 6:58 PM
> > To: Sunil Kovvuri Goutham <sgoutham@marvell.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>; Paolo Abeni
> > <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>;
> > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> > linux- arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > Ang, Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> > <rohan.g.thomas@intel.com>; Looi, Hong Aun <hong.aun.looi@intel.com>;
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > <ilpo.jarvinen@linux.intel.com>
> > 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?

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Sunil Kovvuri Goutham]

> -----Original Message-----
> From: Ng, Boon Khai <boon.khai.ng@intel.com>
> Sent: Tuesday, May 28, 2024 7:37 AM
> To: Sunil Kovvuri Goutham <sgoutham@marvell.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>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Ang, Tien Sung
> <tien.sung.ang@intel.com>; G Thomas, Rohan <rohan.g.thomas@intel.com>;
> Looi, Hong Aun <hong.aun.looi@intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> 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 <sgoutham@marvell.com>
> > Sent: Monday, May 27, 2024 11:36 PM
> > To: Ng, Boon Khai <boon.khai.ng@intel.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>; Paolo Abeni
> > <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > Ang, Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> > <rohan.g.thomas@intel.com>; Looi, Hong Aun <hong.aun.looi@intel.com>;
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > <ilpo.jarvinen@linux.intel.com>
> > 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 <boon.khai.ng@intel.com>
> > > Sent: Monday, May 27, 2024 6:58 PM
> > > To: Sunil Kovvuri Goutham <sgoutham@marvell.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>; Paolo Abeni
> > > <pabeni@redhat.com>; Maxime Coquelin
> > <mcoquelin.stm32@gmail.com>;
> > > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> > > linux- arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > Ang, Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> > > <rohan.g.thomas@intel.com>; Looi, Hong Aun
> > > <hong.aun.looi@intel.com>; Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > > <ilpo.jarvinen@linux.intel.com>
> > > 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.

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> Sent: Tuesday, May 28, 2024 3:18 PM
> To: Ng, Boon Khai <boon.khai.ng@intel.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>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Ang,
> Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Looi, Hong Aun <hong.aun.looi@intel.com>;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> 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 <boon.khai.ng@intel.com>
> > Sent: Tuesday, May 28, 2024 7:37 AM
> > To: Sunil Kovvuri Goutham <sgoutham@marvell.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>; Paolo Abeni
> > <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>;
> > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> > linux- arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > Ang, Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> > <rohan.g.thomas@intel.com>; Looi, Hong Aun <hong.aun.looi@intel.com>;
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > <ilpo.jarvinen@linux.intel.com>
> > 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 <sgoutham@marvell.com>
> > > Sent: Monday, May 27, 2024 11:36 PM
> > > To: Ng, Boon Khai <boon.khai.ng@intel.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>; Paolo
> Abeni
> > > <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>;
> > > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > Ang, Tien Sung <tien.sung.ang@intel.com>; G Thomas, Rohan
> > > <rohan.g.thomas@intel.com>; Looi, Hong Aun
> > > <hong.aun.looi@intel.com>; Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > > <ilpo.jarvinen@linux.intel.com>
> > > 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 <boon.khai.ng@intel.com>
> > > > Sent: Monday, May 27, 2024 6:58 PM
> > > > To: Sunil Kovvuri Goutham <sgoutham@marvell.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>; Paolo
> > > > Abeni <pabeni@redhat.com>; Maxime Coquelin
> > > <mcoquelin.stm32@gmail.com>;
> > > > netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com;
> > > > linux- arm-kernel@lists.infradead.org;
> > > > linux-kernel@vger.kernel.org; Ang, Tien Sung
> > > > <tien.sung.ang@intel.com>; G Thomas, Rohan
> > > > <rohan.g.thomas@intel.com>; Looi, Hong Aun
> > > > <hong.aun.looi@intel.com>; Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> > > > <ilpo.jarvinen@linux.intel.com>
> > > > 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?

Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Andrew Lunn]

> > > 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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> 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
C13FA@DM8PR11MB5751.namprd11.prod.outlook.com/

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.

Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Andrew Lunn]

> 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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, May 28, 2024 3:00 AM
> To: Ng, Boon Khai <boon.khai.ng@intel.com>
> Cc: 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>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Ang, Tien Sung <tien.sung.ang@intel.com>; G
> Thomas, Rohan <rohan.g.thomas@intel.com>; Looi, Hong Aun
> <hong.aun.looi@intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Ilpo Jarvinen
> <ilpo.jarvinen@linux.intel.com>
> 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

Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Andrew Lunn]

> 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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, 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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ilpo Järvinen]

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.

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

 
> 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.

Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Andrew Lunn]

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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> 
> 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

Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Andrew Lunn]

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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> > 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

RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ilpo Järvinen]

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.