Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1410888 > unrolled thread

[PATCH v10 1/2] Documentation: DT: dma: Add Xilinx zynqmp dma device tree binding documentation

Started byKedareswara rao Appana <appana.durga.rao@xilinx.com>
First post2016-06-01 09:30 +0200
Last post2016-06-08 09:50 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v10 1/2] Documentation: DT: dma: Add Xilinx zynqmp dma device tree binding documentation Kedareswara rao Appana <appana.durga.rao@xilinx.com> - 2016-06-01 09:30 +0200
    Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver  support Vinod Koul <vinod.koul@intel.com> - 2016-06-07 09:10 +0200
      RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver  support Appana Durga Kedareswara Rao <appana.durga.rao@xilinx.com> - 2016-06-08 09:50 +0200

#1410888 — [PATCH v10 1/2] Documentation: DT: dma: Add Xilinx zynqmp dma device tree binding documentation

FromKedareswara rao Appana <appana.durga.rao@xilinx.com>
Date2016-06-01 09:30 +0200
Subject[PATCH v10 1/2] Documentation: DT: dma: Add Xilinx zynqmp dma device tree binding documentation
Message-ID<rF8Lg-5r1-19@gated-at.bofh.it>
Device-tree binding documentation for Xilinx zynqmp dma engine
used in Zynq UltraScale+ MPSoC.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes in v10:
- Added Rob Acked-by in the commit message.
Changs in v9:
- Removed include sg runtime configuration parameter
  from the binding doc as suggested by Lars.
Changes in v8:
- Removed all the software runtime configuration parameters
  from the binding doc as suggested by the Lars.
Changes in v7:
- None.
Changes in v6:
- Removed desc-axi-cache/dst-axi-cache/src-axi-cache properties
  from the binding doc as it allow broken combinations when dma-coherent
  is set as suggested by Rob.
- Fixed minor comments given by Rob related coding(lower case DT node name).
Changes in v5:
- Use dma-coherent flag for coherent transfers as suggested by rob.
- Removed unnecessary properties from binding doc as suggested by Rob.
Changes in v4:
- None
Changes in v3:
- None
Changes in v2:
- None.

 .../devicetree/bindings/dma/xilinx/zynqmp_dma.txt  | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/xilinx/zynqmp_dma.txt

diff --git a/Documentation/devicetree/bindings/dma/xilinx/zynqmp_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/zynqmp_dma.txt
new file mode 100644
index 0000000..a784cdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/xilinx/zynqmp_dma.txt
@@ -0,0 +1,27 @@
+Xilinx ZynqMP DMA engine, it does support memory to memory transfers,
+memory to device and device to memory transfers. It also has flow
+control and rate control support for slave/peripheral dma access.
+
+Required properties:
+- compatible		: Should be "xlnx,zynqmp-dma-1.0"
+- reg			: Memory map for gdma/adma module access.
+- interrupt-parent	: Interrupt controller the interrupt is routed through
+- interrupts		: Should contain DMA channel interrupt.
+- xlnx,bus-width	: Axi buswidth in bits. Should contain 128 or 64
+- clock-names		: List of input clocks "clk_main", "clk_apb"
+			  (see clock bindings for details)
+
+Optional properties:
+- dma-coherent		: Present if dma operations are coherent.
+
+Example:
+++++++++
+fpd_dma_chan1: dma@fd500000 {
+	compatible = "xlnx,zynqmp-dma-1.0";
+	reg = <0x0 0xFD500000 0x1000>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 117 4>;
+	clock-names = "clk_main", "clk_apb";
+	xlnx,bus-width = <128>;
+	dma-coherent;
+};
-- 
2.1.2

[toc] | [next] | [standalone]

#1415776 — Re: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support

FromVinod Koul <vinod.koul@intel.com>
Date2016-06-07 09:10 +0200
SubjectRe: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Message-ID<rHjjd-7Z6-61@gated-at.bofh.it>
In reply to#1410888
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 <linux/bitops.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dma/xilinx_dma.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>

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(&chan->lock);
> +	desc = list_first_entry(&chan->free_list, struct zynqmp_dma_desc_sw,
> +				 node);

how about:

	desc = list_first_entry(&chan->free_list,
			struct zynqmp_dma_desc_sw, node);

> +	chan->desc_free_cnt++;
> +	list_add_tail(&sdesc->node, &chan->free_list);
> +	list_for_each_entry_safe(child, next, &sdesc->tx_list, node) {
> +		chan->desc_free_cnt++;
> +		INIT_LIST_HEAD(&child->tx_list);
> +		list_move_tail(&child->node, &chan->free_list);
> +	}
> +	INIT_LIST_HEAD(&sdesc->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", &chan->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

[toc] | [prev] | [next] | [standalone]

#1416993 — RE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support

FromAppana Durga Kedareswara Rao <appana.durga.rao@xilinx.com>
Date2016-06-08 09:50 +0200
SubjectRE: [PATCH v10 2/2] dmaengine: Add Xilinx zynqmp dma engine driver support
Message-ID<rHGpr-5GS-11@gated-at.bofh.it>
In reply to#1415776
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 <linux/bitops.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/dma/xilinx_dma.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dmapool.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> 
> 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(&chan->lock);
> > +	desc = list_first_entry(&chan->free_list, struct zynqmp_dma_desc_sw,
> > +				 node);
> 
> how about:
> 
> 	desc = list_first_entry(&chan->free_list,
> 			struct zynqmp_dma_desc_sw, node);

Ok will fix...

> 
> > +	chan->desc_free_cnt++;
> > +	list_add_tail(&sdesc->node, &chan->free_list);
> > +	list_for_each_entry_safe(child, next, &sdesc->tx_list, node) {
> > +		chan->desc_free_cnt++;
> > +		INIT_LIST_HEAD(&child->tx_list);
> > +		list_move_tail(&child->node, &chan->free_list);
> > +	}
> > +	INIT_LIST_HEAD(&sdesc->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 transfers...

> 
> How would a client know how to configure them?

With the default values of the config parameters driver will work.
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...

> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(zynqmp_dma_channel_set_config);
> 
> Not _GPL?

Ok will fix in the next version...

> 
> > +	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", &chan-
> >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?

Ok will fix in the next version...


Regards,
Kedar.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web