[PATCH 0/4] dw_dmac: cleanup for DT usage

[PATCH 0/4] dw_dmac: cleanup for DT usage

[Andy Shevchenko]

It includes few cleanups when used with DT. Patch 4/4 is a modified version of
the [1].

[1] http://www.spinics.net/lists/arm-kernel/msg225366.html

Andy Shevchenko (3):
  dw_dmac: fix style of the comments
  dw_dmac: rename DT related methods to reflect their belonging
  dw_dmac: make build of DT related methods optional

Arnd Bergmann (1):
  dmaengine: dw_dmac: simplify master selection

 drivers/dma/dw_dmac.c      | 149 +++++++++++++++++++++------------------------
 drivers/dma/dw_dmac_regs.h |   5 +-
 2 files changed, 75 insertions(+), 79 deletions(-)

-- 
1.8.2.rc0.22.gb3600c3

[PATCH 1/4] dw_dmac: fix style of the comments

[Andy Shevchenko]

Let's use capital letter as a first one in the comments.
There is no functional changes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 43e2e89..d6dbb14 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -173,7 +173,7 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 		return;
 
 	if (dws && dws->cfg_hi == ~0 && dws->cfg_lo == ~0) {
-		/* autoconfigure based on request line from DT */
+		/* Autoconfigure based on request line from DT */
 		if (dwc->direction == DMA_MEM_TO_DEV)
 			cfghi = DWC_CFGH_DST_PER(dwc->request_line);
 		else if (dwc->direction == DMA_DEV_TO_MEM)
@@ -473,16 +473,16 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 			(unsigned long long)llp);
 
 	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
-		/* initial residue value */
+		/* Initial residue value */
 		dwc->residue = desc->total_len;
 
-		/* check first descriptors addr */
+		/* Check first descriptors addr */
 		if (desc->txd.phys == llp) {
 			spin_unlock_irqrestore(&dwc->lock, flags);
 			return;
 		}
 
-		/* check first descriptors llp */
+		/* Check first descriptors llp */
 		if (desc->lli.llp == llp) {
 			/* This one is currently in progress */
 			dwc->residue -= dwc_get_sent(dwc);
@@ -588,7 +588,7 @@ inline dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan)
 }
 EXPORT_SYMBOL(dw_dma_get_dst_addr);
 
-/* called with dwc->lock held and all DMAC interrupts disabled */
+/* Called with dwc->lock held and all DMAC interrupts disabled */
 static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
 		u32 status_err, u32 status_xfer)
 {
@@ -626,7 +626,7 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
 
 		dwc_chan_disable(dw, dwc);
 
-		/* make sure DMA does not restart by loading a new list */
+		/* Make sure DMA does not restart by loading a new list */
 		channel_writel(dwc, LLP, 0);
 		channel_writel(dwc, CTL_LO, 0);
 		channel_writel(dwc, CTL_HI, 0);
@@ -1256,7 +1256,7 @@ static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
 	struct dw_dma_filter_args *fargs = param;
 	struct dw_dma_slave *dws = &dwc->slave;
 
-	/* ensure the device matches our channel */
+	/* Ensure the device matches our channel */
         if (chan->device != &fargs->dw->dma)
                 return false;
 
@@ -1323,7 +1323,7 @@ int dw_dma_cyclic_start(struct dma_chan *chan)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
-	/* assert channel is idle */
+	/* Assert channel is idle */
 	if (dma_readl(dw, CH_EN) & dwc->mask) {
 		dev_err(chan2dev(&dwc->chan),
 			"BUG: Attempted to start non-idle channel\n");
@@ -1335,7 +1335,7 @@ int dw_dma_cyclic_start(struct dma_chan *chan)
 	dma_writel(dw, CLEAR.ERROR, dwc->mask);
 	dma_writel(dw, CLEAR.XFER, dwc->mask);
 
-	/* setup DMAC channel registers */
+	/* Setup DMAC channel registers */
 	channel_writel(dwc, LLP, dwc->cdesc->desc[0]->txd.phys);
 	channel_writel(dwc, CTL_LO, DWC_CTLL_LLP_D_EN | DWC_CTLL_LLP_S_EN);
 	channel_writel(dwc, CTL_HI, 0);
@@ -1502,7 +1502,7 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 		last = desc;
 	}
 
-	/* lets make a cyclic list */
+	/* Let's make a cyclic list */
 	last->lli.llp = cdesc->desc[0]->txd.phys;
 
 	dev_dbg(chan2dev(&dwc->chan), "cyclic prepared buf 0x%llx len %zu "
@@ -1707,7 +1707,7 @@ static int dw_probe(struct platform_device *pdev)
 
 	dw->regs = regs;
 
-	/* get hardware configuration parameters */
+	/* Get hardware configuration parameters */
 	if (autocfg) {
 		max_blk_size = dma_readl(dw, MAX_BLK_SIZE);
 
@@ -1729,10 +1729,10 @@ static int dw_probe(struct platform_device *pdev)
 	/* Calculate all channel mask before DMA setup */
 	dw->all_chan_mask = (1 << nr_channels) - 1;
 
-	/* force dma off, just in case */
+	/* Force dma off, just in case */
 	dw_dma_off(dw);
 
-	/* disable BLOCK interrupts as well */
+	/* Disable BLOCK interrupts as well */
 	channel_clear_bit(dw, MASK.BLOCK, dw->all_chan_mask);
 
 	err = devm_request_irq(&pdev->dev, irq, dw_dma_interrupt, 0,
@@ -1742,7 +1742,7 @@ static int dw_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dw);
 
-	/* create a pool of consistent memory blocks for hardware descriptors */
+	/* Create a pool of consistent memory blocks for hardware descriptors */
 	dw->desc_pool = dmam_pool_create("dw_dmac_desc_pool", &pdev->dev,
 					 sizeof(struct dw_desc), 4, 0);
 	if (!dw->desc_pool) {
@@ -1783,7 +1783,7 @@ static int dw_probe(struct platform_device *pdev)
 
 		dwc->direction = DMA_TRANS_NONE;
 
-		/* hardware configuration */
+		/* Hardware configuration */
 		if (autocfg) {
 			unsigned int dwc_params;
 
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 1/4] dw_dmac: fix style of the comments

[Viresh Kumar]

On 22 March 2013 20:13, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Let's use capital letter as a first one in the comments.
> There is no functional changes.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 2/4] dw_dmac: rename DT related methods to reflect their belonging

[Andy Shevchenko]

Since we will have not only DT cases in future let's rename DT related methods
to reflect their belonging.

The rename was done as follows:
	struct dw_dma_filter_args	-> struct dw_dma_of_filter_args
	dw_dma_generic_filter()		-> dw_dma_of_filter()
	dw_dma_xlate()			-> dw_dma_of_xlate()
	dw_dma_id_table			-> dw_dma_of_id_table

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index d6dbb14..274fd7d 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1242,18 +1242,20 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
 }
 
-struct dw_dma_filter_args {
+/*----------------------------------------------------------------------*/
+
+struct dw_dma_of_filter_args {
 	struct dw_dma *dw;
 	unsigned int req;
 	unsigned int src;
 	unsigned int dst;
 };
 
-static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
+static bool dw_dma_of_filter(struct dma_chan *chan, void *param)
 {
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 	struct dw_dma *dw = to_dw_dma(chan->device);
-	struct dw_dma_filter_args *fargs = param;
+	struct dw_dma_of_filter_args *fargs = param;
 	struct dw_dma_slave *dws = &dwc->slave;
 
 	/* Ensure the device matches our channel */
@@ -1273,11 +1275,11 @@ static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
 	return true;
 }
 
-static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
-					 struct of_dma *ofdma)
+static struct dma_chan *dw_dma_of_xlate(struct of_phandle_args *dma_spec,
+					struct of_dma *ofdma)
 {
 	struct dw_dma *dw = ofdma->of_dma_data;
-	struct dw_dma_filter_args fargs = {
+	struct dw_dma_of_filter_args fargs = {
 		.dw = dw,
 	};
 	dma_cap_mask_t cap;
@@ -1298,7 +1300,7 @@ static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
 	dma_cap_set(DMA_SLAVE, cap);
 
 	/* TODO: there should be a simpler way to do this */
-	return dma_request_channel(cap, dw_dma_generic_filter, &fargs);
+	return dma_request_channel(cap, dw_dma_of_filter, &fargs);
 }
 
 /* --------------------- Cyclic DMA API extensions -------------------- */
@@ -1843,7 +1845,7 @@ static int dw_probe(struct platform_device *pdev)
 
 	if (pdev->dev.of_node) {
 		err = of_dma_controller_register(pdev->dev.of_node,
-						 dw_dma_xlate, dw);
+						 dw_dma_of_xlate, dw);
 		if (err && err != -ENODEV)
 			dev_err(&pdev->dev,
 				"could not register of_dma_controller\n");
@@ -1913,11 +1915,11 @@ static const struct dev_pm_ops dw_dev_pm_ops = {
 };
 
 #ifdef CONFIG_OF
-static const struct of_device_id dw_dma_id_table[] = {
+static const struct of_device_id dw_dma_of_id_table[] = {
 	{ .compatible = "snps,dma-spear1340" },
 	{}
 };
-MODULE_DEVICE_TABLE(of, dw_dma_id_table);
+MODULE_DEVICE_TABLE(of, dw_dma_of_id_table);
 #endif
 
 static const struct platform_device_id dw_dma_ids[] = {
@@ -1933,7 +1935,7 @@ static struct platform_driver dw_driver = {
 	.driver = {
 		.name	= "dw_dmac",
 		.pm	= &dw_dev_pm_ops,
-		.of_match_table = of_match_ptr(dw_dma_id_table),
+		.of_match_table = of_match_ptr(dw_dma_of_id_table),
 	},
 	.id_table	= dw_dma_ids,
 };
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 2/4] dw_dmac: rename DT related methods to reflect their belonging

[Viresh Kumar]

On 22 March 2013 20:13, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Since we will have not only DT cases in future let's rename DT related methods
> to reflect their belonging.
>
> The rename was done as follows:
>         struct dw_dma_filter_args       -> struct dw_dma_of_filter_args
>         dw_dma_generic_filter()         -> dw_dma_of_filter()
>         dw_dma_xlate()                  -> dw_dma_of_xlate()
>         dw_dma_id_table                 -> dw_dma_of_id_table
>
> There is no functional change.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 3/4] dw_dmac: make build of DT related methods optional

[Andy Shevchenko]

There is no reason to build custom filter function and translation function
inside the driver in case we have no DT platform.

This patch introduces new method dw_dma_of_controller_register() and moves all
DT related stuff under #ifdef CONFIG_OF. It also helps to distinguish the real
-ENODEV return code of fake one when of_dma_controller_register is not
implemented.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 274fd7d..9b2e186 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1244,6 +1244,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 
 /*----------------------------------------------------------------------*/
 
+#ifdef CONFIG_OF
 struct dw_dma_of_filter_args {
 	struct dw_dma *dw;
 	unsigned int req;
@@ -1303,6 +1304,19 @@ static struct dma_chan *dw_dma_of_xlate(struct of_phandle_args *dma_spec,
 	return dma_request_channel(cap, dw_dma_of_filter, &fargs);
 }
 
+static void dw_dma_of_controller_register(struct dw_dma *dw)
+{
+	struct device *dev = dw->dma.dev;
+	int err;
+
+	err = of_dma_controller_register(dev->of_node, dw_dma_of_xlate, dw);
+	if (err)
+		dev_err(dev, "could not register of_dma_controller\n");
+}
+#else /* !CONFIG_OF */
+static inline void dw_dma_of_controller_register(struct dw_dma *dw) {}
+#endif /* !CONFIG_OF */
+
 /* --------------------- Cyclic DMA API extensions -------------------- */
 
 /**
@@ -1843,13 +1857,8 @@ static int dw_probe(struct platform_device *pdev)
 
 	dma_async_device_register(&dw->dma);
 
-	if (pdev->dev.of_node) {
-		err = of_dma_controller_register(pdev->dev.of_node,
-						 dw_dma_of_xlate, dw);
-		if (err && err != -ENODEV)
-			dev_err(&pdev->dev,
-				"could not register of_dma_controller\n");
-	}
+	if (pdev->dev.of_node)
+		dw_dma_of_controller_register(dw);
 
 	return 0;
 }
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 3/4] dw_dmac: make build of DT related methods optional

[Arnd Bergmann]

On Friday 22 March 2013, Andy Shevchenko wrote:
> There is no reason to build custom filter function and translation function
> inside the driver in case we have no DT platform.
> 
> This patch introduces new method dw_dma_of_controller_register() and moves all
> DT related stuff under #ifdef CONFIG_OF. It also helps to distinguish the real
> -ENODEV return code of fake one when of_dma_controller_register is not
> implemented.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This should have no impact on the object code at all, since everything
inside of the #ifdef is be dropped by the compiler anyway if
of_dma_controller_register is stubbed out.

I generally prefer to have all driver code be compiled  all the time
to catch build regressions independent of the configuration, and leave
the #ifdefs in header files that provide the interfaces.

> -	if (pdev->dev.of_node) {
> -		err = of_dma_controller_register(pdev->dev.of_node,
> -						 dw_dma_of_xlate, dw);
> -		if (err && err != -ENODEV)
> -			dev_err(&pdev->dev,
> -				"could not register of_dma_controller\n");
> -	}
> +	if (pdev->dev.of_node)
> +		dw_dma_of_controller_register(dw);

If you want to remove the "err != -ENODEV" check here, we could rewrite
this as 

	if (IS_ENABLED(CONFIG_OF)) && pdev->dev.of_node) {
		err = of_dma_controller_register(pdev->dev.of_node,
						 dw_dma_of_xlate, dw);
		if (err)
			dev_err(&pdev->dev,
				"could not register of_dma_controller\n");
	}

Or alternatively, we can change the of_dma_controller_register() stub to
return 0 if CONFIG_OF is disabled. That would also take care of similar
code in other dma engine drivers.

	Arnd

Re: [PATCH 3/4] dw_dmac: make build of DT related methods optional

[Andy Shevchenko]

On Fri, 2013-03-22 at 15:19 +0000, Arnd Bergmann wrote: 
> On Friday 22 March 2013, Andy Shevchenko wrote:
> > There is no reason to build custom filter function and translation function
> > inside the driver in case we have no DT platform.
> > 
> > This patch introduces new method dw_dma_of_controller_register() and moves all
> > DT related stuff under #ifdef CONFIG_OF. It also helps to distinguish the real
> > -ENODEV return code of fake one when of_dma_controller_register is not
> > implemented.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This should have no impact on the object code at all, since everything
> inside of the #ifdef is be dropped by the compiler anyway if
> of_dma_controller_register is stubbed out.

Right.


> I generally prefer to have all driver code be compiled  all the time
> to catch build regressions independent of the configuration, and leave
> the #ifdefs in header files that provide the interfaces.

I don't. For checking we have special make targets:

  allnoconfig     - New config where all options are answered with no
  allyesconfig    - New config where all options are accepted with yes
  allmodconfig    - New config selecting modules when possible
  alldefconfig    - New config with all symbols set to default

> > -	if (pdev->dev.of_node) {
> > -		err = of_dma_controller_register(pdev->dev.of_node,
> > -						 dw_dma_of_xlate, dw);
> > -		if (err && err != -ENODEV)
> > -			dev_err(&pdev->dev,
> > -				"could not register of_dma_controller\n");
> > -	}
> > +	if (pdev->dev.of_node)
> > +		dw_dma_of_controller_register(dw);
> 
> If you want to remove the "err != -ENODEV" check here, we could rewrite
> this as 
> 
> 	if (IS_ENABLED(CONFIG_OF)) && pdev->dev.of_node) {
> 		err = of_dma_controller_register(pdev->dev.of_node,
> 						 dw_dma_of_xlate, dw);
> 		if (err)
> 			dev_err(&pdev->dev,
> 				"could not register of_dma_controller\n");
> 	}
> 
> Or alternatively, we can change the of_dma_controller_register() stub to
> return 0 if CONFIG_OF is disabled. That would also take care of similar
> code in other dma engine drivers.

Actually to be aligned with other dmaengine code it should return
-ENOSYS. And by description ENOSYS seems  suitable for "not implemented"
cases.


What about to move all CONFIG_OF stuff into separate file?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/4] dw_dmac: make build of DT related methods optional

[Arnd Bergmann]

On Monday 25 March 2013, Andy Shevchenko wrote:
> > I generally prefer to have all driver code be compiled  all the time
> > to catch build regressions independent of the configuration, and leave
> > the #ifdefs in header files that provide the interfaces.
> 
> I don't. For checking we have special make targets:
> 
>   allnoconfig     - New config where all options are answered with no
>   allyesconfig    - New config where all options are accepted with yes
>   allmodconfig    - New config selecting modules when possible
>   alldefconfig    - New config with all symbols set to default

That will only help on architectures that support CONFIG_OF, although
that probably includes all the common ones except s390 and ia64 nowadays.

> >       if (IS_ENABLED(CONFIG_OF)) && pdev->dev.of_node) {
> >               err = of_dma_controller_register(pdev->dev.of_node,
> >                                                dw_dma_of_xlate, dw);
> >               if (err)
> >                       dev_err(&pdev->dev,
> >                               "could not register of_dma_controller\n");
> >       }
> > 
> > Or alternatively, we can change the of_dma_controller_register() stub to
> > return 0 if CONFIG_OF is disabled. That would also take care of similar
> > code in other dma engine drivers.
> 
> Actually to be aligned with other dmaengine code it should return
> -ENOSYS. And by description ENOSYS seems  suitable for "not implemented"
> cases.

I think we use ENOSYS normally when the absence of the interface is
a fatal error, which it would not be here. This case I think is more
like the clk and regulator interfaces, where you want to bail out
if the functions return an actual error but not if the subsystem is
compiled out.

> What about to move all CONFIG_OF stuff into separate file?

Seems not worth it, and still would lead to the code not being
compile tested by default. Right now, there are two small functions,
and I would hope we can turn that into a single even smaller
function eventually if we get right of the silly requirement to
go through a filter function here.

	Arnd

[PATCH 4/4] dmaengine: dw_dmac: simplify master selection

[Andy Shevchenko]

From: Arnd Bergmann <arnd@arndb.de>

The patch to add the common DMA binding added a dummy dw_dma_slave
structure into the dw_dma_chan structure in order to configure the
masters correctly. It turns out that this can be simplified if we
pick the DMA masters in the dwc_alloc_chan_resources function instead
and save them in the dw_dma_chan structure directly.

This could be simplified further once all users that today use
dw_dma_slave for configuration get converted to device tree based
setup instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-arm-kernel at lists.infradead.org
---
 drivers/dma/dw_dmac.c      | 76 ++++++++++++++++++----------------------------
 drivers/dma/dw_dmac_regs.h |  5 ++-
 2 files changed, 33 insertions(+), 48 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 9b2e186..656c3bc 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -49,29 +49,22 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
 	return slave ? slave->src_master : 1;
 }
 
-#define SRC_MASTER	0
-#define DST_MASTER	1
-
-static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
+static inline void dwc_set_masters(struct dw_dma_chan *dwc)
 {
-	struct dw_dma *dw = to_dw_dma(chan->device);
-	struct dw_dma_slave *dws = chan->private;
-	unsigned int m;
-
-	if (master == SRC_MASTER)
-		m = dwc_get_sms(dws);
-	else
-		m = dwc_get_dms(dws);
+	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
+	struct dw_dma_slave *dws = dwc->chan.private;
+	unsigned char mmax = dw->nr_masters - 1;
 
-	return min_t(unsigned int, dw->nr_masters - 1, m);
+	if (dwc->request_line == ~0) {
+		dwc->src_master = min_t(unsigned char, mmax, dwc_get_sms(dws));
+		dwc->dst_master = min_t(unsigned char, mmax, dwc_get_dms(dws));
+	}
 }
 
 #define DWC_DEFAULT_CTLLO(_chan) ({				\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
 		bool _is_slave = is_slave_direction(_dwc->direction);	\
-		int _dms = dwc_get_master(_chan, DST_MASTER);		\
-		int _sms = dwc_get_master(_chan, SRC_MASTER);		\
 		u8 _smsize = _is_slave ? _sconfig->src_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
@@ -81,8 +74,8 @@ static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
 		 | DWC_CTLL_SRC_MSIZE(_smsize)			\
 		 | DWC_CTLL_LLP_D_EN				\
 		 | DWC_CTLL_LLP_S_EN				\
-		 | DWC_CTLL_DMS(_dms)				\
-		 | DWC_CTLL_SMS(_sms));				\
+		 | DWC_CTLL_DMS(_dwc->dst_master)		\
+		 | DWC_CTLL_SMS(_dwc->src_master));		\
 	})
 
 /*
@@ -92,13 +85,6 @@ static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
  */
 #define NR_DESCS_PER_CHANNEL	64
 
-static inline unsigned int dwc_get_data_width(struct dma_chan *chan, int master)
-{
-	struct dw_dma *dw = to_dw_dma(chan->device);
-
-	return dw->data_width[dwc_get_master(chan, master)];
-}
-
 /*----------------------------------------------------------------------*/
 
 static struct device *chan2dev(struct dma_chan *chan)
@@ -172,13 +158,7 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 	if (dwc->initialized == true)
 		return;
 
-	if (dws && dws->cfg_hi == ~0 && dws->cfg_lo == ~0) {
-		/* Autoconfigure based on request line from DT */
-		if (dwc->direction == DMA_MEM_TO_DEV)
-			cfghi = DWC_CFGH_DST_PER(dwc->request_line);
-		else if (dwc->direction == DMA_DEV_TO_MEM)
-			cfghi = DWC_CFGH_SRC_PER(dwc->request_line);
-	} else if (dws) {
+	if (dws) {
 		/*
 		 * We need controller-specific data to set up slave
 		 * transfers.
@@ -189,9 +169,9 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 		cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
 	} else {
 		if (dwc->direction == DMA_MEM_TO_DEV)
-			cfghi = DWC_CFGH_DST_PER(dwc->dma_sconfig.slave_id);
+			cfghi = DWC_CFGH_DST_PER(dwc->request_line);
 		else if (dwc->direction == DMA_DEV_TO_MEM)
-			cfghi = DWC_CFGH_SRC_PER(dwc->dma_sconfig.slave_id);
+			cfghi = DWC_CFGH_SRC_PER(dwc->request_line);
 	}
 
 	channel_writel(dwc, CFG_LO, cfglo);
@@ -745,6 +725,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		size_t len, unsigned long flags)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dw_desc		*desc;
 	struct dw_desc		*first;
 	struct dw_desc		*prev;
@@ -767,8 +748,8 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 
 	dwc->direction = DMA_MEM_TO_MEM;
 
-	data_width = min_t(unsigned int, dwc_get_data_width(chan, SRC_MASTER),
-			   dwc_get_data_width(chan, DST_MASTER));
+	data_width = min_t(unsigned int, dw->data_width[dwc->src_master],
+			   dw->data_width[dwc->dst_master]);
 
 	src_width = dst_width = min_t(unsigned int, data_width,
 				      dwc_fast_fls(src | dest | len));
@@ -826,6 +807,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned long flags, void *context)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
 	struct dw_desc		*prev;
 	struct dw_desc		*first;
@@ -859,7 +841,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_M2P) :
 			DWC_CTLL_FC(DW_DMA_FC_D_M2P);
 
-		data_width = dwc_get_data_width(chan, SRC_MASTER);
+		data_width = dw->data_width[dwc->src_master];
 
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
@@ -919,7 +901,7 @@ slave_sg_todev_fill_desc:
 		ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) :
 			DWC_CTLL_FC(DW_DMA_FC_D_P2M);
 
-		data_width = dwc_get_data_width(chan, DST_MASTER);
+		data_width = dw->data_width[dwc->dst_master];
 
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
@@ -1020,6 +1002,10 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
 	dwc->direction = sconfig->direction;
 
+	/* Take the request line from slave_id member */
+	if (dwc->request_line == ~0)
+		dwc->request_line = sconfig->slave_id;
+
 	convert_burst(&dwc->dma_sconfig.src_maxburst);
 	convert_burst(&dwc->dma_sconfig.dst_maxburst);
 	convert_slave_id(dwc);
@@ -1170,6 +1156,8 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * doesn't mean what you think it means), and status writeback.
 	 */
 
+	dwc_set_masters(dwc);
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	i = dwc->descs_allocated;
 	while (dwc->descs_allocated < NR_DESCS_PER_CHANNEL) {
@@ -1227,6 +1215,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
 	dwc->initialized = false;
+	dwc->request_line = ~0;
 
 	/* Disable interrupts */
 	channel_clear_bit(dw, MASK.XFER, dwc->mask);
@@ -1255,23 +1244,15 @@ struct dw_dma_of_filter_args {
 static bool dw_dma_of_filter(struct dma_chan *chan, void *param)
 {
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
-	struct dw_dma *dw = to_dw_dma(chan->device);
 	struct dw_dma_of_filter_args *fargs = param;
-	struct dw_dma_slave *dws = &dwc->slave;
 
 	/* Ensure the device matches our channel */
         if (chan->device != &fargs->dw->dma)
                 return false;
 
-	dws->dma_dev	= dw->dma.dev;
-	dws->cfg_hi	= ~0;
-	dws->cfg_lo	= ~0;
-	dws->src_master	= fargs->src;
-	dws->dst_master	= fargs->dst;
-
 	dwc->request_line = fargs->req;
-
-	chan->private = dws;
+	dwc->src_master	= fargs->src;
+	dwc->dst_master	= fargs->dst;
 
 	return true;
 }
@@ -1798,6 +1779,7 @@ static int dw_probe(struct platform_device *pdev)
 		channel_clear_bit(dw, CH_EN, dwc->mask);
 
 		dwc->direction = DMA_TRANS_NONE;
+		dwc->request_line = ~0;
 
 		/* Hardware configuration */
 		if (autocfg) {
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 4d02c36..9b0e12e 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -212,8 +212,11 @@ struct dw_dma_chan {
 	/* hardware configuration */
 	unsigned int		block_size;
 	bool			nollp;
+
+	/* custom slave configuration */
 	unsigned int		request_line;
-	struct dw_dma_slave	slave;
+	unsigned char		src_master;
+	unsigned char		dst_master;
 
 	/* configuration passed via DMA_SLAVE_CONFIG */
 	struct dma_slave_config dma_sconfig;
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 4/4] dmaengine: dw_dmac: simplify master selection

[Andy Shevchenko]

From: Arnd Bergmann <arnd@arndb.de>

The patch to add the common DMA binding added a dummy dw_dma_slave
structure into the dw_dma_chan structure in order to configure the
masters correctly. It turns out that this can be simplified if we
pick the DMA masters in the dwc_alloc_chan_resources function instead
and save them in the dw_dma_chan structure directly.

This could be simplified further once all users that today use
dw_dma_slave for configuration get converted to device tree based
setup instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/dma/dw_dmac.c      | 76 ++++++++++++++++++----------------------------
 drivers/dma/dw_dmac_regs.h |  5 ++-
 2 files changed, 33 insertions(+), 48 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 9b2e186..656c3bc 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -49,29 +49,22 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
 	return slave ? slave->src_master : 1;
 }
 
-#define SRC_MASTER	0
-#define DST_MASTER	1
-
-static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
+static inline void dwc_set_masters(struct dw_dma_chan *dwc)
 {
-	struct dw_dma *dw = to_dw_dma(chan->device);
-	struct dw_dma_slave *dws = chan->private;
-	unsigned int m;
-
-	if (master == SRC_MASTER)
-		m = dwc_get_sms(dws);
-	else
-		m = dwc_get_dms(dws);
+	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
+	struct dw_dma_slave *dws = dwc->chan.private;
+	unsigned char mmax = dw->nr_masters - 1;
 
-	return min_t(unsigned int, dw->nr_masters - 1, m);
+	if (dwc->request_line == ~0) {
+		dwc->src_master = min_t(unsigned char, mmax, dwc_get_sms(dws));
+		dwc->dst_master = min_t(unsigned char, mmax, dwc_get_dms(dws));
+	}
 }
 
 #define DWC_DEFAULT_CTLLO(_chan) ({				\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
 		bool _is_slave = is_slave_direction(_dwc->direction);	\
-		int _dms = dwc_get_master(_chan, DST_MASTER);		\
-		int _sms = dwc_get_master(_chan, SRC_MASTER);		\
 		u8 _smsize = _is_slave ? _sconfig->src_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
@@ -81,8 +74,8 @@ static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
 		 | DWC_CTLL_SRC_MSIZE(_smsize)			\
 		 | DWC_CTLL_LLP_D_EN				\
 		 | DWC_CTLL_LLP_S_EN				\
-		 | DWC_CTLL_DMS(_dms)				\
-		 | DWC_CTLL_SMS(_sms));				\
+		 | DWC_CTLL_DMS(_dwc->dst_master)		\
+		 | DWC_CTLL_SMS(_dwc->src_master));		\
 	})
 
 /*
@@ -92,13 +85,6 @@ static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
  */
 #define NR_DESCS_PER_CHANNEL	64
 
-static inline unsigned int dwc_get_data_width(struct dma_chan *chan, int master)
-{
-	struct dw_dma *dw = to_dw_dma(chan->device);
-
-	return dw->data_width[dwc_get_master(chan, master)];
-}
-
 /*----------------------------------------------------------------------*/
 
 static struct device *chan2dev(struct dma_chan *chan)
@@ -172,13 +158,7 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 	if (dwc->initialized == true)
 		return;
 
-	if (dws && dws->cfg_hi == ~0 && dws->cfg_lo == ~0) {
-		/* Autoconfigure based on request line from DT */
-		if (dwc->direction == DMA_MEM_TO_DEV)
-			cfghi = DWC_CFGH_DST_PER(dwc->request_line);
-		else if (dwc->direction == DMA_DEV_TO_MEM)
-			cfghi = DWC_CFGH_SRC_PER(dwc->request_line);
-	} else if (dws) {
+	if (dws) {
 		/*
 		 * We need controller-specific data to set up slave
 		 * transfers.
@@ -189,9 +169,9 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 		cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
 	} else {
 		if (dwc->direction == DMA_MEM_TO_DEV)
-			cfghi = DWC_CFGH_DST_PER(dwc->dma_sconfig.slave_id);
+			cfghi = DWC_CFGH_DST_PER(dwc->request_line);
 		else if (dwc->direction == DMA_DEV_TO_MEM)
-			cfghi = DWC_CFGH_SRC_PER(dwc->dma_sconfig.slave_id);
+			cfghi = DWC_CFGH_SRC_PER(dwc->request_line);
 	}
 
 	channel_writel(dwc, CFG_LO, cfglo);
@@ -745,6 +725,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		size_t len, unsigned long flags)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dw_desc		*desc;
 	struct dw_desc		*first;
 	struct dw_desc		*prev;
@@ -767,8 +748,8 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 
 	dwc->direction = DMA_MEM_TO_MEM;
 
-	data_width = min_t(unsigned int, dwc_get_data_width(chan, SRC_MASTER),
-			   dwc_get_data_width(chan, DST_MASTER));
+	data_width = min_t(unsigned int, dw->data_width[dwc->src_master],
+			   dw->data_width[dwc->dst_master]);
 
 	src_width = dst_width = min_t(unsigned int, data_width,
 				      dwc_fast_fls(src | dest | len));
@@ -826,6 +807,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned long flags, void *context)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
 	struct dw_desc		*prev;
 	struct dw_desc		*first;
@@ -859,7 +841,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_M2P) :
 			DWC_CTLL_FC(DW_DMA_FC_D_M2P);
 
-		data_width = dwc_get_data_width(chan, SRC_MASTER);
+		data_width = dw->data_width[dwc->src_master];
 
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
@@ -919,7 +901,7 @@ slave_sg_todev_fill_desc:
 		ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) :
 			DWC_CTLL_FC(DW_DMA_FC_D_P2M);
 
-		data_width = dwc_get_data_width(chan, DST_MASTER);
+		data_width = dw->data_width[dwc->dst_master];
 
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
@@ -1020,6 +1002,10 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
 	dwc->direction = sconfig->direction;
 
+	/* Take the request line from slave_id member */
+	if (dwc->request_line == ~0)
+		dwc->request_line = sconfig->slave_id;
+
 	convert_burst(&dwc->dma_sconfig.src_maxburst);
 	convert_burst(&dwc->dma_sconfig.dst_maxburst);
 	convert_slave_id(dwc);
@@ -1170,6 +1156,8 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * doesn't mean what you think it means), and status writeback.
 	 */
 
+	dwc_set_masters(dwc);
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	i = dwc->descs_allocated;
 	while (dwc->descs_allocated < NR_DESCS_PER_CHANNEL) {
@@ -1227,6 +1215,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
 	dwc->initialized = false;
+	dwc->request_line = ~0;
 
 	/* Disable interrupts */
 	channel_clear_bit(dw, MASK.XFER, dwc->mask);
@@ -1255,23 +1244,15 @@ struct dw_dma_of_filter_args {
 static bool dw_dma_of_filter(struct dma_chan *chan, void *param)
 {
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
-	struct dw_dma *dw = to_dw_dma(chan->device);
 	struct dw_dma_of_filter_args *fargs = param;
-	struct dw_dma_slave *dws = &dwc->slave;
 
 	/* Ensure the device matches our channel */
         if (chan->device != &fargs->dw->dma)
                 return false;
 
-	dws->dma_dev	= dw->dma.dev;
-	dws->cfg_hi	= ~0;
-	dws->cfg_lo	= ~0;
-	dws->src_master	= fargs->src;
-	dws->dst_master	= fargs->dst;
-
 	dwc->request_line = fargs->req;
-
-	chan->private = dws;
+	dwc->src_master	= fargs->src;
+	dwc->dst_master	= fargs->dst;
 
 	return true;
 }
@@ -1798,6 +1779,7 @@ static int dw_probe(struct platform_device *pdev)
 		channel_clear_bit(dw, CH_EN, dwc->mask);
 
 		dwc->direction = DMA_TRANS_NONE;
+		dwc->request_line = ~0;
 
 		/* Hardware configuration */
 		if (autocfg) {
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 4d02c36..9b0e12e 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -212,8 +212,11 @@ struct dw_dma_chan {
 	/* hardware configuration */
 	unsigned int		block_size;
 	bool			nollp;
+
+	/* custom slave configuration */
 	unsigned int		request_line;
-	struct dw_dma_slave	slave;
+	unsigned char		src_master;
+	unsigned char		dst_master;
 
 	/* configuration passed via DMA_SLAVE_CONFIG */
 	struct dma_slave_config dma_sconfig;
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 4/4] dmaengine: dw_dmac: simplify master selection

[Viresh Kumar]

On 22 March 2013 20:13, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The patch to add the common DMA binding added a dummy dw_dma_slave
> structure into the dw_dma_chan structure in order to configure the
> masters correctly. It turns out that this can be simplified if we
> pick the DMA masters in the dwc_alloc_chan_resources function instead
> and save them in the dw_dma_chan structure directly.
>
> This could be simplified further once all users that today use
> dw_dma_slave for configuration get converted to device tree based
> setup instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  drivers/dma/dw_dmac.c      | 76 ++++++++++++++++++----------------------------
>  drivers/dma/dw_dmac_regs.h |  5 ++-
>  2 files changed, 33 insertions(+), 48 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 4/4] dmaengine: dw_dmac: simplify master selection

[Viresh Kumar]

On 22 March 2013 20:13, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The patch to add the common DMA binding added a dummy dw_dma_slave
> structure into the dw_dma_chan structure in order to configure the
> masters correctly. It turns out that this can be simplified if we
> pick the DMA masters in the dwc_alloc_chan_resources function instead
> and save them in the dw_dma_chan structure directly.
>
> This could be simplified further once all users that today use
> dw_dma_slave for configuration get converted to device tree based
> setup instead.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/dma/dw_dmac.c      | 76 ++++++++++++++++++----------------------------
>  drivers/dma/dw_dmac_regs.h |  5 ++-
>  2 files changed, 33 insertions(+), 48 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 0/4] dw_dmac: cleanup for DT usage

[Arnd Bergmann]

On Friday 22 March 2013, Andy Shevchenko wrote:
> It includes few cleanups when used with DT. Patch 4/4 is a modified version of
> the [1].
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg225366.html
> 
> Andy Shevchenko (3):
>   dw_dmac: fix style of the comments
>   dw_dmac: rename DT related methods to reflect their belonging
>   dw_dmac: make build of DT related methods optional
> 
> Arnd Bergmann (1):
>   dmaengine: dw_dmac: simplify master selection
> 

Thanks for following up on this!

The first two patches look good to me, but I think the third one is not
needed, since the interface was meant to work without the need for #ifdef
hacks in drivers.

	Arnd