diff mbox

[V3,2/3] dmaengine: dw_dmac: Enhance device tree support

Message ID 169edae67b9a051ca7448e5b62c3cd2c5c1db470.1350360935.git.viresh.kumar@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Viresh Kumar Oct. 16, 2012, 4:19 a.m. UTC
dw_dmac driver already supports device tree but it used to have its platform
data passed the non-DT way.

This patch does following changes:
- pass platform data via DT, non-DT way still takes precedence if both are used.
- create generic filter routine
- Earlier slave information was made available by slave specific filter routines
  in chan->private field. Now, this information would be passed from within dmac
  DT node. Slave drivers would now be required to pass bus_id (a string) as
  parameter to this generic filter(), which would be compared against the slave
  data passed from DT, by the generic filter routine.
- Update binding document

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
V2->V3:
------
- Simplified an equation in filter routine
- renamed variable 'val' as 'tmp' in DT parsing routine

V1->V2:
------
- Optimized filter & DT parsing routine
- Removed unnecessary casts from changes
- renamed filter function
- Fixed function prototype and return value of DT parsing routine for !CONFIG_OF
  case
- use of_get_child_count()

 Documentation/devicetree/bindings/dma/snps-dma.txt |  44 +++++++
 drivers/dma/dw_dmac.c                              | 134 +++++++++++++++++++++
 drivers/dma/dw_dmac_regs.h                         |   4 +
 include/linux/dw_dmac.h                            |  43 ++++---
 4 files changed, 208 insertions(+), 17 deletions(-)

Comments

Vinod Koul Oct. 26, 2012, 8:48 a.m. UTC | #1
On Fri, 2012-10-26 at 14:25 +0530, Viresh Kumar wrote:
> Hi Vinod,
> 
> On 16 October 2012 09:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > dw_dmac driver already supports device tree but it used to have its platform
> > data passed the non-DT way.
> >
> > This patch does following changes:
> > - pass platform data via DT, non-DT way still takes precedence if both are used.
> > - create generic filter routine
> > - Earlier slave information was made available by slave specific filter routines
> >   in chan->private field. Now, this information would be passed from within dmac
> >   DT node. Slave drivers would now be required to pass bus_id (a string) as
> >   parameter to this generic filter(), which would be compared against the slave
> >   data passed from DT, by the generic filter routine.
> > - Update binding document
> 
> I can see that you applied these patches and they are present in
> linux-next. But i feel
> the order of patches is bad.
Yes looks like I forgot to sort the mbox series :(

since all patches were applied nicely, and they seem fairly independent
of each other it should cause issue. Let me know if you wnat me to redo
my -next.
Viresh Kumar Oct. 26, 2012, 8:55 a.m. UTC | #2
Hi Vinod,

On 16 October 2012 09:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.
>
> This patch does following changes:
> - pass platform data via DT, non-DT way still takes precedence if both are used.
> - create generic filter routine
> - Earlier slave information was made available by slave specific filter routines
>   in chan->private field. Now, this information would be passed from within dmac
>   DT node. Slave drivers would now be required to pass bus_id (a string) as
>   parameter to this generic filter(), which would be compared against the slave
>   data passed from DT, by the generic filter routine.
> - Update binding document

I can see that you applied these patches and they are present in
linux-next. But i feel
the order of patches is bad.

git log --oneline gives following:

879a0ec dmaengine: dw_dmac: Update documentation style comments for
dw_dma_platform_data
d0e35f3 dmaengine: dw_dmac: Enhance device tree support
4c4c30c ARM: SPEAr13xx: Pass DW DMAC platform data from DT

The last patch "ARM: SPEAr13xx: Pass DW DMAC platform data from DT" has
dependency on second patch.

The correct order would be:
4c4c30c ARM: SPEAr13xx: Pass DW DMAC platform data from DT
d0e35f3 dmaengine: dw_dmac: Enhance device tree support
879a0ec dmaengine: dw_dmac: Update documentation style comments for
dw_dma_platform_data


PS: We are looking at output of git log. Last patch (or first from
bottom) was applied first.

--
viresh
Viresh Kumar Oct. 26, 2012, 9:05 a.m. UTC | #3
On 26 October 2012 14:18, Vinod Koul <vkoul@infradead.org> wrote:
> On Fri, 2012-10-26 at 14:25 +0530, Viresh Kumar wrote:
>> I can see that you applied these patches and they are present in
>> linux-next. But i feel
>> the order of patches is bad.
> Yes looks like I forgot to sort the mbox series :(
>
> since all patches were applied nicely, and they seem fairly independent
> of each other it should cause issue. Let me know if you wnat me to redo
> my -next.

yes. The ARCH specific patch 3/3 uses a routine created in 2/3. So they have
to be in right order for git bisect to work.

--
viresh
Vinod Koul Oct. 26, 2012, 9:10 a.m. UTC | #4
On Fri, 2012-10-26 at 14:35 +0530, Viresh Kumar wrote:
> >> I can see that you applied these patches and they are present in
> >> linux-next. But i feel
> >> the order of patches is bad.
> > Yes looks like I forgot to sort the mbox series :(
> >
> > since all patches were applied nicely, and they seem fairly independent
> > of each other it should cause issue. Let me know if you wnat me to redo
> > my -next.
> 
> yes. The ARCH specific patch 3/3 uses a routine created in 2/3. So they have
> to be in right order for git bisect to work. 

Done, it should reflect now.
Sorry for the mess.
Arnd Bergmann Dec. 10, 2012, 10:08 p.m. UTC | #5
On Tuesday 16 October 2012, Viresh Kumar wrote:
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.
> 
> This patch does following changes:
> - pass platform data via DT, non-DT way still takes precedence if both are used.
> - create generic filter routine
> - Earlier slave information was made available by slave specific filter routines
>   in chan->private field. Now, this information would be passed from within dmac
>   DT node. Slave drivers would now be required to pass bus_id (a string) as
>   parameter to this generic filter(), which would be compared against the slave
>   data passed from DT, by the generic filter routine.
> - Update binding document
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I'm deeply sorry for the very late complaint on this, but I only now
noticed this patch as I was seeing build breakage in linux-next
because of it.

The build bug is not the problem however, but the abuse of the
API is. Andy, are you sure you understood what this does when
you gave you Reviewed-by comment?

Viresh, there are multiple problems with your approach unfortunately:

* It does not follow the binding from
  Documentation/devicetree/bindings/dma/dma.txt

* It requires slave drivers to know that they are using the dw_dmac
  driver and pass a pointer to dw_generic_filter, which is not
  generic at all

* It requires the dmac node to have information about all slaves

There are also some minor issues, such as the naming of DT
properties, but the above need to be resolved first.

	Arnd
Arnd Bergmann Dec. 10, 2012, 10:21 p.m. UTC | #6
On Monday 10 December 2012, Arnd Bergmann wrote:
> The build bug is not the problem however, but the abuse of the
> API is. Andy, are you sure you understood what this does when
> you gave you Reviewed-by comment?
> 

Ah, I just saw that Andy had the right intuition when commenting on
it at first, but Viresh managed to convince him otherwise, and appearently
neither had followed the discussion about the dmaengine bindings...

	Arnd
Andy Shevchenko Dec. 12, 2012, 8:42 a.m. UTC | #7
On Mon, 2012-12-10 at 22:08 +0000, Arnd Bergmann wrote: 
> The build bug is not the problem however, but the abuse of the
> API is. Andy, are you sure you understood what this does when
> you gave you Reviewed-by comment?

Thank you for pointing this out to refresh my memories and go through
documentation again.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
index c0d85db..5bb3dfb 100644
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
@@ -6,6 +6,26 @@  Required properties:
 - interrupt-parent: Should be the phandle for the interrupt controller
   that services interrupts for this device
 - interrupt: Should contain the DMAC interrupt number
+- nr_channels: Number of channels supported by hardware
+- is_private: The device channels should be marked as private and not for by the
+  general purpose DMA channel allocator. False if not passed.
+- chan_allocation_order: order of allocation of channel, 0 (default): ascending,
+  1: descending
+- chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1:
+  increase from chan n->0
+- block_size: Maximum block size supported by the controller
+- nr_masters: Number of AHB masters supported by the controller
+- data_width: Maximum data width supported by hardware per AHB master
+  (0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
+- slave_info:
+	- bus_id: name of this device channel, not just a device name since
+	  devices may have more than one channel e.g. "foo_tx". For using the
+	  dw_generic_filter(), slave drivers must pass exactly this string as
+	  param to filter function.
+	- cfg_hi: Platform-specific initializer for the CFG_HI register
+	- cfg_lo: Platform-specific initializer for the CFG_LO register
+	- src_master: src master for transfers on allocated channel.
+	- dst_master: dest master for transfers on allocated channel.
 
 Example:
 
@@ -14,4 +34,28 @@  Example:
 		reg = <0xfc000000 0x1000>;
 		interrupt-parent = <&vic1>;
 		interrupts = <12>;
+
+		nr_channels = <8>;
+		chan_allocation_order = <1>;
+		chan_priority = <1>;
+		block_size = <0xfff>;
+		nr_masters = <2>;
+		data_width = <3 3 0 0>;
+
+		slave_info {
+			uart0-tx {
+				bus_id = "uart0-tx";
+				cfg_hi = <0x4000>;	/* 0x8 << 11 */
+				cfg_lo = <0>;
+				src_master = <0>;
+				dst_master = <1>;
+			};
+			spi0-tx {
+				bus_id = "spi0-tx";
+				cfg_hi = <0x2000>;	/* 0x4 << 11 */
+				cfg_lo = <0>;
+				src_master = <0>;
+				dst_master = <0>;
+			};
+		};
 	};
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c4b0eb3..98f33a7 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1179,6 +1179,50 @@  static void dwc_free_chan_resources(struct dma_chan *chan)
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
 }
 
+bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
+{
+	struct dw_dma *dw = to_dw_dma(chan->device);
+	static struct dw_dma *last_dw;
+	static char *last_bus_id;
+	int i = -1;
+
+	/*
+	 * dmaengine framework calls this routine for all channels of all dma
+	 * controller, until true is returned. If 'param' bus_id is not
+	 * registered with a dma controller (dw), then there is no need of
+	 * running below function for all channels of dw.
+	 *
+	 * This block of code does this by saving the parameters of last
+	 * failure. If dw and param are same, i.e. trying on same dw with
+	 * different channel, return false.
+	 */
+	if ((last_dw == dw) && (last_bus_id == param))
+		return false;
+	/*
+	 * Return true:
+	 * - If dw_dma's platform data is not filled with slave info, then all
+	 *   dma controllers are fine for transfer.
+	 * - Or if param is NULL
+	 */
+	if (!dw->sd || !param)
+		return true;
+
+	while (++i < dw->sd_count) {
+		if (!strcmp(dw->sd[i].bus_id, param)) {
+			chan->private = &dw->sd[i];
+			last_dw = NULL;
+			last_bus_id = NULL;
+
+			return true;
+		}
+	}
+
+	last_dw = dw;
+	last_bus_id = param;
+	return false;
+}
+EXPORT_SYMBOL(dw_dma_generic_filter);
+
 /* --------------------- Cyclic DMA API extensions -------------------- */
 
 /**
@@ -1462,6 +1506,91 @@  static void dw_dma_off(struct dw_dma *dw)
 		dw->chan[i].initialized = false;
 }
 
+#ifdef CONFIG_OF
+static struct dw_dma_platform_data *
+__devinit dw_dma_parse_dt(struct platform_device *pdev)
+{
+	struct device_node *sn, *cn, *np = pdev->dev.of_node;
+	struct dw_dma_platform_data *pdata;
+	struct dw_dma_slave *sd;
+	u32 tmp, arr[4];
+
+	if (!np) {
+		dev_err(&pdev->dev, "Missing DT data\n");
+		return NULL;
+	}
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	if (of_property_read_u32(np, "nr_channels", &pdata->nr_channels))
+		return NULL;
+
+	if (of_property_read_bool(np, "is_private"))
+		pdata->is_private = true;
+
+	if (!of_property_read_u32(np, "chan_allocation_order", &tmp))
+		pdata->chan_allocation_order = (unsigned char)tmp;
+
+	if (!of_property_read_u32(np, "chan_priority", &tmp))
+		pdata->chan_priority = tmp;
+
+	if (!of_property_read_u32(np, "block_size", &tmp))
+		pdata->block_size = tmp;
+
+	if (!of_property_read_u32(np, "nr_masters", &tmp)) {
+		if (tmp > 4)
+			return NULL;
+
+		pdata->nr_masters = tmp;
+	}
+
+	if (!of_property_read_u32_array(np, "data_width", arr,
+				pdata->nr_masters))
+		for (tmp = 0; tmp < pdata->nr_masters; tmp++)
+			pdata->data_width[tmp] = arr[tmp];
+
+	/* parse slave data */
+	sn = of_find_node_by_name(np, "slave_info");
+	if (!sn)
+		return pdata;
+
+	/* calculate number of slaves */
+	tmp = of_get_child_count(sn);
+	if (!tmp)
+		return NULL;
+
+	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * tmp, GFP_KERNEL);
+	if (!sd)
+		return NULL;
+
+	pdata->sd = sd;
+	pdata->sd_count = tmp;
+
+	for_each_child_of_node(sn, cn) {
+		sd->dma_dev = &pdev->dev;
+		of_property_read_string(cn, "bus_id", &sd->bus_id);
+		of_property_read_u32(cn, "cfg_hi", &sd->cfg_hi);
+		of_property_read_u32(cn, "cfg_lo", &sd->cfg_lo);
+		if (!of_property_read_u32(cn, "src_master", &tmp))
+			sd->src_master = tmp;
+
+		if (!of_property_read_u32(cn, "dst_master", &tmp))
+			sd->dst_master = tmp;
+		sd++;
+	}
+
+	return pdata;
+}
+#else
+static inline struct dw_dma_platform_data *
+dw_dma_parse_dt(struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit dw_probe(struct platform_device *pdev)
 {
 	struct dw_dma_platform_data *pdata;
@@ -1478,6 +1607,9 @@  static int __devinit dw_probe(struct platform_device *pdev)
 	int			i;
 
 	pdata = dev_get_platdata(&pdev->dev);
+	if (!pdata)
+		pdata = dw_dma_parse_dt(pdev);
+
 	if (!pdata || pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS)
 		return -EINVAL;
 
@@ -1512,6 +1644,8 @@  static int __devinit dw_probe(struct platform_device *pdev)
 	clk_prepare_enable(dw->clk);
 
 	dw->regs = regs;
+	dw->sd = pdata->sd;
+	dw->sd_count = pdata->sd_count;
 
 	/* get hardware configuration parameters */
 	if (autocfg) {
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index ff39fa6..5cc61ba 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -231,6 +231,10 @@  struct dw_dma {
 	struct tasklet_struct	tasklet;
 	struct clk		*clk;
 
+	/* slave information */
+	struct dw_dma_slave	*sd;
+	unsigned int		sd_count;
+
 	u8			all_chan_mask;
 
 	/* hardware configuration */
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index 62a6190..41766de 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -15,6 +15,26 @@ 
 #include <linux/dmaengine.h>
 
 /**
+ * struct dw_dma_slave - Controller-specific information about a slave
+ *
+ * @dma_dev: required DMA master device. Depricated.
+ * @bus_id: name of this device channel, not just a device name since
+ *          devices may have more than one channel e.g. "foo_tx"
+ * @cfg_hi: Platform-specific initializer for the CFG_HI register
+ * @cfg_lo: Platform-specific initializer for the CFG_LO register
+ * @src_master: src master for transfers on allocated channel.
+ * @dst_master: dest master for transfers on allocated channel.
+ */
+struct dw_dma_slave {
+	struct device		*dma_dev;
+	const char		*bus_id;
+	u32			cfg_hi;
+	u32			cfg_lo;
+	u8			src_master;
+	u8			dst_master;
+};
+
+/**
  * struct dw_dma_platform_data - Controller configuration parameters
  * @nr_channels: Number of channels supported by hardware (max 8)
  * @is_private: The device channels should be marked as private and not for
@@ -25,6 +45,8 @@ 
  * @nr_masters: Number of AHB masters supported by the controller
  * @data_width: Maximum data width supported by hardware per AHB master
  *		(0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
+ * @sd: slave specific data. Used for configuring channels
+ * @sd_count: count of slave data structures passed.
  */
 struct dw_dma_platform_data {
 	unsigned int	nr_channels;
@@ -38,6 +60,9 @@  struct dw_dma_platform_data {
 	unsigned short	block_size;
 	unsigned char	nr_masters;
 	unsigned char	data_width[4];
+
+	struct dw_dma_slave *sd;
+	unsigned int sd_count;
 };
 
 /* bursts size */
@@ -52,23 +77,6 @@  enum dw_dma_msize {
 	DW_DMA_MSIZE_256,
 };
 
-/**
- * struct dw_dma_slave - Controller-specific information about a slave
- *
- * @dma_dev: required DMA master device
- * @cfg_hi: Platform-specific initializer for the CFG_HI register
- * @cfg_lo: Platform-specific initializer for the CFG_LO register
- * @src_master: src master for transfers on allocated channel.
- * @dst_master: dest master for transfers on allocated channel.
- */
-struct dw_dma_slave {
-	struct device		*dma_dev;
-	u32			cfg_hi;
-	u32			cfg_lo;
-	u8			src_master;
-	u8			dst_master;
-};
-
 /* Platform-configurable bits in CFG_HI */
 #define DWC_CFGH_FCMODE		(1 << 0)
 #define DWC_CFGH_FIFO_MODE	(1 << 1)
@@ -106,5 +114,6 @@  void dw_dma_cyclic_stop(struct dma_chan *chan);
 dma_addr_t dw_dma_get_src_addr(struct dma_chan *chan);
 
 dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan);
+bool dw_dma_generic_filter(struct dma_chan *chan, void *param);
 
 #endif /* DW_DMAC_H */