[PATCH V3] dma: tegra: register as an OF DMA controller

[PATCH V3] dma: tegra: register as an OF DMA controller

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Call of_dma_controller_register() so that DMA clients can look up the
Tegra DMA controller using standard APIs. This requires the of_xlate()
function to save off the DMA slave ID, and for tegra_dma_slave_config()
not to over-write this information; once DMA client drivers are converted
to dma_request_slave_channel() and DT-based lookups, they won't set this
field of struct dma_slave_config anymore.

Cc: treding at nvidia.com
Cc: pdeschrijver at nvidia.com
Cc: linux-tegra at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: dmaengine at vger.kernel.org
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: Rework of_xlate() to use new dma_get_any_slave_channel().

v2: Use of_dma_slave_xlate() rather than of_dma_simple_xlate(), as
    suggested by Arnd Bergmann.

This patch is part of a series with strong internal depdendencies. I'm
looking for an ack so that I can take the entire series through the Tegra
and arm-soc trees. The series will be part of a stable branch that can be
merged into other subsystems if needed to avoid/resolve dependencies.
---
 drivers/dma/tegra20-apb-dma.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index afa5844c9346..d11bb3620f27 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1,7 +1,7 @@
 /*
  * DMA driver for Nvidia's Tegra20 APB DMA controller.
  *
- * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2012-2013, NVIDIA CORPORATION.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_dma.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
@@ -199,6 +200,7 @@ struct tegra_dma_channel {
 	void			*callback_param;
 
 	/* Channel-slave specific configuration */
+	unsigned int slave_id;
 	struct dma_slave_config dma_sconfig;
 	struct tegra_dma_channel_regs	channel_reg;
 };
@@ -340,6 +342,8 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
 	}
 
 	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
+	if (!tdc->slave_id)
+		tdc->slave_id = sconfig->slave_id;
 	tdc->config_init = true;
 	return 0;
 }
@@ -942,7 +946,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 	ahb_seq |= TEGRA_APBDMA_AHBSEQ_BUS_WIDTH_32;
 
 	csr |= TEGRA_APBDMA_CSR_ONCE | TEGRA_APBDMA_CSR_FLOW;
-	csr |= tdc->dma_sconfig.slave_id << TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
+	csr |= tdc->slave_id << TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
 	if (flags & DMA_PREP_INTERRUPT)
 		csr |= TEGRA_APBDMA_CSR_IE_EOC;
 
@@ -1086,7 +1090,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
 	csr |= TEGRA_APBDMA_CSR_FLOW;
 	if (flags & DMA_PREP_INTERRUPT)
 		csr |= TEGRA_APBDMA_CSR_IE_EOC;
-	csr |= tdc->dma_sconfig.slave_id << TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
+	csr |= tdc->slave_id << TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
 
 	apb_seq |= TEGRA_APBDMA_APBSEQ_WRAP_WORD_1;
 
@@ -1206,6 +1210,25 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 		kfree(sg_req);
 	}
 	clk_disable_unprepare(tdma->dma_clk);
+
+	tdc->slave_id = 0;
+}
+
+static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
+					   struct of_dma *ofdma)
+{
+	struct tegra_dma *tdma = ofdma->of_dma_data;
+	struct dma_chan *chan;
+	struct tegra_dma_channel *tdc;
+
+	chan = dma_get_any_slave_channel(&tdma->dma_dev);
+	if (!chan)
+		return NULL;
+
+	tdc = to_tegra_dma_chan(chan);
+	tdc->slave_id = dma_spec->args[0];
+
+	return chan;
 }
 
 /* Tegra20 specific DMA controller information */
@@ -1383,10 +1406,20 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		goto err_irq;
 	}
 
+	ret = of_dma_controller_register(pdev->dev.of_node,
+					 tegra_dma_of_xlate, tdma);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Tegra20 APB DMA OF registration failed %d\n", ret);
+		goto err_unregister_dma_dev;
+	}
+
 	dev_info(&pdev->dev, "Tegra20 APB DMA driver register %d channels\n",
 			cdata->nr_channels);
 	return 0;
 
+err_unregister_dma_dev:
+	dma_async_device_unregister(&tdma->dma_dev);
 err_irq:
 	while (--i >= 0) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
-- 
1.8.1.5

[PATCH V3] dma: tegra: register as an OF DMA controller

[Arnd Bergmann]

On Tuesday 03 December 2013, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Call of_dma_controller_register() so that DMA clients can look up the
> Tegra DMA controller using standard APIs. This requires the of_xlate()
> function to save off the DMA slave ID, and for tegra_dma_slave_config()
> not to over-write this information; once DMA client drivers are converted
> to dma_request_slave_channel() and DT-based lookups, they won't set this
> field of struct dma_slave_config anymore.
> 
> Cc: treding at nvidia.com
> Cc: pdeschrijver at nvidia.com
> Cc: linux-tegra at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: dmaengine at vger.kernel.org
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Looks great to me.

Acked-by: Arnd Bergmann <arnd@arndb.de>

but see my comments about the dma_get_any_slave_channel. If we want to merge
that function with the existing dma_get_slave_channel function (and change
the three existing users of that one), one line here will of course have
to change.

Also, one (long-running, not just for this driver) comment I have is
about this snippet:

> @@ -340,6 +342,8 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
>         }
>  
>         memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> +       if (!tdc->slave_id)
> +               tdc->slave_id = sconfig->slave_id;
>         tdc->config_init = true;
>         return 0;
>  }

We really need to be better at having a common set of rules regarding what
it actually means to set the slave_id through dma_slave_config(). IMHO
we should just treat it as a bug for any dmaengine driver that is configured
through DT, or we should get rid of this entirely. Looking through the
code now, I only see two dmaengine drivers that actually do this: one is 
the tegra driver (which you are about to change), the other one is shdma,
and that one is already a bit special. Any ideas?

	Arnd

[PATCH V3] dma: tegra: register as an OF DMA controller

[Thierry Reding]

On Tue, Dec 03, 2013 at 01:33:07PM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Call of_dma_controller_register() so that DMA clients can look up the
> Tegra DMA controller using standard APIs. This requires the of_xlate()
> function to save off the DMA slave ID, and for tegra_dma_slave_config()
> not to over-write this information; once DMA client drivers are converted
> to dma_request_slave_channel() and DT-based lookups, they won't set this
> field of struct dma_slave_config anymore.
> 
> Cc: treding at nvidia.com
> Cc: pdeschrijver at nvidia.com
> Cc: linux-tegra at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: dmaengine at vger.kernel.org
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v3: Rework of_xlate() to use new dma_get_any_slave_channel().
> 
> v2: Use of_dma_slave_xlate() rather than of_dma_simple_xlate(), as
>     suggested by Arnd Bergmann.
> 
> This patch is part of a series with strong internal depdendencies. I'm
> looking for an ack so that I can take the entire series through the Tegra
> and arm-soc trees. The series will be part of a stable branch that can be
> merged into other subsystems if needed to avoid/resolve dependencies.
> ---
>  drivers/dma/tegra20-apb-dma.c | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/aa46848f/attachment.sig>

[PATCH V3] dma: tegra: register as an OF DMA controller

[Stephen Warren]

On 12/03/2013 06:34 PM, Arnd Bergmann wrote:
> On Tuesday 03 December 2013, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Call of_dma_controller_register() so that DMA clients can look up the
>> Tegra DMA controller using standard APIs. This requires the of_xlate()
>> function to save off the DMA slave ID, and for tegra_dma_slave_config()
>> not to over-write this information; once DMA client drivers are converted
>> to dma_request_slave_channel() and DT-based lookups, they won't set this
>> field of struct dma_slave_config anymore.
...
> Also, one (long-running, not just for this driver) comment I have is
> about this snippet:
> 
>> @@ -340,6 +342,8 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
>>         }
>>  
>>         memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
>> +       if (!tdc->slave_id)
>> +               tdc->slave_id = sconfig->slave_id;
>>         tdc->config_init = true;
>>         return 0;
>>  }
> 
> We really need to be better at having a common set of rules regarding what
> it actually means to set the slave_id through dma_slave_config(). IMHO
> we should just treat it as a bug for any dmaengine driver that is configured
> through DT, or we should get rid of this entirely.

By the end of this series (for the Tegra driver) I can remove those "+"
lines you quoted above; the slave ID will only come from DT through
of_xlate, and any slave ID in the slave_config will simply be ignored.

[PATCH V3] dma: tegra: register as an OF DMA controller

[Arnd Bergmann]

On Wednesday 04 December 2013, Stephen Warren wrote:
> > 
> > We really need to be better at having a common set of rules regarding what
> > it actually means to set the slave_id through dma_slave_config(). IMHO
> > we should just treat it as a bug for any dmaengine driver that is configured
> > through DT, or we should get rid of this entirely.
> 
> By the end of this series (for the Tegra driver) I can remove those "+"
> lines you quoted above; the slave ID will only come from DT through
> of_xlate, and any slave ID in the slave_config will simply be ignored.

Ok, that's certainly a step in the right direction. However, as I said
my comment wasn't really directed at you and I think it's more important
to have a clear policy that we can enforce across all dmaengine drivers.

It's really up to Vinod and Dan to decide how it should be handled
in general.

	Arnd