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

[PATCH V2] 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 filter
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>
---
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 | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index afa5844c9346..206edd11fd2f 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;
 };
@@ -206,6 +208,7 @@ struct tegra_dma_channel {
 /* tegra_dma: Tegra DMA specific information */
 struct tegra_dma {
 	struct dma_device		dma_dev;
+	struct of_dma_slave_xlate_info	xlate_info;
 	struct device			*dev;
 	struct clk			*dma_clk;
 	struct reset_control		*rst;
@@ -340,6 +343,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 +947,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 +1091,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 +1211,18 @@ 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 int tegra_dma_of_xlate_post_alloc(struct dma_chan *chan,
+					 struct of_phandle_args *dma_spec)
+{
+	struct tegra_dma_channel *tdc = to_tegra_dma_chan(chan);
+
+	tdc->slave_id = dma_spec->args[0];
+
+	return 0;
 }
 
 /* Tegra20 specific DMA controller information */
@@ -1245,6 +1262,8 @@ static const struct of_device_id tegra_dma_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_dma_of_match);
 
+static struct platform_driver tegra_dmac_driver;
+
 static int tegra_dma_probe(struct platform_device *pdev)
 {
 	struct resource	*res;
@@ -1383,10 +1402,22 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		goto err_irq;
 	}
 
+	tdma->xlate_info.device = &tdma->dma_dev;
+	tdma->xlate_info.post_alloc = tegra_dma_of_xlate_post_alloc;
+	ret = of_dma_controller_register(pdev->dev.of_node,
+					 of_dma_slave_xlate, &tdma->xlate_info);
+	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 V2] dma: tegra: register as an OF DMA controller

[Arnd Bergmann]

On Monday 25 November 2013 14:53:36 Stephen Warren wrote:
> 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.

Did I suggest of_dma_slave_xlate()? I don't think I've actually heard
of that function, and I can't find anything in the kernel source or
using google.

Why not just use an open-coded xlate function?

	Arnd

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

[Stephen Warren]

On 11/25/2013 03:09 PM, Arnd Bergmann wrote:
> On Monday 25 November 2013 14:53:36 Stephen Warren wrote:
>> 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.
> 
> Did I suggest of_dma_slave_xlate()? I don't think I've actually heard
> of that function, and I can't find anything in the kernel source or
> using google.
> 
> Why not just use an open-coded xlate function?

Well, you suggested not using of_dma_simple_xlate() since it wasn't
appropriate. I then started to implement an open-coded xlate function,
but found that it was 99% identical to the same thing in the mmp driver,
and hence created a common of_dma_slave_xlate() so as not to just
cut/paste it everywhere. Unfortunately, I only sent that patch to
dmaengine at vger.kernel.org and the DMA maintainers, and there's no
archive of that list:-(

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

[Dan Williams]

On Mon, Nov 25, 2013 at 2:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/25/2013 03:09 PM, Arnd Bergmann wrote:
>> On Monday 25 November 2013 14:53:36 Stephen Warren wrote:
>>> 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.
>>
>> Did I suggest of_dma_slave_xlate()? I don't think I've actually heard
>> of that function, and I can't find anything in the kernel source or
>> using google.
>>
>> Why not just use an open-coded xlate function?
>
> Well, you suggested not using of_dma_simple_xlate() since it wasn't
> appropriate. I then started to implement an open-coded xlate function,
> but found that it was 99% identical to the same thing in the mmp driver,
> and hence created a common of_dma_slave_xlate() so as not to just
> cut/paste it everywhere. Unfortunately, I only sent that patch to
> dmaengine at vger.kernel.org and the DMA maintainers, and there's no
> archive of that list:-(

There is, however, an archive of the patches:

dma: add common of_dma_slave_xlate()
https://patchwork.kernel.org/patch/3234751/

dma: mmp_pdma: use of_dma_slave_xlate()
https://patchwork.kernel.org/patch/3234761/

..btw I think we should squash those two together.

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

[Stephen Warren]

On 11/25/2013 04:09 PM, Dan Williams wrote:
> On Mon, Nov 25, 2013 at 2:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 11/25/2013 03:09 PM, Arnd Bergmann wrote:
>>> On Monday 25 November 2013 14:53:36 Stephen Warren wrote:
>>>> 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.
>>>
>>> Did I suggest of_dma_slave_xlate()? I don't think I've actually heard
>>> of that function, and I can't find anything in the kernel source or
>>> using google.
>>>
>>> Why not just use an open-coded xlate function?
>>
>> Well, you suggested not using of_dma_simple_xlate() since it wasn't
>> appropriate. I then started to implement an open-coded xlate function,
>> but found that it was 99% identical to the same thing in the mmp driver,
>> and hence created a common of_dma_slave_xlate() so as not to just
>> cut/paste it everywhere. Unfortunately, I only sent that patch to
>> dmaengine at vger.kernel.org and the DMA maintainers, and there's no
>> archive of that list:-(
> 
> There is, however, an archive of the patches:
> 
> dma: add common of_dma_slave_xlate()
> https://patchwork.kernel.org/patch/3234751/
> 
> dma: mmp_pdma: use of_dma_slave_xlate()
> https://patchwork.kernel.org/patch/3234761/

Ah, that's useful.

> ..btw I think we should squash those two together.

Isn't it more typical to do core work in one patch, then port each
driver individually? That keeps the patches small and logically
distinct, which could help bisect in some cases. But I guess I can go
either way.

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

[Dan Williams]

On Mon, Nov 25, 2013 at 3:14 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/25/2013 04:09 PM, Dan Williams wrote:
>> On Mon, Nov 25, 2013 at 2:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 11/25/2013 03:09 PM, Arnd Bergmann wrote:
>>>> On Monday 25 November 2013 14:53:36 Stephen Warren wrote:
>>>>> 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.
>>>>
>>>> Did I suggest of_dma_slave_xlate()? I don't think I've actually heard
>>>> of that function, and I can't find anything in the kernel source or
>>>> using google.
>>>>
>>>> Why not just use an open-coded xlate function?
>>>
>>> Well, you suggested not using of_dma_simple_xlate() since it wasn't
>>> appropriate. I then started to implement an open-coded xlate function,
>>> but found that it was 99% identical to the same thing in the mmp driver,
>>> and hence created a common of_dma_slave_xlate() so as not to just
>>> cut/paste it everywhere. Unfortunately, I only sent that patch to
>>> dmaengine at vger.kernel.org and the DMA maintainers, and there's no
>>> archive of that list:-(
>>
>> There is, however, an archive of the patches:
>>
>> dma: add common of_dma_slave_xlate()
>> https://patchwork.kernel.org/patch/3234751/
>>
>> dma: mmp_pdma: use of_dma_slave_xlate()
>> https://patchwork.kernel.org/patch/3234761/
>
> Ah, that's useful.
>
>> ..btw I think we should squash those two together.
>
> Isn't it more typical to do core work in one patch, then port each
> driver individually? That keeps the patches small and logically
> distinct, which could help bisect in some cases. But I guess I can go
> either way.

Including a user conversion in the same patch that introduces the api
change makes it clear what is going on...  for example a reviewer
could see that the code comes from mmp_pdma unmolested.  Inevitably
the first question is always "show me the user".  The bisect case is
also a good example.  Since the api change by itself dead code if we
revert the only user we implicitly want to revert the core change.

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

[Thierry Reding]

On Mon, Nov 25, 2013 at 02:53:36PM -0700, Stephen Warren wrote:
[...]
>  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> +	if (!tdc->slave_id)
> +		tdc->slave_id = sconfig->slave_id;
>  	tdc->config_init = true;

This could use some blank lines to unclutter it a bit.

>  	return 0;
>  }
> @@ -942,7 +947,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;

Perhaps I'm missing something, but couldn't we reuse the .slave_id field
of struct dma_slave_config? It seems like it might be overwritten by the
DMA engine core or users when they call dmaengine_slave_config().

But wouldn't it be better to have the core take care of all the slave ID
management, so we don't have to jump through hoops? Or perhaps the
concept isn't general enough to map well to other drivers.

>  /* Tegra20 specific DMA controller information */
> @@ -1245,6 +1262,8 @@ static const struct of_device_id tegra_dma_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, tegra_dma_of_match);
>  
> +static struct platform_driver tegra_dmac_driver;
> +

This doesn't seem to be used anymore.

>  static int tegra_dma_probe(struct platform_device *pdev)
>  {
>  	struct resource	*res;
> @@ -1383,10 +1402,22 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  		goto err_irq;
>  	}
>  
> +	tdma->xlate_info.device = &tdma->dma_dev;
> +	tdma->xlate_info.post_alloc = tegra_dma_of_xlate_post_alloc;
> +	ret = of_dma_controller_register(pdev->dev.of_node,
> +					 of_dma_slave_xlate, &tdma->xlate_info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Tegra20 APB DMA OF registration failed %d\n", ret);
> +		goto err_unregister_dma_dev;
> +	}

Would it be useful to move this into the core and have it register the
OF parts transparently to the driver? That's of course nothing that
should be done in this patch.

Thierry
-------------- 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/20131129/c1550f62/attachment.sig>

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

[Arnd Bergmann]

On Monday 25 November 2013, Stephen Warren wrote:
> Well, you suggested not using of_dma_simple_xlate() since it wasn't
> appropriate. I then started to implement an open-coded xlate function,
> but found that it was 99% identical to the same thing in the mmp driver,
> and hence created a common of_dma_slave_xlate() so as not to just
> cut/paste it everywhere. Unfortunately, I only sent that patch to
> dmaengine at vger.kernel.org and the DMA maintainers, and there's no
> archive of that list:-(

Ok, I see. However, I think the need for nontrivial code to be duplicated
across drivers is not a sign that we are missing a generic xlate function
and another indirection level, but rather that we got the interface
for dma_get_slave_channel() wrong (yes, that would be my fault).

Can you try coming up with a different method to achieve the same
where you use a different helper from the driver specific xlate
function that does not require a callback?

I think dma_get_slave_channel is great if you have one channel per
request line and you can directly look up the channel from the
DT data, but it is not good if you have pick a channel and work
around the race.

	Arnd

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

[Stephen Warren]

On 11/29/2013 02:08 PM, Arnd Bergmann wrote:
> On Monday 25 November 2013, Stephen Warren wrote:
>> Well, you suggested not using of_dma_simple_xlate() since it wasn't
>> appropriate. I then started to implement an open-coded xlate function,
>> but found that it was 99% identical to the same thing in the mmp driver,
>> and hence created a common of_dma_slave_xlate() so as not to just
>> cut/paste it everywhere. Unfortunately, I only sent that patch to
>> dmaengine at vger.kernel.org and the DMA maintainers, and there's no
>> archive of that list:-(
> 
> Ok, I see. However, I think the need for nontrivial code to be duplicated
> across drivers is not a sign that we are missing a generic xlate function
> and another indirection level, but rather that we got the interface
> for dma_get_slave_channel() wrong (yes, that would be my fault).
> 
> Can you try coming up with a different method to achieve the same
> where you use a different helper from the driver specific xlate
> function that does not require a callback?
> 
> I think dma_get_slave_channel is great if you have one channel per
> request line and you can directly look up the channel from the
> DT data, but it is not good if you have pick a channel and work
> around the race.

Hmm. Can you take a look at "[PATCH V4] dma: add
dma_get_any_slave_channel(), for use in of_xlate()" at the link below.
It still implements this via xlate, but I don't see any benefit in
making drivers use a different API to request slave channels based on
how the DMA controller works.

http://lkml.org/lkml/2013/11/26/408

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

[Stephen Warren]

On 11/29/2013 07:17 AM, Thierry Reding wrote:
> On Mon, Nov 25, 2013 at 02:53:36PM -0700, Stephen Warren wrote: 
> [...]
>> memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig)); +	if
>> (!tdc->slave_id) +		tdc->slave_id = sconfig->slave_id; 
>> tdc->config_init = true;
> 
> This could use some blank lines to unclutter it a bit.

To be honest, I feel the opposite; random blank lines sprinkled in the
middle of related code make the code structure harder to follow.


>> @@ -942,7 +947,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;
> 
> Perhaps I'm missing something, but couldn't we reuse the .slave_id
> field of struct dma_slave_config? It seems like it might be
> overwritten by the DMA engine core or users when they call
> dmaengine_slave_config().

The slave ID seems channel-specific to me, and hence should be managed
at the channel level. struct dma_slave_config is the client-specified
runtime properties. As you mention, I also worry about client drivers
trampling over the dma_slave_config data, so storing it where they
can't doesn't seem like a good idea.

> But wouldn't it be better to have the core take care of all the
> slave ID management, so we don't have to jump through hoops? Or
> perhaps the concept isn't general enough to map well to other
> drivers.

It's a HW specific detail, I think. At least, the representation of
the data in a DT binding is, and hence so is the parsing of the data
from DT. The rest of the code that's in the driver re: slave ID is
trivial enough it doesn't seem worth sharing. And in fact, once this
series is done, we can even remove the conditional assignments to
slave_id and require that it be provided by DT and never from
dma_slave_config, which simplifies it even further.

>> static int tegra_dma_probe(struct platform_device *pdev) { struct
>> resource	*res; @@ -1383,10 +1402,22 @@ static int
>> tegra_dma_probe(struct platform_device *pdev) goto err_irq; }
>> 
>> +	tdma->xlate_info.device = &tdma->dma_dev; +
>> tdma->xlate_info.post_alloc = tegra_dma_of_xlate_post_alloc; +
>> ret = of_dma_controller_register(pdev->dev.of_node, +
>> of_dma_slave_xlate, &tdma->xlate_info); +	if (ret < 0) { +
>> dev_err(&pdev->dev, +			"Tegra20 APB DMA OF registration failed
>> %d\n", ret); +		goto err_unregister_dma_dev; +	}
> 
> Would it be useful to move this into the core and have it register
> the OF parts transparently to the driver? That's of course nothing
> that should be done in this patch.

That'd probably be possible, yes, given a few extra fields in struct
dma_device, e.g. for the of_xlate function pointer. As you say, I
won't address it in this patch though.

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

[Arnd Bergmann]

On Tuesday 03 December 2013, Stephen Warren wrote:
> On 11/29/2013 02:08 PM, Arnd Bergmann wrote:

> > Can you try coming up with a different method to achieve the same
> > where you use a different helper from the driver specific xlate
> > function that does not require a callback?
> > 
> > I think dma_get_slave_channel is great if you have one channel per
> > request line and you can directly look up the channel from the
> > DT data, but it is not good if you have pick a channel and work
> > around the race.
> 
> Hmm. Can you take a look at "[PATCH V4] dma: add
> dma_get_any_slave_channel(), for use in of_xlate()" at the link below.
> It still implements this via xlate, but I don't see any benefit in
> making drivers use a different API to request slave channels based on
> how the DMA controller works.
> 
> http://lkml.org/lkml/2013/11/26/408
> 

Yes, I think that is good. I can think of a few variations of that
that I would prefer slightly over your code, but it's essentially
what I had in mind and I'm fine with that version getting merged
as well. Here are my ideas for further improvements, I'll leave
it up to you and the dmaengine maintainers to decide what to do
about them:

* Rather than calling private_candidate(), open-code the part you
  need and remove the pointless dma_cap_mask comparison:

	err = -EBUSY;
        list_for_each_entry(chan, &dev->channels, device_node) {
                if (!chan->client_count) {
			err = dma_chan_get(chan);
			break;
		}
	}

* Merge the new function with dma_get_slave_channel(). They really
  do different things, but I think it still makes sense as an API
  to require to always pass the dma_device pointer, and drivers
  that want to get an arbitrary channel can just pass NULL as the
  channel pointer.

	Arnd

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

[Thierry Reding]

On Tue, Dec 03, 2013 at 10:59:54AM -0700, Stephen Warren wrote:
> On 11/29/2013 07:17 AM, Thierry Reding wrote:
> > On Mon, Nov 25, 2013 at 02:53:36PM -0700, Stephen Warren wrote: 
> > [...]
> >> memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig)); +	if
> >> (!tdc->slave_id) +		tdc->slave_id = sconfig->slave_id; 
> >> tdc->config_init = true;
> > 
> > This could use some blank lines to unclutter it a bit.
> 
> To be honest, I feel the opposite; random blank lines sprinkled in the
> middle of related code make the code structure harder to follow.

I don't think they are random at all, but we can probably go on arguing
about that for a long time. So if you prefer to keep it cluttered, feel
free to do so. =P

> >> @@ -942,7 +947,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;
> > 
> > Perhaps I'm missing something, but couldn't we reuse the .slave_id
> > field of struct dma_slave_config? It seems like it might be
> > overwritten by the DMA engine core or users when they call
> > dmaengine_slave_config().
> 
> The slave ID seems channel-specific to me, and hence should be managed
> at the channel level. struct dma_slave_config is the client-specified
> runtime properties. As you mention, I also worry about client drivers
> trampling over the dma_slave_config data, so storing it where they
> can't doesn't seem like a good idea.

I think you meant "does seem like a good idea"? The reason why this had
me puzzled is probably that I haven't seen this pattern in subsystems
I'm more familiar with. Like you said, it seems like a channel-specific
property, so clients should have no business modifying it.

But I assume there were valid reasons for doing things this way, so I
withdraw my objections.

> >> static int tegra_dma_probe(struct platform_device *pdev) { struct
> >> resource	*res; @@ -1383,10 +1402,22 @@ static int
> >> tegra_dma_probe(struct platform_device *pdev) goto err_irq; }
> >> 
> >> +	tdma->xlate_info.device = &tdma->dma_dev; +
> >> tdma->xlate_info.post_alloc = tegra_dma_of_xlate_post_alloc; +
> >> ret = of_dma_controller_register(pdev->dev.of_node, +
> >> of_dma_slave_xlate, &tdma->xlate_info); +	if (ret < 0) { +
> >> dev_err(&pdev->dev, +			"Tegra20 APB DMA OF registration failed
> >> %d\n", ret); +		goto err_unregister_dma_dev; +	}
> > 
> > Would it be useful to move this into the core and have it register
> > the OF parts transparently to the driver? That's of course nothing
> > that should be done in this patch.
> 
> That'd probably be possible, yes, given a few extra fields in struct
> dma_device, e.g. for the of_xlate function pointer. As you say, I
> won't address it in this patch though.

Okay, that's fine.

Thierry
-------------- 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/a2e263e8/attachment.sig>

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

[Stephen Warren]

On 12/03/2013 06:22 PM, Arnd Bergmann wrote:
> On Tuesday 03 December 2013, Stephen Warren wrote:
>> On 11/29/2013 02:08 PM, Arnd Bergmann wrote:
> 
>>> Can you try coming up with a different method to achieve the same
>>> where you use a different helper from the driver specific xlate
>>> function that does not require a callback?
>>>
>>> I think dma_get_slave_channel is great if you have one channel per
>>> request line and you can directly look up the channel from the
>>> DT data, but it is not good if you have pick a channel and work
>>> around the race.
>>
>> Hmm. Can you take a look at "[PATCH V4] dma: add
>> dma_get_any_slave_channel(), for use in of_xlate()" at the link below.
>> It still implements this via xlate, but I don't see any benefit in
>> making drivers use a different API to request slave channels based on
>> how the DMA controller works.
>>
>> http://lkml.org/lkml/2013/11/26/408
> 
> Yes, I think that is good. I can think of a few variations of that
> that I would prefer slightly over your code, but it's essentially
> what I had in mind and I'm fine with that version getting merged
> as well. Here are my ideas for further improvements, I'll leave
> it up to you and the dmaengine maintainers to decide what to do
> about them:
> 
> * Rather than calling private_candidate(), open-code the part you
>   need and remove the pointless dma_cap_mask comparison:
> 
> 	err = -EBUSY;
>         list_for_each_entry(chan, &dev->channels, device_node) {
>                 if (!chan->client_count) {
> 			err = dma_chan_get(chan);
> 			break;
> 		}
> 	}

Lars-Peter had specifically suggested to call private_candidate(). Lars,
what do you think about open-coding this? Arnd's suggestion would skip
the DMA_PRIVATE checking that private_candidate() does, and I'm not sure
what the implications of that would be.

> * Merge the new function with dma_get_slave_channel(). They really
>   do different things, but I think it still makes sense as an API
>   to require to always pass the dma_device pointer, and drivers
>   that want to get an arbitrary channel can just pass NULL as the
>   channel pointer.

I suppose one could do that, although the two operations seem pretty
semantically different to me, such that merging them doesn't seem correct.

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

[Lars-Peter Clausen]

On 12/04/2013 06:09 PM, Stephen Warren wrote:
> On 12/03/2013 06:22 PM, Arnd Bergmann wrote:
>> On Tuesday 03 December 2013, Stephen Warren wrote:
>>> On 11/29/2013 02:08 PM, Arnd Bergmann wrote:
>>
>>>> Can you try coming up with a different method to achieve the same
>>>> where you use a different helper from the driver specific xlate
>>>> function that does not require a callback?
>>>>
>>>> I think dma_get_slave_channel is great if you have one channel per
>>>> request line and you can directly look up the channel from the
>>>> DT data, but it is not good if you have pick a channel and work
>>>> around the race.
>>>
>>> Hmm. Can you take a look at "[PATCH V4] dma: add
>>> dma_get_any_slave_channel(), for use in of_xlate()" at the link below.
>>> It still implements this via xlate, but I don't see any benefit in
>>> making drivers use a different API to request slave channels based on
>>> how the DMA controller works.
>>>
>>> http://lkml.org/lkml/2013/11/26/408
>>
>> Yes, I think that is good. I can think of a few variations of that
>> that I would prefer slightly over your code, but it's essentially
>> what I had in mind and I'm fine with that version getting merged
>> as well. Here are my ideas for further improvements, I'll leave
>> it up to you and the dmaengine maintainers to decide what to do
>> about them:
>>
>> * Rather than calling private_candidate(), open-code the part you
>>   need and remove the pointless dma_cap_mask comparison:
>>
>> 	err = -EBUSY;
>>         list_for_each_entry(chan, &dev->channels, device_node) {
>>                 if (!chan->client_count) {
>> 			err = dma_chan_get(chan);
>> 			break;
>> 		}
>> 	}
> 
> Lars-Peter had specifically suggested to call private_candidate(). Lars,
> what do you think about open-coding this? Arnd's suggestion would skip
> the DMA_PRIVATE checking that private_candidate() does, and I'm not sure
> what the implications of that would be.

It's not like this is a hot path or that the mask checking is expensive. I'd
keep it as it is for the sake of not having the same code twice. And the
DMA_PRIVATE check should probably also stay.

- Lars

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

[Dan Williams]

On Tue, Dec 3, 2013 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 03 December 2013, Stephen Warren wrote:
>> On 11/29/2013 02:08 PM, Arnd Bergmann wrote:
>
>> > Can you try coming up with a different method to achieve the same
>> > where you use a different helper from the driver specific xlate
>> > function that does not require a callback?
>> >
>> > I think dma_get_slave_channel is great if you have one channel per
>> > request line and you can directly look up the channel from the
>> > DT data, but it is not good if you have pick a channel and work
>> > around the race.
>>
>> Hmm. Can you take a look at "[PATCH V4] dma: add
>> dma_get_any_slave_channel(), for use in of_xlate()" at the link below.
>> It still implements this via xlate, but I don't see any benefit in
>> making drivers use a different API to request slave channels based on
>> how the DMA controller works.
>>
>> http://lkml.org/lkml/2013/11/26/408
>>
>
> Yes, I think that is good. I can think of a few variations of that
> that I would prefer slightly over your code, but it's essentially
> what I had in mind and I'm fine with that version getting merged
> as well. Here are my ideas for further improvements, I'll leave
> it up to you and the dmaengine maintainers to decide what to do
> about them:
>
> * Rather than calling private_candidate(), open-code the part you
>   need and remove the pointless dma_cap_mask comparison:
>
>         err = -EBUSY;
>         list_for_each_entry(chan, &dev->channels, device_node) {
>                 if (!chan->client_count) {
>                         err = dma_chan_get(chan);
>                         break;
>                 }
>         }
>
> * Merge the new function with dma_get_slave_channel(). They really
>   do different things, but I think it still makes sense as an API
>   to require to always pass the dma_device pointer, and drivers
>   that want to get an arbitrary channel can just pass NULL as the
>   channel pointer.
>

* nod.

I have a similar reaction to dma_request_channel().  There should be
more than enough use case to find some common ground and reduce the
need for magic filter routines.

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

[Arnd Bergmann]

On Friday 06 December 2013, Dan Williams wrote:
> * nod.
> 
> I have a similar reaction to dma_request_channel().  There should be
> more than enough use case to find some common ground and reduce the
> need for magic filter routines.

Right. I didn't really like the addition of the _compat variant, though
I can see how it helps drivers that are transitioning from pdata to
DT probing. Once we have progressed enough with the ARM DT conversion,
a lot of those can surely be converted back to plain 
dma_request_slave_channel(), and if a significant number remain that
won't use DT, we can come up with a way to use the same interface
using the equivalent of clkdev to connect devices to request lines.

Also, once things have cooled down a little, we can do a mass-conversion
to the ERR_PTR variant and kill off the ones that just return NULL
on error.

Looking at the slave drivers that use the old dma_request_channel(),
I see these classes that are not moving to DT:

* Atmel arch/avr32 (some shared with ARM mach-at91, using DT there)
* Renesas arch/sh (some shared with ARM shmobile, using DT there)
* x86 topcliff/PCH
* mfd/timberdale
* ARM ep93xx, sa11x0

The other ones are all ARM or PowerPC platforms that should migrate
to describing their DMA channels in DT in the future and kill their
filter functions.

	Arnd