> -----Original Message-----
> From: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Sent: Tuesday, September 19, 2023 7:53 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; linux@xxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; git (AMD-
> Xilinx) <git@xxxxxxx>
> Subject: Re: [PATCH net-next v6 3/3] net: axienet: Introduce dmaengine
> support
>
>
>
> On 9/18/2023 12:16 PM, Radhey Shyam Pandey wrote:
> > Add dmaengine framework to communicate with the xilinx DMAengine
> > driver(AXIDMA).
> >
> > Axi ethernet driver uses separate channels for transmit and receive.
> > Add support for these channels to handle TX and RX with skb and
> > appropriate callbacks. Also add axi ethernet core interrupt for
> > dmaengine framework support.
> >
> > The dmaengine framework was extended for metadata API support.
> > However it still needs further enhancements to make it well suited for
> > ethernet usecases. The ethernet features i.e ethtool set/get of DMA IP
> > properties, ndo_poll_controller,(mentioned in TODO) are not supported
> > and it requires follow-up discussions.
> >
> > dmaengine support has a dependency on xilinx_dma as it uses
> > xilinx_vdma_channel_set_config() API to reset the DMA IP which
> > internally reset MAC prior to accessing MDIO.
> >
> > Benchmark with netperf:
> >
> > xilinx-zcu102-20232:~$ netperf -H 192.168.10.20 -t TCP_STREAM MIGRATED
> > TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.10.20
> > () port 0 AF_INET
> > Recv Send Send
> > Socket Socket Message Elapsed
> > Size Size Size Time Throughput
> > bytes bytes bytes secs. 10^6bits/sec
> >
> > 131072 16384 16384 10.03 915.55
> >
> > xilinx-zcu102-20232:~$ netperf -H 192.168.10.20 -t UDP_STREAM
> MIGRATED
> > UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.10.20
> > () port 0 AF_INET
> > Socket Message Elapsed Messages
> > Size Size Time Okay Errors Throughput
> > bytes bytes secs # # 10^6bits/sec
> >
> > 212992 65507 10.00 18192 0 953.35
> > 212992 10.00 18192 953.35
> >
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxx>
> > ---
> [snip]
>
> Nice example of how to integrate an Ethernet driver with dmaengine, BTW.
Thanks!
>
>
> > +
> > + /*Fill up app fields for checksum */
>
> Missing space between * and Fill up, there are a few comments where it is
> the opposite where you don't add a space at the end before the *.
Will fix the comment space in next version.
>
> > + if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > + if (lp->features & XAE_FEATURE_FULL_TX_CSUM) {
> > + /* Tx Full Checksum Offload Enabled */
> > + app[0] |= 2;
> > + } else if (lp->features & XAE_FEATURE_PARTIAL_RX_CSUM) {
>
> This is the transmit path here, is this path even taken?
This fragment is c&p from non dmaengine flow which used PARTIAL_RX.
Will fix it to use partial TX csum define.
>
> > + csum_start_off = skb_transport_offset(skb);
> > + csum_index_off = csum_start_off + skb-
> >csum_offset;
> > + /* Tx Partial Checksum Offload Enabled */
> > + app[0] |= 1;
> > + app[1] = (csum_start_off << 16) | csum_index_off;
> > + }
> > + } else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > + app[0] |= 2; /* Tx Full Checksum Offload Enabled */
> > + }
> > +
> > + dma_tx_desc = lp->tx_chan->device->device_prep_slave_sg(lp-
> >tx_chan, skbuf_dma->sgl,
> > + sg_len, DMA_MEM_TO_DEV,
> > + DMA_PREP_INTERRUPT, (void *)app);
> > + if (!dma_tx_desc)
> > + goto xmit_error_unmap_sg;
> > +
> > + skbuf_dma->skb = skb;
> > + skbuf_dma->sg_len = sg_len;
> > + dma_tx_desc->callback_param = lp;
> > + dma_tx_desc->callback_result = axienet_dma_tx_cb;
> > + dmaengine_submit(dma_tx_desc);
> > + dma_async_issue_pending(lp->tx_chan);
> > +
> > + return NETDEV_TX_OK;
> > +
> > +xmit_error_unmap_sg:
> > + dma_unmap_sg(lp->dev, skbuf_dma->sgl, sg_len, DMA_TO_DEVICE);
> > + return NETDEV_TX_OK;
> > +}
> > +
> > /**
> > * axienet_tx_poll - Invoked once a transmit is completed by the
> > * Axi DMA Tx channel.
> > @@ -911,7 +1036,42 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
> > if (!lp->use_dmaengine)
> > return axienet_start_xmit_legacy(skb, ndev);
> > else
> > - return NETDEV_TX_BUSY;
> > + return axienet_start_xmit_dmaengine(skb, ndev); }
> > +
> > +/**
> > + * axienet_dma_rx_cb - DMA engine callback for RX channel.
> > + * @data: Pointer to the skbuf_dma_descriptor structure.
> > + * @result: error reporting through dmaengine_result.
> > + * This function is called by dmaengine driver for RX channel to
> > +notify
> > + * that the packet is received.
> > + */
> > +static void axienet_dma_rx_cb(void *data, const struct
> > +dmaengine_result *result) {
> > + struct axienet_local *lp = data;
> > + struct skbuf_dma_descriptor *skbuf_dma;
> > + size_t meta_len, meta_max_len, rx_len;
> > + struct sk_buff *skb;
> > + u32 *app;
> > +
> > + skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_tail++);
> > + skb = skbuf_dma->skb;
> > + app = dmaengine_desc_get_metadata_ptr(skbuf_dma->desc,
> &meta_len,
> > +&meta_max_len);
>
> app might not be the best name, I understand this refers to a DMA
> application, in a broad sense that it is not specific, maybe cookie, or
> opaque_ptr would work better?
Yes, app fields are specific to DMA. In Descriptor Fields we have
User Application Field 0-4 (APP0-4).
The AXI4 Control stream allows user application metadata associated with
the MM2S channel to be transmitted to a target IP. User application fields 0
through 4 of an MM2S Scatter / Gather Start Of Frame (SOF) descriptor Transmit
Start Of Frame (TXSOF =1) are transmitted on the m_axis_mm2s_cntrl stream
interface along with an associated packet being transmitted on the m_axis_mm2s
stream interface.
Is user_app_metadata/app_metadata looking more meaningful?
> > + dma_unmap_single(lp->dev, skbuf_dma->dma_address, lp-
> >max_frm_size,
> > + DMA_FROM_DEVICE);
> > + /* TODO: Derive app word index programmatically */
> > + rx_len = (app[LEN_APP] & 0xFFFF);
> > + skb_put(skb, rx_len);
> > + skb->protocol = eth_type_trans(skb, lp->ndev);
> > + skb->ip_summed = CHECKSUM_NONE;
> > +
> > + netif_rx(skb);
>
> Could save some cycles and call __netif_rx() here since AFAIR dmaengine
> uses a tasklet? netif_rx() is fine, just would have to compute need_bh_off.
Good point - will switch to __netif_rx().
>
> > + u64_stats_update_begin(&lp->rx_stat_sync);
> > + u64_stats_add(&lp->rx_packets, 1);
> > + u64_stats_add(&lp->rx_bytes, rx_len);
> > + u64_stats_update_end(&lp->rx_stat_sync);
> > + axienet_rx_submit_desc(lp->ndev);
> > + dma_async_issue_pending(lp->rx_chan);
> > }
> >
> > /**
> > @@ -1147,6 +1307,142 @@ static irqreturn_t axienet_eth_irq(int irq,
> > void *_ndev)
> >
> > static void axienet_dma_err_handler(struct work_struct *work);
> >
> > +/**
> > + * axienet_rx_submit_desc - Submit the descriptors with required data
> > + * like call backup API, skb buffer.. etc to dmaengine.
> > + *
> > + * @ndev: net_device pointer
> > + *
> > + *Return: 0, on success.
> > + * non-zero error value on failure
> > + */
> > +static int axienet_rx_submit_desc(struct net_device *ndev) {
> > + struct dma_async_tx_descriptor *dma_rx_desc = NULL;
> > + struct axienet_local *lp = netdev_priv(ndev);
> > + struct skbuf_dma_descriptor *skbuf_dma;
> > + struct sk_buff *skb;
> > + dma_addr_t addr;
> > + int ret;
> > +
> > + skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_head);
> > + if (!skbuf_dma)
> > + return -ENOMEM;
> > + lp->rx_ring_head++;
> > + skb = netdev_alloc_skb(ndev, lp->max_frm_size);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + sg_init_table(skbuf_dma->sgl, 1);
> > + addr = dma_map_single(lp->dev, skb->data, lp->max_frm_size,
> > +DMA_FROM_DEVICE);
>
> Need to check with dma_mapping_error() that the mapping succeeded.
Will add mapping_error check in next version.
>
> Thanks!
>
> pw-bot: cr
> --
> Florian
