Re: [PATCH net-next v2 1/8] net: stmmac: Introduce separate files for FPE implementation

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Oct 18, 2024 at 02:39:07PM +0800, Furong Xu wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> new file mode 100644
> index 000000000000..d4d46a07d6a7
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Furong Xu <0x1207@gmail.com>
> + * stmmac FPE(802.3 Qbu) handling
> + */
> +#include "stmmac.h"
> +
> +#define STMMAC_FPE_MM_MAX_VERIFY_RETRIES	3
> +#define STMMAC_FPE_MM_MAX_VERIFY_TIME_MS	128
> +
> +#define MAC_FPE_CTRL_STS		0x00000234
> +#define TRSP				BIT(19)
> +#define TVER				BIT(18)
> +#define RRSP				BIT(17)
> +#define RVER				BIT(16)
> +#define SRSP				BIT(2)
> +#define SVER				BIT(1)
> +#define EFPE				BIT(0)
> +
> +#define MTL_FPE_CTRL_STS		0x00000c90
> +/* Preemption Classification */
> +#define DWMAC5_PREEMPTION_CLASS		GENMASK(15, 8)
> +/* Additional Fragment Size of preempted frames */
> +#define DWMAC5_ADD_FRAG_SZ		GENMASK(1, 0)
> +
> +#define XGMAC_FPE_CTRL_STS		0x00000280
> +#define XGMAC_EFPE			BIT(0)
> +
> +/* FPE link-partner hand-shaking mPacket type */
> +enum stmmac_mpacket_type {
> +	MPACKET_VERIFY = 0,
> +	MPACKET_RESPONSE = 1,
> +};
> +
> +void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up);
> +void stmmac_fpe_event_status(struct stmmac_priv *priv, int status);
> +void stmmac_fpe_init(struct stmmac_priv *priv);
> +void stmmac_fpe_apply(struct stmmac_priv *priv);
> +
> +void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> +			  u32 num_txq, u32 num_rxq,
> +			  bool tx_enable, bool pmac_enable);
> +void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
> +			     struct stmmac_fpe_cfg *cfg,
> +			     enum stmmac_mpacket_type type);
> +int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev);
> +int dwmac5_fpe_get_add_frag_size(const void __iomem *ioaddr);
> +void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size);
> +int dwmac5_fpe_map_preemption_class(struct net_device *ndev,
> +				    struct netlink_ext_ack *extack, u32 pclass);
> +
> +void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> +			    u32 num_txq, u32 num_rxq,
> +			    bool tx_enable, bool pmac_enable);

2 comments about this header:

- it would be good if it contained a "#pragma once" equivalent, in case
  other headers include it (#ifndef STMMAC_FPE_H #define STMMAC_FPE_H).

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

- You could remove the dependency upon the giant #include "stmmac.h" and
  just provide those definitions necessary for this header to be
  self-consistent. By self-consistent I mean that a dummy C file
  containing just #include "stmmac_fpe.h" should compile without errors.

diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 7e46dca90628..3fe0a555aa9a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o	\
 	      mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o	\
 	      dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \
 	      stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \
-	      stmmac_xdp.o stmmac_est.o stmmac_fpe.o \
+	      stmmac_xdp.o stmmac_est.o stmmac_fpe.o dummy.o \
 	      $(stmmac-y)
 
 stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dummy.c b/drivers/net/ethernet/stmicro/stmmac/dummy.c
new file mode 100644
index 000000000000..fc976afd5e40
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dummy.c
@@ -0,0 +1 @@
+#include "stmmac_fpe.h"

The current stmmac_fpe.h as you've posted it does pass the dummy test,
but I think it can be further minimized. Having headers larger than they
need to be will increase build time, little by little.

Well, those were 3 comments :(