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

[Simon Horman <horms@kernel.org>]

On Tue, Apr 22, 2025 at 12:29:29AM +0800, Boon Khai Ng 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.
> 
> The descriptor format for VLAN is not moved to the common code due
> to hardware-specific differences between DWMAC4 and DWXGMAC.
> 
> For the DWMAC4 IP, the Receive Normal Descriptor 0 (RDES0) is
> formatted as follows:
>     31                                                0
>       ------------------------ -----------------------
> RDES0| Inner VLAN TAG [31:16] | Outer VLAN TAG [15:0] |
>       ------------------------ -----------------------
> 
> For the DWXGMAC IP, the RDES0 format varies based on the
> Tunneled Frame bit (TNP):
> 
> a) For Non-Tunneled Frame (TNP=0)
> 
>     31                                                0
>       ------------------------ -----------------------
> RDES0| Inner VLAN TAG [31:16] | Outer VLAN TAG [15:0] |
>       ------------------------ -----------------------
> 
> b) For Tunneled Frame (TNP=1)
> 
>      31                   8 7                3 2      0
>       --------------------- ------------------ -------
> RDES0| VNID/VSID           | Reserved         | OL2L3 |
>       --------------------- ------------------ ------
> 
> The logic for handling tunneled frames is not yet implemented
> in the dwxgmac2_wrback_get_rx_vlan_tci() function. Therefore,
> it is prudent to maintain separate functions within their
> respective descriptor driver files
> (dwxgmac2_descs.c and dwmac4_descs.c).
> 
> Signed-off-by: Boon Khai Ng <boon.khai.ng@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index a6d395c6bacd..d9f41c047e5e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -614,76 +614,6 @@ static int dwxgmac2_rss_configure(struct mac_device_info *hw,
>  	return 0;
>  }
>  
> -static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash,
> -				      u16 perfect_match, bool is_double)
> -{
> -	void __iomem *ioaddr = hw->pcsr;
> -
> -	writel(hash, ioaddr + XGMAC_VLAN_HASH_TABLE);
> -
> -	if (hash) {
> -		u32 value = readl(ioaddr + XGMAC_PACKET_FILTER);
> -
> -		value |= XGMAC_FILTER_VTFE;
> -
> -		writel(value, ioaddr + XGMAC_PACKET_FILTER);

Here the XGMAC_FILTER_VTFE bit of XGMAC_PACKET_FILTER is set.
However, this logic does not appear in vlan_update_hash()

> -
> -		value = readl(ioaddr + XGMAC_VLAN_TAG);
> -
> -		value |= XGMAC_VLAN_VTHM | XGMAC_VLAN_ETV;
> -		if (is_double) {
> -			value |= XGMAC_VLAN_EDVLP;
> -			value |= XGMAC_VLAN_ESVL;
> -			value |= XGMAC_VLAN_DOVLTC;
> -		} else {
> -			value &= ~XGMAC_VLAN_EDVLP;
> -			value &= ~XGMAC_VLAN_ESVL;
> -			value &= ~XGMAC_VLAN_DOVLTC;
> -		}

And likewise, here value is based on reading from XGMAC_VLAN_TAG.
Whereas in vlan_update_hash is constructed without reading from
XGMAC_VLAN_TAG.

Can I clarify that this is intentional and that vlan_update_hash(),
which is based on the DWMAC4 implementation, will also work for DWXGMAC IP?

> -
> -		value &= ~XGMAC_VLAN_VID;
> -		writel(value, ioaddr + XGMAC_VLAN_TAG);
> -	} else if (perfect_match) {
> -		u32 value = readl(ioaddr + XGMAC_PACKET_FILTER);
> -
> -		value |= XGMAC_FILTER_VTFE;
> -
> -		writel(value, ioaddr + XGMAC_PACKET_FILTER);
> -
> -		value = readl(ioaddr + XGMAC_VLAN_TAG);
> -
> -		value &= ~XGMAC_VLAN_VTHM;
> -		value |= XGMAC_VLAN_ETV;
> -		if (is_double) {
> -			value |= XGMAC_VLAN_EDVLP;
> -			value |= XGMAC_VLAN_ESVL;
> -			value |= XGMAC_VLAN_DOVLTC;
> -		} else {
> -			value &= ~XGMAC_VLAN_EDVLP;
> -			value &= ~XGMAC_VLAN_ESVL;
> -			value &= ~XGMAC_VLAN_DOVLTC;
> -		}
> -
> -		value &= ~XGMAC_VLAN_VID;
> -		writel(value | perfect_match, ioaddr + XGMAC_VLAN_TAG);
> -	} else {
> -		u32 value = readl(ioaddr + XGMAC_PACKET_FILTER);
> -
> -		value &= ~XGMAC_FILTER_VTFE;
> -
> -		writel(value, ioaddr + XGMAC_PACKET_FILTER);
> -
> -		value = readl(ioaddr + XGMAC_VLAN_TAG);
> -
> -		value &= ~(XGMAC_VLAN_VTHM | XGMAC_VLAN_ETV);
> -		value &= ~(XGMAC_VLAN_EDVLP | XGMAC_VLAN_ESVL);
> -		value &= ~XGMAC_VLAN_DOVLTC;
> -		value &= ~XGMAC_VLAN_VID;
> -
> -		writel(value, ioaddr + XGMAC_VLAN_TAG);
> -	}
> -}

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c

...

> +static int vlan_write_filter(struct net_device *dev,
> +			     struct mac_device_info *hw,
> +			     u8 index, u32 data)
> +{
> +	void __iomem *ioaddr = (void __iomem *)dev->base_addr;
> +	int i, timeout = 10;
> +	u32 val;
> +
> +	if (index >= hw->num_vlan)
> +		return -EINVAL;
> +
> +	writel(data, ioaddr + VLAN_TAG_DATA);
> +
> +	val = readl(ioaddr + VLAN_TAG);
> +	val &= ~(VLAN_TAG_CTRL_OFS_MASK |
> +		VLAN_TAG_CTRL_CT |
> +		VLAN_TAG_CTRL_OB);
> +	val |= (index << VLAN_TAG_CTRL_OFS_SHIFT) | VLAN_TAG_CTRL_OB;
> +
> +	writel(val, ioaddr + VLAN_TAG);
> +
> +	for (i = 0; i < timeout; i++) {
> +		val = readl(ioaddr + VLAN_TAG);
> +		if (!(val & VLAN_TAG_CTRL_OB))
> +			return 0;
> +		udelay(1);
> +	}

I am curious to know why readl_poll_timeout() isn't used here
as was the case in dwmac4_write_vlan_filter().

> +
> +	netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");
> +
> +	return -EBUSY;
> +}

...