Received-SPF: pass (google.com: domain of linux-kernel+bounces-192360-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 28 May 2024 15:02:17 +0200
From: Andrew Lunn <andrew@lunn.ch>
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" <netdev@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>,
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.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
Message-ID: <48673551-cada-4194-865f-bc04c1e19c29@lunn.ch>
References: <20240527093339.30883-1-boon.khai.ng@intel.com>
 <20240527093339.30883-2-boon.khai.ng@intel.com>
 <48176576-e1d2-4c45-967a-91cabb982a21@lunn.ch>
 <DM8PR11MB5751469FAA2B01EB6CEB7B50C1F12@DM8PR11MB5751.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DM8PR11MB5751469FAA2B01EB6CEB7B50C1F12@DM8PR11MB5751.namprd11.prod.outlook.com>

> 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