[PATCH net-next 0/5] net: fec: do some cleanup for the driver

[PATCH net-next 0/5] net: fec: do some cleanup for the driver

[Wei Fang]

This patch set removes some unnecessary or invalid code from the FEC
driver. See each patch for details.

Wei Fang (5):
  net: fec: remove useless conditional preprocessor directives
  net: fec: simplify the conditional preprocessor directives
  net: fec: remove struct fec_enet_priv_txrx_info
  net: fec: remove rx_align from fec_enet_private
  net: fec: remove duplicate macros of the BD status

 drivers/net/ethernet/freescale/fec.h      | 30 +-----------
 drivers/net/ethernet/freescale/fec_main.c | 58 +++++++----------------
 2 files changed, 19 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH net-next 1/5] net: fec: remove useless conditional preprocessor directives

[Wei Fang]

The conditional preprocessor directive "#if !defined(CONFIG_M5272)" was
added due to build errors on MCF5272 platform, see commit d13919301d9a
("net: fec: Fix build for MCF5272"). The compilation error was caused by
some register macros not being defined on the MCF5272 platform. However,
this preprocessor directive is not needed in some parts of the driver.
First, removing it will not cause compilation errors. Second, these parts
will check quirks, which do not exist on the MCF7527 platform. Therefore,
we can safely delete these useless preprocessor directives.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 742f3e81cc7c..e0e84f2979c8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1773,7 +1773,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 	__fec32 cbd_bufaddr;
 	u32 sub_len = 4;
 
-#if !defined(CONFIG_M5272)
 	/*If it has the FEC_QUIRK_HAS_RACC quirk property, the bit of
 	 * FEC_RACC_SHIFT16 is set by default in the probe function.
 	 */
@@ -1781,7 +1780,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 		data_start += 2;
 		sub_len += 2;
 	}
-#endif
 
 #if defined(CONFIG_COLDFIRE) && !defined(CONFIG_COLDFIRE_COHERENT_DMA)
 	/*
@@ -2515,9 +2513,7 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 		phy_set_max_speed(phy_dev, 1000);
 		phy_remove_link_mode(phy_dev,
 				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-#if !defined(CONFIG_M5272)
 		phy_support_sym_pause(phy_dev);
-#endif
 	}
 	else
 		phy_set_max_speed(phy_dev, 100);
@@ -4400,11 +4396,9 @@ fec_probe(struct platform_device *pdev)
 	fep->num_rx_queues = num_rx_qs;
 	fep->num_tx_queues = num_tx_qs;
 
-#if !defined(CONFIG_M5272)
 	/* default enable pause frame auto negotiation */
 	if (fep->quirks & FEC_QUIRK_HAS_GBIT)
 		fep->pause_flag |= FEC_PAUSE_FLAG_AUTONEG;
-#endif
 
 	/* Select default pin state */
 	pinctrl_pm_select_default_state(&pdev->dev);
-- 
2.34.1

[PATCH net-next 2/5] net: fec: simplify the conditional preprocessor directives

[Wei Fang]

From the Kconfig file, we can see CONFIG_FEC depends on the following
platform-related options.

ColdFire: M523x, M527x, M5272, M528x, M520x and M532x
S32: ARCH_S32 (ARM64)
i.MX: SOC_IMX28 and ARCH_MXC (ARM and ARM64)

Based on the code of fec driver, only some macro definitions on the
M5272 platform are different from those on other platforms. Therefore,
we can simplify the following complex preprocessor directives to
"if !defined(CONFIG_M5272)".

"#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || \
     defined(CONFIG_M528x) || defined(CONFIG_M520x) || \
     defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
     defined(CONFIG_ARM64)"

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      |  4 +---
 drivers/net/ethernet/freescale/fec_main.c | 27 ++++++-----------------
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 41e0d85d15da..8e438f6e7ec4 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -24,9 +24,7 @@
 #include <linux/timecounter.h>
 #include <net/xdp.h>
 
-#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
-    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
-    defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
+#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
 /*
  *	Just figures, Motorola would have to change the offsets for
  *	registers in the same peripheral device on different models
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index e0e84f2979c8..9d0e5abe5f66 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -253,9 +253,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
  * size bits. Other FEC hardware does not, so we need to take that into
  * account when setting it.
  */
-#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
-    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
-    defined(CONFIG_ARM64)
+#ifndef CONFIG_M5272
 #define	OPT_ARCH_HAS_MAX_FL	1
 #else
 #define	OPT_ARCH_HAS_MAX_FL	0
@@ -2704,9 +2702,7 @@ static int fec_enet_get_regs_len(struct net_device *ndev)
 }
 
 /* List of registers that can be safety be read to dump them with ethtool */
-#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
-	defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
-	defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
+#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
 static __u32 fec_enet_register_version = 2;
 static u32 fec_enet_register_offset[] = {
 	FEC_IEVENT, FEC_IMASK, FEC_R_DES_ACTIVE_0, FEC_X_DES_ACTIVE_0,
@@ -2780,29 +2776,20 @@ static u32 fec_enet_register_offset[] = {
 static void fec_enet_get_regs(struct net_device *ndev,
 			      struct ethtool_regs *regs, void *regbuf)
 {
+	u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	u32 __iomem *theregs = (u32 __iomem *)fep->hwp;
+	u32 *reg_list = fec_enet_register_offset;
 	struct device *dev = &fep->pdev->dev;
 	u32 *buf = (u32 *)regbuf;
 	u32 i, off;
 	int ret;
-#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
-	defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
-	defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
-	u32 *reg_list;
-	u32 reg_cnt;
-
-	if (!of_machine_is_compatible("fsl,imx6ul")) {
-		reg_list = fec_enet_register_offset;
-		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
-	} else {
+
+#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
+	if (of_machine_is_compatible("fsl,imx6ul")) {
 		reg_list = fec_enet_register_offset_6ul;
 		reg_cnt = ARRAY_SIZE(fec_enet_register_offset_6ul);
 	}
-#else
-	/* coldfire */
-	static u32 *reg_list = fec_enet_register_offset;
-	static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
 #endif
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-- 
2.34.1

[PATCH net-next 3/5] net: fec: remove struct fec_enet_priv_txrx_info

[Wei Fang]

The struct fec_enet_priv_txrx_info has three members: offset, page and
skb. The offset is only initialized in the driver and is not used, and
we can see that it likely will not be used in the future. The skb is
never initialized and used in the driver. Therefore, struct
fec_enet_priv_txrx_info can be directly replaced by struct page.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      |  8 +-------
 drivers/net/ethernet/freescale/fec_main.c | 11 +++++------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 8e438f6e7ec4..c5bbc2c16a4f 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -528,12 +528,6 @@ struct bufdesc_prop {
 	unsigned char dsize_log2;
 };
 
-struct fec_enet_priv_txrx_info {
-	int	offset;
-	struct	page *page;
-	struct  sk_buff *skb;
-};
-
 enum {
 	RX_XDP_REDIRECT = 0,
 	RX_XDP_PASS,
@@ -573,7 +567,7 @@ struct fec_enet_priv_tx_q {
 
 struct fec_enet_priv_rx_q {
 	struct bufdesc_prop bd;
-	struct  fec_enet_priv_txrx_info rx_skb_info[RX_RING_SIZE];
+	struct page *rx_buf[RX_RING_SIZE];
 
 	/* page_pool */
 	struct page_pool *page_pool;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9d0e5abe5f66..5de86c8bc78e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1655,8 +1655,7 @@ static int fec_enet_update_cbd(struct fec_enet_priv_rx_q *rxq,
 	if (unlikely(!new_page))
 		return -ENOMEM;
 
-	rxq->rx_skb_info[index].page = new_page;
-	rxq->rx_skb_info[index].offset = FEC_ENET_XDP_HEADROOM;
+	rxq->rx_buf[index] = new_page;
 	phys_addr = page_pool_get_dma_addr(new_page) + FEC_ENET_XDP_HEADROOM;
 	bdp->cbd_bufaddr = cpu_to_fec32(phys_addr);
 
@@ -1834,7 +1833,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
 		ndev->stats.rx_bytes += pkt_len;
 
 		index = fec_enet_get_bd_index(bdp, &rxq->bd);
-		page = rxq->rx_skb_info[index].page;
+		page = rxq->rx_buf[index];
 		cbd_bufaddr = bdp->cbd_bufaddr;
 		if (fec_enet_update_cbd(rxq, bdp, index)) {
 			ndev->stats.rx_dropped++;
@@ -3309,7 +3308,8 @@ static void fec_enet_free_buffers(struct net_device *ndev)
 	for (q = 0; q < fep->num_rx_queues; q++) {
 		rxq = fep->rx_queue[q];
 		for (i = 0; i < rxq->bd.ring_size; i++)
-			page_pool_put_full_page(rxq->page_pool, rxq->rx_skb_info[i].page, false);
+			page_pool_put_full_page(rxq->page_pool, rxq->rx_buf[i],
+						false);
 
 		for (i = 0; i < XDP_STATS_TOTAL; i++)
 			rxq->stats[i] = 0;
@@ -3443,8 +3443,7 @@ fec_enet_alloc_rxq_buffers(struct net_device *ndev, unsigned int queue)
 		phys_addr = page_pool_get_dma_addr(page) + FEC_ENET_XDP_HEADROOM;
 		bdp->cbd_bufaddr = cpu_to_fec32(phys_addr);
 
-		rxq->rx_skb_info[i].page = page;
-		rxq->rx_skb_info[i].offset = FEC_ENET_XDP_HEADROOM;
+		rxq->rx_buf[i] = page;
 		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY);
 
 		if (fep->bufdesc_ex) {
-- 
2.34.1

[PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Wei Fang]

The rx_align was introduced by the commit 41ef84ce4c72 ("net: fec: change
FEC alignment according to i.mx6 sx requirement"). Because the i.MX6 SX
requires RX buffer must be 64 bytes alignment.

Since the commit 95698ff6177b ("net: fec: using page pool to manage RX
buffers"), the address of the RX buffer is always the page address plus
128 bytes, so RX buffer is always 64-byte aligned. Therefore, rx_align
has no effect since that commit, and we can safely remove it.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      | 1 -
 drivers/net/ethernet/freescale/fec_main.c | 6 +-----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index c5bbc2c16a4f..a25dca9c7d71 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -660,7 +660,6 @@ struct fec_enet_private {
 	struct pm_qos_request pm_qos_req;
 
 	unsigned int tx_align;
-	unsigned int rx_align;
 
 	/* hw interrupt coalesce */
 	unsigned int rx_pkts_itr;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 5de86c8bc78e..cf598d5260fb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -4069,10 +4069,8 @@ static int fec_enet_init(struct net_device *ndev)
 
 	WARN_ON(dsize != (1 << dsize_log2));
 #if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
-	fep->rx_align = 0xf;
 	fep->tx_align = 0xf;
 #else
-	fep->rx_align = 0x3;
 	fep->tx_align = 0x3;
 #endif
 	fep->rx_pkts_itr = FEC_ITR_ICFT_DEFAULT;
@@ -4161,10 +4159,8 @@ static int fec_enet_init(struct net_device *ndev)
 		fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
 	}
 
-	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) {
+	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES)
 		fep->tx_align = 0;
-		fep->rx_align = 0x3f;
-	}
 
 	ndev->hw_features = ndev->features;
 
-- 
2.34.1

[PATCH net-next 5/5] net: fec: remove duplicate macros of the BD status

[Wei Fang]

There are two sets of macros used to define the status bits of TX and RX
BDs, one is the BD_SC_xx macros, the other one is the BD_ENET_xx macros.
For the BD_SC_xx macros, only BD_SC_WRAP is used in the driver. But the
BD_ENET_xx macros are more widely used in the driver, and they define
more bits of the BD status. Therefore, let us remove the BD_SC_xx macros
from now on.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      | 17 -----------------
 drivers/net/ethernet/freescale/fec_main.c |  8 ++++----
 2 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index a25dca9c7d71..7b4d1fc8e7eb 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -240,23 +240,6 @@ struct bufdesc_ex {
 	__fec16 res0[4];
 };
 
-/*
- *	The following definitions courtesy of commproc.h, which where
- *	Copyright (c) 1997 Dan Malek (dmalek@jlc.net).
- */
-#define BD_SC_EMPTY	((ushort)0x8000)	/* Receive is empty */
-#define BD_SC_READY	((ushort)0x8000)	/* Transmit is ready */
-#define BD_SC_WRAP	((ushort)0x2000)	/* Last buffer descriptor */
-#define BD_SC_INTRPT	((ushort)0x1000)	/* Interrupt on change */
-#define BD_SC_CM	((ushort)0x0200)	/* Continuous mode */
-#define BD_SC_ID	((ushort)0x0100)	/* Rec'd too many idles */
-#define BD_SC_P		((ushort)0x0100)	/* xmt preamble */
-#define BD_SC_BR	((ushort)0x0020)	/* Break received */
-#define BD_SC_FR	((ushort)0x0010)	/* Framing error */
-#define BD_SC_PR	((ushort)0x0008)	/* Parity error */
-#define BD_SC_OV	((ushort)0x0002)	/* Overrun */
-#define BD_SC_CD	((ushort)0x0001)	/* ?? */
-
 /* Buffer descriptor control/status used by Ethernet receive.
  */
 #define BD_ENET_RX_EMPTY	((ushort)0x8000)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index cf598d5260fb..3d227c9c5ba5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1010,7 +1010,7 @@ static void fec_enet_bd_init(struct net_device *dev)
 
 		/* Set the last buffer to wrap */
 		bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
-		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
+		bdp->cbd_sc |= cpu_to_fec16(BD_ENET_RX_WRAP);
 
 		rxq->bd.cur = rxq->bd.base;
 	}
@@ -1060,7 +1060,7 @@ static void fec_enet_bd_init(struct net_device *dev)
 
 		/* Set the last buffer to wrap */
 		bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
-		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
+		bdp->cbd_sc |= cpu_to_fec16(BD_ENET_TX_WRAP);
 		txq->dirty_tx = bdp;
 	}
 }
@@ -3456,7 +3456,7 @@ fec_enet_alloc_rxq_buffers(struct net_device *ndev, unsigned int queue)
 
 	/* Set the last buffer to wrap. */
 	bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
-	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
+	bdp->cbd_sc |= cpu_to_fec16(BD_ENET_RX_WRAP);
 	return 0;
 
  err_alloc:
@@ -3492,7 +3492,7 @@ fec_enet_alloc_txq_buffers(struct net_device *ndev, unsigned int queue)
 
 	/* Set the last buffer to wrap. */
 	bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
-	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
+	bdp->cbd_sc |= cpu_to_fec16(BD_ENET_TX_WRAP);
 
 	return 0;
 
-- 
2.34.1

Re: [PATCH net-next 1/5] net: fec: remove useless conditional preprocessor directives

[Frank Li]

On Tue, Nov 11, 2025 at 06:00:53PM +0800, Wei Fang wrote:
> The conditional preprocessor directive "#if !defined(CONFIG_M5272)" was
> added due to build errors on MCF5272 platform, see commit d13919301d9a
> ("net: fec: Fix build for MCF5272"). The compilation error was caused by
> some register macros not being defined on the MCF5272 platform.
> However,
> this preprocessor directive is not needed in some parts of the driver.
> First, removing it will not cause compilation errors. Second, these parts
> will check quirks, which do not exist on the MCF7527 platform. Therefore,
> we can safely delete these useless preprocessor directives.

How about

Drop conditional preprocessor directives added to fix build errors on the
MCF5272 platform (see commit d13919301d9a "net: fec: Fix build for
MCF5272"). The compilation errors were originally caused by some register
macros not being defined on that platform.

The driver now uses quirks to dynamically handle platform differences,
so these directives are no longer required and can be safely removed
without causing compilation or functional issue.

Frank

>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 742f3e81cc7c..e0e84f2979c8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1773,7 +1773,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  	__fec32 cbd_bufaddr;
>  	u32 sub_len = 4;
>
> -#if !defined(CONFIG_M5272)
>  	/*If it has the FEC_QUIRK_HAS_RACC quirk property, the bit of
>  	 * FEC_RACC_SHIFT16 is set by default in the probe function.
>  	 */
> @@ -1781,7 +1780,6 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  		data_start += 2;
>  		sub_len += 2;
>  	}
> -#endif
>
>  #if defined(CONFIG_COLDFIRE) && !defined(CONFIG_COLDFIRE_COHERENT_DMA)
>  	/*
> @@ -2515,9 +2513,7 @@ static int fec_enet_mii_probe(struct net_device *ndev)
>  		phy_set_max_speed(phy_dev, 1000);
>  		phy_remove_link_mode(phy_dev,
>  				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> -#if !defined(CONFIG_M5272)
>  		phy_support_sym_pause(phy_dev);
> -#endif
>  	}
>  	else
>  		phy_set_max_speed(phy_dev, 100);
> @@ -4400,11 +4396,9 @@ fec_probe(struct platform_device *pdev)
>  	fep->num_rx_queues = num_rx_qs;
>  	fep->num_tx_queues = num_tx_qs;
>
> -#if !defined(CONFIG_M5272)
>  	/* default enable pause frame auto negotiation */
>  	if (fep->quirks & FEC_QUIRK_HAS_GBIT)
>  		fep->pause_flag |= FEC_PAUSE_FLAG_AUTONEG;
> -#endif
>
>  	/* Select default pin state */
>  	pinctrl_pm_select_default_state(&pdev->dev);
> --
> 2.34.1
>

Re: [PATCH net-next 2/5] net: fec: simplify the conditional preprocessor directives

[Frank Li]

On Tue, Nov 11, 2025 at 06:00:54PM +0800, Wei Fang wrote:
> From the Kconfig file, we can see CONFIG_FEC depends on the following
> platform-related options.
>
> ColdFire: M523x, M527x, M5272, M528x, M520x and M532x
> S32: ARCH_S32 (ARM64)
> i.MX: SOC_IMX28 and ARCH_MXC (ARM and ARM64)
>
> Based on the code of fec driver, only some macro definitions on the
> M5272 platform are different from those on other platforms. Therefore,
> we can simplify the following complex preprocessor directives to
> "if !defined(CONFIG_M5272)".
>
> "#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || \
>      defined(CONFIG_M528x) || defined(CONFIG_M520x) || \
>      defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
>      defined(CONFIG_ARM64)"
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  4 +---
>  drivers/net/ethernet/freescale/fec_main.c | 27 ++++++-----------------
>  2 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 41e0d85d15da..8e438f6e7ec4 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -24,9 +24,7 @@
>  #include <linux/timecounter.h>
>  #include <net/xdp.h>
>
> -#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
> -    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
> -    defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
>  /*
>   *	Just figures, Motorola would have to change the offsets for
>   *	registers in the same peripheral device on different models
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index e0e84f2979c8..9d0e5abe5f66 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -253,9 +253,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>   * size bits. Other FEC hardware does not, so we need to take that into
>   * account when setting it.
>   */
> -#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
> -    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
> -    defined(CONFIG_ARM64)
> +#ifndef CONFIG_M5272
>  #define	OPT_ARCH_HAS_MAX_FL	1
>  #else
>  #define	OPT_ARCH_HAS_MAX_FL	0
> @@ -2704,9 +2702,7 @@ static int fec_enet_get_regs_len(struct net_device *ndev)
>  }
>
>  /* List of registers that can be safety be read to dump them with ethtool */
> -#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
> -	defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
> -	defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
>  static __u32 fec_enet_register_version = 2;
>  static u32 fec_enet_register_offset[] = {
>  	FEC_IEVENT, FEC_IMASK, FEC_R_DES_ACTIVE_0, FEC_X_DES_ACTIVE_0,
> @@ -2780,29 +2776,20 @@ static u32 fec_enet_register_offset[] = {
>  static void fec_enet_get_regs(struct net_device *ndev,
>  			      struct ethtool_regs *regs, void *regbuf)
>  {
> +	u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	u32 __iomem *theregs = (u32 __iomem *)fep->hwp;
> +	u32 *reg_list = fec_enet_register_offset;
>  	struct device *dev = &fep->pdev->dev;
>  	u32 *buf = (u32 *)regbuf;
>  	u32 i, off;
>  	int ret;
> -#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
> -	defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
> -	defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> -	u32 *reg_list;
> -	u32 reg_cnt;
> -
> -	if (!of_machine_is_compatible("fsl,imx6ul")) {
> -		reg_list = fec_enet_register_offset;
> -		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> -	} else {
> +
> +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
> +	if (of_machine_is_compatible("fsl,imx6ul")) {

There are stub of_machine_is_compatible(), so needn't #ifdef here.

Frank
>  		reg_list = fec_enet_register_offset_6ul;
>  		reg_cnt = ARRAY_SIZE(fec_enet_register_offset_6ul);
>  	}
> -#else
> -	/* coldfire */
> -	static u32 *reg_list = fec_enet_register_offset;
> -	static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
>  #endif
>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret < 0)
> --
> 2.34.1
>

Re: [PATCH net-next 3/5] net: fec: remove struct fec_enet_priv_txrx_info

[Frank Li]

On Tue, Nov 11, 2025 at 06:00:55PM +0800, Wei Fang wrote:
> The struct fec_enet_priv_txrx_info has three members: offset, page and
> skb. The offset is only initialized in the driver and is not used, and

the skb is never initialized and used in the driver. The both will not
be used in the future, Therefore, replace struct fec_enet_priv_txrx_info
directly with struct page.

> we can see that it likely will not be used in the future. The skb is
> never initialized and used in the driver. Therefore, struct
> fec_enet_priv_txrx_info can be directly replaced by struct page.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  8 +-------
>  drivers/net/ethernet/freescale/fec_main.c | 11 +++++------
>  2 files changed, 6 insertions(+), 13 deletions(-)
>
...
>
> @@ -1834,7 +1833,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16 queue_id, int budget)
>  		ndev->stats.rx_bytes += pkt_len;
>
>  		index = fec_enet_get_bd_index(bdp, &rxq->bd);
> -		page = rxq->rx_skb_info[index].page;
> +		page = rxq->rx_buf[index];
>  		cbd_bufaddr = bdp->cbd_bufaddr;
>  		if (fec_enet_update_cbd(rxq, bdp, index)) {
>  			ndev->stats.rx_dropped++;
> @@ -3309,7 +3308,8 @@ static void fec_enet_free_buffers(struct net_device *ndev)
>  	for (q = 0; q < fep->num_rx_queues; q++) {
>  		rxq = fep->rx_queue[q];
>  		for (i = 0; i < rxq->bd.ring_size; i++)
> -			page_pool_put_full_page(rxq->page_pool, rxq->rx_skb_info[i].page, false);
> +			page_pool_put_full_page(rxq->page_pool, rxq->rx_buf[i],
> +						false);

move to previous line.

Frank
>
>  		for (i = 0; i < XDP_STATS_TOTAL; i++)
>  			rxq->stats[i] = 0;
> @@ -3443,8 +3443,7 @@ fec_enet_alloc_rxq_buffers(struct net_device *ndev, unsigned int queue)
>  		phys_addr = page_pool_get_dma_addr(page) + FEC_ENET_XDP_HEADROOM;
>  		bdp->cbd_bufaddr = cpu_to_fec32(phys_addr);
>
> -		rxq->rx_skb_info[i].page = page;
> -		rxq->rx_skb_info[i].offset = FEC_ENET_XDP_HEADROOM;
> +		rxq->rx_buf[i] = page;
>  		bdp->cbd_sc = cpu_to_fec16(BD_ENET_RX_EMPTY);
>
>  		if (fep->bufdesc_ex) {
> --
> 2.34.1
>

Re: [PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Frank Li]

On Tue, Nov 11, 2025 at 06:00:56PM +0800, Wei Fang wrote:
> The rx_align was introduced by the commit 41ef84ce4c72 ("net: fec: change
> FEC alignment according to i.mx6 sx requirement"). Because the i.MX6 SX
> requires RX buffer must be 64 bytes alignment.
>
> Since the commit 95698ff6177b ("net: fec: using page pool to manage RX
> buffers"), the address of the RX buffer is always the page address plus
> 128 bytes, so RX buffer is always 64-byte aligned. Therefore, rx_align
> has no effect since that commit, and we can safely remove it.

I suggest keep it as it because we need know this kind limitation in case
future code change broke prediction
'net: fec: using page pool to manage RX ..."

Frank
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      | 1 -
>  drivers/net/ethernet/freescale/fec_main.c | 6 +-----
>  2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index c5bbc2c16a4f..a25dca9c7d71 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -660,7 +660,6 @@ struct fec_enet_private {
>  	struct pm_qos_request pm_qos_req;
>
>  	unsigned int tx_align;
> -	unsigned int rx_align;
>
>  	/* hw interrupt coalesce */
>  	unsigned int rx_pkts_itr;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 5de86c8bc78e..cf598d5260fb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -4069,10 +4069,8 @@ static int fec_enet_init(struct net_device *ndev)
>
>  	WARN_ON(dsize != (1 << dsize_log2));
>  #if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> -	fep->rx_align = 0xf;
>  	fep->tx_align = 0xf;
>  #else
> -	fep->rx_align = 0x3;
>  	fep->tx_align = 0x3;
>  #endif
>  	fep->rx_pkts_itr = FEC_ITR_ICFT_DEFAULT;
> @@ -4161,10 +4159,8 @@ static int fec_enet_init(struct net_device *ndev)
>  		fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
>  	}
>
> -	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) {
> +	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES)
>  		fep->tx_align = 0;
> -		fep->rx_align = 0x3f;
> -	}
>
>  	ndev->hw_features = ndev->features;
>
> --
> 2.34.1
>

Re: [PATCH net-next 5/5] net: fec: remove duplicate macros of the BD status

[Frank Li]

On Tue, Nov 11, 2025 at 06:00:57PM +0800, Wei Fang wrote:
> There are two sets of macros used to define the status bits of TX and RX
> BDs, one is the BD_SC_xx macros, the other one is the BD_ENET_xx macros.
> For the BD_SC_xx macros, only BD_SC_WRAP is used in the driver. But the
> BD_ENET_xx macros are more widely used in the driver, and they define
> more bits of the BD status. Therefore, let us remove the BD_SC_xx macros
> from now on.

nit: remove "let us",

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      | 17 -----------------
>  drivers/net/ethernet/freescale/fec_main.c |  8 ++++----
>  2 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index a25dca9c7d71..7b4d1fc8e7eb 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -240,23 +240,6 @@ struct bufdesc_ex {
>  	__fec16 res0[4];
>  };
>
> -/*
> - *	The following definitions courtesy of commproc.h, which where
> - *	Copyright (c) 1997 Dan Malek (dmalek@jlc.net).
> - */
> -#define BD_SC_EMPTY	((ushort)0x8000)	/* Receive is empty */
> -#define BD_SC_READY	((ushort)0x8000)	/* Transmit is ready */
> -#define BD_SC_WRAP	((ushort)0x2000)	/* Last buffer descriptor */
> -#define BD_SC_INTRPT	((ushort)0x1000)	/* Interrupt on change */
> -#define BD_SC_CM	((ushort)0x0200)	/* Continuous mode */
> -#define BD_SC_ID	((ushort)0x0100)	/* Rec'd too many idles */
> -#define BD_SC_P		((ushort)0x0100)	/* xmt preamble */
> -#define BD_SC_BR	((ushort)0x0020)	/* Break received */
> -#define BD_SC_FR	((ushort)0x0010)	/* Framing error */
> -#define BD_SC_PR	((ushort)0x0008)	/* Parity error */
> -#define BD_SC_OV	((ushort)0x0002)	/* Overrun */
> -#define BD_SC_CD	((ushort)0x0001)	/* ?? */
> -
>  /* Buffer descriptor control/status used by Ethernet receive.
>   */
>  #define BD_ENET_RX_EMPTY	((ushort)0x8000)
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index cf598d5260fb..3d227c9c5ba5 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1010,7 +1010,7 @@ static void fec_enet_bd_init(struct net_device *dev)
>
>  		/* Set the last buffer to wrap */
>  		bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
> -		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
> +		bdp->cbd_sc |= cpu_to_fec16(BD_ENET_RX_WRAP);
>
>  		rxq->bd.cur = rxq->bd.base;
>  	}
> @@ -1060,7 +1060,7 @@ static void fec_enet_bd_init(struct net_device *dev)
>
>  		/* Set the last buffer to wrap */
>  		bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
> -		bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
> +		bdp->cbd_sc |= cpu_to_fec16(BD_ENET_TX_WRAP);
>  		txq->dirty_tx = bdp;
>  	}
>  }
> @@ -3456,7 +3456,7 @@ fec_enet_alloc_rxq_buffers(struct net_device *ndev, unsigned int queue)
>
>  	/* Set the last buffer to wrap. */
>  	bdp = fec_enet_get_prevdesc(bdp, &rxq->bd);
> -	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
> +	bdp->cbd_sc |= cpu_to_fec16(BD_ENET_RX_WRAP);
>  	return 0;
>
>   err_alloc:
> @@ -3492,7 +3492,7 @@ fec_enet_alloc_txq_buffers(struct net_device *ndev, unsigned int queue)
>
>  	/* Set the last buffer to wrap. */
>  	bdp = fec_enet_get_prevdesc(bdp, &txq->bd);
> -	bdp->cbd_sc |= cpu_to_fec16(BD_SC_WRAP);
> +	bdp->cbd_sc |= cpu_to_fec16(BD_ENET_TX_WRAP);
>
>  	return 0;
>
> --
> 2.34.1
>

RE: [PATCH net-next 2/5] net: fec: simplify the conditional preprocessor directives

[Wei Fang]

> > -	if (!of_machine_is_compatible("fsl,imx6ul")) {
> > -		reg_list = fec_enet_register_offset;
> > -		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > -	} else {
> > +
> > +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
> > +	if (of_machine_is_compatible("fsl,imx6ul")) {
> 
> There are stub of_machine_is_compatible(), so needn't #ifdef here.
> 

fec_enet_register_offset_6ul is not defined when CONFIG_M5272 is
enabled, so we still need it here.

> >  		reg_list = fec_enet_register_offset_6ul;
> >  		reg_cnt = ARRAY_SIZE(fec_enet_register_offset_6ul);
> >  	}
> > -#else
> > -	/* coldfire */
> > -	static u32 *reg_list = fec_enet_register_offset;
> > -	static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> >  #endif
> >  	ret = pm_runtime_resume_and_get(dev);
> >  	if (ret < 0)
> > --
> > 2.34.1
> >

RE: [PATCH net-next 3/5] net: fec: remove struct fec_enet_priv_txrx_info

[Wei Fang]

> On Tue, Nov 11, 2025 at 06:00:55PM +0800, Wei Fang wrote:
> > The struct fec_enet_priv_txrx_info has three members: offset, page and
> > skb. The offset is only initialized in the driver and is not used, and
> 
> the skb is never initialized and used in the driver. The both will not be used in the
> future, Therefore, replace struct fec_enet_priv_txrx_info directly with struct
> page.
> 
> > we can see that it likely will not be used in the future. The skb is
> > never initialized and used in the driver. Therefore, struct
> > fec_enet_priv_txrx_info can be directly replaced by struct page.
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/fec.h      |  8 +-------
> >  drivers/net/ethernet/freescale/fec_main.c | 11 +++++------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> >
> ...
> >
> > @@ -1834,7 +1833,7 @@ fec_enet_rx_queue(struct net_device *ndev, u16
> queue_id, int budget)
> >  		ndev->stats.rx_bytes += pkt_len;
> >
> >  		index = fec_enet_get_bd_index(bdp, &rxq->bd);
> > -		page = rxq->rx_skb_info[index].page;
> > +		page = rxq->rx_buf[index];
> >  		cbd_bufaddr = bdp->cbd_bufaddr;
> >  		if (fec_enet_update_cbd(rxq, bdp, index)) {
> >  			ndev->stats.rx_dropped++;
> > @@ -3309,7 +3308,8 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> >  	for (q = 0; q < fep->num_rx_queues; q++) {
> >  		rxq = fep->rx_queue[q];
> >  		for (i = 0; i < rxq->bd.ring_size; i++)
> > -			page_pool_put_full_page(rxq->page_pool,
> rxq->rx_skb_info[i].page, false);
> > +			page_pool_put_full_page(rxq->page_pool, rxq->rx_buf[i],
> > +						false);
> 
> move to previous line.

The line should no more than 80 characters

RE: [PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Wei Fang]

> On Tue, Nov 11, 2025 at 06:00:56PM +0800, Wei Fang wrote:
> > The rx_align was introduced by the commit 41ef84ce4c72 ("net: fec: change
> > FEC alignment according to i.mx6 sx requirement"). Because the i.MX6 SX
> > requires RX buffer must be 64 bytes alignment.
> >
> > Since the commit 95698ff6177b ("net: fec: using page pool to manage RX
> > buffers"), the address of the RX buffer is always the page address plus
> > 128 bytes, so RX buffer is always 64-byte aligned. Therefore, rx_align
> > has no effect since that commit, and we can safely remove it.
> 
> I suggest keep it as it because we need know this kind limitation in case
> future code change broke prediction
> 'net: fec: using page pool to manage RX ..."

We still have tx_align to get the limitation. Or we can add a comment
to doc the limitation. Invalid code in the driver is pointless.

Re: [PATCH net-next 1/5] net: fec: remove useless conditional preprocessor directives

[Andrew Lunn]

On Tue, Nov 11, 2025 at 06:00:53PM +0800, Wei Fang wrote:
> The conditional preprocessor directive "#if !defined(CONFIG_M5272)" was
> added due to build errors on MCF5272 platform, see commit d13919301d9a
> ("net: fec: Fix build for MCF5272"). The compilation error was caused by
> some register macros not being defined on the MCF5272 platform. However,
> this preprocessor directive is not needed in some parts of the driver.
> First, removing it will not cause compilation errors. Second, these parts
> will check quirks, which do not exist on the MCF7527 platform. Therefore,
> we can safely delete these useless preprocessor directives.

> @@ -2515,9 +2513,7 @@ static int fec_enet_mii_probe(struct net_device *ndev)
>  		phy_set_max_speed(phy_dev, 1000);
>  		phy_remove_link_mode(phy_dev,
>  				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> -#if !defined(CONFIG_M5272)
>  		phy_support_sym_pause(phy_dev);
> -#endif
>  	}

I think the explanation could be better.

I assume the M5272 only supported Fast Ethernet, so fep->quirks &
FEC_QUIRK_HAS_GBIT was never true?

>  	else
>  		phy_set_max_speed(phy_dev, 100);
> @@ -4400,11 +4396,9 @@ fec_probe(struct platform_device *pdev)
>  	fep->num_rx_queues = num_rx_qs;
>  	fep->num_tx_queues = num_tx_qs;
>  
> -#if !defined(CONFIG_M5272)
>  	/* default enable pause frame auto negotiation */
>  	if (fep->quirks & FEC_QUIRK_HAS_GBIT)
>  		fep->pause_flag |= FEC_PAUSE_FLAG_AUTONEG;
> -#endif

Same here?

Maybe the commit message should actually say that M5272 only supported
Fast Ethernet, so these conditions cannot be true, and so the #ifdef
guard can be removed.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Andrew Lunn]

On Tue, Nov 11, 2025 at 06:00:56PM +0800, Wei Fang wrote:
> The rx_align was introduced by the commit 41ef84ce4c72 ("net: fec: change
> FEC alignment according to i.mx6 sx requirement"). Because the i.MX6 SX
> requires RX buffer must be 64 bytes alignment.
> 
> Since the commit 95698ff6177b ("net: fec: using page pool to manage RX
> buffers"), the address of the RX buffer is always the page address plus
> 128 bytes, so RX buffer is always 64-byte aligned.

It is not obvious to me where this 128 bytes is added.

	Andrew

RE: [PATCH net-next 1/5] net: fec: remove useless conditional preprocessor directives

[Wei Fang]

> On Tue, Nov 11, 2025 at 06:00:53PM +0800, Wei Fang wrote:
> > The conditional preprocessor directive "#if !defined(CONFIG_M5272)" was
> > added due to build errors on MCF5272 platform, see commit d13919301d9a
> > ("net: fec: Fix build for MCF5272"). The compilation error was caused by
> > some register macros not being defined on the MCF5272 platform. However,
> > this preprocessor directive is not needed in some parts of the driver.
> > First, removing it will not cause compilation errors. Second, these parts
> > will check quirks, which do not exist on the MCF7527 platform. Therefore,
> > we can safely delete these useless preprocessor directives.
> 
> > @@ -2515,9 +2513,7 @@ static int fec_enet_mii_probe(struct net_device
> *ndev)
> >  		phy_set_max_speed(phy_dev, 1000);
> >  		phy_remove_link_mode(phy_dev,
> >  				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> > -#if !defined(CONFIG_M5272)
> >  		phy_support_sym_pause(phy_dev);
> > -#endif
> >  	}
> 
> I think the explanation could be better.
> 
> I assume the M5272 only supported Fast Ethernet, so fep->quirks &
> FEC_QUIRK_HAS_GBIT was never true?

From the driver, ColdFire platforms do not have the quirks, so it is
never be true for these platforms.

> 
> >  	else
> >  		phy_set_max_speed(phy_dev, 100);
> > @@ -4400,11 +4396,9 @@ fec_probe(struct platform_device *pdev)
> >  	fep->num_rx_queues = num_rx_qs;
> >  	fep->num_tx_queues = num_tx_qs;
> >
> > -#if !defined(CONFIG_M5272)
> >  	/* default enable pause frame auto negotiation */
> >  	if (fep->quirks & FEC_QUIRK_HAS_GBIT)
> >  		fep->pause_flag |= FEC_PAUSE_FLAG_AUTONEG;
> > -#endif
> 
> Same here?
> 
> Maybe the commit message should actually say that M5272 only supported
> Fast Ethernet, so these conditions cannot be true, and so the #ifdef
> guard can be removed.
> 

Yeah, I will improve the commit message in v2.

> 
> ---
> pw-bot: cr

RE: [PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Wei Fang]

> On Tue, Nov 11, 2025 at 06:00:56PM +0800, Wei Fang wrote:
> > The rx_align was introduced by the commit 41ef84ce4c72 ("net: fec:
> > change FEC alignment according to i.mx6 sx requirement"). Because the
> > i.MX6 SX requires RX buffer must be 64 bytes alignment.
> >
> > Since the commit 95698ff6177b ("net: fec: using page pool to manage RX
> > buffers"), the address of the RX buffer is always the page address
> > plus
> > 128 bytes, so RX buffer is always 64-byte aligned.
> 
> It is not obvious to me where this 128 bytes is added.
> 

Sorry, I misremembered the value of XDP_PACKET_HEADROOM. it should
be 256 bytes.

See fec_enet_alloc_rxq_buffers():

phys_addr = page_pool_get_dma_addr(page) + FEC_ENET_XDP_HEADROOM;
bdp->cbd_bufaddr = cpu_to_fec32(phys_addr);

I will correct it in v2, thanks

Re: [PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Andrew Lunn]

> Sorry, I misremembered the value of XDP_PACKET_HEADROOM. it should
> be 256 bytes.
> 
> See fec_enet_alloc_rxq_buffers():
> 
> phys_addr = page_pool_get_dma_addr(page) + FEC_ENET_XDP_HEADROOM;
> bdp->cbd_bufaddr = cpu_to_fec32(phys_addr);
> 
> I will correct it in v2, thanks

So you could add a BUILD_BUG_ON() test which makes sure
FEC_ENET_XDP_HEADROOM gives you the needed alignment. That way it is
both "documented" and enforced. And it costs nothing at runtime.

	Andrew

Re: [PATCH net-next 2/5] net: fec: simplify the conditional preprocessor directives

[Frank Li]

On Wed, Nov 12, 2025 at 01:53:15AM +0000, Wei Fang wrote:
> > > -	if (!of_machine_is_compatible("fsl,imx6ul")) {
> > > -		reg_list = fec_enet_register_offset;
> > > -		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > > -	} else {
> > > +
> > > +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
> > > +	if (of_machine_is_compatible("fsl,imx6ul")) {
> >
> > There are stub of_machine_is_compatible(), so needn't #ifdef here.
> >
>
> fec_enet_register_offset_6ul is not defined when CONFIG_M5272 is
> enabled, so we still need it here.

Is it possible to remove ifdef for fec_enet_register_offset_6ul?

Frank
>
> > >  		reg_list = fec_enet_register_offset_6ul;
> > >  		reg_cnt = ARRAY_SIZE(fec_enet_register_offset_6ul);
> > >  	}
> > > -#else
> > > -	/* coldfire */
> > > -	static u32 *reg_list = fec_enet_register_offset;
> > > -	static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > >  #endif
> > >  	ret = pm_runtime_resume_and_get(dev);
> > >  	if (ret < 0)
> > > --
> > > 2.34.1
> > >

RE: [PATCH net-next 4/5] net: fec: remove rx_align from fec_enet_private

[Wei Fang]

> > Sorry, I misremembered the value of XDP_PACKET_HEADROOM. it should
> > be 256 bytes.
> >
> > See fec_enet_alloc_rxq_buffers():
> >
> > phys_addr = page_pool_get_dma_addr(page) + FEC_ENET_XDP_HEADROOM;
> > bdp->cbd_bufaddr = cpu_to_fec32(phys_addr);
> >
> > I will correct it in v2, thanks
> 
> So you could add a BUILD_BUG_ON() test which makes sure
> FEC_ENET_XDP_HEADROOM gives you the needed alignment. That way it is
> both "documented" and enforced. And it costs nothing at runtime.
> 

Yes, I will add such a check, thanks

RE: [PATCH net-next 2/5] net: fec: simplify the conditional preprocessor directives

[Wei Fang]

> On Wed, Nov 12, 2025 at 01:53:15AM +0000, Wei Fang wrote:
> > > > -	if (!of_machine_is_compatible("fsl,imx6ul")) {
> > > > -		reg_list = fec_enet_register_offset;
> > > > -		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > > > -	} else {
> > > > +
> > > > +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
> > > > +	if (of_machine_is_compatible("fsl,imx6ul")) {
> > >
> > > There are stub of_machine_is_compatible(), so needn't #ifdef here.
> > >
> >
> > fec_enet_register_offset_6ul is not defined when CONFIG_M5272 is
> > enabled, so we still need it here.
> 
> Is it possible to remove ifdef for fec_enet_register_offset_6ul?
> 

Yes, I will move fec_enet_register_offset_6ul out of the "#ifdef " guard.

RE: [PATCH net-next 2/5] net: fec: simplify the conditional preprocessor directives

[Wei Fang]

> > On Wed, Nov 12, 2025 at 01:53:15AM +0000, Wei Fang wrote:
> > > > > -	if (!of_machine_is_compatible("fsl,imx6ul")) {
> > > > > -		reg_list = fec_enet_register_offset;
> > > > > -		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > > > > -	} else {
> > > > > +
> > > > > +#if !defined(CONFIG_M5272) || defined(CONFIG_COMPILE_TEST)
> > > > > +	if (of_machine_is_compatible("fsl,imx6ul")) {
> > > >
> > > > There are stub of_machine_is_compatible(), so needn't #ifdef here.
> > > >
> > >
> > > fec_enet_register_offset_6ul is not defined when CONFIG_M5272 is
> > > enabled, so we still need it here.
> >
> > Is it possible to remove ifdef for fec_enet_register_offset_6ul?
> >
> 
> Yes, I will move fec_enet_register_offset_6ul out of the "#ifdef " guard.

Sorry, remove the guard for fec_enet_register_offset_6ul will cause
build errors, because there are many macros are not defined for M5272.
I'm going to use this version in v3.