Message-ID: <ec29a7b6-e941-4006-9bb7-84334e6e48ea@iscas.ac.cn>
Date: Thu, 10 Jul 2025 12:42:14 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Simon Horman <horms@...nel.org>, Vivian Wang <uwu@...m.page>
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>,
 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
Subject: Re: [PATCH net-next v4 2/2] net: spacemit: Add K1 Ethernet MAC

Hi Simon,

Thanks for your suggestions.

On 7/7/25 18:01, Simon Horman wrote:
> On Thu, Jul 03, 2025 at 05:42:03PM +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".
>>
>> This driver is based on "k1x-emac" in the same directory in the vendor's
>> tree [1]. Some debugging tunables have been fixed to vendor-recommended
>> defaults, and PTP support is not included yet.
>>
>> [1]: https://github.com/spacemit-com/linux-k1x
>>
>> Tested-by: Junhui Liu <junhui.liu@...moral.tech>
>> Signed-off-by: Vivian Wang <wangruikang@...as.ac.cn>
> ...
>
>> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
> ...
>
>> +/**
>> + * struct emac_desc_ring - Software-side information for one descriptor ring
>> + * Same struture used for both RX and TX
> nit: structure

Thanks, will fix in next version.

>
>> + * @desc_addr: Virtual address to the descriptor ring memory
>> + * @desc_dma_addr: DMA address of the descriptor ring
>> + * @total_size: Size of ring in bytes
>> + * @total_cnt: Number of descriptors
>> + * @head: Next descriptor to associate a buffer with
>> + * @tail: Next descriptor to check status bit
>> + * @rx_desc_buf: Array of descriptors for RX
>> + * @tx_desc_buf: Array of descriptors for TX, with max of two buffers each
>> + */
> ...
>
>> +static void emac_set_mac_addr(struct emac_priv *priv, const unsigned char *addr)
>> +{
>> +	emac_wr(priv, MAC_ADDRESS1_HIGH, ((addr[1] << 8) | addr[0]));
> nit: no need for inner parentheses here,
>       the order of operations is on your side
>
> 	emac_wr(priv, MAC_ADDRESS1_HIGH, addr[1] << 8 | addr[0]);
>
>> +	emac_wr(priv, MAC_ADDRESS1_MED, ((addr[3] << 8) | addr[2]));
>> +	emac_wr(priv, MAC_ADDRESS1_LOW, ((addr[5] << 8) | addr[4]));
>> +}

I will remove unnecessary these parens in next version.

> ...
>
>> +static int emac_rx_frame_status(struct emac_priv *priv, struct emac_desc *desc)
>> +{
>> +	/* Drop if not last descriptor */
>> +	if (!(desc->desc0 & RX_DESC_0_LAST_DESCRIPTOR)) {
>> +		netdev_dbg(priv->ndev, "RX not last descriptor\n");
> Unless I am mistaken these logs can occur on the basis of user
> (Network packet) input. If so, I think rate limited debug
> messages are more appropriate here and below.

This particular one shouldn't be, but the rest are indeed triggered 
based on network packet input, and in any case these can happen 
per-packet. I will make these ratelimited in the next version.

>> +		return RX_FRAME_DISCARD;
>> +	}
>> +
>> +	if (desc->desc0 & RX_DESC_0_FRAME_RUNT) {
>> +		netdev_dbg(priv->ndev, "RX runt frame\n");
>> +		return RX_FRAME_DISCARD;
>> +	}
>> +
>> +	if (desc->desc0 & RX_DESC_0_FRAME_CRC_ERR) {
>> +		netdev_dbg(priv->ndev, "RX frame CRC error\n");
>> +		return RX_FRAME_DISCARD;
>> +	}
>> +
>> +	if (desc->desc0 & RX_DESC_0_FRAME_MAX_LEN_ERR) {
>> +		netdev_dbg(priv->ndev, "RX frame exceeds max length\n");
>> +		return RX_FRAME_DISCARD;
>> +	}
>> +
>> +	if (desc->desc0 & RX_DESC_0_FRAME_JABBER_ERR) {
>> +		netdev_dbg(priv->ndev, "RX frame jabber error\n");
>> +		return RX_FRAME_DISCARD;
>> +	}
>> +
>> +	if (desc->desc0 & RX_DESC_0_FRAME_LENGTH_ERR) {
>> +		netdev_dbg(priv->ndev, "RX frame length error\n");
>> +		return RX_FRAME_DISCARD;
>> +	}
>> +
>> +	if (rx_frame_len(desc) <= ETH_FCS_LEN ||
>> +	    rx_frame_len(desc) > priv->dma_buf_sz) {
>> +		netdev_dbg(priv->ndev, "RX frame length unacceptable\n");
>> +		return RX_FRAME_DISCARD;
>> +	}
>> +	return RX_FRAME_OK;
>> +}
> ...
>
>> +static int emac_resume(struct device *dev)
>> +{
>> +	struct emac_priv *priv = dev_get_drvdata(dev);
>> +	struct net_device *ndev = priv->ndev;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(priv->bus_clk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!netif_running(ndev))
>> +		return 0;
>> +
>> +	ret = emac_open(ndev);
>> +	if (ret)
> Smatch flags that priv->bus_clk resources are leaked here, and I agree.
>
>> +		return ret;
>> +
>> +	netif_device_attach(ndev);
>> +	return 0;
>> +}
> I would suggest addressing that like this.
> (Compile tested only!)
>
> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
> index 6158e776bc67..ebd02ec2bb01 100644
> --- a/drivers/net/ethernet/spacemit/k1_emac.c
> +++ b/drivers/net/ethernet/spacemit/k1_emac.c
> @@ -1843,10 +1843,14 @@ static int emac_resume(struct device *dev)
>   
>   	ret = emac_open(ndev);
>   	if (ret)
> -		return ret;
> +		goto err_clk_disable_unprepare;
>   
>   	netif_device_attach(ndev);
>   	return 0;
> +
> +err_clk_disable_unprepare:
> +	clk_disable_unprepare(priv->bus_clk);
> +	return ret;
>   }
>   
>   static int emac_suspend(struct device *dev)

Thanks for the tip. I will fix the error handling path in emac_resume 
next version.

> ...
>
>> diff --git a/drivers/net/ethernet/spacemit/k1_emac.h b/drivers/net/ethernet/spacemit/k1_emac.h
> ...
>
>> +struct emac_hw_stats {
>> +	u32 tx_ok_pkts;
>> +	u32 tx_total_pkts;
>> +	u32 tx_ok_bytes;
>> +	u32 tx_err_pkts;
>> +	u32 tx_singleclsn_pkts;
>> +	u32 tx_multiclsn_pkts;
>> +	u32 tx_lateclsn_pkts;
>> +	u32 tx_excessclsn_pkts;
>> +	u32 tx_unicast_pkts;
>> +	u32 tx_multicast_pkts;
>> +	u32 tx_broadcast_pkts;
>> +	u32 tx_pause_pkts;
>> +	u32 rx_ok_pkts;
>> +	u32 rx_total_pkts;
>> +	u32 rx_crc_err_pkts;
>> +	u32 rx_align_err_pkts;
>> +	u32 rx_err_total_pkts;
>> +	u32 rx_ok_bytes;
>> +	u32 rx_total_bytes;
>> +	u32 rx_unicast_pkts;
>> +	u32 rx_multicast_pkts;
>> +	u32 rx_broadcast_pkts;
>> +	u32 rx_pause_pkts;
>> +	u32 rx_len_err_pkts;
>> +	u32 rx_len_undersize_pkts;
>> +	u32 rx_len_oversize_pkts;
>> +	u32 rx_len_fragment_pkts;
>> +	u32 rx_len_jabber_pkts;
>> +	u32 rx_64_pkts;
>> +	u32 rx_65_127_pkts;
>> +	u32 rx_128_255_pkts;
>> +	u32 rx_256_511_pkts;
>> +	u32 rx_512_1023_pkts;
>> +	u32 rx_1024_1518_pkts;
>> +	u32 rx_1519_plus_pkts;
>> +	u32 rx_drp_fifo_full_pkts;
>> +	u32 rx_truncate_fifo_full_pkts;
>> +};
> Many of the stats above appear to cover stats covered by struct
> rtnl_link_stats64, ethtool_pause_stats, struct ethtool_rmon_stats, and
> possibly others standardised in ethtool.h. Please only report standard
> counters using standard mechanisms. And only use get_ethtool_stats to
> report non-standard counters.
>
> Link: https://www.kernel.org/doc/html/v6.16-rc4/networking/statistics.html#notes-for-driver-authors

I will move statistics to standard counters wherever available. Thanks 
for the tip.

And thanks again for your suggestions.

Vivian "dramforever" Wang

> ...
>

Powered by blists - more mailing lists