dma: tegra: register as an OF DMA controller

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.

Quoted 7 lines · 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?

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · 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.

Quoted 5 lines

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:-(

Quoted 16 lines · Stephen Warren

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · 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.

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 25 lines · Dan Williams

Quoted 16 lines · Stephen Warren

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · 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.

Quoted 5 lines

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?

Quoted 7 lines

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:-(

Quoted 7 lines

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.

Quoted 1 line

..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.

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:-(

Quoted 7 lines · Stephen Warren

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.

Quoted 8 lines · Arnd Bergmann

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

Quoted 11 lines · Stephen Warren

Quoted 8 lines · Arnd Bergmann

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.

Quoted 7 lines

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.

Quoted 30 lines · Arnd Bergmann

Quoted 11 lines · Stephen Warren

Quoted 8 lines · Arnd Bergmann

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.

Quoted 7 lines

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:

Quoted 7 lines

	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.

Quoted 11 lines · Stephen Warren

Quoted 8 lines · Arnd Bergmann

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.

Quoted 8 lines

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.

drivers/dma/tegra20-apb-dma.c +0 -0

 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;
 	}

On Mon, Nov 25, 2013 at 02:53:36PM -0700, Stephen Warren wrote:
[...]

Quoted 3 lines

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.

Quoted 4 lines

(quoted diff) +0 -0

@@ -942,7 +947,7 @@ static struct dma_async_tx_descriptor
 *tegra_dma_prep_slave_sg( ahb_seq |=
 TEGRA_APBDMA_AHBSEQ_BUS_WIDTH_32;
 

Quoted 3 lines

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.

Quoted 4 lines

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; }

Quoted 6 lines

(quoted diff) +1 -0

+	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.

Quoted 7 lines · Thierry Reding

On Mon, Nov 25, 2013 at 02:53:36PM -0700, Stephen Warren wrote:
[...]

Quoted 3 lines

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.

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -942,7 +947,7 @@ static struct dma_async_tx_descriptor
 *tegra_dma_prep_slave_sg( ahb_seq |=
 TEGRA_APBDMA_AHBSEQ_BUS_WIDTH_32;
 

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 4 lines

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; }

Quoted 10 lines

Quoted 6 lines

(quoted diff) +1 -0

+	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.