Re: [net-next] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Paul,

Thanks for your review!

On 2024-04-15 08:34:09 +0100, Paul Barker wrote:
> On 14/04/2024 14:59, Niklas Söderlund wrote:
> > Add initial support for Renesas Ethernet-TSN End-station device of R-Car
> > V4H. The Ethernet End-station can connect to an Ethernet network using a
> > 10 Mbps, 100 Mbps, or 1 Gbps full-duplex link via MII/GMII/RMII/RGMII.
> > Depending on the connected PHY.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > * Changes since RFC
> > - Fix issues in MDIO communication.
> > - Use a dedicated OF node for the MDIO bus.
> > ---
> >  MAINTAINERS                           |    8 +
> >  drivers/net/ethernet/renesas/Kconfig  |   11 +
> >  drivers/net/ethernet/renesas/Makefile |    2 +
> >  drivers/net/ethernet/renesas/rtsn.c   | 1421 +++++++++++++++++++++++++
> >  drivers/net/ethernet/renesas/rtsn.h   |  464 ++++++++
> >  5 files changed, 1906 insertions(+)
> >  create mode 100644 drivers/net/ethernet/renesas/rtsn.c
> >  create mode 100644 drivers/net/ethernet/renesas/rtsn.h
> 
> <snip>
> 
> > diff --git a/drivers/net/ethernet/renesas/rtsn.c b/drivers/net/ethernet/renesas/rtsn.c
> > new file mode 100644
> > index 000000000000..291ab421d68f
> > --- /dev/null
> > +++ b/drivers/net/ethernet/renesas/rtsn.c
> 
> <snip>
> 
> > +static bool rtsn_rx(struct net_device *ndev, int *quota)
> > +{
> > +	struct rtsn_ext_ts_desc *desc;
> > +	struct rtsn_private *priv;
> > +	struct sk_buff *skb;
> > +	dma_addr_t dma_addr;
> > +	int boguscnt;
> 
> I find the variable name `boguscnt` very unclear, I'm not sure if it
> means the count is bogus, or it is counting bogus items?
> 
> I don't think you need to match what I've done in ravb_main.c exactly,
> but I'd prefer to see a better variable name here.

I like the changes you did in this area for RAVB, I will reuse some of 
it in v2 of this.

> 
> > +	u16 pkt_len;
> > +	u32 get_ts;
> > +	int entry;
> > +	int limit;
> > +
> > +	priv = netdev_priv(ndev);
> > +
> > +	entry = priv->cur_rx % priv->num_rx_ring;
> > +	desc = &priv->rx_ring[entry];
> > +
> > +	boguscnt = priv->dirty_rx + priv->num_rx_ring - priv->cur_rx;
> > +	boguscnt = min(boguscnt, *quota);
> > +	limit = boguscnt;
> > +
> > +	while ((desc->die_dt & DT_MASK) != DT_FEMPTY) {
> > +		dma_rmb();
> > +		pkt_len = le16_to_cpu(desc->info_ds) & RX_DS;
> > +		if (--boguscnt < 0)
> > +			break;
> > +
> > +		skb = priv->rx_skb[entry];
> > +		priv->rx_skb[entry] = NULL;
> > +		dma_addr = le32_to_cpu(desc->dptr);
> > +		dma_unmap_single(ndev->dev.parent, dma_addr, PKT_BUF_SZ,
> > +				 DMA_FROM_DEVICE);
> > +
> > +		get_ts = priv->ptp_priv->tstamp_rx_ctrl &
> > +			RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
> > +		if (get_ts) {
> > +			struct skb_shared_hwtstamps *shhwtstamps;
> > +			struct timespec64 ts;
> > +
> > +			shhwtstamps = skb_hwtstamps(skb);
> > +			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > +
> > +			ts.tv_sec = (u64)le32_to_cpu(desc->ts_sec);
> > +			ts.tv_nsec = le32_to_cpu(desc->ts_nsec & cpu_to_le32(0x3fffffff));
> > +
> > +			shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
> > +		}
> > +
> > +		skb_put(skb, pkt_len);
> > +		skb->protocol = eth_type_trans(skb, ndev);
> > +		netif_receive_skb(skb);
> > +		ndev->stats.rx_packets++;
> > +		ndev->stats.rx_bytes += pkt_len;
> > +
> > +		entry = (++priv->cur_rx) % priv->num_rx_ring;
> > +		desc = &priv->rx_ring[entry];
> > +	}
> > +
> > +	/* Refill the RX ring buffers */
> > +	for (; priv->cur_rx - priv->dirty_rx > 0; priv->dirty_rx++) {
> > +		entry = priv->dirty_rx % priv->num_rx_ring;
> > +		desc = &priv->rx_ring[entry];
> > +		desc->info_ds = cpu_to_le16(PKT_BUF_SZ);
> > +
> > +		if (!priv->rx_skb[entry]) {
> > +			skb = netdev_alloc_skb(ndev,
> > +					       PKT_BUF_SZ + RTSN_ALIGN - 1);
> 
> I'll send my work using a page pool today as an RFC so you can see if it
> would be beneficial to use that here as well. I was going to hold off
> until the bugfix patches have merged so that I don't need to go through
> another RFC round, but it will be good to get some more review on the
> series anyway.

I like the page pool idea, but there is no real benefit for it in this 
driver at the moment. I would like to play and learn a bit more with it 
in RAVB. And once I know more I can convert this driver too if it fits.

> 
> > +			if (!skb)
> > +				break;
> > +			skb_reserve(skb, NET_IP_ALIGN);
> > +			dma_addr = dma_map_single(ndev->dev.parent, skb->data,
> > +						  le16_to_cpu(desc->info_ds),
> > +						  DMA_FROM_DEVICE);
> > +			if (dma_mapping_error(ndev->dev.parent, dma_addr))
> > +				desc->info_ds = cpu_to_le16(0);
> > +			desc->dptr = cpu_to_le32(dma_addr);
> > +			skb_checksum_none_assert(skb);
> > +			priv->rx_skb[entry] = skb;
> > +		}
> > +		dma_wmb();
> > +		desc->die_dt = DT_FEMPTY | D_DIE;
> > +	}
> > +
> > +	desc = &priv->rx_ring[priv->num_rx_ring];
> > +	desc->die_dt = DT_LINK;
> > +
> > +	*quota -= limit - (++boguscnt);
> > +
> > +	return boguscnt <= 0;
> > +}
> > +
> > +static int rtsn_poll(struct napi_struct *napi, int budget)
> > +{
> > +	struct rtsn_private *priv;
> > +	struct net_device *ndev;
> > +	unsigned long flags;
> > +	int quota = budget;
> > +
> > +	ndev = napi->dev;
> > +	priv = netdev_priv(ndev);
> > +
> > +	/* Processing RX Descriptor Ring */
> > +	if (rtsn_rx(ndev, &quota))
> > +		goto out;
> > +
> > +	/* Processing TX Descriptor Ring */
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	rtsn_tx_free(ndev, true);
> > +	netif_wake_subqueue(ndev, 0);
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	napi_complete(napi);
> 
> We should use napi_complete_done() here as described in
> Documentation/networking/napi.rst. That will require rtsn_rx() to return
> the number of packets received so that it can be passed as the work_done
> argument to napi_complete_done().

Good point will update in v2.

> 
> > +
> > +	/* Re-enable TX/RX interrupts */
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	rtsn_ctrl_data_irq(priv, true);
> > +	__iowmb();
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +out:
> > +	return budget - quota;
> > +}
> 
> <snip>
> 
> > +static int rtsn_probe(struct platform_device *pdev)
> > +{
> > +	struct rtsn_private *priv;
> > +	struct net_device *ndev;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	ndev = alloc_etherdev_mqs(sizeof(struct rtsn_private), TX_NUM_CHAINS,
> > +				  RX_NUM_CHAINS);
> > +	if (!ndev)
> > +		return -ENOMEM;
> > +
> > +	priv = netdev_priv(ndev);
> > +	priv->pdev = pdev;
> > +	priv->ndev = ndev;
> > +	priv->ptp_priv = rcar_gen4_ptp_alloc(pdev);
> > +
> > +	spin_lock_init(&priv->lock);
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		ret = -PTR_ERR(priv->clk);
> > +		goto error_alloc;
> > +	}
> > +
> > +	priv->reset = devm_reset_control_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->reset)) {
> > +		ret = -PTR_ERR(priv->reset);
> > +		goto error_alloc;
> > +	}
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsnes");
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "Can't find tsnes resource\n");
> > +		ret = -EINVAL;
> > +		goto error_alloc;
> > +	}
> > +
> > +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv->base)) {
> > +		ret = PTR_ERR(priv->base);
> > +		goto error_alloc;
> > +	}
> > +
> > +	SET_NETDEV_DEV(ndev, &pdev->dev);
> > +	ether_setup(ndev);
> > +
> > +	ndev->features = NETIF_F_RXCSUM;
> > +	ndev->hw_features = NETIF_F_RXCSUM;
> 
> A quick skim of the datasheet suggests that TX checksum calculation is
> also supported. It's probably worth listing which hardware features this
> driver supports/does not support in the commit message.
> 
> Thanks,
> 
> -- 
> Paul Barker






-- 
Kind Regards,
Niklas Söderlund