Message-ID: <7895b23a-2b50-4f3e-bdef-f9b7397beef2@iscas.ac.cn>
Date: Thu, 11 Sep 2025 18:23:12 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Philipp Zabel <p.zabel@...gutronix.de>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Vadim Fedorenko
 <vadim.fedorenko@...ux.dev>, Junhui Liu <junhui.liu@...moral.tech>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
 spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Troy Mitchell <troy.mitchell@...ux.spacemit.com>, Vivian Wang <uwu@...m.page>
Subject: 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:
>> 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 <wangruikang@...as.ac.cn>
>> Reviewed-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>> Reviewed-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
>> Tested-by: Junhui Liu <junhui.liu@...moral.tech>
>> Tested-by: Troy Mitchell <troy.mitchell@...ux.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)

> ...
>
>> +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.

>> +{
>> +	return readl(priv->iobase + reg);
>> +}
> ...
>
>> +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.
>
>> +	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;
>> +}
> ...
>
>> +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.

>> +	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;
>> +}
> ...
>
>> +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.

>> +	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;
>> +}
> ...
>
>> +/*
>> + * 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)

>> +
>> +	return 0;
>> +}
> ...
>
>> +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.

>> +
>> +	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.

>> +	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]) ?
>
>> +	}
>> +
>> +	/* 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.

>> +	}
>> +
>> +	mod_timer(&priv->stats_timer, jiffies + EMAC_STATS_TIMER_PERIOD * HZ);
>> +}
> ...
>
>> +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.

>> +
>> +	return result;
>> +}
> ...

Thanks for your review.

Vivian "dramforever" Wang

Powered by blists - more mailing lists