Re: [PATCH net-next v10 2/5] net: spacemit: Add K1 Ethernet MAC

Hi Simon,

On 9/11/25 17:44, Simon Horman wrote:

On Mon, Sep 08, 2025 at 08:34:26PM +0800, Vivian Wang wrote:

quoted

The Ethernet MACs found on SpacemiT K1 appears to be a custom design
that only superficially resembles some other embedded MACs. SpacemiT
refers to them as "EMAC", so let's just call the driver "k1_emac".

Supports RGMII and RMII interfaces. Includes support for MAC hardware
statistics counters. PTP support is not implemented.

Signed-off-by: Vivian Wang <redacted>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Reviewed-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Tested-by: Junhui Liu <redacted>
Tested-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
 drivers/net/ethernet/Kconfig            |    1 +
 drivers/net/ethernet/Makefile           |    1 +
 drivers/net/ethernet/spacemit/Kconfig   |   29 +
 drivers/net/ethernet/spacemit/Makefile  |    6 +
 drivers/net/ethernet/spacemit/k1_emac.c | 2156 +++++++++++++++++++++++++++++++

This is a large patch, so I'm sure I've missed some things.
But, overall, I think this is coming together.
Thanks for your recent updates.

As the Kernel Patch Robot noticed a problem,
I've provided some minor feedback for your consideration.

(That's the function at the end)

...

quoted

+static void emac_wr(struct emac_priv *priv, u32 reg, u32 val)
+{
+	writel(val, priv->iobase + reg);
+}
+
+static int emac_rd(struct emac_priv *priv, u32 reg)

nit: maybe u32 would be a more suitable return type.

Ah, right, will change to u32 in the next version.

quoted

+{
+	return readl(priv->iobase + reg);
+}

...

quoted

+static int emac_alloc_tx_resources(struct emac_priv *priv)
+{
+	struct emac_desc_ring *tx_ring = &priv->tx_ring;
+	struct platform_device *pdev = priv->pdev;
+	u32 size;
+
+	size = sizeof(struct emac_tx_desc_buffer) * tx_ring->total_cnt;
+
+	tx_ring->tx_desc_buf = kzalloc(size, GFP_KERNEL);

nit: I think you can use kcalloc() here.

quoted

+	if (!tx_ring->tx_desc_buf)
+		return -ENOMEM;
+
+	tx_ring->total_size = tx_ring->total_cnt * sizeof(struct emac_desc);
+	tx_ring->total_size = ALIGN(tx_ring->total_size, PAGE_SIZE);
+
+	tx_ring->desc_addr = dma_alloc_coherent(&pdev->dev, tx_ring->total_size,
+						&tx_ring->desc_dma_addr,
+						GFP_KERNEL);
+	if (!tx_ring->desc_addr) {
+		kfree(tx_ring->tx_desc_buf);
+		return -ENOMEM;
+	}
+
+	tx_ring->head = 0;
+	tx_ring->tail = 0;
+
+	return 0;
+}

...

quoted

+static int emac_alloc_rx_resources(struct emac_priv *priv)
+{
+	struct emac_desc_ring *rx_ring = &priv->rx_ring;
+	struct platform_device *pdev = priv->pdev;
+	u32 buf_len;
+
+	buf_len = sizeof(struct emac_rx_desc_buffer) * rx_ring->total_cnt;
+
+	rx_ring->rx_desc_buf = kzalloc(buf_len, GFP_KERNEL);

Ditto.

Will change these uses of kcalloc for these array allocations in next
version.

quoted

+	if (!rx_ring->rx_desc_buf)
+		return -ENOMEM;
+
+	rx_ring->total_size = rx_ring->total_cnt * sizeof(struct emac_desc);
+
+	rx_ring->total_size = ALIGN(rx_ring->total_size, PAGE_SIZE);
+
+	rx_ring->desc_addr = dma_alloc_coherent(&pdev->dev, rx_ring->total_size,
+						&rx_ring->desc_dma_addr,
+						GFP_KERNEL);
+	if (!rx_ring->desc_addr) {
+		kfree(rx_ring->rx_desc_buf);
+		return -ENOMEM;
+	}
+
+	rx_ring->head = 0;
+	rx_ring->tail = 0;
+
+	return 0;
+}

...

quoted

+static int emac_mii_read(struct mii_bus *bus, int phy_addr, int regnum)
+{
+	struct emac_priv *priv = bus->priv;
+	u32 cmd = 0, val;
+	int ret;
+
+	cmd |= phy_addr & 0x1F;
+	cmd |= (regnum & 0x1F) << 5;

nit: I think this could benefit from using FIELD_PREP
     Likewise for similar patterns in this patch.

Right... I'll take a look, thanks.

quoted

+	cmd |= MREGBIT_START_MDIO_TRANS | MREGBIT_MDIO_READ_WRITE;
+
+	emac_wr(priv, MAC_MDIO_DATA, 0x0);
+	emac_wr(priv, MAC_MDIO_CONTROL, cmd);
+
+	ret = readl_poll_timeout(priv->iobase + MAC_MDIO_CONTROL, val,
+				 !((val >> 15) & 0x1), 100, 10000);
+
+	if (ret)
+		return ret;
+
+	val = emac_rd(priv, MAC_MDIO_DATA);
+	return val;
+}

...

quoted

+/*
+ * Even though this MAC supports gigabit operation, it only provides 32-bit
+ * statistics counters. The most overflow-prone counters are the "bytes" ones,
+ * which at gigabit overflow about twice a minute.
+ *
+ * Therefore, we maintain the high 32 bits of counters ourselves, incrementing
+ * every time statistics seem to go backwards. Also, update periodically to
+ * catch overflows when we are not otherwise checking the statistics often
+ * enough.
+ */
+
+#define EMAC_STATS_TIMER_PERIOD		20
+
+static int emac_read_stat_cnt(struct emac_priv *priv, u8 cnt, u32 *res,
+			      u32 control_reg, u32 high_reg, u32 low_reg)
+{
+	u32 val;
+	int ret;
+
+	/* The "read" bit is the same for TX and RX */
+
+	val = MREGBIT_START_TX_COUNTER_READ | cnt;
+	emac_wr(priv, control_reg, val);
+	val = emac_rd(priv, control_reg);
+
+	ret = readl_poll_timeout_atomic(priv->iobase + control_reg, val,
+					!(val & MREGBIT_START_TX_COUNTER_READ),
+					100, 10000);
+
+	if (ret) {
+		netdev_err(priv->ndev, "Read stat timeout\n");
+		return ret;
+	}
+
+	*res = emac_rd(priv, high_reg) << 16;
+	*res |= (u16)emac_rd(priv, low_reg);

nit: I think lower_16_bits() and lower_16_bits() would be appropriate here.

This one is building up a 32-bit value instead of splitting a 32-bit
value in two, and we don't have those macros in linux/wordpart.h. So I
think I'll make a local helper:

static u32 emac_make_stat(u16 high, u16 low)

quoted

+
+	return 0;
+}

...

quoted

+static void emac_update_counter(u64 *counter, u32 new_low)
+{
+	u32 old_low = (u32)*counter;
+	u64 high = *counter >> 32;

Similarly, lower_32_bits() and upper_32_bits here.

Thanks, this one I'll change to {lower,upper}_32_bits.

quoted

+
+	if (old_low > new_low) {
+		/* Overflowed, increment high 32 bits */
+		high++;
+	}
+
+	*counter = (high << 32) | new_low;
+}
+
+static void emac_stats_update(struct emac_priv *priv)
+{
+	u64 *tx_stats_off = (u64 *)&priv->tx_stats_off;
+	u64 *rx_stats_off = (u64 *)&priv->rx_stats_off;
+	u64 *tx_stats = (u64 *)&priv->tx_stats;
+	u64 *rx_stats = (u64 *)&priv->rx_stats;

nit: I think it would be interesting to use a union containing
     1. the existing tx/rx stats struct and 2. an array of u64.
     This may allow avoiding this cast. Which seems nice to me.
     But YMMV.

Looks like I can use a union with a DECLARE_FLEX_ARRAY for this. I'll
change it in the next version.

quoted

+	u32 i, res;
+
+	assert_spin_locked(&priv->stats_lock);
+
+	if (!netif_running(priv->ndev) || !netif_device_present(priv->ndev)) {
+		/* Not up, don't try to update */
+		return;
+	}
+
+	for (i = 0; i < sizeof(priv->tx_stats) / sizeof(*tx_stats); i++) {
+		/*
+		 * If reading stats times out, everything is broken and there's
+		 * nothing we can do. Reading statistics also can't return an
+		 * error, so just return without updating and without
+		 * rescheduling.
+		 */
+		if (emac_tx_read_stat_cnt(priv, i, &res))
+			return;
+
+		/*
+		 * Re-initializing while bringing interface up resets counters
+		 * to zero, so to provide continuity, we add the values saved
+		 * last time we did emac_down() to the new hardware-provided
+		 * value.
+		 */
+		emac_update_counter(&tx_stats[i], res + (u32)tx_stats_off[i]);

nit: maybe lower_32_bits(tx_stats_off[i]) ?

quoted

+	}
+
+	/* Similar remarks as TX stats */
+	for (i = 0; i < sizeof(priv->rx_stats) / sizeof(*rx_stats); i++) {
+		if (emac_rx_read_stat_cnt(priv, i, &res))
+			return;
+		emac_update_counter(&rx_stats[i], res + (u32)rx_stats_off[i]);

Likewise, here for rx_stats_off[i].

Thanks, these I will use lower_32_bits in these two places in the next
version.

quoted

+	}
+
+	mod_timer(&priv->stats_timer, jiffies + EMAC_STATS_TIMER_PERIOD * HZ);
+}

...

quoted

+static u64 emac_get_stat_tx_dropped(struct emac_priv *priv)
+{
+	u64 result;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		result += READ_ONCE(per_cpu(*priv->stat_tx_dropped, cpu));
+	}

nit: no need for {} here ?

Thanks for the catch, but with regards to this entire function, I'm
moving tx_dropped to dstats, so this would be moot.

quoted

+
+	return result;
+}

...

Thanks for your review.

Vivian "dramforever" Wang