Patchwork [2/9] dma: tegra20-apbdma: move to generic device tree bindings

login
register
mail settings
Submitter Richard Zhao
Date July 24, 2013, 4:09 a.m.
Message ID <1374639002-16753-3-git-send-email-rizhao@nvidia.com>
Download mbox | patch
Permalink /patch/261272/
State Superseded, archived
Headers show

Comments

Richard Zhao - July 24, 2013, 4:09 a.m.
Update tegra20-apbdma driver to adopt generic DMA device tree bindings.
It calls of_dma_controller_register() with of_dma_simple_xlate to get
the generic DMA device tree helper support. The #dma-cells for apbdma
must be 1, which is slave ID.

The existing nvidia,dma-request-selector still works there, and the
support will be removed after all clients get converted to generic DMA
device tree helper.

Signed-off-by: Richard Zhao <rizhao@nvidia.com>
---
 drivers/dma/tegra20-apb-dma.c | 46 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)
Stephen Warren - July 26, 2013, 7:27 p.m.
(Stripping the Cc list a lot)

On 07/23/2013 10:09 PM, Richard Zhao wrote:
> Update tegra20-apbdma driver to adopt generic DMA device tree bindings.
> It calls of_dma_controller_register() with of_dma_simple_xlate to get
> the generic DMA device tree helper support. The #dma-cells for apbdma
> must be 1, which is slave ID.
> 
> The existing nvidia,dma-request-selector still works there, and the
> support will be removed after all clients get converted to generic DMA
> device tree helper.

(BTW, I would like to take this series through the Tegra tree to
simplify dependencies. So, I'm looking for ack's on the drivers rather
than for them to be applied. I had hoped Richard would point this out
when posting the patches)

> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c

> +static bool tegra_dma_filter_fn(struct dma_chan *dc, void *param)
> +{
> +	if (dc->device->dev->driver == &tegra_dmac_driver.driver) {
> +		struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> +		unsigned req = *(unsigned *)param;
> +
> +		tdc->slave_id = req;
> +
> +		return true;
> +	}
> +	return false;
> +}
> +
> +static struct of_dma_filter_info tegra_dma_info = {
> +	.filter_fn = tegra_dma_filter_fn,
> +};

Why does this driver need to define a filter function? I thought the
dmaengine's DT support would already match DMA clients/engines based on
the phandle in DT? Isn't the need to pass the slave channel ID into
drivers something pretty fundamental to dmaengine, and hence something
it already supports?
--
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
Richard Zhao - July 30, 2013, 3:06 a.m.
On Sat, Jul 27, 2013 at 03:27:58AM +0800, Stephen Warren wrote:
> (Stripping the Cc list a lot)
> 
> On 07/23/2013 10:09 PM, Richard Zhao wrote:
> > Update tegra20-apbdma driver to adopt generic DMA device tree bindings.
> > It calls of_dma_controller_register() with of_dma_simple_xlate to get
> > the generic DMA device tree helper support. The #dma-cells for apbdma
> > must be 1, which is slave ID.
> > 
> > The existing nvidia,dma-request-selector still works there, and the
> > support will be removed after all clients get converted to generic DMA
> > device tree helper.
> 
> (BTW, I would like to take this series through the Tegra tree to
> simplify dependencies. So, I'm looking for ack's on the drivers rather
> than for them to be applied. I had hoped Richard would point this out
> when posting the patches)
Sorry I forgot to add maintainr-pick-up suggestions.
> 
> > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> 
> > +static bool tegra_dma_filter_fn(struct dma_chan *dc, void *param)
> > +{
> > +	if (dc->device->dev->driver == &tegra_dmac_driver.driver) {
> > +		struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> > +		unsigned req = *(unsigned *)param;
> > +
> > +		tdc->slave_id = req;
> > +
> > +		return true;
> > +	}
> > +	return false;
> > +}
> > +
> > +static struct of_dma_filter_info tegra_dma_info = {
> > +	.filter_fn = tegra_dma_filter_fn,
> > +};
> 
> Why does this driver need to define a filter function? I thought the
> dmaengine's DT support would already match DMA clients/engines based on
> the phandle in DT? Isn't the need to pass the slave channel ID into
> drivers something pretty fundamental to dmaengine, and hence something
> it already supports?
of_dma_simple_xlate only pass slave id to filter function. It needs
change to pass dt node too. I didn't start work on the common code
change yet. Do you think we need to hold it until common code change?

Thanks
Richard
> --
> 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
--
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
Stephen Warren - July 30, 2013, 4:44 p.m.
On 07/29/2013 09:06 PM, Richard Zhao wrote:
> On Sat, Jul 27, 2013 at 03:27:58AM +0800, Stephen Warren wrote:
>> (Stripping the Cc list a lot)
>>
>> On 07/23/2013 10:09 PM, Richard Zhao wrote:
>>> Update tegra20-apbdma driver to adopt generic DMA device tree bindings.
>>> It calls of_dma_controller_register() with of_dma_simple_xlate to get
>>> the generic DMA device tree helper support. The #dma-cells for apbdma
>>> must be 1, which is slave ID.
>>>
>>> The existing nvidia,dma-request-selector still works there, and the
>>> support will be removed after all clients get converted to generic DMA
>>> device tree helper.

>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>
>>> +static bool tegra_dma_filter_fn(struct dma_chan *dc, void *param)
>>> +{
>>> +	if (dc->device->dev->driver == &tegra_dmac_driver.driver) {
>>> +		struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> +		unsigned req = *(unsigned *)param;
>>> +
>>> +		tdc->slave_id = req;
>>> +
>>> +		return true;
>>> +	}
>>> +	return false;
>>> +}
>>> +
>>> +static struct of_dma_filter_info tegra_dma_info = {
>>> +	.filter_fn = tegra_dma_filter_fn,
>>> +};
>>
>> Why does this driver need to define a filter function? I thought the
>> dmaengine's DT support would already match DMA clients/engines based on
>> the phandle in DT? Isn't the need to pass the slave channel ID into
>> drivers something pretty fundamental to dmaengine, and hence something
>> it already supports?
>
> of_dma_simple_xlate only pass slave id to filter function. It needs
> change to pass dt node too. I didn't start work on the common code
> change yet. Do you think we need to hold it until common code change?

Oh dear, I can't see how the core DT DMA support can work for anyone
without the ability to limit DMA clients to the specific DMA controller
that's specified in DT. If that functionality truly is missing, then it
certainly needs to be added first.
--
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
Richard Zhao - July 31, 2013, 3:52 a.m.
On Wed, Jul 31, 2013 at 12:44:18AM +0800, Stephen Warren wrote:
> On 07/29/2013 09:06 PM, Richard Zhao wrote:
> > On Sat, Jul 27, 2013 at 03:27:58AM +0800, Stephen Warren wrote:
> >> (Stripping the Cc list a lot)
> >>
> >> On 07/23/2013 10:09 PM, Richard Zhao wrote:
> >>> Update tegra20-apbdma driver to adopt generic DMA device tree bindings.
> >>> It calls of_dma_controller_register() with of_dma_simple_xlate to get
> >>> the generic DMA device tree helper support. The #dma-cells for apbdma
> >>> must be 1, which is slave ID.
> >>>
> >>> The existing nvidia,dma-request-selector still works there, and the
> >>> support will be removed after all clients get converted to generic DMA
> >>> device tree helper.
> 
> >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> >>
> >>> +static bool tegra_dma_filter_fn(struct dma_chan *dc, void *param)
> >>> +{
> >>> +	if (dc->device->dev->driver == &tegra_dmac_driver.driver) {
> >>> +		struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> >>> +		unsigned req = *(unsigned *)param;
> >>> +
> >>> +		tdc->slave_id = req;
> >>> +
> >>> +		return true;
> >>> +	}
> >>> +	return false;
> >>> +}
> >>> +
> >>> +static struct of_dma_filter_info tegra_dma_info = {
> >>> +	.filter_fn = tegra_dma_filter_fn,
> >>> +};
> >>
> >> Why does this driver need to define a filter function? I thought the
> >> dmaengine's DT support would already match DMA clients/engines based on
> >> the phandle in DT? Isn't the need to pass the slave channel ID into
> >> drivers something pretty fundamental to dmaengine, and hence something
> >> it already supports?
> >
> > of_dma_simple_xlate only pass slave id to filter function. It needs
> > change to pass dt node too. I didn't start work on the common code
> > change yet. Do you think we need to hold it until common code change?
> 
> Oh dear, I can't see how the core DT DMA support can work for anyone
> without the ability to limit DMA clients to the specific DMA controller
> that's specified in DT. If that functionality truly is missing, then it
> certainly needs to be added first.

omap-dma uses the same 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
Stephen Warren - July 31, 2013, 9:33 p.m.
On 07/30/2013 09:52 PM, Richard Zhao wrote:
> On Wed, Jul 31, 2013 at 12:44:18AM +0800, Stephen Warren wrote:
>> On 07/29/2013 09:06 PM, Richard Zhao wrote:
>>> On Sat, Jul 27, 2013 at 03:27:58AM +0800, Stephen Warren wrote:
>>>> (Stripping the Cc list a lot)
>>>>
>>>> On 07/23/2013 10:09 PM, Richard Zhao wrote:
>>>>> Update tegra20-apbdma driver to adopt generic DMA device tree bindings.
>>>>> It calls of_dma_controller_register() with of_dma_simple_xlate to get
>>>>> the generic DMA device tree helper support. The #dma-cells for apbdma
>>>>> must be 1, which is slave ID.
...
>>>> Why does this driver need to define a filter function? I thought the
>>>> dmaengine's DT support would already match DMA clients/engines based on
>>>> the phandle in DT? Isn't the need to pass the slave channel ID into
>>>> drivers something pretty fundamental to dmaengine, and hence something
>>>> it already supports?
>>>
>>> of_dma_simple_xlate only pass slave id to filter function. It needs
>>> change to pass dt node too. I didn't start work on the common code
>>> change yet. Do you think we need to hold it until common code change?
>>
>> Oh dear, I can't see how the core DT DMA support can work for anyone
>> without the ability to limit DMA clients to the specific DMA controller
>> that's specified in DT. If that functionality truly is missing, then it
>> certainly needs to be added first.
> 
> omap-dma uses the same way. 

The fact that some other driver does the same wrong thing isn't really a
good excuse not to fix it now.
--
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

Patch

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index f137914..0e12f78 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -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 */
+	int slave_id;
 	struct dma_slave_config dma_sconfig;
 	struct tegra_dma_channel_regs	channel_reg;
 };
@@ -219,6 +221,8 @@  struct tegra_dma {
 	struct tegra_dma_channel channels[0];
 };
 
+static struct platform_driver tegra_dmac_driver;
+
 static inline void tdma_write(struct tegra_dma *tdma, u32 reg, u32 val)
 {
 	writel(val, tdma->base_addr + reg);
@@ -339,6 +343,14 @@  static int tegra_dma_slave_config(struct dma_chan *dc,
 	}
 
 	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
+
+	/* If we didn't get slave_id from DT when request channel, use the one
+	 * passed here.
+	 * It makes compatible with legacy nvidia,dma-request-selector.
+	 */
+	if (tdc->slave_id == -EINVAL)
+		tdc->slave_id = sconfig->slave_id;
+
 	tdc->config_init = true;
 	return 0;
 }
@@ -943,7 +955,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;
 
@@ -1087,7 +1099,7 @@  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;
 
@@ -1209,6 +1221,23 @@  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	clk_disable_unprepare(tdma->dma_clk);
 }
 
+static bool tegra_dma_filter_fn(struct dma_chan *dc, void *param)
+{
+	if (dc->device->dev->driver == &tegra_dmac_driver.driver) {
+		struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+		unsigned req = *(unsigned *)param;
+
+		tdc->slave_id = req;
+
+		return true;
+	}
+	return false;
+}
+
+static struct of_dma_filter_info tegra_dma_info = {
+	.filter_fn = tegra_dma_filter_fn,
+};
+
 /* Tegra20 specific DMA controller information */
 static const struct tegra_dma_chip_data tegra20_dma_chip_data = {
 	.nr_channels		= 16,
@@ -1345,6 +1374,7 @@  static int tegra_dma_probe(struct platform_device *pdev)
 				&tdma->dma_dev.channels);
 		tdc->tdma = tdma;
 		tdc->id = i;
+		tdc->slave_id = -EINVAL;
 
 		tasklet_init(&tdc->tasklet, tegra_dma_tasklet,
 				(unsigned long)tdc);
@@ -1378,10 +1408,21 @@  static int tegra_dma_probe(struct platform_device *pdev)
 		goto err_irq;
 	}
 
+	ret = of_dma_controller_register(pdev->dev.of_node,
+					of_dma_simple_xlate, &tegra_dma_info);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Tegra20 APB DMA controller registration failed %d\n",
+			ret);
+		goto err_of_dma;
+	}
+
 	dev_info(&pdev->dev, "Tegra20 APB DMA driver register %d channels\n",
 			cdata->nr_channels);
 	return 0;
 
+err_of_dma:
+	dma_async_device_unregister(&tdma->dma_dev);
 err_irq:
 	while (--i >= 0) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
@@ -1401,6 +1442,7 @@  static int tegra_dma_remove(struct platform_device *pdev)
 	int i;
 	struct tegra_dma_channel *tdc;
 
+	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&tdma->dma_dev);
 
 	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {