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

[Troy Mitchell <troy.mitchell@linux.spacemit.com>]

On Thu, Aug 28, 2025 at 04:06:58PM +0800, Vivian Wang wrote:
> 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.
Alright, you’ve convinced me.
I did wrap it like this before, but it was opposed—I guess it really depends on
whether the maintainer likes it.

[...]
> >> +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
Haha, You are so cute, Vivian.
I'm surprised you were able to find it, because I went back to check after
writing the last comment. So I wasn't able to find the final one myself.

                - Troy

(This really is the final one this time.)
> >> +	}
> >> +
> >> +	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
> 
>