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@xxxxxxxxxx>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxx>
...
> 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;
> +}
...
