Some clean-up changes for the tegra-apb DMA driver.
These have been compile and boot tested for ARM and ARM64. Summary of the
ARM results are below.
Test summary
------------
Build: zImage:
Pass: ( 2/ 2): multi_v7_defconfig, tegra_defconfig
Build: Image:
Pass: ( 1/ 1): defconfig
Boot to userspace: defconfig:
Pass: ( 1/ 1): qemu-vexpress64
Boot to userspace: multi_v7_defconfig:
Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1,
tegra20-trimslice, tegra30-beaver
Boot to userspace: tegra_defconfig:
Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1,
tegra20-trimslice, tegra30-beaver
Jon Hunter (6):
dmaengine: tegra-apb: Correct runtime-pm usage
dmaengine: tegra-apb: Use dev_get_drvdata()
dmaengine: tegra-apb: Save and restore word count
dmaengine: tegra-apb: Only save channel state for those in use
dmaengine: tegra-apb: Update driver to use GFP_NOWAIT
dmaengine: tegra-apb: Free interrupts before killing tasklets
drivers/dma/tegra20-apb-dma.c | 83 ++++++++++++++++++++++++-------------------
1 file changed, 46 insertions(+), 37 deletions(-)
--
2.1.4
The tegra-apb DMA driver enables runtime-pm but never calls
pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
The driver manages the clocks by directly calling clk_prepare_enable()
and clk_unprepare_disable().
Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
the consequence of this is that if runtime-pm is disabled, then the clocks
will remain on the entire time the driver is loaded. However, if
runtime-pm is disabled, then power is not most likely not a concern.
Signed-off-by: Jon Hunter <[email protected]>
---
V2 changes:
- Fixed return value for allocating channel
- Fixed test for return value from pm_runtime_get_sync()
drivers/dma/tegra20-apb-dma.c | 49 ++++++++++++++++++-------------------------
1 file changed, 20 insertions(+), 29 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c8f79dcaaee8..6007111fac1d 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
dma_cookie_init(&tdc->dma_chan);
tdc->config_init = false;
- ret = clk_prepare_enable(tdma->dma_clk);
+
+ ret = pm_runtime_get_sync(tdma->dev);
if (ret < 0)
- dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
- return ret;
+ return ret;
+
+ return 0;
}
static void tegra_dma_free_chan_resources(struct dma_chan *dc)
@@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
list_del(&sg_req->node);
kfree(sg_req);
}
- clk_disable_unprepare(tdma->dma_clk);
+ pm_runtime_put(tdma->dev);
tdc->slave_id = 0;
}
@@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
spin_lock_init(&tdma->global_lock);
pm_runtime_enable(&pdev->dev);
- if (!pm_runtime_enabled(&pdev->dev)) {
+ if (!pm_runtime_enabled(&pdev->dev))
ret = tegra_dma_runtime_resume(&pdev->dev);
- if (ret) {
- dev_err(&pdev->dev, "dma_runtime_resume failed %d\n",
- ret);
- goto err_pm_disable;
- }
- }
+ else
+ ret = pm_runtime_get_sync(&pdev->dev);
- /* Enable clock before accessing registers */
- ret = clk_prepare_enable(tdma->dma_clk);
if (ret < 0) {
- dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
- goto err_pm_disable;
+ pm_runtime_disable(&pdev->dev);
+ return ret;
}
/* Reset DMA controller */
@@ -1382,7 +1378,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
- clk_disable_unprepare(tdma->dma_clk);
+ pm_runtime_put(&pdev->dev);
INIT_LIST_HEAD(&tdma->dma_dev.channels);
for (i = 0; i < cdata->nr_channels; i++) {
@@ -1485,7 +1481,6 @@ err_irq:
tasklet_kill(&tdc->tasklet);
}
-err_pm_disable:
pm_runtime_disable(&pdev->dev);
if (!pm_runtime_status_suspended(&pdev->dev))
tegra_dma_runtime_suspend(&pdev->dev);
@@ -1539,11 +1534,10 @@ static int tegra_dma_runtime_resume(struct device *dev)
static int tegra_dma_pm_suspend(struct device *dev)
{
struct tegra_dma *tdma = dev_get_drvdata(dev);
- int i;
- int ret;
+ int i, ret;
/* Enable clock before accessing register */
- ret = tegra_dma_runtime_resume(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0)
return ret;
@@ -1560,18 +1554,17 @@ static int tegra_dma_pm_suspend(struct device *dev)
}
/* Disable clock */
- tegra_dma_runtime_suspend(dev);
+ pm_runtime_put(dev);
return 0;
}
static int tegra_dma_pm_resume(struct device *dev)
{
struct tegra_dma *tdma = dev_get_drvdata(dev);
- int i;
- int ret;
+ int i, ret;
/* Enable clock before accessing register */
- ret = tegra_dma_runtime_resume(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0)
return ret;
@@ -1592,16 +1585,14 @@ static int tegra_dma_pm_resume(struct device *dev)
}
/* Disable clock */
- tegra_dma_runtime_suspend(dev);
+ pm_runtime_put(dev);
return 0;
}
#endif
static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
-#ifdef CONFIG_PM
- .runtime_suspend = tegra_dma_runtime_suspend,
- .runtime_resume = tegra_dma_runtime_resume,
-#endif
+ SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
+ NULL)
SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
};
--
2.1.4
In the tegra_dma_runtime_suspend/resume functions, the pdev structure
is not needed, and so just call dev_get_drvdata() to get the device
data structure.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/dma/tegra20-apb-dma.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 6007111fac1d..802d66a25de6 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1509,8 +1509,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
static int tegra_dma_runtime_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct tegra_dma *tdma = platform_get_drvdata(pdev);
+ struct tegra_dma *tdma = dev_get_drvdata(dev);
clk_disable_unprepare(tdma->dma_clk);
return 0;
@@ -1518,8 +1517,7 @@ static int tegra_dma_runtime_suspend(struct device *dev)
static int tegra_dma_runtime_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct tegra_dma *tdma = platform_get_drvdata(pdev);
+ struct tegra_dma *tdma = dev_get_drvdata(dev);
int ret;
ret = clk_prepare_enable(tdma->dma_clk);
--
2.1.4
Newer tegra devices have a separate word count register per channel that
contains the number of words to be transferred. This register is not
saved or restored by the suspend/resume helpers for these newer devices
and so ensure that it is.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/dma/tegra20-apb-dma.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 802d66a25de6..94f59c3fcdca 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1549,6 +1549,9 @@ static int tegra_dma_pm_suspend(struct device *dev)
ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
+ if (tdma->chip_data->support_separate_wcount_reg)
+ ch_reg->wcount = tdc_read(tdc,
+ TEGRA_APBDMA_CHAN_WCOUNT);
}
/* Disable clock */
@@ -1574,6 +1577,9 @@ static int tegra_dma_pm_resume(struct device *dev)
struct tegra_dma_channel *tdc = &tdma->channels[i];
struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
+ if (tdma->chip_data->support_separate_wcount_reg)
+ tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
+ ch_reg->wcount);
tdc_write(tdc, TEGRA_APBDMA_CHAN_APBSEQ, ch_reg->apb_seq);
tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, ch_reg->apb_ptr);
tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
--
2.1.4
Currently the tegra-apb DMA driver suspend/resume helpers, save and
restore the registers for all channels regardless of whether they are
in use or not. Change this so that only channels that have been
allocated and configured are saved and restored.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/dma/tegra20-apb-dma.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 94f59c3fcdca..4fc0851069cd 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1544,6 +1544,12 @@ static int tegra_dma_pm_suspend(struct device *dev)
struct tegra_dma_channel *tdc = &tdma->channels[i];
struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
+ /*
+ * Only save the state of DMA channels that are in use.
+ */
+ if (!tdc->config_init)
+ continue;
+
ch_reg->csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
ch_reg->ahb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
@@ -1577,6 +1583,12 @@ static int tegra_dma_pm_resume(struct device *dev)
struct tegra_dma_channel *tdc = &tdma->channels[i];
struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
+ /*
+ * Only restore the state of DMA channels that are in use.
+ */
+ if (!tdc->config_init)
+ continue;
+
if (tdma->chip_data->support_separate_wcount_reg)
tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
ch_reg->wcount);
--
2.1.4
The tegra20-apb-dma driver currently uses the flag GFP_ATOMIC when
allocating memory for structures used in conjunction with the DMA
descriptors. It is preferred that dmaengine drivers use GFP_NOWAIT
instead and so the emergency memory pool will not be used by these
drivers.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/dma/tegra20-apb-dma.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4fc0851069cd..004edcdb2d2e 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -296,7 +296,7 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
spin_unlock_irqrestore(&tdc->lock, flags);
/* Allocate DMA desc */
- dma_desc = kzalloc(sizeof(*dma_desc), GFP_ATOMIC);
+ dma_desc = kzalloc(sizeof(*dma_desc), GFP_NOWAIT);
if (!dma_desc) {
dev_err(tdc2dev(tdc), "dma_desc alloc failed\n");
return NULL;
@@ -336,7 +336,7 @@ static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
}
spin_unlock_irqrestore(&tdc->lock, flags);
- sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_ATOMIC);
+ sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT);
if (!sg_req)
dev_err(tdc2dev(tdc), "sg_req alloc failed\n");
return sg_req;
--
2.1.4
On probe failure or driver removal, before killing any tasklets, ensure
that the channel interrupt is freed to ensure that another channel
interrupt cannot occur and schedule the tasklet again.
Signed-off-by: Jon Hunter <[email protected]>
---
V2 changes:
- Updated patch to use free_irq instead of disable_irq
drivers/dma/tegra20-apb-dma.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 004edcdb2d2e..d3892a106c1e 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1396,8 +1396,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
}
tdc->irq = res->start;
snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
- ret = devm_request_irq(&pdev->dev, tdc->irq,
- tegra_dma_isr, 0, tdc->name, tdc);
+ ret = request_irq(tdc->irq, tegra_dma_isr, 0, tdc->name, tdc);
if (ret) {
dev_err(&pdev->dev,
"request_irq failed with err %d channel %d\n",
@@ -1478,6 +1477,8 @@ err_unregister_dma_dev:
err_irq:
while (--i >= 0) {
struct tegra_dma_channel *tdc = &tdma->channels[i];
+
+ free_irq(tdc->irq, tdc);
tasklet_kill(&tdc->tasklet);
}
@@ -1497,6 +1498,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
tdc = &tdma->channels[i];
+ free_irq(tdc->irq, tdc);
tasklet_kill(&tdc->tasklet);
}
--
2.1.4
On Mon, Nov 9, 2015 at 3:23 PM, Jon Hunter <[email protected]> wrote:
> The tegra-apb DMA driver enables runtime-pm but never calls
> pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
> The driver manages the clocks by directly calling clk_prepare_enable()
> and clk_unprepare_disable().
>
> Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
> with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
> the consequence of this is that if runtime-pm is disabled, then the clocks
> will remain on the entire time the driver is loaded. However, if
> runtime-pm is disabled, then power is not most likely not a concern.
Nitpick
> @@ -1539,11 +1534,10 @@ static int tegra_dma_runtime_resume(struct device *dev)
> static int tegra_dma_pm_suspend(struct device *dev)
> {
> struct tegra_dma *tdma = dev_get_drvdata(dev);
> - int i;
> - int ret;
> + int i, ret;
> static int tegra_dma_pm_resume(struct device *dev)
> {
> struct tegra_dma *tdma = dev_get_drvdata(dev);
> - int i;
> - int ret;
> + int i, ret;
Do you really need that?
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 9, 2015 at 3:23 PM, Jon Hunter <[email protected]> wrote:
> Currently the tegra-apb DMA driver suspend/resume helpers, save and
> restore the registers for all channels regardless of whether they are
> in use or not. Change this so that only channels that have been
> allocated and configured are saved and restored.
Nitpick
> + /*
> + * Only save the state of DMA channels that are in use.
> + */
One line?
> + /*
> + * Only restore the state of DMA channels that are in use.
> + */
Same.
--
With Best Regards,
Andy Shevchenko
On 09/11/15 13:57, Andy Shevchenko wrote:
> On Mon, Nov 9, 2015 at 3:23 PM, Jon Hunter <[email protected]> wrote:
>> The tegra-apb DMA driver enables runtime-pm but never calls
>> pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
>> The driver manages the clocks by directly calling clk_prepare_enable()
>> and clk_unprepare_disable().
>>
>> Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
>> with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
>> the consequence of this is that if runtime-pm is disabled, then the clocks
>> will remain on the entire time the driver is loaded. However, if
>> runtime-pm is disabled, then power is not most likely not a concern.
>
> Nitpick
>
>> @@ -1539,11 +1534,10 @@ static int tegra_dma_runtime_resume(struct device *dev)
>> static int tegra_dma_pm_suspend(struct device *dev)
>> {
>> struct tegra_dma *tdma = dev_get_drvdata(dev);
>> - int i;
>> - int ret;
>> + int i, ret;
>
>> static int tegra_dma_pm_resume(struct device *dev)
>> {
>> struct tegra_dma *tdma = dev_get_drvdata(dev);
>> - int i;
>> - int ret;
>> + int i, ret;
>
> Do you really need that?
Rhetorical question? ;-)
I can drop that.
Jon
On 09/11/15 13:58, Andy Shevchenko wrote:
> On Mon, Nov 9, 2015 at 3:23 PM, Jon Hunter <[email protected]> wrote:
>> Currently the tegra-apb DMA driver suspend/resume helpers, save and
>> restore the registers for all channels regardless of whether they are
>> in use or not. Change this so that only channels that have been
>> allocated and configured are saved and restored.
>
> Nitpick
>
>> + /*
>> + * Only save the state of DMA channels that are in use.
>> + */
>
> One line?
>
>> + /*
>> + * Only restore the state of DMA channels that are in use.
>> + */
>
> Same.
Ok.
Jon