Re: [PATCH net-next v3 1/2] net: stmmac: Refactor VLAN implementation

[Furong Xu <0x1207@gmail.com>]

On Tue,  8 Apr 2025 16:13:53 +0800, Boon Khai Ng <boon.khai.ng@altera.com> wrote:

> Refactor VLAN implementation by moving common code for DWMAC4 and
> DWXGMAC IPs into a separate VLAN module. VLAN implementation for
> DWMAC4 and DWXGMAC differs only for CSR base address, the descriptor
> for the VLAN ID and VLAN VALID bit field.
> 
> Signed-off-by: Boon Khai Ng <boon.khai.ng@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/Makefile  |   2 +-
>  drivers/net/ethernet/stmicro/stmmac/common.h  |   1 +
>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h  |  40 ---
>  .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 295 +-----------------
>  .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  13 -
>  .../ethernet/stmicro/stmmac/dwxgmac2_core.c   |  87 ------
>  drivers/net/ethernet/stmicro/stmmac/hwif.c    |   8 +
>  drivers/net/ethernet/stmicro/stmmac/hwif.h    |  61 ++--
>  .../net/ethernet/stmicro/stmmac/stmmac_vlan.c | 294 +++++++++++++++++
>  .../net/ethernet/stmicro/stmmac/stmmac_vlan.h |  63 ++++
>  10 files changed, 401 insertions(+), 463 deletions(-)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.h
> 
[...]
> +static void vlan_update_hash(struct mac_device_info *hw, u32 hash,
> +			     __le16 perfect_match, bool is_double)
> +{
> +	void __iomem *ioaddr = hw->pcsr;
> +	u32 value;
> +
> +	writel(hash, ioaddr + VLAN_HASH_TABLE);
> +
> +	value = readl(ioaddr + VLAN_TAG);
> +
> +	if (hash) {
> +		value |= VLAN_VTHM | VLAN_ETV;
> +		if (is_double) {
> +			value |= VLAN_EDVLP;
> +			value |= VLAN_ESVL;
> +			value |= VLAN_DOVLTC;

I can confirm that 802.1ad (QinQ) has been broken on stmmac for years,
and it will be so nice if this refactoring includes some fixes for QinQ

> +		}
> +
> +		writel(value, ioaddr + VLAN_TAG);
> +	} else if (perfect_match) {
> +		u32 value = VLAN_ETV;
> +
> +		if (is_double) {
> +			value |= VLAN_EDVLP;
> +			value |= VLAN_ESVL;
> +			value |= VLAN_DOVLTC;
> +		}
> +
> +		writel(value | perfect_match, ioaddr + VLAN_TAG);
> +	} else {
> +		value &= ~(VLAN_VTHM | VLAN_ETV);
> +		value &= ~(VLAN_EDVLP | VLAN_ESVL);
> +		value &= ~VLAN_DOVLTC;
> +		value &= ~VLAN_VID;
> +
> +		writel(value, ioaddr + VLAN_TAG);
> +	}
> +}
> +
> +static void vlan_enable(struct mac_device_info *hw, u32 type)
> +{
> +	void __iomem *ioaddr = hw->pcsr;
> +	u32 value;
> +
> +	value = readl(ioaddr + VLAN_INCL);
> +	value |= VLAN_VLTI;
> +	value |= VLAN_CSVL; /* Only use SVLAN */
> +	value &= ~VLAN_VLC;
> +	value |= (type << VLAN_VLC_SHIFT) & VLAN_VLC;
> +	writel(value, ioaddr + VLAN_INCL);
> +}
> +
> +static void vlan_rx_hw(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);

So, as the comment above, ETH_P_8021AD or ETH_P_8021Q shall be set selectively
depend on the frame type.

> +	}
> +}