| Message ID | 20230510085031.1116327-1-sarath.babu.naidu.gaddam@amd.com |
|---|---|
| Headers | show |
| Series | net: axienet: Introduce dmaengine | expand |
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Wednesday, May 10, 2023 3:39 PM > To: Gaddam, Sarath Babu Naidu > <sarath.babu.naidu.gaddam@amd.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org > Cc: linux@armlinux.org.uk; Simek, Michal <michal.simek@amd.com>; > Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sarangi, > Anirudha <anirudha.sarangi@amd.com>; Katakam, Harini > <harini.katakam@amd.com>; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH net-next V3 1/3] dt-bindings: net: xilinx_axienet: > Introduce dmaengine binding support > > On 10/05/2023 10:50, Sarath Babu Naidu Gaddam wrote: > > From: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> > > > > The axiethernet driver will use dmaengine framework to communicate > > with dma controller IP instead of built-in dma programming sequence. > > Subject: drop second/last, redundant "bindings". The "dt-bindings" > prefix is already stating that these are bindings. > > Actually also drop "dmaenging" as it is Linuxism. Focus on hardware, e.g. > "Add DMA support". > > > > > To request dma transmit and receive channels the axiethernet driver > > uses generic dmas, dma-names properties. > > > > Also to support the backward compatibility, use "dmas" property to > > identify as it should use dmaengine framework or legacy > > driver(built-in dma programming). > > > > At this point it is recommended to use dmaengine framework but it's > > optional. Once the solution is stable will make dmas as required > > properties. > > > > Signed-off-by: Radhey Shyam Pandey > <radhey.shyam.pandey@xilinx.com> > > Signed-off-by: Sarath Babu Naidu Gaddam > > <sarath.babu.naidu.gaddam@amd.com> > > --- > > These changes are on top of below txt to yaml conversion discussion > > https://lore.kernel.org/all/20230308061223.1358637-1- > sarath.babu.naidu > > .gaddam@amd.com/#Z2e.:20230308061223.1358637-1- > sarath.babu.naidu.gadda > > m::40amd.com:1bindings:net:xlnx::2caxi-ethernet.yaml > > > > Changes in V3: > > 1) Reverted reg and interrupts property to support backward > compatibility. > > 2) Moved dmas and dma-names properties from Required properties. > > > > Changes in V2: > > - None. > > --- > > .../devicetree/bindings/net/xlnx,axi-ethernet.yaml | 12 > ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml > > b/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml > > index 80843c177029..9dfa1976e260 100644 > > --- a/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml > > +++ b/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml > > @@ -122,6 +122,16 @@ properties: > > modes, where "pcs-handle" should be used to point to the PCS/PMA > PHY, > > and "phy-handle" should point to an external PHY if exists. > > > > + dmas: > > + items: > > + - description: TX DMA Channel phandle and DMA request line > number > > + - description: RX DMA Channel phandle and DMA request line > > + number > > + > > + dma-names: > > + items: > > + - const: tx_chan0 > > tx > > > + - const: rx_chan0 > > rx We want to support more channels in the future, currently we support AXI DMA which has only one tx and rx channel. In future we want to extend support for multichannel DMA (MCDMA) which has 16 TX and 16 RX channels. To uniquely identify each channel, we are using chan suffix. Depending on the usecase AXI ethernet driver can request any combination of multichannel DMA channels. dma-names = tx_chan0, tx_chan1, rx_chan0, rx_chan1; will update the commit message with same. > Why doing these differently than all other devices? To make the axi ethernet driver generic to be hooked to any complaint dma IP i.e AXIDMA, AXIMCDMA without any modification.The inspiration behind this dmaengine adoption is to reuse the in-kernel xilinx dma engine driver and remove redundant dma programming sequence from the ethernet driver. Above information is explained in the cover letter https://lore.kernel.org/all/20230510085031.1116327-1-sarath.babu.naidu.gaddam@amd.com/ Thanks, Sarath
> -----Original Message----- > From: Subbaraya Sundeep Bhatta <sbhatta@marvell.com> > Sent: Wednesday, May 10, 2023 7:14 PM > To: Gaddam, Sarath Babu Naidu > <sarath.babu.naidu.gaddam@amd.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org > Cc: linux@armlinux.org.uk; Simek, Michal <michal.simek@amd.com>; > Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sarangi, > Anirudha <anirudha.sarangi@amd.com>; Katakam, Harini > <harini.katakam@amd.com>; git (AMD-Xilinx) <git@amd.com> > Subject: RE: [EXT] [PATCH net-next V3 3/3] net: axienet: Introduce > dmaengine support > > Hi, > > >-----Original Message----- > >From: Sarath Babu Naidu Gaddam > <sarath.babu.naidu.gaddam@amd.com> > >Sent: Wednesday, May 10, 2023 2:21 PM > >To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > >pabeni@redhat.com; robh+dt@kernel.org; > >krzysztof.kozlowski+dt@linaro.org > >Cc: linux@armlinux.org.uk; michal.simek@amd.com; > >radhey.shyam.pandey@amd.com; netdev@vger.kernel.org; > >devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >linux- kernel@vger.kernel.org; anirudha.sarangi@amd.com; > >harini.katakam@amd.com; sarath.babu.naidu.gaddam@amd.com; > git@amd.com > >Subject: [PATCH net-next V3 3/3] net: axienet: Introduce dmaengine > >support > > > >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 > during > >the axidma RFC[1] discussion. 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, > >trigger reset of DMA IP from ethernet are not supported (mentioned in > >TODO) and it requires follow-up discussion and dma framework > enhancement. > > > >[1]: https://urldefense.proofpoint.com/v2/url?u=https- > >3A__lore.kernel.org_lkml_1522665546-2D10035-2D1-2Dgit-2Dsend- > 2Demail- > >2D&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=wYboOaw70DU5hR > M5HDwOR > >Jx_MfD- > hXXKii2eobNikgU&m=qvFFGXjULUP3IPFOil5aKySdkumrp8V0TYK4kQ- > >yzsWPmRolFaQvfKMhaR11_dZv&s=brEWb1Til18VCodb- > H4tST0HOBXKIJtL- > >2ztGmMZz_8&e= > >radheys@xilinx.com > > > >Signed-off-by: Radhey Shyam Pandey > <radhey.shyam.pandey@xilinx.com> > >Signed-off-by: Sarath Babu Naidu Gaddam > ><sarath.babu.naidu.gaddam@amd.com> > >--- > >Performance numbers(Mbps): > > > > | TCP | UDP | > > ----------------- > > | Tx | 920 | 800 | > > ----------------- > > | Rx | 620 | 910 | > > > >Changes in V3: > >1) New patch for dmaengine framework support. > >--- > > drivers/net/ethernet/xilinx/xilinx_axienet.h | 6 + > > .../net/ethernet/xilinx/xilinx_axienet_main.c | 331 > +++++++++++++++++- > > 2 files changed, 335 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h > >b/drivers/net/ethernet/xilinx/xilinx_axienet.h > >index 10917d997d27..fbe00c5390d5 100644 > >--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h > >+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h > >@@ -436,6 +436,9 @@ struct axidma_bd { > > * @coalesce_count_tx: Store the irq coalesce on TX side. > > * @coalesce_usec_tx: IRQ coalesce delay for TX > > * @has_dmas: flag to check dmaengine framework usage. > >+ * @tx_chan: TX DMA channel. > >+ * @rx_chan: RX DMA channel. > >+ * @skb_cache: Custom skb slab allocator > > */ > > struct axienet_local { > > struct net_device *ndev; > >@@ -501,6 +504,9 @@ struct axienet_local { > > u32 coalesce_count_tx; > > u32 coalesce_usec_tx; > > u8 has_dmas; > >+ struct dma_chan *tx_chan; > >+ struct dma_chan *rx_chan; > >+ struct kmem_cache *skb_cache; > > }; > > > > /** > >diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > >b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > >index 8678fc09245a..662c77ff0e99 100644 > >--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > >+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > >@@ -37,6 +37,9 @@ > > #include <linux/phy.h> > > #include <linux/mii.h> > > #include <linux/ethtool.h> > >+#include <linux/dmaengine.h> > >+#include <linux/dma-mapping.h> > >+#include <linux/dma/xilinx_dma.h> > > > > #include "xilinx_axienet.h" > > > >@@ -46,6 +49,9 @@ > > #define TX_BD_NUM_MIN (MAX_SKB_FRAGS + 1) > > #define TX_BD_NUM_MAX 4096 > > #define RX_BD_NUM_MAX 4096 > >+#define DMA_NUM_APP_WORDS 5 > >+#define LEN_APP 4 > >+#define RX_BUF_NUM_DEFAULT 128 > > > > /* Must be shorter than length of ethtool_drvinfo.driver field to fit */ > > #define DRIVER_NAME "xaxienet" > >@@ -56,6 +62,16 @@ > > > > #define AXIENET_USE_DMA(lp) ((lp)->has_dmas) > > > >+struct axi_skbuff { > >+ struct scatterlist sgl[MAX_SKB_FRAGS + 1]; > >+ struct dma_async_tx_descriptor *desc; > >+ dma_addr_t dma_address; > >+ struct sk_buff *skb; > >+ int sg_len; > >+} __packed; > >+ > >+static int axienet_rx_submit_desc(struct net_device *ndev); > >+ > > /* Match table for of_platform binding */ static const struct > >of_device_id axienet_of_match[] = { > > { .compatible = "xlnx,axi-ethernet-1.00.a", }, @@ -728,6 > +744,108 @@ > >static inline int axienet_check_tx_bd_space(struct axienet_local *lp, > > return 0; > > } > > > >+/** > >+ * axienet_dma_tx_cb - DMA engine callback for TX channel. > >+ * @data: Pointer to the axi_skbuff structure > >+ * @result: error reporting through dmaengine_result. > >+ * This function is called by dmaengine driver for TX channel to > >+notify > >+ * that the transmit is done. > >+ */ > >+static void axienet_dma_tx_cb(void *data, const struct > >+dmaengine_result > >*result) > >+{ > >+ struct axi_skbuff *axi_skb = data; > >+ > >+ struct net_device *netdev = axi_skb->skb->dev; > >+ struct axienet_local *lp = netdev_priv(netdev); > >+ > >+ u64_stats_update_begin(&lp->tx_stat_sync); > >+ u64_stats_add(&lp->tx_bytes, axi_skb->skb->len); > >+ u64_stats_add(&lp->tx_packets, 1); > >+ u64_stats_update_end(&lp->tx_stat_sync); > >+ > >+ dma_unmap_sg(lp->dev, axi_skb->sgl, axi_skb->sg_len, > >DMA_MEM_TO_DEV); > >+ dev_kfree_skb_any(axi_skb->skb); > >+ kmem_cache_free(lp->skb_cache, axi_skb); } > >+ > >+/** > >+ * axienet_start_xmit_dmaengine - Starts the transmission. > >+ * @skb: sk_buff pointer that contains data to be Txed. > >+ * @ndev: Pointer to net_device structure. > >+ * > >+ * Return: NETDEV_TX_OK, on success > >+ * NETDEV_TX_BUSY, if any memory failure or SG error. > >+ * > >+ * This function is invoked from xmit to initiate transmission. The > >+ * function sets the skbs , call back API, SG etc. > >+ * Additionally if checksum offloading is supported, > >+ * it populates AXI Stream Control fields with appropriate values. > >+ */ > >+static netdev_tx_t > >+axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device > >+*ndev) { > >+ struct dma_async_tx_descriptor *dma_tx_desc = NULL; > >+ struct axienet_local *lp = netdev_priv(ndev); > >+ u32 app[DMA_NUM_APP_WORDS] = {0}; > >+ struct axi_skbuff *axi_skb; > >+ u32 csum_start_off; > >+ u32 csum_index_off; > >+ int sg_len; > >+ int ret; > >+ > >+ sg_len = skb_shinfo(skb)->nr_frags + 1; > >+ axi_skb = kmem_cache_zalloc(lp->skb_cache, GFP_KERNEL); > >+ if (!axi_skb) > >+ return NETDEV_TX_BUSY; > >+ > >+ sg_init_table(axi_skb->sgl, sg_len); > >+ ret = skb_to_sgvec(skb, axi_skb->sgl, 0, skb->len); > >+ if (unlikely(ret < 0)) > >+ goto xmit_error_skb_sgvec; > >+ > >+ ret = dma_map_sg(lp->dev, axi_skb->sgl, sg_len, > DMA_TO_DEVICE); > >+ if (ret == 0) > >+ goto xmit_error_skb_sgvec; > >+ > >+ /*Fill up app fields for checksum */ > >+ 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) { > >+ 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, > >axi_skb->sgl, > >+ sg_len, DMA_MEM_TO_DEV, > >+ DMA_PREP_INTERRUPT, (void *)app); > >+ > >+ if (!dma_tx_desc) > >+ goto xmit_error_prep; > >+ > >+ axi_skb->skb = skb; > >+ axi_skb->sg_len = sg_len; > >+ dma_tx_desc->callback_param = axi_skb; > >+ 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_prep: > >+ dma_unmap_sg(lp->dev, axi_skb->sgl, sg_len, DMA_TO_DEVICE); > >+xmit_error_skb_sgvec: > >+ kmem_cache_free(lp->skb_cache, axi_skb); > >+ return NETDEV_TX_BUSY; > >+} > >+ > > /** > > * axienet_tx_poll - Invoked once a transmit is completed by the > > * Axi DMA Tx channel. > >@@ -912,7 +1030,42 @@ axienet_start_xmit(struct sk_buff *skb, struct > >net_device *ndev) > > if (!AXIENET_USE_DMA(lp)) > > return axienet_start_xmit_legacy(skb, ndev); > > else > >- return NETDEV_TX_BUSY; > >+ return axienet_start_xmit_dmaengine(skb, ndev); > > You can avoid this if else by > if (AXIENET_USE_DMA(lp)) > return axienet_start_xmit_dmaengine(skb, ndev); > > and no need of defining axienet_start_xmit_legacy function in patch 2/3. > _legacy may not be correct since you support both in-built dma or with > dma engine just by turning on/off dt properties. Also does this driver roll > back to using in-built dma if DMA_ENGINE is not compiled in? We wanted to separate it to support future maintenance or deprecation with a separate wrapper. It is only a preference and either approach is ok. There is no fallback to built in DMA. The intention is to use dmaengine as the preferred approach and eventually deprecate built-in dma. If DMA_ENGINE is not present, the dma channel request fails through the usual exit path. Will address remaining review comments of 2/3 and 3/3 in the next Version. Thanks, Sarath
On 11/05/2023 13:32, Gaddam, Sarath Babu Naidu wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Wednesday, May 10, 2023 3:39 PM >> To: Gaddam, Sarath Babu Naidu >> <sarath.babu.naidu.gaddam@amd.com>; davem@davemloft.net; >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org >> Cc: linux@armlinux.org.uk; Simek, Michal <michal.simek@amd.com>; >> Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; >> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sarangi, >> Anirudha <anirudha.sarangi@amd.com>; Katakam, Harini >> <harini.katakam@amd.com>; git (AMD-Xilinx) <git@amd.com> >> Subject: Re: [PATCH net-next V3 1/3] dt-bindings: net: xilinx_axienet: >> Introduce dmaengine binding support >> >> On 10/05/2023 10:50, Sarath Babu Naidu Gaddam wrote: >>> From: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> >>> >>> The axiethernet driver will use dmaengine framework to communicate >>> with dma controller IP instead of built-in dma programming sequence. >> >> Subject: drop second/last, redundant "bindings". The "dt-bindings" >> prefix is already stating that these are bindings. >> >> Actually also drop "dmaenging" as it is Linuxism. Focus on hardware, e.g. >> "Add DMA support". >> >>> >>> To request dma transmit and receive channels the axiethernet driver >>> uses generic dmas, dma-names properties. >>> >>> Also to support the backward compatibility, use "dmas" property to >>> identify as it should use dmaengine framework or legacy >>> driver(built-in dma programming). >>> >>> At this point it is recommended to use dmaengine framework but it's >>> optional. Once the solution is stable will make dmas as required >>> properties. >>> >>> Signed-off-by: Radhey Shyam Pandey >> <radhey.shyam.pandey@xilinx.com> >>> Signed-off-by: Sarath Babu Naidu Gaddam >>> <sarath.babu.naidu.gaddam@amd.com> >>> --- >>> These changes are on top of below txt to yaml conversion discussion >>> https://lore.kernel.org/all/20230308061223.1358637-1- >> sarath.babu.naidu >>> .gaddam@amd.com/#Z2e.:20230308061223.1358637-1- >> sarath.babu.naidu.gadda >>> m::40amd.com:1bindings:net:xlnx::2caxi-ethernet.yaml >>> >>> Changes in V3: >>> 1) Reverted reg and interrupts property to support backward >> compatibility. >>> 2) Moved dmas and dma-names properties from Required properties. >>> >>> Changes in V2: >>> - None. >>> --- >>> .../devicetree/bindings/net/xlnx,axi-ethernet.yaml | 12 >> ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml >>> b/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml >>> index 80843c177029..9dfa1976e260 100644 >>> --- a/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml >>> +++ b/Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml >>> @@ -122,6 +122,16 @@ properties: >>> modes, where "pcs-handle" should be used to point to the PCS/PMA >> PHY, >>> and "phy-handle" should point to an external PHY if exists. >>> >>> + dmas: >>> + items: >>> + - description: TX DMA Channel phandle and DMA request line >> number >>> + - description: RX DMA Channel phandle and DMA request line >>> + number >>> + >>> + dma-names: >>> + items: >>> + - const: tx_chan0 >> >> tx >> >>> + - const: rx_chan0 >> >> rx > > We want to support more channels in the future, currently we support > AXI DMA which has only one tx and rx channel. In future we want to > extend support for multichannel DMA (MCDMA) which has 16 TX and > 16 RX channels. To uniquely identify each channel, we are using chan > suffix. Depending on the usecase AXI ethernet driver can request any > combination of multichannel DMA channels. > > dma-names = tx_chan0, tx_chan1, rx_chan0, rx_chan1; > > will update the commit message with same. I expect the binding to be complete, otherwise you get comments like this. Add missing parts to the binding and resend. Best regards, Krzysztof
On 17/05/2023 14:06, Gaddam, Sarath Babu Naidu wrote: >>>>> + dma-names: >>>>> + items: >>>>> + - const: tx_chan0 >>>> >>>> tx >>>> >>>>> + - const: rx_chan0 >>>> >>>> rx >>> >>> We want to support more channels in the future, currently we support >>> AXI DMA which has only one tx and rx channel. In future we want to >>> extend support for multichannel DMA (MCDMA) which has 16 TX and >>> 16 RX channels. To uniquely identify each channel, we are using chan >>> suffix. Depending on the usecase AXI ethernet driver can request any >>> combination of multichannel DMA channels. >>> >>> dma-names = tx_chan0, tx_chan1, rx_chan0, rx_chan1; >>> >>> will update the commit message with same. >> >> I expect the binding to be complete, otherwise you get comments like this. >> Add missing parts to the binding and resend. > > Binding is complete for current supported DMA (single channel). We will > extend when we add MCDMA. What doe sit mean "current supported DMA"? By driver? or by hardware? If the former, then how does it matter for the bindings? If the latter, then your hardware is going to change? Then you will have different set of compatibles and then can use different names. > > We will describe the reason for using channel suffix in the description as > below. > > dma-names: > items: > - const: tx_chan0 > - const: rx_chan0 > description: | > Chan suffix is used for identifying each channel uniquely. > Current DMA has only one Tx and Rx channel but it will be > extended to support for multichannel DMA (MCDMA) which > has 16 TX and 16 RX channels. Depending on the usecase AXI > ethernet driver can request any combination of multichannel > DMA channels. No, because I don't understand what is "will be extended". Bindings should be complete. If they are going to be extended, it means they are not complete. If they cannot be complete, which happens, please provide a reason. There was no reason so far, except your claim it is complete. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Wednesday, May 17, 2023 8:19 PM > To: Gaddam, Sarath Babu Naidu > <sarath.babu.naidu.gaddam@amd.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org > Cc: linux@armlinux.org.uk; Simek, Michal <michal.simek@amd.com>; > Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sarangi, > Anirudha <anirudha.sarangi@amd.com>; Katakam, Harini > <harini.katakam@amd.com>; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH net-next V3 1/3] dt-bindings: net: xilinx_axienet: > Introduce dmaengine binding support > > On 17/05/2023 14:06, Gaddam, Sarath Babu Naidu wrote: > >>>>> + dma-names: > >>>>> + items: > >>>>> + - const: tx_chan0 > >>>> > >>>> tx > >>>> > >>>>> + - const: rx_chan0 > >>>> > >>>> rx > >>> > >>> We want to support more channels in the future, currently we > support > >>> AXI DMA which has only one tx and rx channel. In future we want to > >>> extend support for multichannel DMA (MCDMA) which has 16 TX and > >>> 16 RX channels. To uniquely identify each channel, we are using chan > >>> suffix. Depending on the usecase AXI ethernet driver can request any > >>> combination of multichannel DMA channels. > >>> > >>> dma-names = tx_chan0, tx_chan1, rx_chan0, rx_chan1; > >>> > >>> will update the commit message with same. > >> > >> I expect the binding to be complete, otherwise you get comments like > this. > >> Add missing parts to the binding and resend. > > > > Binding is complete for current supported DMA (single channel). We > > will extend when we add MCDMA. > > What doe sit mean "current supported DMA"? By driver? or by hardware? > If the former, then how does it matter for the bindings? > > If the latter, then your hardware is going to change? Then you will have > different set of compatibles and then can use different names. > > > > > We will describe the reason for using channel suffix in the > > description as below. > > > > dma-names: > > items: > > - const: tx_chan0 > > - const: rx_chan0 > > description: | > > Chan suffix is used for identifying each channel uniquely. > > Current DMA has only one Tx and Rx channel but it will be > > extended to support for multichannel DMA (MCDMA) which > > has 16 TX and 16 RX channels. Depending on the usecase AXI > > ethernet driver can request any combination of multichannel > > DMA channels. > > No, because I don't understand what is "will be extended". Bindings > should be complete. If they are going to be extended, it means they are > not complete. If they cannot be complete, which happens, please provide > a reason. There was no reason so far, except your claim it is complete. We will re-spin another series with complete bindings including MCDMA support. Thanks, Sarath
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. Backward compatibility support: To support backward compatibility, we are planning to use below approach, 1) Use "dmas" property as an optional for now to differentiate dmaengine based ethernet Driver or built-in dma ethernet driver. Will move this property to required property some time later. 2) after some time, will introduce a new compatible string to support the dmaengine method, This new compatible name will use different APIs for init and data transfer. 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 Changes in V3: 1) Moved RFC to PATCH. 2) Removed ethtool get/set coalesce, will be added later. 3) Added backward comapatible support. 4) 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: 1) Add ethtool get/set coalesce and DMA reset using DMAengine framework. 2) Add performance numbers. 3) Remove .txt and change the name of file to xlnx,axiethernet.yaml. 4) 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 (1): dt-bindings: net: xilinx_axienet: Introduce dmaengine binding support Sarath Babu Naidu Gaddam (2): net: axienet: Preparatory changes for dmaengine support net: axienet: Introduce dmaengine support .../bindings/net/xlnx,axi-ethernet.yaml | 12 + drivers/net/ethernet/xilinx/xilinx_axienet.h | 8 + .../net/ethernet/xilinx/xilinx_axienet_main.c | 636 ++++++++++++++---- 3 files changed, 533 insertions(+), 123 deletions(-)