| Message ID | 1385416416-3536-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Stephen Warren |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org and the DMA maintainers, and there's no archive of that list:-( -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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];