RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
> -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Tuesday, June 28, 2016 9:45 AM > To: Punnaiah Choudary Kalluri <punn...@xilinx.com> > Cc: Appana Durga Kedareswara Rao <appa...@xilinx.com>; > robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek > <mich...@xilinx.com>; Soren Brinkmann <sor...@xilinx.com>; > dan.j.willi...@intel.com; moritz.fisc...@ettus.com; > laurent.pinch...@ideasonboard.com; l...@debethencourt.com; Srikanth > Vemula <svem...@xilinx.com>; Anirudha Sarangi <anir...@xilinx.com>; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; dmaeng...@vger.kernel.org > Subject: Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine > driver support > > On Tue, Jun 21, 2016 at 05:29:42PM +, Punnaiah Choudary Kalluri wrote: > > > > Ok agree with you for the scenario that I have mentioned above. > > > > Other simple dma mode feature that I missed to explain is configuring the > > Dma descriptors. It provides a register interface for configuring the dma > transaction. > > So, no need to maintain the descriptors in memory and it will be useful for > the system > > that are in crunch of memory. > > And why are these not coming out in the first place, which makes me think it > is fishy! > > Do you mean programming DMA descriptors to hardware and you can use > registers instead of memory? > Yes. > > > > How do you want us to handle this case? > > > > > > Okay I am convince now this is not right approach. Please remove this > > > custom interface and then implement slave for required slave scenarios! > > > > > > > Assume controller is having 8 channels and four of them are used for slave > > Dma and others are for memcpy. > > Controller didn't have the per channel priority control but providing the > rate control > > Mechanism. > > How does the use of few for memcpy and few for slave change things? IMO > it > doesn't, you program the channel accordingly > > > > > So, I need some interface for configuring the rate control per channel at > run time irrespective > > Of whether the channel is allocated for slave dma or memcpy dma. > > why? > This is to prioritize the channel over other channels at runtime. Also, if the slave device doesn't have a flow control implemented, Then rate control is one mechanism for controlling the transactions Between the source and destination. > > Is it wrong having the configurable dma parameters for dma memcpy > operation? We are exposing the > > Hw capabilities to the user for better dma transaction management. > > For memcpy yes. Memcpy is a generic case where people do not do driver > specific stuff. So I tend ot push back on that.. > Ok. Then we will consider using slave dma if the memcpy requires custom Settings (the settings might be for debug purpose or there is real hw design that mandates changes in default optimized settings). > For slave, existing APIs allow you to program the additional parameters.. > FWIW, rate control is a generic parameter which if you justify enough can be > added to dma_slave_config > As said above, rate control will be helpful for the controller that doesn't have Per channel priority option and also cases where slave device/controller that Doesn't have Flow control implemented. Thanks, Punnaiah > Thanks > -- > ~Vinod
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
> -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Tuesday, June 28, 2016 9:45 AM > To: Punnaiah Choudary Kalluri > Cc: Appana Durga Kedareswara Rao ; > robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek > ; Soren Brinkmann ; > dan.j.willi...@intel.com; moritz.fisc...@ettus.com; > laurent.pinch...@ideasonboard.com; l...@debethencourt.com; Srikanth > Vemula ; Anirudha Sarangi ; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; dmaeng...@vger.kernel.org > Subject: Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine > driver support > > On Tue, Jun 21, 2016 at 05:29:42PM +, Punnaiah Choudary Kalluri wrote: > > > > Ok agree with you for the scenario that I have mentioned above. > > > > Other simple dma mode feature that I missed to explain is configuring the > > Dma descriptors. It provides a register interface for configuring the dma > transaction. > > So, no need to maintain the descriptors in memory and it will be useful for > the system > > that are in crunch of memory. > > And why are these not coming out in the first place, which makes me think it > is fishy! > > Do you mean programming DMA descriptors to hardware and you can use > registers instead of memory? > Yes. > > > > How do you want us to handle this case? > > > > > > Okay I am convince now this is not right approach. Please remove this > > > custom interface and then implement slave for required slave scenarios! > > > > > > > Assume controller is having 8 channels and four of them are used for slave > > Dma and others are for memcpy. > > Controller didn't have the per channel priority control but providing the > rate control > > Mechanism. > > How does the use of few for memcpy and few for slave change things? IMO > it > doesn't, you program the channel accordingly > > > > > So, I need some interface for configuring the rate control per channel at > run time irrespective > > Of whether the channel is allocated for slave dma or memcpy dma. > > why? > This is to prioritize the channel over other channels at runtime. Also, if the slave device doesn't have a flow control implemented, Then rate control is one mechanism for controlling the transactions Between the source and destination. > > Is it wrong having the configurable dma parameters for dma memcpy > operation? We are exposing the > > Hw capabilities to the user for better dma transaction management. > > For memcpy yes. Memcpy is a generic case where people do not do driver > specific stuff. So I tend ot push back on that.. > Ok. Then we will consider using slave dma if the memcpy requires custom Settings (the settings might be for debug purpose or there is real hw design that mandates changes in default optimized settings). > For slave, existing APIs allow you to program the additional parameters.. > FWIW, rate control is a generic parameter which if you justify enough can be > added to dma_slave_config > As said above, rate control will be helpful for the controller that doesn't have Per channel priority option and also cases where slave device/controller that Doesn't have Flow control implemented. Thanks, Punnaiah > Thanks > -- > ~Vinod
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Tue, Jun 21, 2016 at 05:29:42PM +, Punnaiah Choudary Kalluri wrote: > > Ok agree with you for the scenario that I have mentioned above. > > Other simple dma mode feature that I missed to explain is configuring the > Dma descriptors. It provides a register interface for configuring the dma > transaction. > So, no need to maintain the descriptors in memory and it will be useful for > the system > that are in crunch of memory. And why are these not coming out in the first place, which makes me think it is fishy! Do you mean programming DMA descriptors to hardware and you can use registers instead of memory? > > How do you want us to handle this case? > > > Okay I am convince now this is not right approach. Please remove this > > custom interface and then implement slave for required slave scenarios! > > > > Assume controller is having 8 channels and four of them are used for slave > Dma and others are for memcpy. > Controller didn't have the per channel priority control but providing the > rate control > Mechanism. How does the use of few for memcpy and few for slave change things? IMO it doesn't, you program the channel accordingly > > So, I need some interface for configuring the rate control per channel at run > time irrespective > Of whether the channel is allocated for slave dma or memcpy dma. why? > Is it wrong having the configurable dma parameters for dma memcpy operation? > We are exposing the > Hw capabilities to the user for better dma transaction management. For memcpy yes. Memcpy is a generic case where people do not do driver specific stuff. So I tend ot push back on that.. For slave, existing APIs allow you to program the additional parameters.. FWIW, rate control is a generic parameter which if you justify enough can be added to dma_slave_config Thanks -- ~Vinod
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Tue, Jun 21, 2016 at 05:29:42PM +, Punnaiah Choudary Kalluri wrote: > > Ok agree with you for the scenario that I have mentioned above. > > Other simple dma mode feature that I missed to explain is configuring the > Dma descriptors. It provides a register interface for configuring the dma > transaction. > So, no need to maintain the descriptors in memory and it will be useful for > the system > that are in crunch of memory. And why are these not coming out in the first place, which makes me think it is fishy! Do you mean programming DMA descriptors to hardware and you can use registers instead of memory? > > How do you want us to handle this case? > > > Okay I am convince now this is not right approach. Please remove this > > custom interface and then implement slave for required slave scenarios! > > > > Assume controller is having 8 channels and four of them are used for slave > Dma and others are for memcpy. > Controller didn't have the per channel priority control but providing the > rate control > Mechanism. How does the use of few for memcpy and few for slave change things? IMO it doesn't, you program the channel accordingly > > So, I need some interface for configuring the rate control per channel at run > time irrespective > Of whether the channel is allocated for slave dma or memcpy dma. why? > Is it wrong having the configurable dma parameters for dma memcpy operation? > We are exposing the > Hw capabilities to the user for better dma transaction management. For memcpy yes. Memcpy is a generic case where people do not do driver specific stuff. So I tend ot push back on that.. For slave, existing APIs allow you to program the additional parameters.. FWIW, rate control is a generic parameter which if you justify enough can be added to dma_slave_config Thanks -- ~Vinod
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod, > -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Tuesday, June 21, 2016 9:11 PM > To: Appana Durga Kedareswara Rao <appa...@xilinx.com> > Cc: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek > <mich...@xilinx.com>; Soren Brinkmann <sor...@xilinx.com>; > dan.j.willi...@intel.com; moritz.fisc...@ettus.com; > laurent.pinch...@ideasonboard.com; l...@debethencourt.com; Srikanth > Vemula <svem...@xilinx.com>; Anirudha Sarangi <anir...@xilinx.com>; > Punnaiah Choudary Kalluri <punn...@xilinx.com>; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; dmaeng...@vger.kernel.org > Subject: Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine > driver support > > On Thu, Jun 16, 2016 at 07:19:38AM +, Appana Durga Kedareswara Rao > wrote: > > Hi Vinod, > > > > Thanks for the review... > > > > > > > > On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara > Rao > > > wrote: > > > > > > Yes it is HW capability. It can be either in simple mode or SG > > > > > > mode Earlier In the driver this configuration is read from the > > > > > > device-tree But as per lars and your suggestion moved it as runtime > config > > > parameters. > > > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > > > I do not think there is point to have this > > > > > > > > You mean always keep the device in SG mode and provide an option > For > > > > simple dma mode if user want to use simple DMA mode?? > > > > > > Yes, why would anyone want to use single if sg is available? > > If you have sg mode always enabled, but sg_list is size 1, that its > essentially a single > True. As we said, simple mode ha few additional features like write only And read only dma. So, if user want then here is the provision. > Btw SPI is a slave case, not a memcpy! > Correct. We are working on slave dma support and will patch to this driver. > > > > There are few features that are available in the simple DMA mode > won't > > > > Available in SG mode like write only DMA , read only DMA mode etc... > > > > > > Can you explain what these are, how they are used by clients? > > > > Currently it is not implemented but there are cases like, > > > > We want to initialize the some region with known value, then write only > dma helps > > We want to read dummy data from the fifo, then read only dma helps. > > Read will be another transfer, perhpas sg lnegth = 1 > In sg case, both source and destination locations need to be configured. In simple dma and read-only mode, only source address is required. Simple Dma just read the data from source location and discard that data. It will save Unnecessary write to destination here. > > Best case is SPI fifo. > > Nope > Why? > > > > > > > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > > > > > can you describe these parameters? > > > > > > ratectl: > > > > > > Rate control can be independently enabled per channel. When rate > > > > > > control is enabled, the DMA channel uses the rate control count to > > > > > > schedule > > > > > successive data read transactions. > > > > > > > > > > And how is this used by client? > > > > > > > > When rate control is enabled, ZDMA channel uses the rate control > count > > > > To schedule successive data read transactions I mean kind of flow > > > > control to schedule Transactions at fixed intervals instead of pumping > > > > the transfers without delay or whenever bus is available > > > > > > > > Rate control count register definition (11:0): > > > > Scheduling interval for SRC AXI transaction, only used if rate control > > > > is enabled > > > > > > Okay, why would anyone want transactions at fixed interval. We are > talking > > > about DMA, so please
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod, > -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Tuesday, June 21, 2016 9:11 PM > To: Appana Durga Kedareswara Rao > Cc: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek > ; Soren Brinkmann ; > dan.j.willi...@intel.com; moritz.fisc...@ettus.com; > laurent.pinch...@ideasonboard.com; l...@debethencourt.com; Srikanth > Vemula ; Anirudha Sarangi ; > Punnaiah Choudary Kalluri ; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; dmaeng...@vger.kernel.org > Subject: Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine > driver support > > On Thu, Jun 16, 2016 at 07:19:38AM +, Appana Durga Kedareswara Rao > wrote: > > Hi Vinod, > > > > Thanks for the review... > > > > > > > > On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara > Rao > > > wrote: > > > > > > Yes it is HW capability. It can be either in simple mode or SG > > > > > > mode Earlier In the driver this configuration is read from the > > > > > > device-tree But as per lars and your suggestion moved it as runtime > config > > > parameters. > > > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > > > I do not think there is point to have this > > > > > > > > You mean always keep the device in SG mode and provide an option > For > > > > simple dma mode if user want to use simple DMA mode?? > > > > > > Yes, why would anyone want to use single if sg is available? > > If you have sg mode always enabled, but sg_list is size 1, that its > essentially a single > True. As we said, simple mode ha few additional features like write only And read only dma. So, if user want then here is the provision. > Btw SPI is a slave case, not a memcpy! > Correct. We are working on slave dma support and will patch to this driver. > > > > There are few features that are available in the simple DMA mode > won't > > > > Available in SG mode like write only DMA , read only DMA mode etc... > > > > > > Can you explain what these are, how they are used by clients? > > > > Currently it is not implemented but there are cases like, > > > > We want to initialize the some region with known value, then write only > dma helps > > We want to read dummy data from the fifo, then read only dma helps. > > Read will be another transfer, perhpas sg lnegth = 1 > In sg case, both source and destination locations need to be configured. In simple dma and read-only mode, only source address is required. Simple Dma just read the data from source location and discard that data. It will save Unnecessary write to destination here. > > Best case is SPI fifo. > > Nope > Why? > > > > > > > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > > > > > can you describe these parameters? > > > > > > ratectl: > > > > > > Rate control can be independently enabled per channel. When rate > > > > > > control is enabled, the DMA channel uses the rate control count to > > > > > > schedule > > > > > successive data read transactions. > > > > > > > > > > And how is this used by client? > > > > > > > > When rate control is enabled, ZDMA channel uses the rate control > count > > > > To schedule successive data read transactions I mean kind of flow > > > > control to schedule Transactions at fixed intervals instead of pumping > > > > the transfers without delay or whenever bus is available > > > > > > > > Rate control count register definition (11:0): > > > > Scheduling interval for SRC AXI transaction, only used if rate control > > > > is enabled > > > > > > Okay, why would anyone want transactions at fixed interval. We are > talking > > > about DMA, so please give me transfers without delay or whenever bus > is > > > available! > > > > Could be that there are certain IPs that requires dela
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
> On Tue, Jun 21, 2016 at 04:19:38PM +, Punnaiah Choudary Kalluri wrote: > > > > > > > > mode Earlier In the driver this configuration is read from the > > > > > > > > device-tree But as per lars and your suggestion moved it as > runtime > > > config > > > > > parameters. > > > > > > > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > > > > > > > I do not think there is point to have this > > > > > > > > > > > > You mean always keep the device in SG mode and provide an > option > > > For > > > > > > simple dma mode if user want to use simple DMA mode?? > > > > > > > > > > Yes, why would anyone want to use single if sg is available? > > > > > > If you have sg mode always enabled, but sg_list is size 1, that its > > > essentially a single > > > > > > > True. As we said, simple mode ha few additional features like write only > > And read only dma. So, if user want then here is the provision. > > > > > Btw SPI is a slave case, not a memcpy! > > > > > > > Correct. We are working on slave dma support and will patch to this driver. > > And in slave cases, you can use slave config to pass the values which are > required, so we dont need this custome interface! > Ok agree with you for the scenario that I have mentioned above. Other simple dma mode feature that I missed to explain is configuring the Dma descriptors. It provides a register interface for configuring the dma transaction. So, no need to maintain the descriptors in memory and it will be useful for the system that are in crunch of memory. How do you want us to handle this case? > > > > > > > > > > > > > > > > > > > > > > src_issue: > > > > > > > > Tells outstanding transaction on SRC. > > > > > > > > > > > > > > This should be read only then, right? > > > > > > > > > > > > It is a Read/Write register > > > > > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq- > > > ultrascal > > > > > > e-registers.html By default it is configured for Max transactions. > > > > > > If user want to limit it they can limit it using this config option. > > > > > > > > > > And why would they want that? > > > > > > > > Could be that there are certain IPs that my not support burst operations > or > > > > May not support all burst lengths it will be helpful. > > > > Since IP is providing the feature, we are exploring it to the user. > > > > > > > > > > > > > > > > > Burst_len: > > > > > > > > Configures the burst length of the src and dst transfers... > > > > > > > > > > > > > > Hmmm, but you are on memcpy, so that should be programmed > for > > > > > throughput? > > > > > > > > > > > > Yes... > > > > > > > > > > So max burst lengths then, why would anyone configure lesser? > > > > > > > > Depends on the destination and source IPs. > > > > > > Not for memory copy > > > > > > > Depends on the system and how source and destination IPs are > interconnected. > > Sometimes there is a limitation from interconnection also. > > Also some flash controllers providing linear access to NAND or NOR > memories may > > Have limitation in burst length but still user can use memory copy with > desired burst > > Length. > > Some of those cases are treated as slave for obvious reasons. > > Please check existing solutions before inventing new ones. Everyone thinks > that their hardware and thereby driver is a novel case, but on closer > inspection that is usually not the case, different falvour yes, novel no! > > Okay I am convince now this is not right approach. Please remove this > custom interface and then implement slave for required slave scenarios! > Assume controller is having 8 channels and four of them are used for slave Dma and others are for memcpy. Controller didn't have the per channel priority control but providing the rate control Mechanism. So, I need some interface for configuring the rate control per channel at run time irrespective Of whether the channel is allocated for slave dma or memcpy dma. Is it wrong having the configurable dma parameters for dma memcpy operation? We are exposing the Hw capabilities to the user for better dma transaction management. > > > > > > This email and any attachments are intended for the sole use of the named > recipient(s) and contain(s) confidential information that may be proprietary, > privileged or copyrighted under applicable law. If you are not the intended > recipient, do not read, copy, or forward this email message or any > attachments. Delete this email message and any attachments immediately. > > Really! > My apologies :) Thanks, Punnaiah > > -- > ~Vinod
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
> On Tue, Jun 21, 2016 at 04:19:38PM +, Punnaiah Choudary Kalluri wrote: > > > > > > > > mode Earlier In the driver this configuration is read from the > > > > > > > > device-tree But as per lars and your suggestion moved it as > runtime > > > config > > > > > parameters. > > > > > > > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > > > > > > > I do not think there is point to have this > > > > > > > > > > > > You mean always keep the device in SG mode and provide an > option > > > For > > > > > > simple dma mode if user want to use simple DMA mode?? > > > > > > > > > > Yes, why would anyone want to use single if sg is available? > > > > > > If you have sg mode always enabled, but sg_list is size 1, that its > > > essentially a single > > > > > > > True. As we said, simple mode ha few additional features like write only > > And read only dma. So, if user want then here is the provision. > > > > > Btw SPI is a slave case, not a memcpy! > > > > > > > Correct. We are working on slave dma support and will patch to this driver. > > And in slave cases, you can use slave config to pass the values which are > required, so we dont need this custome interface! > Ok agree with you for the scenario that I have mentioned above. Other simple dma mode feature that I missed to explain is configuring the Dma descriptors. It provides a register interface for configuring the dma transaction. So, no need to maintain the descriptors in memory and it will be useful for the system that are in crunch of memory. How do you want us to handle this case? > > > > > > > > > > > > > > > > > > > > > > src_issue: > > > > > > > > Tells outstanding transaction on SRC. > > > > > > > > > > > > > > This should be read only then, right? > > > > > > > > > > > > It is a Read/Write register > > > > > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq- > > > ultrascal > > > > > > e-registers.html By default it is configured for Max transactions. > > > > > > If user want to limit it they can limit it using this config option. > > > > > > > > > > And why would they want that? > > > > > > > > Could be that there are certain IPs that my not support burst operations > or > > > > May not support all burst lengths it will be helpful. > > > > Since IP is providing the feature, we are exploring it to the user. > > > > > > > > > > > > > > > > > Burst_len: > > > > > > > > Configures the burst length of the src and dst transfers... > > > > > > > > > > > > > > Hmmm, but you are on memcpy, so that should be programmed > for > > > > > throughput? > > > > > > > > > > > > Yes... > > > > > > > > > > So max burst lengths then, why would anyone configure lesser? > > > > > > > > Depends on the destination and source IPs. > > > > > > Not for memory copy > > > > > > > Depends on the system and how source and destination IPs are > interconnected. > > Sometimes there is a limitation from interconnection also. > > Also some flash controllers providing linear access to NAND or NOR > memories may > > Have limitation in burst length but still user can use memory copy with > desired burst > > Length. > > Some of those cases are treated as slave for obvious reasons. > > Please check existing solutions before inventing new ones. Everyone thinks > that their hardware and thereby driver is a novel case, but on closer > inspection that is usually not the case, different falvour yes, novel no! > > Okay I am convince now this is not right approach. Please remove this > custom interface and then implement slave for required slave scenarios! > Assume controller is having 8 channels and four of them are used for slave Dma and others are for memcpy. Controller didn't have the per channel priority control but providing the rate control Mechanism. So, I need some interface for configuring the rate control per channel at run time irrespective Of whether the channel is allocated for slave dma or memcpy dma. Is it wrong having the configurable dma parameters for dma memcpy operation? We are exposing the Hw capabilities to the user for better dma transaction management. > > > > > > This email and any attachments are intended for the sole use of the named > recipient(s) and contain(s) confidential information that may be proprietary, > privileged or copyrighted under applicable law. If you are not the intended > recipient, do not read, copy, or forward this email message or any > attachments. Delete this email message and any attachments immediately. > > Really! > My apologies :) Thanks, Punnaiah > > -- > ~Vinod
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Tue, Jun 21, 2016 at 04:19:38PM +, Punnaiah Choudary Kalluri wrote: > > > > > > > mode Earlier In the driver this configuration is read from the > > > > > > > device-tree But as per lars and your suggestion moved it as > > > > > > > runtime > > config > > > > parameters. > > > > > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > > > > > I do not think there is point to have this > > > > > > > > > > You mean always keep the device in SG mode and provide an option > > For > > > > > simple dma mode if user want to use simple DMA mode?? > > > > > > > > Yes, why would anyone want to use single if sg is available? > > > > If you have sg mode always enabled, but sg_list is size 1, that its > > essentially a single > > > > True. As we said, simple mode ha few additional features like write only > And read only dma. So, if user want then here is the provision. > > > Btw SPI is a slave case, not a memcpy! > > > > Correct. We are working on slave dma support and will patch to this driver. And in slave cases, you can use slave config to pass the values which are required, so we dont need this custome interface! > > > > > > There are few features that are available in the simple DMA mode > > won't > > > > > Available in SG mode like write only DMA , read only DMA mode etc... > > > > > > > > Can you explain what these are, how they are used by clients? > > > > > > Currently it is not implemented but there are cases like, > > > > > > We want to initialize the some region with known value, then write only > > dma helps > > > We want to read dummy data from the fifo, then read only dma helps. > > > > Read will be another transfer, perhpas sg lnegth = 1 > > > > > In sg case, both source and destination locations need to be configured. > In simple dma and read-only mode, only source address is required. Simple > Dma just read the data from source location and discard that data. It will > save > Unnecessary write to destination here. > > > > Best case is SPI fifo. > > > > Nope > > > > Why? beacuse you are messing with APIs. SPI will be slave where we know how to program the values! > > > > > > > > > > > > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > > > > > > > can you describe these parameters? > > > > > > > ratectl: > > > > > > > Rate control can be independently enabled per channel. When rate > > > > > > > control is enabled, the DMA channel uses the rate control count to > > > > > > > schedule > > > > > > successive data read transactions. > > > > > > > > > > > > And how is this used by client? > > > > > > > > > > When rate control is enabled, ZDMA channel uses the rate control > > count > > > > > To schedule successive data read transactions I mean kind of flow > > > > > control to schedule Transactions at fixed intervals instead of pumping > > > > > the transfers without delay or whenever bus is available > > > > > > > > > > Rate control count register definition (11:0): > > > > > Scheduling interval for SRC AXI transaction, only used if rate control > > > > > is enabled > > > > > > > > Okay, why would anyone want transactions at fixed interval. We are > > talking > > > > about DMA, so please give me transfers without delay or whenever bus > > is > > > > available! > > > > > > Could be that there are certain IPs that requires delay in b/w > > > transactions. > > > Since IP is providing this feature we are exploring it to the user. > > > > I kind of don't agree to that! > > > > It's a kind of rate control mechanism. We can aslo use this feature to > prioritize other > Channel by deliberately controlling the current channel transfer scheduling. > > > > > > > > > > > > > > > > > > > src_issue: > > > > > > > Tells outstanding transaction on SRC. > > > > > > > > > > > > This should be read only then, right? > > > > > > > > > > It is a Read/Write register > > > > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq- > > ultrascal > > > > > e-registers.html By default it is configured for Max transactions. > > > > > If user want to limit it they can limit it using this config option. > > > > > > > > And why would they want that? > > > > > > Could be that there are certain IPs that my not support burst operations > > > or > > > May not support all burst lengths it will be helpful. > > > Since IP is providing the feature, we are exploring it to the user. > > > > > > > > > > > > > > Burst_len: > > > > > > > Configures the burst length of the src and dst transfers... > > > > > > > > > > > > Hmmm, but you are on memcpy, so that should be programmed for > > > > throughput? > > > > > > > > > > Yes... > > > > > > > > So max burst lengths then, why would anyone configure lesser? > > > > > > Depends on the
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Tue, Jun 21, 2016 at 04:19:38PM +, Punnaiah Choudary Kalluri wrote: > > > > > > > mode Earlier In the driver this configuration is read from the > > > > > > > device-tree But as per lars and your suggestion moved it as > > > > > > > runtime > > config > > > > parameters. > > > > > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > > > > > I do not think there is point to have this > > > > > > > > > > You mean always keep the device in SG mode and provide an option > > For > > > > > simple dma mode if user want to use simple DMA mode?? > > > > > > > > Yes, why would anyone want to use single if sg is available? > > > > If you have sg mode always enabled, but sg_list is size 1, that its > > essentially a single > > > > True. As we said, simple mode ha few additional features like write only > And read only dma. So, if user want then here is the provision. > > > Btw SPI is a slave case, not a memcpy! > > > > Correct. We are working on slave dma support and will patch to this driver. And in slave cases, you can use slave config to pass the values which are required, so we dont need this custome interface! > > > > > > There are few features that are available in the simple DMA mode > > won't > > > > > Available in SG mode like write only DMA , read only DMA mode etc... > > > > > > > > Can you explain what these are, how they are used by clients? > > > > > > Currently it is not implemented but there are cases like, > > > > > > We want to initialize the some region with known value, then write only > > dma helps > > > We want to read dummy data from the fifo, then read only dma helps. > > > > Read will be another transfer, perhpas sg lnegth = 1 > > > > > In sg case, both source and destination locations need to be configured. > In simple dma and read-only mode, only source address is required. Simple > Dma just read the data from source location and discard that data. It will > save > Unnecessary write to destination here. > > > > Best case is SPI fifo. > > > > Nope > > > > Why? beacuse you are messing with APIs. SPI will be slave where we know how to program the values! > > > > > > > > > > > > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > > > > > > > can you describe these parameters? > > > > > > > ratectl: > > > > > > > Rate control can be independently enabled per channel. When rate > > > > > > > control is enabled, the DMA channel uses the rate control count to > > > > > > > schedule > > > > > > successive data read transactions. > > > > > > > > > > > > And how is this used by client? > > > > > > > > > > When rate control is enabled, ZDMA channel uses the rate control > > count > > > > > To schedule successive data read transactions I mean kind of flow > > > > > control to schedule Transactions at fixed intervals instead of pumping > > > > > the transfers without delay or whenever bus is available > > > > > > > > > > Rate control count register definition (11:0): > > > > > Scheduling interval for SRC AXI transaction, only used if rate control > > > > > is enabled > > > > > > > > Okay, why would anyone want transactions at fixed interval. We are > > talking > > > > about DMA, so please give me transfers without delay or whenever bus > > is > > > > available! > > > > > > Could be that there are certain IPs that requires delay in b/w > > > transactions. > > > Since IP is providing this feature we are exploring it to the user. > > > > I kind of don't agree to that! > > > > It's a kind of rate control mechanism. We can aslo use this feature to > prioritize other > Channel by deliberately controlling the current channel transfer scheduling. > > > > > > > > > > > > > > > > > > > src_issue: > > > > > > > Tells outstanding transaction on SRC. > > > > > > > > > > > > This should be read only then, right? > > > > > > > > > > It is a Read/Write register > > > > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq- > > ultrascal > > > > > e-registers.html By default it is configured for Max transactions. > > > > > If user want to limit it they can limit it using this config option. > > > > > > > > And why would they want that? > > > > > > Could be that there are certain IPs that my not support burst operations > > > or > > > May not support all burst lengths it will be helpful. > > > Since IP is providing the feature, we are exploring it to the user. > > > > > > > > > > > > > > Burst_len: > > > > > > > Configures the burst length of the src and dst transfers... > > > > > > > > > > > > Hmmm, but you are on memcpy, so that should be programmed for > > > > throughput? > > > > > > > > > > Yes... > > > > > > > > So max burst lengths then, why would anyone configure lesser? > > > > > > Depends on the
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Thu, Jun 16, 2016 at 07:19:38AM +, Appana Durga Kedareswara Rao wrote: > Hi Vinod, > > Thanks for the review... > > > > > On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara Rao > > wrote: > > > > > Yes it is HW capability. It can be either in simple mode or SG > > > > > mode Earlier In the driver this configuration is read from the > > > > > device-tree But as per lars and your suggestion moved it as runtime > > > > > config > > parameters. > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > I do not think there is point to have this > > > > > > You mean always keep the device in SG mode and provide an option For > > > simple dma mode if user want to use simple DMA mode?? > > > > Yes, why would anyone want to use single if sg is available? If you have sg mode always enabled, but sg_list is size 1, that its essentially a single Btw SPI is a slave case, not a memcpy! > > > There are few features that are available in the simple DMA mode won't > > > Available in SG mode like write only DMA , read only DMA mode etc... > > > > Can you explain what these are, how they are used by clients? > > Currently it is not implemented but there are cases like, > > We want to initialize the some region with known value, then write only dma > helps > We want to read dummy data from the fifo, then read only dma helps. Read will be another transfer, perhpas sg lnegth = 1 > Best case is SPI fifo. Nope > > > > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > > > can you describe these parameters? > > > > > ratectl: > > > > > Rate control can be independently enabled per channel. When rate > > > > > control is enabled, the DMA channel uses the rate control count to > > > > > schedule > > > > successive data read transactions. > > > > > > > > And how is this used by client? > > > > > > When rate control is enabled, ZDMA channel uses the rate control count > > > To schedule successive data read transactions I mean kind of flow > > > control to schedule Transactions at fixed intervals instead of pumping > > > the transfers without delay or whenever bus is available > > > > > > Rate control count register definition (11:0): > > > Scheduling interval for SRC AXI transaction, only used if rate control > > > is enabled > > > > Okay, why would anyone want transactions at fixed interval. We are talking > > about DMA, so please give me transfers without delay or whenever bus is > > available! > > Could be that there are certain IPs that requires delay in b/w transactions. > Since IP is providing this feature we are exploring it to the user. I kind of don't agree to that! > > > > > > > > > > src_issue: > > > > > Tells outstanding transaction on SRC. > > > > > > > > This should be read only then, right? > > > > > > It is a Read/Write register > > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascal > > > e-registers.html By default it is configured for Max transactions. > > > If user want to limit it they can limit it using this config option. > > > > And why would they want that? > > Could be that there are certain IPs that my not support burst operations or > May not support all burst lengths it will be helpful. > Since IP is providing the feature, we are exploring it to the user. > > > > > > > > Burst_len: > > > > > Configures the burst length of the src and dst transfers... > > > > > > > > Hmmm, but you are on memcpy, so that should be programmed for > > throughput? > > > > > > Yes... > > > > So max burst lengths then, why would anyone configure lesser? > > Depends on the destination and source IPs. Not for memory copy > > > > > > > > > How would a client know how to configure them? > > > > > > > > > > With the default values of the config parameters driver will work. > > > > > > > > But how will client know what is default! > > > > > > Default values means IP default state after reset. > > > If user not aware of the above parameters also still the driver will work > > > for > > basic functionality. > > > Do you want me to implement one more API get_config so that Whenever > > > user will call the get_config he will know the default values Of the > > > config parameters? > > > > Looking at above I think we do not warrant programming them. Assume defaults > > in your driver for performance. Like max burst lengths, Max transactions, > > without delay scheduling. > > > > If you disagree, which is fine, please provide the cases where a client > > would > > need to program these to different values. > > As said above there are use cases for SPI and others but currently we didn't > tested > It. > > > > > > > > If user has specific requirement to change these parameters they > > > > > can pass It to the driver
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Thu, Jun 16, 2016 at 07:19:38AM +, Appana Durga Kedareswara Rao wrote: > Hi Vinod, > > Thanks for the review... > > > > > On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara Rao > > wrote: > > > > > Yes it is HW capability. It can be either in simple mode or SG > > > > > mode Earlier In the driver this configuration is read from the > > > > > device-tree But as per lars and your suggestion moved it as runtime > > > > > config > > parameters. > > > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > > > I do not think there is point to have this > > > > > > You mean always keep the device in SG mode and provide an option For > > > simple dma mode if user want to use simple DMA mode?? > > > > Yes, why would anyone want to use single if sg is available? If you have sg mode always enabled, but sg_list is size 1, that its essentially a single Btw SPI is a slave case, not a memcpy! > > > There are few features that are available in the simple DMA mode won't > > > Available in SG mode like write only DMA , read only DMA mode etc... > > > > Can you explain what these are, how they are used by clients? > > Currently it is not implemented but there are cases like, > > We want to initialize the some region with known value, then write only dma > helps > We want to read dummy data from the fifo, then read only dma helps. Read will be another transfer, perhpas sg lnegth = 1 > Best case is SPI fifo. Nope > > > > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > > > can you describe these parameters? > > > > > ratectl: > > > > > Rate control can be independently enabled per channel. When rate > > > > > control is enabled, the DMA channel uses the rate control count to > > > > > schedule > > > > successive data read transactions. > > > > > > > > And how is this used by client? > > > > > > When rate control is enabled, ZDMA channel uses the rate control count > > > To schedule successive data read transactions I mean kind of flow > > > control to schedule Transactions at fixed intervals instead of pumping > > > the transfers without delay or whenever bus is available > > > > > > Rate control count register definition (11:0): > > > Scheduling interval for SRC AXI transaction, only used if rate control > > > is enabled > > > > Okay, why would anyone want transactions at fixed interval. We are talking > > about DMA, so please give me transfers without delay or whenever bus is > > available! > > Could be that there are certain IPs that requires delay in b/w transactions. > Since IP is providing this feature we are exploring it to the user. I kind of don't agree to that! > > > > > > > > > > src_issue: > > > > > Tells outstanding transaction on SRC. > > > > > > > > This should be read only then, right? > > > > > > It is a Read/Write register > > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascal > > > e-registers.html By default it is configured for Max transactions. > > > If user want to limit it they can limit it using this config option. > > > > And why would they want that? > > Could be that there are certain IPs that my not support burst operations or > May not support all burst lengths it will be helpful. > Since IP is providing the feature, we are exploring it to the user. > > > > > > > > Burst_len: > > > > > Configures the burst length of the src and dst transfers... > > > > > > > > Hmmm, but you are on memcpy, so that should be programmed for > > throughput? > > > > > > Yes... > > > > So max burst lengths then, why would anyone configure lesser? > > Depends on the destination and source IPs. Not for memory copy > > > > > > > > > How would a client know how to configure them? > > > > > > > > > > With the default values of the config parameters driver will work. > > > > > > > > But how will client know what is default! > > > > > > Default values means IP default state after reset. > > > If user not aware of the above parameters also still the driver will work > > > for > > basic functionality. > > > Do you want me to implement one more API get_config so that Whenever > > > user will call the get_config he will know the default values Of the > > > config parameters? > > > > Looking at above I think we do not warrant programming them. Assume defaults > > in your driver for performance. Like max burst lengths, Max transactions, > > without delay scheduling. > > > > If you disagree, which is fine, please provide the cases where a client > > would > > need to program these to different values. > > As said above there are use cases for SPI and others but currently we didn't > tested > It. > > > > > > > > If user has specific requirement to change these parameters they > > > > > can pass It to the driver
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod, Thanks for the review... > > On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara Rao > wrote: > > > > Yes it is HW capability. It can be either in simple mode or SG > > > > mode Earlier In the driver this configuration is read from the > > > > device-tree But as per lars and your suggestion moved it as runtime > > > > config > parameters. > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > I do not think there is point to have this > > > > You mean always keep the device in SG mode and provide an option For > > simple dma mode if user want to use simple DMA mode?? > > Yes, why would anyone want to use single if sg is available? > > > > > There are few features that are available in the simple DMA mode won't > > Available in SG mode like write only DMA , read only DMA mode etc... > > Can you explain what these are, how they are used by clients? Currently it is not implemented but there are cases like, We want to initialize the some region with known value, then write only dma helps We want to read dummy data from the fifo, then read only dma helps. Best case is SPI fifo. > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > can you describe these parameters? > > > > ratectl: > > > > Rate control can be independently enabled per channel. When rate > > > > control is enabled, the DMA channel uses the rate control count to > > > > schedule > > > successive data read transactions. > > > > > > And how is this used by client? > > > > When rate control is enabled, ZDMA channel uses the rate control count > > To schedule successive data read transactions I mean kind of flow > > control to schedule Transactions at fixed intervals instead of pumping > > the transfers without delay or whenever bus is available > > > > Rate control count register definition (11:0): > > Scheduling interval for SRC AXI transaction, only used if rate control > > is enabled > > Okay, why would anyone want transactions at fixed interval. We are talking > about DMA, so please give me transfers without delay or whenever bus is > available! Could be that there are certain IPs that requires delay in b/w transactions. Since IP is providing this feature we are exploring it to the user. > > > > > > src_issue: > > > > Tells outstanding transaction on SRC. > > > > > > This should be read only then, right? > > > > It is a Read/Write register > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascal > > e-registers.html By default it is configured for Max transactions. > > If user want to limit it they can limit it using this config option. > > And why would they want that? Could be that there are certain IPs that my not support burst operations or May not support all burst lengths it will be helpful. Since IP is providing the feature, we are exploring it to the user. > > > > > Burst_len: > > > > Configures the burst length of the src and dst transfers... > > > > > > Hmmm, but you are on memcpy, so that should be programmed for > throughput? > > > > Yes... > > So max burst lengths then, why would anyone configure lesser? Depends on the destination and source IPs. > > > > > > How would a client know how to configure them? > > > > > > > > With the default values of the config parameters driver will work. > > > > > > But how will client know what is default! > > > > Default values means IP default state after reset. > > If user not aware of the above parameters also still the driver will work > > for > basic functionality. > > Do you want me to implement one more API get_config so that Whenever > > user will call the get_config he will know the default values Of the > > config parameters? > > Looking at above I think we do not warrant programming them. Assume defaults > in your driver for performance. Like max burst lengths, Max transactions, > without delay scheduling. > > If you disagree, which is fine, please provide the cases where a client would > need to program these to different values. As said above there are use cases for SPI and others but currently we didn't tested It. > > > > > If user has specific requirement to change these parameters they > > > > can pass It to the driver using set_config API and all these > > > > parameters are Documented in the include/linux/dma/xilinx_dma.h file... > > > > > > Can you give me an example where user would like to do that > > > > I am using customized dma test client. > > There I am calling this set_config API before triggering memcpy/SG > operations. > > Well that is precisely a problem. The generic applications wont know "your" > custom API and will not configure that! As said above, generic application can work with default values. These are not custom parameters and we can say these
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod, Thanks for the review... > > On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara Rao > wrote: > > > > Yes it is HW capability. It can be either in simple mode or SG > > > > mode Earlier In the driver this configuration is read from the > > > > device-tree But as per lars and your suggestion moved it as runtime > > > > config > parameters. > > > > > > If sg mode is available why would anyone _not_ want it? > > > > > > I do not think there is point to have this > > > > You mean always keep the device in SG mode and provide an option For > > simple dma mode if user want to use simple DMA mode?? > > Yes, why would anyone want to use single if sg is available? > > > > > There are few features that are available in the simple DMA mode won't > > Available in SG mode like write only DMA , read only DMA mode etc... > > Can you explain what these are, how they are used by clients? Currently it is not implemented but there are cases like, We want to initialize the some region with known value, then write only dma helps We want to read dummy data from the fifo, then read only dma helps. Best case is SPI fifo. > > > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > > > can you describe these parameters? > > > > ratectl: > > > > Rate control can be independently enabled per channel. When rate > > > > control is enabled, the DMA channel uses the rate control count to > > > > schedule > > > successive data read transactions. > > > > > > And how is this used by client? > > > > When rate control is enabled, ZDMA channel uses the rate control count > > To schedule successive data read transactions I mean kind of flow > > control to schedule Transactions at fixed intervals instead of pumping > > the transfers without delay or whenever bus is available > > > > Rate control count register definition (11:0): > > Scheduling interval for SRC AXI transaction, only used if rate control > > is enabled > > Okay, why would anyone want transactions at fixed interval. We are talking > about DMA, so please give me transfers without delay or whenever bus is > available! Could be that there are certain IPs that requires delay in b/w transactions. Since IP is providing this feature we are exploring it to the user. > > > > > > src_issue: > > > > Tells outstanding transaction on SRC. > > > > > > This should be read only then, right? > > > > It is a Read/Write register > > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascal > > e-registers.html By default it is configured for Max transactions. > > If user want to limit it they can limit it using this config option. > > And why would they want that? Could be that there are certain IPs that my not support burst operations or May not support all burst lengths it will be helpful. Since IP is providing the feature, we are exploring it to the user. > > > > > Burst_len: > > > > Configures the burst length of the src and dst transfers... > > > > > > Hmmm, but you are on memcpy, so that should be programmed for > throughput? > > > > Yes... > > So max burst lengths then, why would anyone configure lesser? Depends on the destination and source IPs. > > > > > > How would a client know how to configure them? > > > > > > > > With the default values of the config parameters driver will work. > > > > > > But how will client know what is default! > > > > Default values means IP default state after reset. > > If user not aware of the above parameters also still the driver will work > > for > basic functionality. > > Do you want me to implement one more API get_config so that Whenever > > user will call the get_config he will know the default values Of the > > config parameters? > > Looking at above I think we do not warrant programming them. Assume defaults > in your driver for performance. Like max burst lengths, Max transactions, > without delay scheduling. > > If you disagree, which is fine, please provide the cases where a client would > need to program these to different values. As said above there are use cases for SPI and others but currently we didn't tested It. > > > > > If user has specific requirement to change these parameters they > > > > can pass It to the driver using set_config API and all these > > > > parameters are Documented in the include/linux/dma/xilinx_dma.h file... > > > > > > Can you give me an example where user would like to do that > > > > I am using customized dma test client. > > There I am calling this set_config API before triggering memcpy/SG > operations. > > Well that is precisely a problem. The generic applications wont know "your" > custom API and will not configure that! As said above, generic application can work with default values. These are not custom parameters and we can say these
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara Rao wrote: > > > Yes it is HW capability. It can be either in simple mode or SG mode > > > Earlier In the driver this configuration is read from the device-tree > > > But as per lars and your suggestion moved it as runtime config parameters. > > > > If sg mode is available why would anyone _not_ want it? > > > > I do not think there is point to have this > > You mean always keep the device in SG mode and provide an option > For simple dma mode if user want to use simple DMA mode?? Yes, why would anyone want to use single if sg is available? > > There are few features that are available in the simple DMA mode won't > Available in SG mode like write only DMA , read only DMA mode etc... Can you explain what these are, how they are used by clients? > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > can you describe these parameters? > > > ratectl: > > > Rate control can be independently enabled per channel. When rate > > > control is enabled, the DMA channel uses the rate control count to > > > schedule > > successive data read transactions. > > > > And how is this used by client? > > When rate control is enabled, ZDMA channel uses the rate control count > To schedule successive data read transactions I mean kind of flow control to > schedule > Transactions at fixed intervals instead of pumping the transfers without > delay or whenever bus is available > > Rate control count register definition (11:0): > Scheduling interval for SRC AXI transaction, only used if rate control is > enabled Okay, why would anyone want transactions at fixed interval. We are talking about DMA, so please give me transfers without delay or whenever bus is available! > > > src_issue: > > > Tells outstanding transaction on SRC. > > > > This should be read only then, right? > > It is a Read/Write register > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html > > By default it is configured for Max transactions. > If user want to limit it they can limit it using this config option. And why would they want that? > > > Burst_len: > > > Configures the burst length of the src and dst transfers... > > > > Hmmm, but you are on memcpy, so that should be programmed for throughput? > > Yes... So max burst lengths then, why would anyone configure lesser? > > > > How would a client know how to configure them? > > > > > > With the default values of the config parameters driver will work. > > > > But how will client know what is default! > > Default values means IP default state after reset. > If user not aware of the above parameters also still the driver will work for > basic functionality. > Do you want me to implement one more API get_config so that > Whenever user will call the get_config he will know the default values > Of the config parameters? Looking at above I think we do not warrant programming them. Assume defaults in your driver for performance. Like max burst lengths, Max transactions, without delay scheduling. If you disagree, which is fine, please provide the cases where a client would need to program these to different values. > > > If user has specific requirement to change these parameters they can > > > pass It to the driver using set_config API and all these parameters > > > are Documented in the include/linux/dma/xilinx_dma.h file... > > > > Can you give me an example where user would like to do that > > I am using customized dma test client. > There I am calling this set_config API before triggering memcpy/SG operations. Well that is precisely a problem. The generic applications wont know "your" custom API and will not configure that! -- ~Vinod
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Tue, Jun 14, 2016 at 08:18:09AM +, Appana Durga Kedareswara Rao wrote: > > > Yes it is HW capability. It can be either in simple mode or SG mode > > > Earlier In the driver this configuration is read from the device-tree > > > But as per lars and your suggestion moved it as runtime config parameters. > > > > If sg mode is available why would anyone _not_ want it? > > > > I do not think there is point to have this > > You mean always keep the device in SG mode and provide an option > For simple dma mode if user want to use simple DMA mode?? Yes, why would anyone want to use single if sg is available? > > There are few features that are available in the simple DMA mode won't > Available in SG mode like write only DMA , read only DMA mode etc... Can you explain what these are, how they are used by clients? > > > > > + chan->config.ratectrl = cfg->ratectrl; > > > > > + chan->config.src_issue = cfg->src_issue; > > > > > + chan->config.src_burst_len = cfg->src_burst_len; > > > > > + chan->config.dst_burst_len = cfg->dst_burst_len; > > > > > > > > can you describe these parameters? > > > ratectl: > > > Rate control can be independently enabled per channel. When rate > > > control is enabled, the DMA channel uses the rate control count to > > > schedule > > successive data read transactions. > > > > And how is this used by client? > > When rate control is enabled, ZDMA channel uses the rate control count > To schedule successive data read transactions I mean kind of flow control to > schedule > Transactions at fixed intervals instead of pumping the transfers without > delay or whenever bus is available > > Rate control count register definition (11:0): > Scheduling interval for SRC AXI transaction, only used if rate control is > enabled Okay, why would anyone want transactions at fixed interval. We are talking about DMA, so please give me transfers without delay or whenever bus is available! > > > src_issue: > > > Tells outstanding transaction on SRC. > > > > This should be read only then, right? > > It is a Read/Write register > http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html > > By default it is configured for Max transactions. > If user want to limit it they can limit it using this config option. And why would they want that? > > > Burst_len: > > > Configures the burst length of the src and dst transfers... > > > > Hmmm, but you are on memcpy, so that should be programmed for throughput? > > Yes... So max burst lengths then, why would anyone configure lesser? > > > > How would a client know how to configure them? > > > > > > With the default values of the config parameters driver will work. > > > > But how will client know what is default! > > Default values means IP default state after reset. > If user not aware of the above parameters also still the driver will work for > basic functionality. > Do you want me to implement one more API get_config so that > Whenever user will call the get_config he will know the default values > Of the config parameters? Looking at above I think we do not warrant programming them. Assume defaults in your driver for performance. Like max burst lengths, Max transactions, without delay scheduling. If you disagree, which is fine, please provide the cases where a client would need to program these to different values. > > > If user has specific requirement to change these parameters they can > > > pass It to the driver using set_config API and all these parameters > > > are Documented in the include/linux/dma/xilinx_dma.h file... > > > > Can you give me an example where user would like to do that > > I am using customized dma test client. > There I am calling this set_config API before triggering memcpy/SG operations. Well that is precisely a problem. The generic applications wont know "your" custom API and will not configure that! -- ~Vinod
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod,
Thanks for the review...
>
> On Wed, Jun 08, 2016 at 07:40:52AM +, Appana Durga Kedareswara Rao
> wrote:
> > > > +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan
> > > > +*chan, void *desc)
> > >
> > > eod? 80 line?
>
> What's eod?
End of descriptor...
>
> > > > +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> > > > + struct zynqmp_dma_config *cfg) {
> > > > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > > > +
> > > > + chan->config.ovrfetch = cfg->ovrfetch;
> > > > + chan->config.has_sg = cfg->has_sg;
> > >
> > > is this HW capability? if so why would anyone not like to use it!
> >
> > Yes it is HW capability. It can be either in simple mode or SG mode
> > Earlier In the driver this configuration is read from the device-tree
> > But as per lars and your suggestion moved it as runtime config parameters.
>
> If sg mode is available why would anyone _not_ want it?
>
> I do not think there is point to have this
You mean always keep the device in SG mode and provide an option
For simple dma mode if user want to use simple DMA mode??
There are few features that are available in the simple DMA mode won't
Available in SG mode like write only DMA , read only DMA mode etc...
>
> >
> > >
> > > > + chan->config.ratectrl = cfg->ratectrl;
> > > > + chan->config.src_issue = cfg->src_issue;
> > > > + chan->config.src_burst_len = cfg->src_burst_len;
> > > > + chan->config.dst_burst_len = cfg->dst_burst_len;
> > >
> > > can you describe these parameters?
> > ratectl:
> > Rate control can be independently enabled per channel. When rate
> > control is enabled, the DMA channel uses the rate control count to schedule
> successive data read transactions.
>
> And how is this used by client?
When rate control is enabled, ZDMA channel uses the rate control count
To schedule successive data read transactions I mean kind of flow control to
schedule
Transactions at fixed intervals instead of pumping the transfers without delay
or whenever bus is available
Rate control count register definition (11:0):
Scheduling interval for SRC AXI transaction, only used if rate control is
enabled
>
> > src_issue:
> > Tells outstanding transaction on SRC.
>
> This should be read only then, right?
It is a Read/Write register
http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
By default it is configured for Max transactions.
If user want to limit it they can limit it using this config option.
>
> > Burst_len:
> > Configures the burst length of the src and dst transfers...
>
> Hmmm, but you are on memcpy, so that should be programmed for throughput?
Yes...
>
> > >
> > > How would a client know how to configure them?
> >
> > With the default values of the config parameters driver will work.
>
> But how will client know what is default!
Default values means IP default state after reset.
If user not aware of the above parameters also still the driver will work for
basic functionality.
Do you want me to implement one more API get_config so that
Whenever user will call the get_config he will know the default values
Of the config parameters?
>
> > If user has specific requirement to change these parameters they can
> > pass It to the driver using set_config API and all these parameters
> > are Documented in the include/linux/dma/xilinx_dma.h file...
>
> Can you give me an example where user would like to do that
I am using customized dma test client.
There I am calling this set_config API before triggering memcpy/SG operations.
Regards,
Kedar.
>
> --
> ~Vinod
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in the
> body
> of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod,
Thanks for the review...
>
> On Wed, Jun 08, 2016 at 07:40:52AM +, Appana Durga Kedareswara Rao
> wrote:
> > > > +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan
> > > > +*chan, void *desc)
> > >
> > > eod? 80 line?
>
> What's eod?
End of descriptor...
>
> > > > +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> > > > + struct zynqmp_dma_config *cfg) {
> > > > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > > > +
> > > > + chan->config.ovrfetch = cfg->ovrfetch;
> > > > + chan->config.has_sg = cfg->has_sg;
> > >
> > > is this HW capability? if so why would anyone not like to use it!
> >
> > Yes it is HW capability. It can be either in simple mode or SG mode
> > Earlier In the driver this configuration is read from the device-tree
> > But as per lars and your suggestion moved it as runtime config parameters.
>
> If sg mode is available why would anyone _not_ want it?
>
> I do not think there is point to have this
You mean always keep the device in SG mode and provide an option
For simple dma mode if user want to use simple DMA mode??
There are few features that are available in the simple DMA mode won't
Available in SG mode like write only DMA , read only DMA mode etc...
>
> >
> > >
> > > > + chan->config.ratectrl = cfg->ratectrl;
> > > > + chan->config.src_issue = cfg->src_issue;
> > > > + chan->config.src_burst_len = cfg->src_burst_len;
> > > > + chan->config.dst_burst_len = cfg->dst_burst_len;
> > >
> > > can you describe these parameters?
> > ratectl:
> > Rate control can be independently enabled per channel. When rate
> > control is enabled, the DMA channel uses the rate control count to schedule
> successive data read transactions.
>
> And how is this used by client?
When rate control is enabled, ZDMA channel uses the rate control count
To schedule successive data read transactions I mean kind of flow control to
schedule
Transactions at fixed intervals instead of pumping the transfers without delay
or whenever bus is available
Rate control count register definition (11:0):
Scheduling interval for SRC AXI transaction, only used if rate control is
enabled
>
> > src_issue:
> > Tells outstanding transaction on SRC.
>
> This should be read only then, right?
It is a Read/Write register
http://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
By default it is configured for Max transactions.
If user want to limit it they can limit it using this config option.
>
> > Burst_len:
> > Configures the burst length of the src and dst transfers...
>
> Hmmm, but you are on memcpy, so that should be programmed for throughput?
Yes...
>
> > >
> > > How would a client know how to configure them?
> >
> > With the default values of the config parameters driver will work.
>
> But how will client know what is default!
Default values means IP default state after reset.
If user not aware of the above parameters also still the driver will work for
basic functionality.
Do you want me to implement one more API get_config so that
Whenever user will call the get_config he will know the default values
Of the config parameters?
>
> > If user has specific requirement to change these parameters they can
> > pass It to the driver using set_config API and all these parameters
> > are Documented in the include/linux/dma/xilinx_dma.h file...
>
> Can you give me an example where user would like to do that
I am using customized dma test client.
There I am calling this set_config API before triggering memcpy/SG operations.
Regards,
Kedar.
>
> --
> ~Vinod
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in the
> body
> of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Wed, Jun 08, 2016 at 07:40:52AM +, Appana Durga Kedareswara Rao wrote:
> > > +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan *chan,
> > > +void *desc)
> >
> > eod? 80 line?
What's eod?
> > > +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> > > + struct zynqmp_dma_config *cfg)
> > > +{
> > > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > > +
> > > + chan->config.ovrfetch = cfg->ovrfetch;
> > > + chan->config.has_sg = cfg->has_sg;
> >
> > is this HW capability? if so why would anyone not like to use it!
>
> Yes it is HW capability. It can be either in simple mode or SG mode
> Earlier In the driver this configuration is read from the device-tree
> But as per lars and your suggestion moved it as runtime config parameters.
If sg mode is available why would anyone _not_ want it?
I do not think there is point to have this
>
> >
> > > + chan->config.ratectrl = cfg->ratectrl;
> > > + chan->config.src_issue = cfg->src_issue;
> > > + chan->config.src_burst_len = cfg->src_burst_len;
> > > + chan->config.dst_burst_len = cfg->dst_burst_len;
> >
> > can you describe these parameters?
> ratectl:
> Rate control can be independently enabled per channel. When rate control is
> enabled, the
> DMA channel uses the rate control count to schedule successive data read
> transactions.
And how is this used by client?
> src_issue:
> Tells outstanding transaction on SRC.
This should be read only then, right?
> Burst_len:
> Configures the burst length of the src and dst transfers...
Hmmm, but you are on memcpy, so that should be programmed for throughput?
> >
> > How would a client know how to configure them?
>
> With the default values of the config parameters driver will work.
But how will client know what is default!
> If user has specific requirement to change these parameters they can pass
> It to the driver using set_config API and all these parameters are
> Documented in the include/linux/dma/xilinx_dma.h file...
Can you give me an example where user would like to do that
--
~Vinod
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Wed, Jun 08, 2016 at 07:40:52AM +, Appana Durga Kedareswara Rao wrote:
> > > +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan *chan,
> > > +void *desc)
> >
> > eod? 80 line?
What's eod?
> > > +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> > > + struct zynqmp_dma_config *cfg)
> > > +{
> > > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > > +
> > > + chan->config.ovrfetch = cfg->ovrfetch;
> > > + chan->config.has_sg = cfg->has_sg;
> >
> > is this HW capability? if so why would anyone not like to use it!
>
> Yes it is HW capability. It can be either in simple mode or SG mode
> Earlier In the driver this configuration is read from the device-tree
> But as per lars and your suggestion moved it as runtime config parameters.
If sg mode is available why would anyone _not_ want it?
I do not think there is point to have this
>
> >
> > > + chan->config.ratectrl = cfg->ratectrl;
> > > + chan->config.src_issue = cfg->src_issue;
> > > + chan->config.src_burst_len = cfg->src_burst_len;
> > > + chan->config.dst_burst_len = cfg->dst_burst_len;
> >
> > can you describe these parameters?
> ratectl:
> Rate control can be independently enabled per channel. When rate control is
> enabled, the
> DMA channel uses the rate control count to schedule successive data read
> transactions.
And how is this used by client?
> src_issue:
> Tells outstanding transaction on SRC.
This should be read only then, right?
> Burst_len:
> Configures the burst length of the src and dst transfers...
Hmmm, but you are on memcpy, so that should be programmed for throughput?
> >
> > How would a client know how to configure them?
>
> With the default values of the config parameters driver will work.
But how will client know what is default!
> If user has specific requirement to change these parameters they can pass
> It to the driver using set_config API and all these parameters are
> Documented in the include/linux/dma/xilinx_dma.h file...
Can you give me an example where user would like to do that
--
~Vinod
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod,
Thanks for the review...
>
> On Wed, Jun 01, 2016 at 12:53:59PM +0530, Kedareswara rao Appana wrote:
> > +config XILINX_ZYNQMP_DMA
> > + tristate "Xilinx ZynqMP DMA Engine"
> > + depends on (ARCH_ZYNQ || MICROBLAZE || ARM64)
> > + select DMA_ENGINE
> > + help
> > + Enable support for Xilinx ZynqMP DMA controller.
>
> Kconfig and makefile is sorted alphabetically, pls update this
Ok Sure will fix in the next version...
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> do you really need so many headers?
Will remove unwanted header files in the next version...
>
> > +static inline void zynqmp_dma_writeq(struct zynqmp_dma_chan *chan, u32
> reg,
> > +u64 value)
> > +{
> > +#if defined(CONFIG_PHYS_ADDR_T_64BIT)
> > + writeq(value, chan->regs + reg);
> > +#else
> > + lo_hi_writeq(value, chan->regs + reg); #endif
>
> why do you need this? can you explain..
writeq is available only on 64-bit platforms to make it work for every platform
I have modified like this.
Will fix in the next version will use only lo_hi_writeq which is available
across all the platforms.
>
> > +static inline bool zynqmp_dma_chan_is_idle(struct zynqmp_dma_chan
> > +*chan) {
> > + return chan->idle;
> > +
>
> empty line not required
OK will fix...
>
>
> > +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan *chan,
> > +void *desc)
>
> eod? 80 line?
It is exactly 80lines and check patch.pl didn't complained about it
>
>
> > + val = 0;
> > + if (chan->config.has_sg)
> > + val |= ZYNQMP_DMA_POINT_TYPE_SG;
>
> why not val = and get rid of assign to 0 above
Ok Will fix in the next version.
>
> > + writel(val, chan->regs + ZYNQMP_DMA_CTRL0);
>
> okay why write 0 for no sg mode?
Ok will fix in the next version..
>
> > +
> > + val = 0;
> > + if (chan->is_dmacoherent) {
> > + val |= ZYNQMP_DMA_AXCOHRNT;
> > + val = (val & ~ZYNQMP_DMA_AXCACHE) |
> > + (ZYNQMP_DMA_AXCACHE_VAL <<
> ZYNQMP_DMA_AXCACHE_OFST);
> > + }
> > + writel(val, chan->regs + ZYNQMP_DMA_DSCR_ATTR);
>
> same comments here too
Ok will fix in the next version..
>
> > + spin_lock_bh(>lock);
> > + desc = list_first_entry(>free_list, struct zynqmp_dma_desc_sw,
> > +node);
>
> how about:
>
> desc = list_first_entry(>free_list,
> struct zynqmp_dma_desc_sw, node);
Ok will fix...
>
> > + chan->desc_free_cnt++;
> > + list_add_tail(>node, >free_list);
> > + list_for_each_entry_safe(child, next, >tx_list, node) {
> > + chan->desc_free_cnt++;
> > + INIT_LIST_HEAD(>tx_list);
> > + list_move_tail(>node, >free_list);
> > + }
> > + INIT_LIST_HEAD(>tx_list);
>
> why are you initializing list in free?
Will fix in the next version of the patch...
>
> > +static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan) {
> > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > + struct zynqmp_dma_desc_sw *desc;
> > + int i;
> > +
> > + chan->sw_desc_pool = kzalloc(sizeof(*desc) *
> ZYNQMP_DMA_NUM_DESCS,
> > +GFP_KERNEL);
> > + if (!chan->sw_desc_pool)
> > + return -ENOMEM;
>
> empty line here pls
Ok will fix...
>
> > +static enum dma_status zynqmp_dma_tx_status(struct dma_chan *dchan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate) {
> > + return dma_cookie_status(dchan, cookie, txstate);
>
> why do you need this wrapper, assign dma_cookie_status as your status
> callback.
Ok will fix...
>
> > +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> > + struct zynqmp_dma_config *cfg)
> > +{
> > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > +
> > + chan->config.ovrfetch = cfg->ovrfetch;
> > + chan->config.has_sg = cfg->has_sg;
>
> is this HW capability? if so why would anyone not like to use it!
Yes it is HW capability. It can be either in simple mode or SG mode
Earlier In the driver this configuration is read from the device-tree
But as per lars and your suggestion moved it as runtime config parameters.
>
> > + chan->config.ratectrl = cfg->ratectrl;
> > + chan->config.src_issue = cfg->src_issue;
> > + chan->config.src_burst_len = cfg->src_burst_len;
> > + chan->config.dst_burst_len = cfg->dst_burst_len;
>
> can you describe these parameters?
ratectl:
Rate control can be independently enabled per channel. When rate control is
enabled, the
DMA channel uses the rate control count to schedule successive data read
transactions.
src_issue:
Tells outstanding transaction on SRC.
Burst_len:
Configures the burst length of the src and dst
RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Hi Vinod,
Thanks for the review...
>
> On Wed, Jun 01, 2016 at 12:53:59PM +0530, Kedareswara rao Appana wrote:
> > +config XILINX_ZYNQMP_DMA
> > + tristate "Xilinx ZynqMP DMA Engine"
> > + depends on (ARCH_ZYNQ || MICROBLAZE || ARM64)
> > + select DMA_ENGINE
> > + help
> > + Enable support for Xilinx ZynqMP DMA controller.
>
> Kconfig and makefile is sorted alphabetically, pls update this
Ok Sure will fix in the next version...
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> do you really need so many headers?
Will remove unwanted header files in the next version...
>
> > +static inline void zynqmp_dma_writeq(struct zynqmp_dma_chan *chan, u32
> reg,
> > +u64 value)
> > +{
> > +#if defined(CONFIG_PHYS_ADDR_T_64BIT)
> > + writeq(value, chan->regs + reg);
> > +#else
> > + lo_hi_writeq(value, chan->regs + reg); #endif
>
> why do you need this? can you explain..
writeq is available only on 64-bit platforms to make it work for every platform
I have modified like this.
Will fix in the next version will use only lo_hi_writeq which is available
across all the platforms.
>
> > +static inline bool zynqmp_dma_chan_is_idle(struct zynqmp_dma_chan
> > +*chan) {
> > + return chan->idle;
> > +
>
> empty line not required
OK will fix...
>
>
> > +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan *chan,
> > +void *desc)
>
> eod? 80 line?
It is exactly 80lines and check patch.pl didn't complained about it
>
>
> > + val = 0;
> > + if (chan->config.has_sg)
> > + val |= ZYNQMP_DMA_POINT_TYPE_SG;
>
> why not val = and get rid of assign to 0 above
Ok Will fix in the next version.
>
> > + writel(val, chan->regs + ZYNQMP_DMA_CTRL0);
>
> okay why write 0 for no sg mode?
Ok will fix in the next version..
>
> > +
> > + val = 0;
> > + if (chan->is_dmacoherent) {
> > + val |= ZYNQMP_DMA_AXCOHRNT;
> > + val = (val & ~ZYNQMP_DMA_AXCACHE) |
> > + (ZYNQMP_DMA_AXCACHE_VAL <<
> ZYNQMP_DMA_AXCACHE_OFST);
> > + }
> > + writel(val, chan->regs + ZYNQMP_DMA_DSCR_ATTR);
>
> same comments here too
Ok will fix in the next version..
>
> > + spin_lock_bh(>lock);
> > + desc = list_first_entry(>free_list, struct zynqmp_dma_desc_sw,
> > +node);
>
> how about:
>
> desc = list_first_entry(>free_list,
> struct zynqmp_dma_desc_sw, node);
Ok will fix...
>
> > + chan->desc_free_cnt++;
> > + list_add_tail(>node, >free_list);
> > + list_for_each_entry_safe(child, next, >tx_list, node) {
> > + chan->desc_free_cnt++;
> > + INIT_LIST_HEAD(>tx_list);
> > + list_move_tail(>node, >free_list);
> > + }
> > + INIT_LIST_HEAD(>tx_list);
>
> why are you initializing list in free?
Will fix in the next version of the patch...
>
> > +static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan) {
> > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > + struct zynqmp_dma_desc_sw *desc;
> > + int i;
> > +
> > + chan->sw_desc_pool = kzalloc(sizeof(*desc) *
> ZYNQMP_DMA_NUM_DESCS,
> > +GFP_KERNEL);
> > + if (!chan->sw_desc_pool)
> > + return -ENOMEM;
>
> empty line here pls
Ok will fix...
>
> > +static enum dma_status zynqmp_dma_tx_status(struct dma_chan *dchan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate) {
> > + return dma_cookie_status(dchan, cookie, txstate);
>
> why do you need this wrapper, assign dma_cookie_status as your status
> callback.
Ok will fix...
>
> > +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> > + struct zynqmp_dma_config *cfg)
> > +{
> > + struct zynqmp_dma_chan *chan = to_chan(dchan);
> > +
> > + chan->config.ovrfetch = cfg->ovrfetch;
> > + chan->config.has_sg = cfg->has_sg;
>
> is this HW capability? if so why would anyone not like to use it!
Yes it is HW capability. It can be either in simple mode or SG mode
Earlier In the driver this configuration is read from the device-tree
But as per lars and your suggestion moved it as runtime config parameters.
>
> > + chan->config.ratectrl = cfg->ratectrl;
> > + chan->config.src_issue = cfg->src_issue;
> > + chan->config.src_burst_len = cfg->src_burst_len;
> > + chan->config.dst_burst_len = cfg->dst_burst_len;
>
> can you describe these parameters?
ratectl:
Rate control can be independently enabled per channel. When rate control is
enabled, the
DMA channel uses the rate control count to schedule successive data read
transactions.
src_issue:
Tells outstanding transaction on SRC.
Burst_len:
Configures the burst length of the src and dst
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Wed, Jun 01, 2016 at 12:53:59PM +0530, Kedareswara rao Appana wrote:
> +config XILINX_ZYNQMP_DMA
> + tristate "Xilinx ZynqMP DMA Engine"
> + depends on (ARCH_ZYNQ || MICROBLAZE || ARM64)
> + select DMA_ENGINE
> + help
> + Enable support for Xilinx ZynqMP DMA controller.
Kconfig and makefile is sorted alphabetically, pls update this
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
do you really need so many headers?
> +static inline void zynqmp_dma_writeq(struct zynqmp_dma_chan *chan, u32 reg,
> + u64 value)
> +{
> +#if defined(CONFIG_PHYS_ADDR_T_64BIT)
> + writeq(value, chan->regs + reg);
> +#else
> + lo_hi_writeq(value, chan->regs + reg);
> +#endif
why do you need this? can you explain..
> +static inline bool zynqmp_dma_chan_is_idle(struct zynqmp_dma_chan *chan)
> +{
> + return chan->idle;
> +
empty line not required
> +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan *chan, void
> *desc)
eod? 80 line?
> + val = 0;
> + if (chan->config.has_sg)
> + val |= ZYNQMP_DMA_POINT_TYPE_SG;
why not val = and get rid of assign to 0 above
> + writel(val, chan->regs + ZYNQMP_DMA_CTRL0);
okay why write 0 for no sg mode?
> +
> + val = 0;
> + if (chan->is_dmacoherent) {
> + val |= ZYNQMP_DMA_AXCOHRNT;
> + val = (val & ~ZYNQMP_DMA_AXCACHE) |
> + (ZYNQMP_DMA_AXCACHE_VAL << ZYNQMP_DMA_AXCACHE_OFST);
> + }
> + writel(val, chan->regs + ZYNQMP_DMA_DSCR_ATTR);
same comments here too
> + spin_lock_bh(>lock);
> + desc = list_first_entry(>free_list, struct zynqmp_dma_desc_sw,
> + node);
how about:
desc = list_first_entry(>free_list,
struct zynqmp_dma_desc_sw, node);
> + chan->desc_free_cnt++;
> + list_add_tail(>node, >free_list);
> + list_for_each_entry_safe(child, next, >tx_list, node) {
> + chan->desc_free_cnt++;
> + INIT_LIST_HEAD(>tx_list);
> + list_move_tail(>node, >free_list);
> + }
> + INIT_LIST_HEAD(>tx_list);
why are you initializing list in free?
> +static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan)
> +{
> + struct zynqmp_dma_chan *chan = to_chan(dchan);
> + struct zynqmp_dma_desc_sw *desc;
> + int i;
> +
> + chan->sw_desc_pool = kzalloc(sizeof(*desc) * ZYNQMP_DMA_NUM_DESCS,
> + GFP_KERNEL);
> + if (!chan->sw_desc_pool)
> + return -ENOMEM;
empty line here pls
> +static enum dma_status zynqmp_dma_tx_status(struct dma_chan *dchan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + return dma_cookie_status(dchan, cookie, txstate);
why do you need this wrapper, assign dma_cookie_status as your status
callback.
> +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> + struct zynqmp_dma_config *cfg)
> +{
> + struct zynqmp_dma_chan *chan = to_chan(dchan);
> +
> + chan->config.ovrfetch = cfg->ovrfetch;
> + chan->config.has_sg = cfg->has_sg;
is this HW capability? if so why would anyone not like to use it!
> + chan->config.ratectrl = cfg->ratectrl;
> + chan->config.src_issue = cfg->src_issue;
> + chan->config.src_burst_len = cfg->src_burst_len;
> + chan->config.dst_burst_len = cfg->dst_burst_len;
can you describe these parameters?
How would a client know how to configure them?
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(zynqmp_dma_channel_set_config);
Not _GPL?
> + chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
> + chan->config.src_issue = ZYNQMP_DMA_SRC_ISSUE_RST_VAL;
> + chan->config.dst_burst_len = ZYNQMP_DMA_AWLEN_RST_VAL;
> + chan->config.src_burst_len = ZYNQMP_DMA_ARLEN_RST_VAL;
> + err = of_property_read_u32(node, "xlnx,bus-width", >bus_width);
> + if ((err < 0) && ((chan->bus_width != ZYNQMP_DMA_BUS_WIDTH_64) ||
> + (chan->bus_width != ZYNQMP_DMA_BUS_WIDTH_128))) {
> + dev_err(zdev->dev, "invalid bus-width value");
> + return err;
> + }
> +
> + chan->bus_width = chan->bus_width / 8;
?
why not update it once?
--
~Vinod
Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
On Wed, Jun 01, 2016 at 12:53:59PM +0530, Kedareswara rao Appana wrote:
> +config XILINX_ZYNQMP_DMA
> + tristate "Xilinx ZynqMP DMA Engine"
> + depends on (ARCH_ZYNQ || MICROBLAZE || ARM64)
> + select DMA_ENGINE
> + help
> + Enable support for Xilinx ZynqMP DMA controller.
Kconfig and makefile is sorted alphabetically, pls update this
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
do you really need so many headers?
> +static inline void zynqmp_dma_writeq(struct zynqmp_dma_chan *chan, u32 reg,
> + u64 value)
> +{
> +#if defined(CONFIG_PHYS_ADDR_T_64BIT)
> + writeq(value, chan->regs + reg);
> +#else
> + lo_hi_writeq(value, chan->regs + reg);
> +#endif
why do you need this? can you explain..
> +static inline bool zynqmp_dma_chan_is_idle(struct zynqmp_dma_chan *chan)
> +{
> + return chan->idle;
> +
empty line not required
> +static void zynqmp_dma_desc_config_eod(struct zynqmp_dma_chan *chan, void
> *desc)
eod? 80 line?
> + val = 0;
> + if (chan->config.has_sg)
> + val |= ZYNQMP_DMA_POINT_TYPE_SG;
why not val = and get rid of assign to 0 above
> + writel(val, chan->regs + ZYNQMP_DMA_CTRL0);
okay why write 0 for no sg mode?
> +
> + val = 0;
> + if (chan->is_dmacoherent) {
> + val |= ZYNQMP_DMA_AXCOHRNT;
> + val = (val & ~ZYNQMP_DMA_AXCACHE) |
> + (ZYNQMP_DMA_AXCACHE_VAL << ZYNQMP_DMA_AXCACHE_OFST);
> + }
> + writel(val, chan->regs + ZYNQMP_DMA_DSCR_ATTR);
same comments here too
> + spin_lock_bh(>lock);
> + desc = list_first_entry(>free_list, struct zynqmp_dma_desc_sw,
> + node);
how about:
desc = list_first_entry(>free_list,
struct zynqmp_dma_desc_sw, node);
> + chan->desc_free_cnt++;
> + list_add_tail(>node, >free_list);
> + list_for_each_entry_safe(child, next, >tx_list, node) {
> + chan->desc_free_cnt++;
> + INIT_LIST_HEAD(>tx_list);
> + list_move_tail(>node, >free_list);
> + }
> + INIT_LIST_HEAD(>tx_list);
why are you initializing list in free?
> +static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan)
> +{
> + struct zynqmp_dma_chan *chan = to_chan(dchan);
> + struct zynqmp_dma_desc_sw *desc;
> + int i;
> +
> + chan->sw_desc_pool = kzalloc(sizeof(*desc) * ZYNQMP_DMA_NUM_DESCS,
> + GFP_KERNEL);
> + if (!chan->sw_desc_pool)
> + return -ENOMEM;
empty line here pls
> +static enum dma_status zynqmp_dma_tx_status(struct dma_chan *dchan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + return dma_cookie_status(dchan, cookie, txstate);
why do you need this wrapper, assign dma_cookie_status as your status
callback.
> +int zynqmp_dma_channel_set_config(struct dma_chan *dchan,
> + struct zynqmp_dma_config *cfg)
> +{
> + struct zynqmp_dma_chan *chan = to_chan(dchan);
> +
> + chan->config.ovrfetch = cfg->ovrfetch;
> + chan->config.has_sg = cfg->has_sg;
is this HW capability? if so why would anyone not like to use it!
> + chan->config.ratectrl = cfg->ratectrl;
> + chan->config.src_issue = cfg->src_issue;
> + chan->config.src_burst_len = cfg->src_burst_len;
> + chan->config.dst_burst_len = cfg->dst_burst_len;
can you describe these parameters?
How would a client know how to configure them?
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(zynqmp_dma_channel_set_config);
Not _GPL?
> + chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
> + chan->config.src_issue = ZYNQMP_DMA_SRC_ISSUE_RST_VAL;
> + chan->config.dst_burst_len = ZYNQMP_DMA_AWLEN_RST_VAL;
> + chan->config.src_burst_len = ZYNQMP_DMA_ARLEN_RST_VAL;
> + err = of_property_read_u32(node, "xlnx,bus-width", >bus_width);
> + if ((err < 0) && ((chan->bus_width != ZYNQMP_DMA_BUS_WIDTH_64) ||
> + (chan->bus_width != ZYNQMP_DMA_BUS_WIDTH_128))) {
> + dev_err(zdev->dev, "invalid bus-width value");
> + return err;
> + }
> +
> + chan->bus_width = chan->bus_width / 8;
?
why not update it once?
--
~Vinod
