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@xxxxxxxxxxxx>
> > ---
> > * 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, "a))
> > + 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
