[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241018092138.xo2zoadx2jng27rp@skbuf>
Date: Fri, 18 Oct 2024 12:21:38 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Furong Xu <0x1207@...il.com>
Cc: netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Andrew Lunn <andrew@...n.ch>, Simon Horman <horms@...nel.org>,
Serge Semin <fancer.lancer@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu <joabreu@...opsys.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>, xfr@...look.com
Subject: Re: [PATCH net-next v2 1/8] net: stmmac: Introduce separate files
for FPE implementation
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@...il.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 :(
Powered by blists - more mailing lists