Message-ID: <e55d8a16-5e2c-4a46-99fd-8ea485269843@lunn.ch>
Date: Wed, 18 Jun 2025 23:17:37 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Vivian Wang <wangruikang@...as.ac.cn>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
	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>,
	Vivian Wang <uwu@...m.page>,
	Lukas Bulwahn <lukas.bulwahn@...hat.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/6] net: spacemit: Add K1 Ethernet MAC

> +/* The sizes (in bytes) of a ethernet packet */
> +#define ETHERNET_HEADER_SIZE		14

Please replace with ETH_HLEN

> +#define MINIMUM_ETHERNET_FRAME_SIZE	64 /* Incl. FCS */

I assume this device supports VLANS? If so, the minimum should
actually be 68. You can then use ETH_MIN_MTU

> +#define ETHERNET_FCS_SIZE		4

ETH_FCS_LEN

> +static int emac_tx_mem_map(struct emac_priv *priv, struct sk_buff *skb,
> +			   u32 max_tx_len, u32 frag_num)
> +{
> +	struct emac_desc tx_desc, *tx_desc_addr;
> +	u32 skb_linear_len = skb_headlen(skb);
> +	struct emac_tx_desc_buffer *tx_buf;
> +	u32 len, i, f, first, buf_idx = 0;
> +	struct emac_desc_ring *tx_ring;
> +	phys_addr_t addr;
> +
> +	tx_ring = &priv->tx_ring;
> +
> +	i = tx_ring->head;
> +	first = i;
> +
> +	if (++i == tx_ring->total_cnt)
> +		i = 0;
> +
> +	/* If the data is fragmented */
> +	for (f = 0; f < frag_num; f++) {
> +		const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
> +
> +		len = skb_frag_size(frag);
> +
> +		buf_idx = (f + 1) % 2;
> +
> +		/* First frag fill into second buffer of first descriptor */
> +		if (f == 0) {
> +			tx_buf = &tx_ring->tx_desc_buf[first];
> +			tx_desc_addr = &((struct emac_desc *)
> +						 tx_ring->desc_addr)[first];
> +			memset(&tx_desc, 0, sizeof(tx_desc));
> +		} else {
> +			/*
> +			 * From second frags to more frags,
> +			 * we only get new descriptor when frag num is odd.
> +			 */
> +			if (!buf_idx) {
> +				tx_buf = &tx_ring->tx_desc_buf[i];
> +				tx_desc_addr = &((struct emac_desc *)
> +							 tx_ring->desc_addr)[i];
> +				memset(&tx_desc, 0, sizeof(tx_desc));
> +			}
> +		}
> +		tx_buf->buf[buf_idx].dma_len = len;
> +
> +		addr = skb_frag_dma_map(&priv->pdev->dev, frag, 0,
> +					skb_frag_size(frag), DMA_TO_DEVICE);
> +
> +		if (dma_mapping_error(&priv->pdev->dev, addr)) {
> +			netdev_err(priv->ndev, "fail to map dma page: %d\n", f);
> +			goto dma_map_err;
> +		}
> +		tx_buf->buf[buf_idx].dma_addr = addr;
> +
> +		tx_buf->buf[buf_idx].map_as_page = true;
> +
> +		/* Every desc has two buffers for packet */
> +		if (buf_idx) {
> +			tx_desc.buffer_addr_2 = addr;
> +			tx_desc.desc1 |= make_buf_size_2(len);
> +		} else {
> +			tx_desc.buffer_addr_1 = addr;
> +			tx_desc.desc1 = make_buf_size_1(len);
> +
> +			if (++i == tx_ring->total_cnt) {
> +				tx_desc.desc1 |= TX_DESC_1_END_RING;
> +				i = 0;
> +			}
> +		}
> +
> +		if (f == 0) {
> +			*tx_desc_addr = tx_desc;
> +			continue;
> +		}
> +
> +		if (f == frag_num - 1) {
> +			tx_desc.desc1 |= TX_DESC_1_LAST_SEGMENT;
> +			tx_buf->skb = skb;
> +			if (emac_tx_should_interrupt(priv, frag_num + 1))
> +				tx_desc.desc1 |=
> +					TX_DESC_1_INTERRUPT_ON_COMPLETION;
> +		}
> +
> +		*tx_desc_addr = tx_desc;
> +		dma_wmb();
> +		WRITE_ONCE(tx_desc_addr->desc0, tx_desc.desc0 | TX_DESC_0_OWN);
> +	}
> +
> +	/* fill out first descriptor for skb linear data */
> +	tx_buf = &tx_ring->tx_desc_buf[first];
> +
> +	tx_buf->buf[0].dma_len = skb_linear_len;
> +
> +	addr = dma_map_single(&priv->pdev->dev, skb->data, skb_linear_len,
> +			      DMA_TO_DEVICE);
> +	if (dma_mapping_error(&priv->pdev->dev, addr)) {
> +		netdev_err(priv->ndev, "dma_map_single failed\n");
> +		goto dma_map_err;
> +	}
> +
> +	tx_buf->buf[0].dma_addr = addr;
> +
> +	tx_buf->buf[0].buff_addr = skb->data;
> +	tx_buf->buf[0].map_as_page = false;
> +
> +	/* Fill TX descriptor */
> +	tx_desc_addr = &((struct emac_desc *)tx_ring->desc_addr)[first];
> +
> +	tx_desc = *tx_desc_addr;
> +
> +	tx_desc.buffer_addr_1 = addr;
> +	tx_desc.desc1 |= make_buf_size_1(skb_linear_len);
> +	tx_desc.desc1 |= TX_DESC_1_FIRST_SEGMENT;
> +
> +	/* If last desc for ring, set end ring flag */
> +	if (first == tx_ring->total_cnt - 1)
> +		tx_desc.desc1 |= TX_DESC_1_END_RING;
> +
> +	/*
> +	 * If frag_num is more than 1, data need another desc, so current
> +	 * descriptor isn't last piece of packet data.
> +	 */
> +	tx_desc.desc1 |= frag_num > 1 ? 0 : TX_DESC_1_LAST_SEGMENT;
> +
> +	if (frag_num <= 1 && emac_tx_should_interrupt(priv, 1))
> +		tx_desc.desc1 |= TX_DESC_1_INTERRUPT_ON_COMPLETION;
> +
> +	/* Only last descriptor has skb pointer */
> +	if (tx_desc.desc1 & TX_DESC_1_LAST_SEGMENT)
> +		tx_buf->skb = skb;
> +
> +	*tx_desc_addr = tx_desc;
> +	dma_wmb();
> +	WRITE_ONCE(tx_desc_addr->desc0, tx_desc.desc0 | TX_DESC_0_OWN);
> +
> +	emac_dma_start_transmit(priv);
> +
> +	tx_ring->head = i;
> +
> +	return 0;
> +
> +dma_map_err:
> +	dev_kfree_skb_any(skb);
> +	priv->ndev->stats.tx_dropped++;
> +	return 0;
> +}

This is a rather large function. Can parts of it be pulled out into
helpers? The Coding style document says:

  Functions should be short and sweet, and do just one thing. They
  should fit on one or two screenfuls of text (the ISO/ANSI screen
  size is 80x24, as we all know), and do one thing and do that well.

> +static int emac_mdio_init(struct emac_priv *priv)
> +{
> +	struct device *dev = &priv->pdev->dev;
> +	struct device_node *mii_np;
> +	struct mii_bus *mii;
> +	int ret;
> +
> +	mii_np = of_get_available_child_by_name(dev->of_node, "mdio-bus");
> +	if (!mii_np) {
> +		if (of_phy_is_fixed_link(dev->of_node)) {
> +			if ((of_phy_register_fixed_link(dev->of_node) < 0))
> +				return -ENODEV;
> +
> +			return 0;
> +		}
> +
> +		dev_info(dev, "No mdio-bus child node found");
> +		return 0;
> +	}

An mdio-bus node is normally optional. You can pass NULL to
devm_of_mdiobus_register() and it will do the correct thing, register
the bus, and scan it for devices.

An MDIO bus and fixed link are also not mutually exclusive. When the
MAC is connected to an Ethernet switch, you often see an fixed-link,
and have an MDIO bus, with the switches management interface being
MDIO.

> +static int emac_ethtool_get_regs_len(struct net_device *dev)
> +{
> +	return EMAC_REG_SPACE_SIZE;
> +}
> +
> +static void emac_ethtool_get_regs(struct net_device *dev,
> +				  struct ethtool_regs *regs, void *space)
> +{
> +	struct emac_priv *priv = netdev_priv(dev);
> +	u32 *reg_space = space;
> +	int i;
> +
> +	regs->version = 1;
> +
> +	for (i = 0; i < EMAC_DMA_REG_CNT; i++)
> +		reg_space[i] = emac_rd(priv, DMA_CONFIGURATION + i * 4);
> +
> +	for (i = 0; i < EMAC_MAC_REG_CNT; i++)
> +		reg_space[i + EMAC_DMA_REG_CNT] =
> +			emac_rd(priv, MAC_GLOBAL_CONTROL + i * 4);
> +}

Given this implementation, it would be more readable, and less future
extension error prone, if emac_ethtool_get_regs_len() returned

EMAC_DMA_REG_CNT + EMAC_MAC_REG_CNT

> +static int emac_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
> +{
> +	if (!netif_running(ndev))
> +		return -EINVAL;
> +
> +	return phy_mii_ioctl(ndev->phydev, rq, cmd);
> +}

phy_do_ioctl_running().

> +static int emac_phy_connect(struct net_device *ndev)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	struct device *dev = &priv->pdev->dev;
> +	struct phy_device *phydev;
> +	struct device_node *np;
> +	int ret;
> +
> +	ret = of_get_phy_mode(dev->of_node, &priv->phy_interface);
> +	if (ret) {
> +		dev_err(dev, "No phy-mode found");
> +		return ret;
> +	}
> +
> +	np = of_parse_phandle(dev->of_node, "phy-handle", 0);
> +	if (!np && of_phy_is_fixed_link(dev->of_node))
> +		np = of_node_get(dev->of_node);
> +	if (!np) {
> +		dev_err(dev, "No PHY specified");
> +		return -ENODEV;
> +	}
> +
> +	ret = emac_phy_interface_config(priv);
> +	if (ret)
> +		goto err_node_put;
> +
> +	phydev = of_phy_connect(ndev, np, &emac_adjust_link, 0,
> +				priv->phy_interface);
> +	if (IS_ERR_OR_NULL(phydev)) {
> +		dev_err(dev, "Could not attach to PHY\n");
> +		ret = phydev ? PTR_ERR(phydev) : -ENODEV;
> +		goto err_node_put;
> +	}

The documentation for of_phy_connect() says:

 * If successful, returns a pointer to the phy_device with the embedded
 * struct device refcount incremented by one, or NULL on failure. The
 * refcount must be dropped by calling phy_disconnect() or phy_detach().

An error code is not possible. So you can simply this.
	
    Andrew

---
pw-bot: cr

Powered by blists - more mailing lists