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

[Vivian Wang <wangruikang@iscas.ac.cn>]

Hi Troy,

On 8/28/25 09:33, Troy Mitchell wrote:
> On Tue, Aug 26, 2025 at 02:23:47PM +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.
>>
>> Tested-by: Junhui Liu <junhui.liu@pigmoral.tech>
>> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
>> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> ---
> Hi Vivian,
> I have tested your patch on MUSE-PI.
> So here:
>
> Tested-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
>
> But I still have a minor style comment on the code.
Thanks for your Tested-by and review.
>>  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 | 2193 +++++++++++++++++++++++++++++++
>>  drivers/net/ethernet/spacemit/k1_emac.h |  426 ++++++
>>  6 files changed, 2656 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9e558d5893cfbbda0baa7ad21a7209dadda9487e
>> --- /dev/null
>> +++ b/drivers/net/ethernet/spacemit/k1_emac.c
>> @@ -0,0 +1,2193 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SpacemiT K1 Ethernet driver
>> + *
>> + * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
>> + * Copyright (C) 2025 Vivian Wang <wangruikang@iscas.ac.cn>
>> + */
>> +
> [...]
>> +
>> +static void emac_wr(struct emac_priv *priv, u32 reg, u32 val)
>> +{
>> +	writel(val, priv->iobase + reg);
>> +}
> basically short and obvious code like this:
>
> writel(val, priv->iobase + reg)
>
> you replace:
>
> emac_wr(priv, reg, val)
>
> It's not helpful..You could just use writel/readl directly
>> +
>> +static int emac_rd(struct emac_priv *priv, u32 reg)
>> +{
>> +	return readl(priv->iobase + reg);
>> +}
> ditto.

I have decided against inlining these wrappers.

Firstly, the wrappers being mostly trivial is not it being "not
helpful". In long blocks of register access code, especially, having
just emac_rd(priv,...) or emac_wr(priv,...) repeated is easier to
recognize and harder to mess up (e.g. precedence of "priv->iomem + ...").

Secondly, they serve as documentation that a normal register access for
this driver is a 32-bit read or write. This is despite the fact that the
registers all "look like" they each only have the low 16 bits.

>> +
>> +static int emac_phy_interface_config(struct emac_priv *priv)
>> +{
>> +	u32 val = 0, mask = PHY_INTF_RGMII;
>> +
>> +	switch (priv->phy_interface) {
>> +	case PHY_INTERFACE_MODE_RMII:
>> +		mask |= REF_CLK_SEL;
>> +		break;
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +	case PHY_INTERFACE_MODE_RGMII_TXID:
>> +		val |= PHY_INTF_RGMII;
>> +
>> +		mask |= RGMII_TX_CLK_SEL;
>> +		break;
>> +	default:
>> +		netdev_err(priv->ndev, "Unsupported PHY interface %d",
>> +			   priv->phy_interface);
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_update_bits(priv->regmap_apmu,
>> +			   priv->regmap_apmu_offset + APMU_EMAC_CTRL_REG,
>> +			   mask, val);
>> +
>> +	return 0;
>> +}
>> +
> [...]
>> +static bool emac_tx_should_interrupt(struct emac_priv *priv, u32 pkt_num)
>> +{
>> +	bool should_interrupt;
>> +
>> +	/* Manage TX mitigation */
>> +	priv->tx_count_frames += pkt_num;
>> +	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> +		emac_tx_coal_timer_resched(priv);
>> +		should_interrupt = false;
>> +	} else {
>> +		priv->tx_count_frames = 0;
>> +		should_interrupt = true;
>> +	}
>> +
>> +	return should_interrupt;
> If we really need this variable?
>
> How about this:
> if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>   emac_tx_coal_timer_resched(priv);
>   return false;
> }
>
> priv->tx_count_frames = 0;
> return true;

I will simplify this in the next version.

>> +}
>> +
>> +static void emac_free_tx_buf(struct emac_priv *priv, int i)
>> +{
>> +	struct emac_tx_desc_buffer *tx_buf;
>> +	struct emac_desc_ring *tx_ring;
>> +	struct desc_buf *buf;
>> +	int j;
>> +
>> +	tx_ring = &priv->tx_ring;
>> +	tx_buf = &tx_ring->tx_desc_buf[i];
>> +
>> +	for (j = 0; j < 2; j++) {
>> +		buf = &tx_buf->buf[j];
>> +		if (buf->dma_addr) {
>> +			if (buf->map_as_page)
>> +				dma_unmap_page(&priv->pdev->dev, buf->dma_addr,
>> +					       buf->dma_len, DMA_TO_DEVICE);
>> +			else
>> +				dma_unmap_single(&priv->pdev->dev,
>> +						 buf->dma_addr, buf->dma_len,
>> +						 DMA_TO_DEVICE);
>> +
>> +			buf->dma_addr = 0;
>> +			buf->map_as_page = false;
>> +			buf->buff_addr = NULL;
>> +		}
> if (!buf->dma_addr)
>   continue;
>
> Best regards,
> Troy
You tricked me! There's one more review comment below :P
>> +	}
>> +
>> +	if (tx_buf->skb) {
>> +		dev_kfree_skb_any(tx_buf->skb);
>> +		tx_buf->skb = NULL;
>> +	}
>> +}
>> +
>> +static void emac_clean_tx_desc_ring(struct emac_priv *priv)
>> +{
>> +	struct emac_desc_ring *tx_ring = &priv->tx_ring;
>> +	u32 i;
>> +
>> +	/* Free all the TX ring skbs */
>> +	for (i = 0; i < tx_ring->total_cnt; i++)
>> +		emac_free_tx_buf(priv, i);
>> +
>> +	tx_ring->head = 0;
>> +	tx_ring->tail = 0;
>> +}
>> +
>> +static void emac_clean_rx_desc_ring(struct emac_priv *priv)
>> +{
>> +	struct emac_rx_desc_buffer *rx_buf;
>> +	struct emac_desc_ring *rx_ring;
>> +	u32 i;
>> +
>> +	rx_ring = &priv->rx_ring;
>> +
>> +	/* Free all the RX ring skbs */
>> +	for (i = 0; i < rx_ring->total_cnt; i++) {
>> +		rx_buf = &rx_ring->rx_desc_buf[i];
>> +		if (rx_buf->skb) {
>> +			dma_unmap_single(&priv->pdev->dev, rx_buf->dma_addr,
>> +					 rx_buf->dma_len, DMA_FROM_DEVICE);
>> +
>> +			dev_kfree_skb(rx_buf->skb);
>> +			rx_buf->skb = NULL;
>> +		}
> if (!rx_buf->skb)
>   continue;

I will change both of these to "if (...) continue;" in the next version.

Thanks,
Vivian "dramforever" Wang