Re: [PATCH v3 8/8] DMA: Freescale: add suspend resume functions for DMA driver
On 04/10/2014 08:05 PM, Andy Shevchenko wrote:
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
From: Hongbo Zhang
This patch adds suspend resume functions for Freescale DMA driver.
.prepare callback is used to stop further descriptors from being added into the
pending queue, and also issue pending queues into execution if there is any.
.suspend callback makes sure all the pending jobs are cleaned up and all the
channels are idle, and save the mode registers.
.resume callback re-initializes the channels by restore the mode registers.
Like we discussed with Vinod [1] the DMA controller drivers should go to
suspend after users and come back before them.
After you reconsider this point the patch logic might be modified a lot.
Looked through that discussions, I really had thought such problem for a
while.
For the dma-controller and dma-user, we don't know which .suspend
callback is executed firstly, so the idea would be: which ever is called
earlier, the dma-controller driver suspend function should be as robust
as possible.
It is better the dma-user .suspend callback is called earlier, some
clean-ups should be done here such as stop dma request, but we cannot
make sure every dma-user has a .suspend callback, some dma-users don't
pay attention to or even don't care the suspend at all for some reason.
So even the suspend_late is used, we cannot assume every dma-user's
activity is cleaned up neatly, dma-controller driver should be robust to
handle this gracefully, that was my design target.
In the prepare() function, clean up the pending queue and stop receive
new coming request, and in the suspend() function I do some register
saving works. I don't think my code needs to be modified much, a
possible change according to Vinod's idea would be:
use .suspend instead of my current .prepare
use . suspend_late instead of my current .suspend
e.g. postpone all my functions to be executed later, this method works
and seems better for client/low-level module drivers.
The reason I didn't use the above functions was that I had read this
Documentation/power/devices.txt, search the definitions of prepare,
suspend, suspend_late and suspend_noirq, I think my usage complies with
that definitions strictly, I can do the modification above if the
maintainer like and if nobody says I break this law.
(Moreover, you abuse your own position to use only setters/getters to
access to the DMAc registers)
My shame, I will update it.
(reason is the setters/getters patch isn't merged into our internal
tree, but this suspend patch has been done. I am trying to sync our
internal kernel and the community now)
[1] http://www.spinics.net/lists/kernel/msg1650974.html
Signed-off-by: Hongbo Zhang
---
drivers/dma/fsldma.c | 100 ++
drivers/dma/fsldma.h | 16
2 files changed, 116 insertions(+)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index c9bf54a..d6da222 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -400,6 +400,14 @@ static dma_cookie_t fsl_dma_tx_submit(struct
dma_async_tx_descriptor *tx)
spin_lock_bh(>desc_lock);
+#ifdef CONFIG_PM
+ if (unlikely(chan->pm_state != RUNNING)) {
+ chan_dbg(chan, "cannot submit due to suspend\n");
+ spin_unlock_bh(>desc_lock);
+ return -1;
+ }
+#endif
+
/*
* assign cookies to all of the software descriptors
* that make up this transaction
@@ -1311,6 +1319,9 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
INIT_LIST_HEAD(>ld_running);
INIT_LIST_HEAD(>ld_completed);
chan->idle = true;
+#ifdef CONFIG_PM
+ chan->pm_state = RUNNING;
+#endif
chan->common.device = >common;
dma_cookie_init(>common);
@@ -1450,6 +1461,92 @@ static int fsldma_of_remove(struct platform_device *op)
return 0;
}
+#ifdef CONFIG_PM
+static int fsldma_prepare(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ int i;
+
+ for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
+ chan = fdev->chan[i];
+ if (!chan)
+ continue;
+
+ spin_lock_bh(>desc_lock);
+ chan->pm_state = SUSPENDING;
+ if (!list_empty(>ld_pending))
+ fsl_chan_xfer_ld_queue(chan);
+ spin_unlock_bh(>desc_lock);
+ }
+
+ return 0;
+}
+
+static int fsldma_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ int i;
+
+ for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
+ chan = fdev->chan[i];
+ if
Re: [PATCH v3 8/8] DMA: Freescale: add suspend resume functions for DMA driver
On 04/10/2014 08:05 PM, Andy Shevchenko wrote:
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
From: Hongbo Zhang hongbo.zh...@freescale.com
This patch adds suspend resume functions for Freescale DMA driver.
.prepare callback is used to stop further descriptors from being added into the
pending queue, and also issue pending queues into execution if there is any.
.suspend callback makes sure all the pending jobs are cleaned up and all the
channels are idle, and save the mode registers.
.resume callback re-initializes the channels by restore the mode registers.
Like we discussed with Vinod [1] the DMA controller drivers should go to
suspend after users and come back before them.
After you reconsider this point the patch logic might be modified a lot.
Looked through that discussions, I really had thought such problem for a
while.
For the dma-controller and dma-user, we don't know which .suspend
callback is executed firstly, so the idea would be: which ever is called
earlier, the dma-controller driver suspend function should be as robust
as possible.
It is better the dma-user .suspend callback is called earlier, some
clean-ups should be done here such as stop dma request, but we cannot
make sure every dma-user has a .suspend callback, some dma-users don't
pay attention to or even don't care the suspend at all for some reason.
So even the suspend_late is used, we cannot assume every dma-user's
activity is cleaned up neatly, dma-controller driver should be robust to
handle this gracefully, that was my design target.
In the prepare() function, clean up the pending queue and stop receive
new coming request, and in the suspend() function I do some register
saving works. I don't think my code needs to be modified much, a
possible change according to Vinod's idea would be:
use .suspend instead of my current .prepare
use . suspend_late instead of my current .suspend
e.g. postpone all my functions to be executed later, this method works
and seems better for client/low-level module drivers.
The reason I didn't use the above functions was that I had read this
Documentation/power/devices.txt, search the definitions of prepare,
suspend, suspend_late and suspend_noirq, I think my usage complies with
that definitions strictly, I can do the modification above if the
maintainer like and if nobody says I break this law.
(Moreover, you abuse your own position to use only setters/getters to
access to the DMAc registers)
My shame, I will update it.
(reason is the setters/getters patch isn't merged into our internal
tree, but this suspend patch has been done. I am trying to sync our
internal kernel and the community now)
[1] http://www.spinics.net/lists/kernel/msg1650974.html
Signed-off-by: Hongbo Zhang hongbo.zh...@freescale.com
---
drivers/dma/fsldma.c | 100 ++
drivers/dma/fsldma.h | 16
2 files changed, 116 insertions(+)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index c9bf54a..d6da222 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -400,6 +400,14 @@ static dma_cookie_t fsl_dma_tx_submit(struct
dma_async_tx_descriptor *tx)
spin_lock_bh(chan-desc_lock);
+#ifdef CONFIG_PM
+ if (unlikely(chan-pm_state != RUNNING)) {
+ chan_dbg(chan, cannot submit due to suspend\n);
+ spin_unlock_bh(chan-desc_lock);
+ return -1;
+ }
+#endif
+
/*
* assign cookies to all of the software descriptors
* that make up this transaction
@@ -1311,6 +1319,9 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
INIT_LIST_HEAD(chan-ld_running);
INIT_LIST_HEAD(chan-ld_completed);
chan-idle = true;
+#ifdef CONFIG_PM
+ chan-pm_state = RUNNING;
+#endif
chan-common.device = fdev-common;
dma_cookie_init(chan-common);
@@ -1450,6 +1461,92 @@ static int fsldma_of_remove(struct platform_device *op)
return 0;
}
+#ifdef CONFIG_PM
+static int fsldma_prepare(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ int i;
+
+ for (i = 0; i FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
+ chan = fdev-chan[i];
+ if (!chan)
+ continue;
+
+ spin_lock_bh(chan-desc_lock);
+ chan-pm_state = SUSPENDING;
+ if (!list_empty(chan-ld_pending))
+ fsl_chan_xfer_ld_queue(chan);
+ spin_unlock_bh(chan-desc_lock);
+ }
+
+ return 0;
+}
+
+static int fsldma_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ int i;
+
+ for (i = 0; i
Re: [PATCH v3 8/8] DMA: Freescale: add suspend resume functions for DMA driver
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
> From: Hongbo Zhang
>
> This patch adds suspend resume functions for Freescale DMA driver.
> .prepare callback is used to stop further descriptors from being added into
> the
> pending queue, and also issue pending queues into execution if there is any.
> .suspend callback makes sure all the pending jobs are cleaned up and all the
> channels are idle, and save the mode registers.
> .resume callback re-initializes the channels by restore the mode registers.
Like we discussed with Vinod [1] the DMA controller drivers should go to
suspend after users and come back before them.
After you reconsider this point the patch logic might be modified a lot.
(Moreover, you abuse your own position to use only setters/getters to
access to the DMAc registers)
[1] http://www.spinics.net/lists/kernel/msg1650974.html
>
> Signed-off-by: Hongbo Zhang
> ---
> drivers/dma/fsldma.c | 100
> ++
> drivers/dma/fsldma.h | 16
> 2 files changed, 116 insertions(+)
>
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index c9bf54a..d6da222 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -400,6 +400,14 @@ static dma_cookie_t fsl_dma_tx_submit(struct
> dma_async_tx_descriptor *tx)
>
> spin_lock_bh(>desc_lock);
>
> +#ifdef CONFIG_PM
> + if (unlikely(chan->pm_state != RUNNING)) {
> + chan_dbg(chan, "cannot submit due to suspend\n");
> + spin_unlock_bh(>desc_lock);
> + return -1;
> + }
> +#endif
> +
> /*
>* assign cookies to all of the software descriptors
>* that make up this transaction
> @@ -1311,6 +1319,9 @@ static int fsl_dma_chan_probe(struct fsldma_device
> *fdev,
> INIT_LIST_HEAD(>ld_running);
> INIT_LIST_HEAD(>ld_completed);
> chan->idle = true;
> +#ifdef CONFIG_PM
> + chan->pm_state = RUNNING;
> +#endif
>
> chan->common.device = >common;
> dma_cookie_init(>common);
> @@ -1450,6 +1461,92 @@ static int fsldma_of_remove(struct platform_device *op)
> return 0;
> }
>
> +#ifdef CONFIG_PM
> +static int fsldma_prepare(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct fsldma_device *fdev = platform_get_drvdata(pdev);
> + struct fsldma_chan *chan;
> + int i;
> +
> + for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
> + chan = fdev->chan[i];
> + if (!chan)
> + continue;
> +
> + spin_lock_bh(>desc_lock);
> + chan->pm_state = SUSPENDING;
> + if (!list_empty(>ld_pending))
> + fsl_chan_xfer_ld_queue(chan);
> + spin_unlock_bh(>desc_lock);
> + }
> +
> + return 0;
> +}
> +
> +static int fsldma_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct fsldma_device *fdev = platform_get_drvdata(pdev);
> + struct fsldma_chan *chan;
> + int i;
> +
> + for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
> + chan = fdev->chan[i];
> + if (!chan)
> + continue;
> +
> + spin_lock_bh(>desc_lock);
> + if (!chan->idle)
> + goto out;
> + chan->regs_save.mr = DMA_IN(chan, >regs->mr, 32);
> + chan->pm_state = SUSPENDED;
> + spin_unlock_bh(>desc_lock);
> + }
> + return 0;
> +
> +out:
> + for (; i >= 0; i--) {
> + chan = fdev->chan[i];
> + if (!chan)
> + continue;
> + chan->pm_state = RUNNING;
> + spin_unlock_bh(>desc_lock);
> + }
> + return -EBUSY;
> +}
> +
> +static int fsldma_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct fsldma_device *fdev = platform_get_drvdata(pdev);
> + struct fsldma_chan *chan;
> + u32 mode;
> + int i;
> +
> + for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
> + chan = fdev->chan[i];
> + if (!chan)
> + continue;
> +
> + spin_lock_bh(>desc_lock);
> + mode = chan->regs_save.mr
> + & ~FSL_DMA_MR_CS & ~FSL_DMA_MR_CC & ~FSL_DMA_MR_CA;
> + DMA_OUT(chan, >regs->mr, mode, 32);
> + chan->pm_state = RUNNING;
> + spin_unlock_bh(>desc_lock);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops fsldma_pm_ops = {
> + .prepare= fsldma_prepare,
> + .suspend= fsldma_suspend,
> + .resume = fsldma_resume,
> +};
> +#endif
> +
> static const struct of_device_id fsldma_of_ids[] = {
> { .compatible = "fsl,elo3-dma", },
> { .compatible = "fsl,eloplus-dma", },
> @@ -1462,6 +1559,9 @@ static struct platform_driver fsldma_of_driver = {
>
Re: [PATCH v3 8/8] DMA: Freescale: add suspend resume functions for DMA driver
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
From: Hongbo Zhang hongbo.zh...@freescale.com
This patch adds suspend resume functions for Freescale DMA driver.
.prepare callback is used to stop further descriptors from being added into
the
pending queue, and also issue pending queues into execution if there is any.
.suspend callback makes sure all the pending jobs are cleaned up and all the
channels are idle, and save the mode registers.
.resume callback re-initializes the channels by restore the mode registers.
Like we discussed with Vinod [1] the DMA controller drivers should go to
suspend after users and come back before them.
After you reconsider this point the patch logic might be modified a lot.
(Moreover, you abuse your own position to use only setters/getters to
access to the DMAc registers)
[1] http://www.spinics.net/lists/kernel/msg1650974.html
Signed-off-by: Hongbo Zhang hongbo.zh...@freescale.com
---
drivers/dma/fsldma.c | 100
++
drivers/dma/fsldma.h | 16
2 files changed, 116 insertions(+)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index c9bf54a..d6da222 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -400,6 +400,14 @@ static dma_cookie_t fsl_dma_tx_submit(struct
dma_async_tx_descriptor *tx)
spin_lock_bh(chan-desc_lock);
+#ifdef CONFIG_PM
+ if (unlikely(chan-pm_state != RUNNING)) {
+ chan_dbg(chan, cannot submit due to suspend\n);
+ spin_unlock_bh(chan-desc_lock);
+ return -1;
+ }
+#endif
+
/*
* assign cookies to all of the software descriptors
* that make up this transaction
@@ -1311,6 +1319,9 @@ static int fsl_dma_chan_probe(struct fsldma_device
*fdev,
INIT_LIST_HEAD(chan-ld_running);
INIT_LIST_HEAD(chan-ld_completed);
chan-idle = true;
+#ifdef CONFIG_PM
+ chan-pm_state = RUNNING;
+#endif
chan-common.device = fdev-common;
dma_cookie_init(chan-common);
@@ -1450,6 +1461,92 @@ static int fsldma_of_remove(struct platform_device *op)
return 0;
}
+#ifdef CONFIG_PM
+static int fsldma_prepare(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ int i;
+
+ for (i = 0; i FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
+ chan = fdev-chan[i];
+ if (!chan)
+ continue;
+
+ spin_lock_bh(chan-desc_lock);
+ chan-pm_state = SUSPENDING;
+ if (!list_empty(chan-ld_pending))
+ fsl_chan_xfer_ld_queue(chan);
+ spin_unlock_bh(chan-desc_lock);
+ }
+
+ return 0;
+}
+
+static int fsldma_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ int i;
+
+ for (i = 0; i FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
+ chan = fdev-chan[i];
+ if (!chan)
+ continue;
+
+ spin_lock_bh(chan-desc_lock);
+ if (!chan-idle)
+ goto out;
+ chan-regs_save.mr = DMA_IN(chan, chan-regs-mr, 32);
+ chan-pm_state = SUSPENDED;
+ spin_unlock_bh(chan-desc_lock);
+ }
+ return 0;
+
+out:
+ for (; i = 0; i--) {
+ chan = fdev-chan[i];
+ if (!chan)
+ continue;
+ chan-pm_state = RUNNING;
+ spin_unlock_bh(chan-desc_lock);
+ }
+ return -EBUSY;
+}
+
+static int fsldma_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct fsldma_device *fdev = platform_get_drvdata(pdev);
+ struct fsldma_chan *chan;
+ u32 mode;
+ int i;
+
+ for (i = 0; i FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
+ chan = fdev-chan[i];
+ if (!chan)
+ continue;
+
+ spin_lock_bh(chan-desc_lock);
+ mode = chan-regs_save.mr
+ ~FSL_DMA_MR_CS ~FSL_DMA_MR_CC ~FSL_DMA_MR_CA;
+ DMA_OUT(chan, chan-regs-mr, mode, 32);
+ chan-pm_state = RUNNING;
+ spin_unlock_bh(chan-desc_lock);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops fsldma_pm_ops = {
+ .prepare= fsldma_prepare,
+ .suspend= fsldma_suspend,
+ .resume = fsldma_resume,
+};
+#endif
+
static const struct of_device_id fsldma_of_ids[] = {
{ .compatible = fsl,elo3-dma, },
{ .compatible = fsl,eloplus-dma, },
@@ -1462,6 +1559,9 @@ static struct platform_driver fsldma_of_driver = {
.name = fsl-elo-dma,
.owner =
