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
> 
[...]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index 31bdbab9a46c..0a57c5e7497d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -9,6 +9,7 @@
>  #include "stmmac_fpe.h"
>  #include "stmmac_ptp.h"
>  #include "stmmac_est.h"
> +#include "stmmac_vlan.h"
>  #include "dwmac4_descs.h"
>  #include "dwxgmac2.h"
>  
> @@ -120,6 +121,7 @@ static const struct stmmac_hwif_entry {
>  	const void *tc;
>  	const void *mmc;
>  	const void *est;
> +	const void *vlan;
>  	int (*setup)(struct stmmac_priv *priv);
>  	int (*quirks)(struct stmmac_priv *priv);
>  } stmmac_hw[] = {
> @@ -197,6 +199,7 @@ static const struct stmmac_hwif_entry {
>  		.desc = &dwmac4_desc_ops,
>  		.dma = &dwmac4_dma_ops,
>  		.mac = &dwmac410_ops,
> +		.vlan = &dwmac_vlan_ops,

Rename dwmac_vlan_ops to dwmac4_vlan_ops will be better,
just like dwmac4_desc_ops/dwmac4_dma_ops

[...]
> +const struct stmmac_vlan_ops dwmac_vlan_ops = {
> +	.update_vlan_hash = vlan_update_hash,
> +	.enable_vlan = vlan_enable,
> +	.add_hw_vlan_rx_fltr = vlan_add_hw_rx_fltr,
> +	.del_hw_vlan_rx_fltr = vlan_del_hw_rx_fltr,
> +	.restore_hw_vlan_rx_fltr = vlan_restore_hw_rx_fltr,
> +	.rx_hw_vlan = vlan_rx_hw,
> +	.set_hw_vlan_mode = vlan_set_hw_mode,
> +};
> +
> +const struct stmmac_vlan_ops dwxlgmac2_vlan_ops = {
> +	.update_vlan_hash = vlan_update_hash,
> +	.enable_vlan = vlan_enable,
> +};

dwxlgmac2_vlan_ops looks redundant here, another new struct contains
totally identical members.

stmmac_do_void_callback()/stmmac_do_callback() handles NULL function
pointers so good, we can leave the un-implemented functions as NULL.

Are you trying to avoid something undefined here?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.h
> new file mode 100644
> index 000000000000..29e7be83161e
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2025, Altera Corporation
> + * stmmac VLAN(802.1Q) handling
> + */
> +
> +#ifndef __STMMAC_VLAN_H__
> +#define __STMMAC_VLAN_H__
> +
> +#include <linux/bitfield.h>
> +
> +#define VLAN_TAG			0x00000050
> +#define VLAN_TAG_DATA			0x00000054
> +#define VLAN_HASH_TABLE			0x00000058
> +#define VLAN_INCL			0x00000060
> +
> +#define HW_FEATURE3			0x00000128
> +
> +/* MAC VLAN */
> +#define VLAN_EDVLP			BIT(26)
> +#define VLAN_VTHM			BIT(25)
> +#define VLAN_DOVLTC			BIT(20)
> +#define VLAN_ESVL			BIT(18)
> +#define VLAN_ETV			BIT(16)
> +#define VLAN_VID			GENMASK(15, 0)
> +#define VLAN_VLTI			BIT(20)
> +#define VLAN_CSVL			BIT(19)
> +#define VLAN_VLC			GENMASK(17, 16)
> +#define VLAN_VLC_SHIFT			16
> +#define VLAN_VLHT			GENMASK(15, 0)
> +
> +/* MAC VLAN Tag */
> +#define VLAN_TAG_VID			GENMASK(15, 0)
> +#define VLAN_TAG_ETV			BIT(16)
> +
> +/* MAC VLAN Tag Control */
> +#define VLAN_TAG_CTRL_OB		BIT(0)
> +#define VLAN_TAG_CTRL_CT		BIT(1)
> +#define VLAN_TAG_CTRL_OFS_MASK		GENMASK(6, 2)
> +#define VLAN_TAG_CTRL_OFS_SHIFT		2
> +#define VLAN_TAG_CTRL_EVLS_MASK		GENMASK(22, 21)
> +#define VLAN_TAG_CTRL_EVLS_SHIFT	21
> +#define VLAN_TAG_CTRL_EVLRXS		BIT(24)
> +
> +#define VLAN_TAG_STRIP_NONE		FIELD_PREP(VLAN_TAG_CTRL_EVLS_MASK, 0x0)
> +#define VLAN_TAG_STRIP_PASS		FIELD_PREP(VLAN_TAG_CTRL_EVLS_MASK, 0x1)
> +#define VLAN_TAG_STRIP_FAIL		FIELD_PREP(VLAN_TAG_CTRL_EVLS_MASK, 0x2)
> +#define VLAN_TAG_STRIP_ALL		FIELD_PREP(VLAN_TAG_CTRL_EVLS_MASK, 0x3)
> +
> +/* MAC VLAN Tag Data/Filter */
> +#define VLAN_TAG_DATA_VID		GENMASK(15, 0)
> +#define VLAN_TAG_DATA_VEN		BIT(16)
> +#define VLAN_TAG_DATA_ETV		BIT(17)
> +
> +/* MAC VLAN HW FEAT */
> +#define VLAN_HW_FEAT_NRVF		GENMASK(2, 0)
> +
> +extern const struct stmmac_vlan_ops dwmac_vlan_ops;
> +extern const struct stmmac_vlan_ops dwxlgmac2_vlan_ops;
> +
> +u32 stmmac_get_num_vlan(void __iomem *ioaddr);
> +
> +#endif /* __STMMAC_VLAN_H__ */

It is a good practice to only keep inside the header those definitions
which are truly exported by stmmac_vlan.c towards external callers.
That means those #defines which are only used within stmmac_vlan.c
shouldn't be here, but inside stmmac_vlan.c file.