[PATCH] dma: tegra: implement suspend/resume callbacks

[PATCH] dma: tegra: implement suspend/resume callbacks

[Laxman Dewangan]

Implement suspend/resume callbacks to store APB DMA channel's
register on suspend and restore APB DMA channel's register on
resume.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/dma/tegra20-apb-dma.c |   65 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 5a0b66c..ce19340 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/clk/tegra.h>
@@ -199,6 +200,7 @@ struct tegra_dma_channel {
 
 	/* Channel-slave specific configuration */
 	struct dma_slave_config dma_sconfig;
+	struct tegra_dma_channel_regs	channel_reg;
 };
 
 /* tegra_dma: Tegra DMA specific information */
@@ -1440,11 +1442,74 @@ static int tegra_dma_runtime_resume(struct device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int tegra_dma_pm_suspend(struct device *dev)
+{
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
+	int i;
+	int ret;
+
+	/* Enable clock before accessing register */
+	ret = tegra_dma_runtime_resume(dev);
+	if (ret < 0)
+		return ret;
+
+	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
+	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
+		struct tegra_dma_channel *tdc = &tdma->channels[i];
+		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
+
+		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);
+		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
+		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
+	}
+
+	/* Disable clock */
+	tegra_dma_runtime_suspend(dev);
+	return 0;
+}
+
+static int tegra_dma_pm_resume(struct device *dev)
+{
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
+	int i;
+	int ret;
+
+	/* Enable clock before accessing register */
+	ret = tegra_dma_runtime_resume(dev);
+	if (ret < 0)
+		return ret;
+
+	tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen);
+	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
+	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
+
+	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
+		struct tegra_dma_channel *tdc = &tdma->channels[i];
+		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
+
+		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);
+		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
+		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
+			(ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB));
+	}
+
+	/* Disable clock */
+	tegra_dma_runtime_suspend(dev);
+	return 0;
+}
+#endif
+
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
 #ifdef CONFIG_PM_RUNTIME
 	.runtime_suspend = tegra_dma_runtime_suspend,
 	.runtime_resume = tegra_dma_runtime_resume,
 #endif
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
 };
 
 static struct platform_driver tegra_dmac_driver = {
-- 
1.7.1.1

Re: [PATCH] dma: tegra: implement suspend/resume callbacks

[Vinod Koul]

On Wed, Apr 24, 2013 at 03:24:27PM +0530, Laxman Dewangan wrote:
> Implement suspend/resume callbacks to store APB DMA channel's
> register on suspend and restore APB DMA channel's register on
> resume.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c |   65 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 5a0b66c..ce19340 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/clk/tegra.h>
> @@ -199,6 +200,7 @@ struct tegra_dma_channel {
>  
>  	/* Channel-slave specific configuration */
>  	struct dma_slave_config dma_sconfig;
> +	struct tegra_dma_channel_regs	channel_reg;
>  };
>  
>  /* tegra_dma: Tegra DMA specific information */
> @@ -1440,11 +1442,74 @@ static int tegra_dma_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_dma_pm_suspend(struct device *dev)
> +{
> +	struct tegra_dma *tdma = dev_get_drvdata(dev);
> +	int i;
> +	int ret;
> +
> +	/* Enable clock before accessing register */
> +	ret = tegra_dma_runtime_resume(dev);
> +	if (ret < 0)
> +		return ret;
You dont seem to handle suspend when DMA is active? Otherwise looks fine.
Stephen, you okay with this patch?

--
~Vinod
> +
> +	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
> +	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
> +		struct tegra_dma_channel *tdc = &tdma->channels[i];
> +		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
> +
> +		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);
> +		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
> +		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
> +	}
> +
> +	/* Disable clock */
> +	tegra_dma_runtime_suspend(dev);
> +	return 0;
> +}
> +
> +static int tegra_dma_pm_resume(struct device *dev)
> +{
> +	struct tegra_dma *tdma = dev_get_drvdata(dev);
> +	int i;
> +	int ret;
> +
> +	/* Enable clock before accessing register */
> +	ret = tegra_dma_runtime_resume(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen);
> +	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
> +	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
> +
> +	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
> +		struct tegra_dma_channel *tdc = &tdma->channels[i];
> +		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
> +
> +		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);
> +		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
> +		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
> +			(ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB));
> +	}
> +
> +	/* Disable clock */
> +	tegra_dma_runtime_suspend(dev);
> +	return 0;
> +}
> +#endif
>  static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
>  #ifdef CONFIG_PM_RUNTIME
>  	.runtime_suspend = tegra_dma_runtime_suspend,
>  	.runtime_resume = tegra_dma_runtime_resume,
>  #endif
> +	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
>  };
>  
>  static struct platform_driver tegra_dmac_driver = {
> -- 
> 1.7.1.1
> 

Re: [PATCH] dma: tegra: implement suspend/resume callbacks

[Laxman Dewangan]

On Tuesday 30 April 2013 04:00 PM, Vinod Koul wrote:
> On Wed, Apr 24, 2013 at 03:24:27PM +0530, Laxman Dewangan wrote:
>> +
>> +	/* Enable clock before accessing register */
>> +	ret = tegra_dma_runtime_resume(dev);
>> +	if (ret < 0)
>> +		return ret;
> You dont seem to handle suspend when DMA is active? Otherwise looks fine.
> Stephen, you okay with this patch?

The client of dma need to gracefully stop the transfer and then do 
suspend himself.
Also driver suspend can happen even if there is no allocation of dma. In 
this case, clock is disabled so enabling explicitly here.

Re: [PATCH] dma: tegra: implement suspend/resume callbacks

[Stephen Warren]

On 04/30/2013 04:30 AM, Vinod Koul wrote:
> On Wed, Apr 24, 2013 at 03:24:27PM +0530, Laxman Dewangan wrote:
>> Implement suspend/resume callbacks to store APB DMA channel's
>> register on suspend and restore APB DMA channel's register on
>> resume.
...
> You dont seem to handle suspend when DMA is active? Otherwise looks fine.
> Stephen, you okay with this patch?

Yes, I think this looks fine. Sorry for the slow response; I was on
vacation.

One question though: Laxman mentioned that DMA clients were responsible
for suspending their DMA accesses themselves. Does the dmaengine core
define the semantics here; are DMA drivers supposed to handle suspend
with active DMAs, or should DMA clients suspend their DMA accesses
themselves as Laxman suggests? If the latter, I wonder if we actually
need to save/restore all the registers, since after resume, a new DMA
access would be started in all cases, which would then reprogram the HW.

Re: [PATCH] dma: tegra: implement suspend/resume callbacks

[Vinod Koul]

On Thu, May 02, 2013 at 12:42:30PM -0600, Stephen Warren wrote:
> On 04/30/2013 04:30 AM, Vinod Koul wrote:
> > On Wed, Apr 24, 2013 at 03:24:27PM +0530, Laxman Dewangan wrote:
> >> Implement suspend/resume callbacks to store APB DMA channel's
> >> register on suspend and restore APB DMA channel's register on
> >> resume.
> ...
> > You dont seem to handle suspend when DMA is active? Otherwise looks fine.
> > Stephen, you okay with this patch?
> 
> Yes, I think this looks fine. Sorry for the slow response; I was on
> vacation.
> 
> One question though: Laxman mentioned that DMA clients were responsible
> for suspending their DMA accesses themselves. Does the dmaengine core
> define the semantics here; are DMA drivers supposed to handle suspend
> with active DMAs, or should DMA clients suspend their DMA accesses
> themselves as Laxman suggests? If the latter, I wonder if we actually
> need to save/restore all the registers, since after resume, a new DMA
> access would be started in all cases, which would then reprogram the HW.
No dmaengine doesnt define semantics.

so in case of suspend being invoked when dma is active then driver would need to
stop ongoing DMA and then save. 
But in embedded systems this is not a very typical case, so if your usage
guarantees that this wont happen then it should be okay, but I think it is a
better idea to handle this case.

--
~Vinod