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

[Simon Horman <horms@kernel.org>]

On Wed, Jul 02, 2025 at 02:01:41PM +0800, Vivian Wang wrote:

...

> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c

...

> +static int emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	int nfrags = skb_shinfo(skb)->nr_frags;
> +	struct device *dev = &priv->pdev->dev;
> +
> +	if (unlikely(emac_tx_avail(priv) < nfrags + 1)) {
> +		if (!netif_queue_stopped(ndev)) {
> +			netif_stop_queue(ndev);
> +			dev_err_ratelimited(dev, "TX ring full, stop TX queue\n");
> +		}
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	emac_tx_mem_map(priv, skb);
> +
> +	ndev->stats.tx_packets++;
> +	ndev->stats.tx_bytes += skb->len;
> +
> +	/* Make sure there is space in the ring for the next TX. */
> +	if (unlikely(emac_tx_avail(priv) <= MAX_SKB_FRAGS + 2))
> +		netif_stop_queue(ndev);
> +
> +	return NETDEV_TX_OK;
> +}

> +
> +static u32 emac_tx_read_stat_cnt(struct emac_priv *priv, u8 cnt)
> +{
> +	u32 val, tmp;
> +	int ret;
> +
> +	val = 0x8000 | cnt;
> +	emac_wr(priv, MAC_TX_STATCTR_CONTROL, val);
> +	val = emac_rd(priv, MAC_TX_STATCTR_CONTROL);
> +
> +	ret = readl_poll_timeout_atomic(priv->iobase + MAC_TX_STATCTR_CONTROL,
> +					val, !(val & 0x8000), 100, 10000);
> +
> +	if (ret) {
> +		netdev_err(priv->ndev, "read TX stat timeout\n");
> +		return ret;
> +	}
> +
> +	tmp = emac_rd(priv, MAC_TX_STATCTR_DATA_HIGH);
> +	val = tmp << 16;
> +	tmp = emac_rd(priv, MAC_TX_STATCTR_DATA_LOW);
> +	val |= tmp;
> +
> +	return val;
> +}
> +
> +static u32 emac_rx_read_stat_cnt(struct emac_priv *priv, u8 cnt)
> +{
> +	u32 val, tmp;
> +	int ret;
> +
> +	val = 0x8000 | cnt;
> +	emac_wr(priv, MAC_RX_STATCTR_CONTROL, val);
> +	val = emac_rd(priv, MAC_RX_STATCTR_CONTROL);
> +
> +	ret = readl_poll_timeout_atomic(priv->iobase + MAC_RX_STATCTR_CONTROL,
> +					val, !(val & 0x8000), 100, 10000);
> +
> +	if (ret) {
> +		netdev_err(priv->ndev, "read RX stat timeout\n");
> +		return ret;
> +	}
> +
> +	tmp = emac_rd(priv, MAC_RX_STATCTR_DATA_HIGH);
> +	val = tmp << 16;
> +	tmp = emac_rd(priv, MAC_RX_STATCTR_DATA_LOW);
> +	val |= tmp;
> +
> +	return val;
> +}
> +
> +static int emac_set_mac_address(struct net_device *ndev, void *addr)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	int ret = eth_mac_addr(ndev, addr);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* If running, set now; if not running it will be set in emac_up. */
> +	if (netif_running(ndev))
> +		emac_set_mac_addr(priv, ndev->dev_addr);
> +
> +	return 0;
> +}
> +
> +static void emac_mac_multicast_filter_clear(struct emac_priv *priv)
> +{
> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE1, 0x0);
> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE2, 0x0);
> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE3, 0x0);
> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE4, 0x0);
> +}
> +
> +/* Configure Multicast and Promiscuous modes */
> +static void emac_rx_mode_set(struct net_device *ndev)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	u32 crc32, bit, reg, hash, val;
> +	struct netdev_hw_addr *ha;
> +	u32 mc_filter[4] = { 0 };
> +
> +	val = emac_rd(priv, MAC_ADDRESS_CONTROL);
> +
> +	val &= ~MREGBIT_PROMISCUOUS_MODE;
> +
> +	if (ndev->flags & IFF_PROMISC) {
> +		/* Enable promisc mode */
> +		val |= MREGBIT_PROMISCUOUS_MODE;
> +	} else if ((ndev->flags & IFF_ALLMULTI) ||
> +		   (netdev_mc_count(ndev) > HASH_TABLE_SIZE)) {
> +		/* Accept all multicast frames by setting every bit */
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE1, 0xffff);
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE2, 0xffff);
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE3, 0xffff);
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE4, 0xffff);
> +	} else if (!netdev_mc_empty(ndev)) {
> +		emac_mac_multicast_filter_clear(priv);
> +		netdev_for_each_mc_addr(ha, ndev) {
> +			/* Calculate the CRC of the MAC address */
> +			crc32 = ether_crc(ETH_ALEN, ha->addr);
> +
> +			/*
> +			 * The hash table is an array of 4 16-bit registers. It
> +			 * is treated like an array of 64 bits (bits[hash]). Use
> +			 * the upper 6 bits of the above CRC as the hash value.
> +			 */
> +			hash = (crc32 >> 26) & 0x3F;
> +			reg = hash / 16;
> +			bit = hash % 16;
> +			mc_filter[reg] |= BIT(bit);
> +		}
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE1, mc_filter[0]);
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE2, mc_filter[1]);
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE3, mc_filter[2]);
> +		emac_wr(priv, MAC_MULTICAST_HASH_TABLE4, mc_filter[3]);
> +	}
> +
> +	emac_wr(priv, MAC_ADDRESS_CONTROL, val);
> +}
> +
> +static int emac_change_mtu(struct net_device *ndev, int mtu)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	u32 frame_len;
> +
> +	if (netif_running(ndev)) {
> +		netdev_err(ndev, "must be stopped to change MTU\n");
> +		return -EBUSY;
> +	}
> +
> +	frame_len = mtu + ETH_HLEN + ETH_FCS_LEN;
> +
> +	if (frame_len <= EMAC_DEFAULT_BUFSIZE)
> +		priv->dma_buf_sz = EMAC_DEFAULT_BUFSIZE;
> +	else if (frame_len <= EMAC_RX_BUF_2K)
> +		priv->dma_buf_sz = EMAC_RX_BUF_2K;
> +	else
> +		priv->dma_buf_sz = EMAC_RX_BUF_4K;
> +
> +	ndev->mtu = mtu;
> +
> +	return 0;
> +}
> +
> +static void emac_tx_timeout(struct net_device *ndev, unsigned int txqueue)
> +{
> +	struct emac_priv *priv = netdev_priv(ndev);
> +
> +	schedule_work(&priv->tx_timeout_task);
> +}
> +
> +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;
> +	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;
> +}
> +
> +static int emac_mii_write(struct mii_bus *bus, int phy_addr, int regnum,
> +			  u16 value)
> +{
> +	struct emac_priv *priv = bus->priv;
> +	u32 cmd = 0, val;
> +	int ret;
> +
> +	emac_wr(priv, MAC_MDIO_DATA, value);
> +
> +	cmd |= phy_addr & 0x1F;
> +	cmd |= (regnum & 0x1F) << 5;
> +	cmd |= MREGBIT_START_MDIO_TRANS;
> +
> +	emac_wr(priv, MAC_MDIO_CONTROL, cmd);
> +
> +	ret = readl_poll_timeout(priv->iobase + MAC_MDIO_CONTROL, val,
> +				 !((val >> 15) & 0x1), 100, 10000);
> +
> +	return ret;
> +}
> +
> +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 = devm_mdiobus_alloc(dev);
> +	if (!mii)
> +		return -ENOMEM;
> +
> +	mii->priv = priv;
> +	mii->name = "k1_emac_mii";
> +	mii->read = emac_mii_read;
> +	mii->write = emac_mii_write;
> +	mii->parent = dev;
> +	mii->phy_mask = 0xffffffff;
> +	snprintf(mii->id, MII_BUS_ID_SIZE, "%s", priv->pdev->name);
> +
> +	mii_np = of_get_available_child_by_name(dev->of_node, "mdio-bus");
> +
> +	ret = devm_of_mdiobus_register(dev, mii, mii_np);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Failed to register mdio bus\n");
> +
> +	of_node_put(mii_np);
> +	return ret;
> +}
> +
> +static void emac_get_strings(struct net_device *dev, u32 stringset, u8 *data)
> +{
> +	int i;
> +
> +	switch (stringset) {
> +	case ETH_SS_STATS:
> +		for (i = 0; i < ARRAY_SIZE(emac_ethtool_stats); i++) {
> +			memcpy(data, emac_ethtool_stats[i].str,
> +			       ETH_GSTRING_LEN);
> +			data += ETH_GSTRING_LEN;
> +		}
> +		break;
> +	}
> +}
> +
> +static int emac_get_sset_count(struct net_device *dev, int sset)
> +{
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		return ARRAY_SIZE(emac_ethtool_stats);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static void emac_stats_update(struct emac_priv *priv)
> +{
> +	struct emac_hw_stats *hwstats = priv->hw_stats;
> +	u32 *stats = (u32 *)hwstats;
> +	int i;
> +
> +	for (i = 0; i < EMAC_TX_STATS_NUM; i++)
> +		stats[i] = emac_tx_read_stat_cnt(priv, i);
> +
> +	for (i = 0; i < EMAC_RX_STATS_NUM; i++)
> +		stats[i + EMAC_TX_STATS_NUM] = emac_rx_read_stat_cnt(priv, i);
> +}
> +
> +static void emac_get_ethtool_stats(struct net_device *dev,
> +				   struct ethtool_stats *stats, u64 *data)
> +{
> +	struct emac_priv *priv = netdev_priv(dev);
> +	struct emac_hw_stats *hwstats;
> +	unsigned long flags;
> +	u32 *data_src;
> +	u64 *data_dst;
> +	int i;
> +
> +	hwstats = priv->hw_stats;
> +
> +	if (netif_running(dev) && netif_device_present(dev)) {
> +		if (spin_trylock_irqsave(&priv->stats_lock, flags)) {
> +			emac_stats_update(priv);
> +			spin_unlock_irqrestore(&priv->stats_lock, flags);
> +		}
> +	}
> +
> +	data_dst = data;
> +
> +	for (i = 0; i < ARRAY_SIZE(emac_ethtool_stats); i++) {
> +		data_src = (u32 *)hwstats + emac_ethtool_stats[i].offset;
> +		*data_dst++ = (u64)(*data_src);
> +	}
> +}
> +
> +static int emac_ethtool_get_regs_len(struct net_device *dev)
> +{
> +	return (EMAC_DMA_REG_CNT + EMAC_MAC_REG_CNT) * sizeof(u32);
> +}
> +
> +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);
> +}
> +
> +static void emac_get_drvinfo(struct net_device *dev,
> +			     struct ethtool_drvinfo *info)
> +{
> +	strscpy(info->driver, DRIVER_NAME, sizeof(info->driver));
> +	info->n_stats = ARRAY_SIZE(emac_ethtool_stats);
> +}
> +
> +static void emac_tx_timeout_task(struct work_struct *work)
> +{
> +	struct net_device *ndev;
> +	struct emac_priv *priv;
> +
> +	priv = container_of(work, struct emac_priv, tx_timeout_task);
> +	ndev = priv->ndev;
> +
> +	rtnl_lock();
> +
> +	/* No need to reset if already down */
> +	if (!netif_running(ndev)) {
> +		rtnl_unlock();
> +		return;
> +	}
> +
> +	netdev_err(ndev, "MAC reset due to TX timeout\n");
> +
> +	netif_trans_update(ndev); /* prevent tx timeout */
> +	dev_close(ndev);
> +	dev_open(ndev, NULL);
> +
> +	rtnl_unlock();
> +}
> +
> +static void emac_sw_init(struct emac_priv *priv)
> +{
> +	priv->dma_buf_sz = EMAC_DEFAULT_BUFSIZE;
> +
> +	priv->tx_ring.total_cnt = DEFAULT_TX_RING_NUM;
> +	priv->rx_ring.total_cnt = DEFAULT_RX_RING_NUM;
> +
> +	spin_lock_init(&priv->stats_lock);
> +
> +	INIT_WORK(&priv->tx_timeout_task, emac_tx_timeout_task);
> +
> +	priv->tx_coal_frames = EMAC_TX_FRAMES;
> +	priv->tx_coal_timeout = EMAC_TX_COAL_TIMEOUT;
> +
> +	timer_setup(&priv->txtimer, emac_tx_coal_timer, 0);
> +}
> +

...

> +static irqreturn_t emac_interrupt_handler(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = (struct net_device *)dev_id;
> +	struct emac_priv *priv = netdev_priv(ndev);
> +	bool should_schedule = false;
> +	u32 status;
> +	u32 clr = 0;

nit: Reverse xmas tree - longest line to shortest - for
     these local variable declarations please.

     Edward Cree's tool can be helpful here:
     https://github.com/ecree-solarflare/xmastree/commits/master/

...

> +static const struct net_device_ops emac_netdev_ops = {
> +	.ndo_open               = emac_open,
> +	.ndo_stop               = emac_close,
> +	.ndo_start_xmit         = emac_start_xmit,

I think that of emac_start_xmit should return netdev_tx_t rather than int
to match the type of the .ndo_start_xmit member of this structure.

Flagged by Clang 20.1.7 [-Wincompatible-function-pointer-types-strict]

> +	.ndo_set_mac_address    = emac_set_mac_address,
> +	.ndo_eth_ioctl          = phy_do_ioctl_running,
> +	.ndo_change_mtu         = emac_change_mtu,
> +	.ndo_tx_timeout         = emac_tx_timeout,
> +	.ndo_set_rx_mode        = emac_rx_mode_set,
> +};

...

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv