| Message ID | 1386102787-21839-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | Stephen Warren |
| Headers | show |
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@nvidia.com > Cc: pdeschrijver@nvidia.com > Cc: linux-tegra@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Vinod Koul <vinod.koul@intel.com> > Cc: dmaengine@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 -- 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 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@nvidia.com > Cc: pdeschrijver@nvidia.com > Cc: linux-tegra@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Vinod Koul <vinod.koul@intel.com> > Cc: dmaengine@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>
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. -- 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 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 -- 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..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];