| Message ID | 1695064615-3164315-1-git-send-email-radhey.shyam.pandey@amd.com |
|---|---|
| Headers | show |
| Series | net: axienet: Introduce dmaengine | expand |
> -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: Tuesday, September 19, 2023 7:53 AM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; Simek, Michal > <michal.simek@amd.com>; linux@armlinux.org.uk > Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; git (AMD- > Xilinx) <git@amd.com> > 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@amd.com> > > --- > [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
The axiethernet driver can use the dmaengine framework to communicate with the xilinx DMAengine driver(AXIDMA, MCDMA). The inspiration behind this dmaengine adoption is to reuse the in-kernel xilinx dma engine driver[1] and remove redundant dma programming sequence[2] from the ethernet driver. This simplifies the ethernet driver and also makes it generic to be hooked to any complaint dma IP i.e AXIDMA, MCDMA without any modification. The dmaengine framework was extended for metadata API support during the axidma RFC[3] discussion. However, it still needs further enhancements to make it well suited for ethernet usecases. Comments, suggestions, thoughts to implement remaining functional features are very welcome! [1]: https://github.com/torvalds/linux/blob/master/drivers/dma/xilinx/xilinx_dma.c [2]: https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/xilinx/xilinx_axienet_main.c#L238 [3]: http://lkml.iu.edu/hypermail/linux/kernel/1804.0/00367.html [4]: https://lore.kernel.org/all/20221124102745.2620370-1-sarath.babu.naidu.gaddam@amd.com Changes in v6: - Remove patchset 1-7 as it was applied to dmaengine tree in v5 version. - Added Krzysztof reviewed-by tag for dmaengine binding patch. - Rename struct axi_skbuff to skbuf_dma_descriptor and removed __packed attribute. - Drop kmem_cache implementation and switch to using ring buffers. - Remove __inline from axienet_init_dmaengine(). - Name labels after the target. - Add error check for platform_get_irq_optional(). - Fix double space and no empty lines between call and its error check. Changes in v5: - Fix git am failure on net-next - Addressed DT binding review comments i.e Modified commit description to remove dmaengine framework references and instead describe how axiethernet IP uses DMA channels. - Fix "^[tr]x_chan[0-9]|1[0-5]$" -> "^[tr]x_chan([0-9]|1[0-5])$" - Drop generic dmas description. - Fix kmem_cache resource leak. - Merge Xilinx DMA enhancements and optimization[4] into this series. Changes in V4: - Updated commit description about tx/rx channels name(1/3). - Removed "dt-bindings" and "dmaengine" strings in subject(1/3). - Extended dmas and dma-names to support MCDMA channel names(1/3). - Rename has_dmas to use_dmaegine(2/3). - Remove the AXIENET_USE_DMA(2/3). - Remove the AXIENET_USE_DMA(3/3). - Add dev_err_probe for dma_request_chan error handling(3/3). - Add kmem_cache_destroy for create in axienet_setup_dma_chan(3/3). Changes in V3: - Moved RFC to PATCH. - Removed ethtool get/set coalesce, will be added later. - Added backward comapatible support. - Split the dmaengine support patch of V2 into two patches(2/3 and 3/3). https://lore.kernel.org/all/20220920055703.13246-4-sarath.babu.naidu.gaddam@amd.com/ Changes in V2: - Add ethtool get/set coalesce and DMA reset using DMAengine framework. - Add performance numbers. - Remove .txt and change the name of file to xlnx,axiethernet.yaml. - Fix DT check warning(Fix DT check warning('device_type' does not match any of the regexes:'pinctrl-[0-9]+' From schema: Documentation/ devicetree/bindings/net/xilinx_axienet.yaml). Radhey Shyam Pandey (2): dt-bindings: net: xlnx,axi-ethernet: Introduce DMA support net: axienet: Introduce dmaengine support Sarath Babu Naidu Gaddam (1): net: axienet: Preparatory changes for dmaengine support .../bindings/net/xlnx,axi-ethernet.yaml | 16 + drivers/net/ethernet/xilinx/Kconfig | 1 + drivers/net/ethernet/xilinx/xilinx_axienet.h | 35 + .../net/ethernet/xilinx/xilinx_axienet_main.c | 661 ++++++++++++++---- 4 files changed, 590 insertions(+), 123 deletions(-)