Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Vinod Koul]

On Tue, Dec 10, 2013 at 10:06:13PM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> This is to disable/enable DW_DMAC hw during suspend/resume.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw/pci.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
> index e89fc24..97bc3a2 100644
> --- a/drivers/dma/dw/pci.c
> +++ b/drivers/dma/dw/pci.c
> @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev)
>  		dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +
> +static int dw_pci_suspend_noirq(struct device *dev)
> +{
> +	struct pci_dev *pci = to_pci_dev(dev);
> +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> +
> +	return dw_dma_suspend(chip);
> +};
> +
> +static int dw_pci_resume_noirq(struct device *dev)
> +{
> +	struct pci_dev *pci = to_pci_dev(dev);
> +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> +
> +	return dw_dma_resume(chip);
> +};
> +
> +#else /* !CONFIG_PM_SLEEP */
> +
> +#define dw_pci_suspend_noirq	NULL
> +#define dw_pci_resume_noirq	NULL
> +
> +#endif /* !CONFIG_PM_SLEEP */
How about SET_SYSTEM_SLEEP_PM_OPS instead?

--
~Vinod
> +
> +static const struct dev_pm_ops dw_pci_dev_pm_ops = {
> +	.suspend_noirq = dw_pci_suspend_noirq,
> +	.resume_noirq = dw_pci_resume_noirq,
> +};
> +
>  static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = {
>  	/* Medfield */
>  	{ PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata },
> @@ -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = {
>  	.id_table	= dw_pci_id_table,
>  	.probe		= dw_pci_probe,
>  	.remove		= dw_pci_remove,
> +	.driver	= {
> +		.pm	= &dw_pci_dev_pm_ops,
> +	},
>  };
>  
>  module_pci_driver(dw_pci_driver);
> -- 
> 1.7.4.4
> 

-- 

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2928 bytes --]

On Tue, 2013-12-10 at 15:40 +0530, Vinod Koul wrote:
> On Tue, Dec 10, 2013 at 10:06:13PM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > 
> > This is to disable/enable DW_DMAC hw during suspend/resume.
> > 
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dw/pci.c |   33 +++++++++++++++++++++++++++++++++
> >  1 files changed, 33 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
> > index e89fc24..97bc3a2 100644
> > --- a/drivers/dma/dw/pci.c
> > +++ b/drivers/dma/dw/pci.c
> > @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev)
> >  		dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret);
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +
> > +static int dw_pci_suspend_noirq(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> > +
> > +	return dw_dma_suspend(chip);
> > +};
> > +
> > +static int dw_pci_resume_noirq(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> > +
> > +	return dw_dma_resume(chip);
> > +};
> > +
> > +#else /* !CONFIG_PM_SLEEP */
> > +
> > +#define dw_pci_suspend_noirq	NULL
> > +#define dw_pci_resume_noirq	NULL
> > +
> > +#endif /* !CONFIG_PM_SLEEP */
> How about SET_SYSTEM_SLEEP_PM_OPS instead?

So, we are using *_noirq versions of the functions here. What happened
when we switch to normal ones? Any side effects?

> 
> --
> ~Vinod
> > +
> > +static const struct dev_pm_ops dw_pci_dev_pm_ops = {
> > +	.suspend_noirq = dw_pci_suspend_noirq,
> > +	.resume_noirq = dw_pci_resume_noirq,
> > +};
> > +
> >  static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = {
> >  	/* Medfield */
> >  	{ PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata },
> > @@ -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = {
> >  	.id_table	= dw_pci_id_table,
> >  	.probe		= dw_pci_probe,
> >  	.remove		= dw_pci_remove,
> > +	.driver	= {
> > +		.pm	= &dw_pci_dev_pm_ops,
> > +	},
> >  };
> >  
> >  module_pci_driver(dw_pci_driver);
> > -- 
> > 1.7.4.4
> > 
> 

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

This is to disable/enable DW_DMAC hw during suspend/resume.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw/pci.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c
index e89fc24..97bc3a2 100644
--- a/drivers/dma/dw/pci.c
+++ b/drivers/dma/dw/pci.c
@@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev)
 		dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret);
 }
 
+#ifdef CONFIG_PM_SLEEP
+
+static int dw_pci_suspend_noirq(struct device *dev)
+{
+	struct pci_dev *pci = to_pci_dev(dev);
+	struct dw_dma_chip *chip = pci_get_drvdata(pci);
+
+	return dw_dma_suspend(chip);
+};
+
+static int dw_pci_resume_noirq(struct device *dev)
+{
+	struct pci_dev *pci = to_pci_dev(dev);
+	struct dw_dma_chip *chip = pci_get_drvdata(pci);
+
+	return dw_dma_resume(chip);
+};
+
+#else /* !CONFIG_PM_SLEEP */
+
+#define dw_pci_suspend_noirq	NULL
+#define dw_pci_resume_noirq	NULL
+
+#endif /* !CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops dw_pci_dev_pm_ops = {
+	.suspend_noirq = dw_pci_suspend_noirq,
+	.resume_noirq = dw_pci_resume_noirq,
+};
+
 static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = {
 	/* Medfield */
 	{ PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata },
@@ -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = {
 	.id_table	= dw_pci_id_table,
 	.probe		= dw_pci_probe,
 	.remove		= dw_pci_remove,
+	.driver	= {
+		.pm	= &dw_pci_dev_pm_ops,
+	},
 };
 
 module_pci_driver(dw_pci_driver);
-- 
1.7.4.4

RE: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Chew, Chiau Ee]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3279 bytes --]

Vinod,

As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM callback whereby the callbacks would be executed after IRQ handlers have been disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal suspend/resume PM callback. Looking at the Desginware DMAC platform code (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM callback. Is it advisable to use the normal suspend/resume PM callback instead of *_noirq suspend/PM callback? 

Thanks,
Chiau Ee

-----Original Message-----
From: Shevchenko, Andriy 
Sent: Tuesday, December 10, 2013 7:57 PM
To: Koul, Vinod
Cc: Chew, Chiau Ee; Viresh Kumar; Andy Shevchenko; Williams, Dan J; dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

On Tue, 2013-12-10 at 15:40 +0530, Vinod Koul wrote:
> On Tue, Dec 10, 2013 at 10:06:13PM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > 
> > This is to disable/enable DW_DMAC hw during suspend/resume.
> > 
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dw/pci.c |   33 +++++++++++++++++++++++++++++++++
> >  1 files changed, 33 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index 
> > e89fc24..97bc3a2 100644
> > --- a/drivers/dma/dw/pci.c
> > +++ b/drivers/dma/dw/pci.c
> > @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev)
> >  		dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret);  
> > }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +
> > +static int dw_pci_suspend_noirq(struct device *dev) {
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> > +
> > +	return dw_dma_suspend(chip);
> > +};
> > +
> > +static int dw_pci_resume_noirq(struct device *dev) {
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> > +
> > +	return dw_dma_resume(chip);
> > +};
> > +
> > +#else /* !CONFIG_PM_SLEEP */
> > +
> > +#define dw_pci_suspend_noirq	NULL
> > +#define dw_pci_resume_noirq	NULL
> > +
> > +#endif /* !CONFIG_PM_SLEEP */
> How about SET_SYSTEM_SLEEP_PM_OPS instead?

So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects?

> 
> --
> ~Vinod
> > +
> > +static const struct dev_pm_ops dw_pci_dev_pm_ops = {
> > +	.suspend_noirq = dw_pci_suspend_noirq,
> > +	.resume_noirq = dw_pci_resume_noirq, };
> > +
> >  static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = {
> >  	/* Medfield */
> >  	{ PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata }, @@ 
> > -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = {
> >  	.id_table	= dw_pci_id_table,
> >  	.probe		= dw_pci_probe,
> >  	.remove		= dw_pci_remove,
> > +	.driver	= {
> > +		.pm	= &dw_pci_dev_pm_ops,
> > +	},
> >  };
> >  
> >  module_pci_driver(dw_pci_driver);
> > --
> > 1.7.4.4
> > 
> 

--
Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Vinod Koul]

On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote:
> Vinod,
Please do *NOT* top post

Please fix your MUA to wrap lines with 80chars. I have fix below to make it
readble...

> As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> callback whereby the callbacks would be executed after IRQ handlers have been
> disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> suspend/resume PM callback. Looking at the Desginware DMAC platform code
> (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> callback. Is it advisable to use the normal suspend/resume PM callback instead
> of *_noirq suspend/PM callback? 

i dont see a reason why we need the noirq versions

--
~Vinod

> 
> Thanks,
> Chiau Ee
> 
> -----Original Message-----
> From: Shevchenko, Andriy 
> Sent: Tuesday, December 10, 2013 7:57 PM
> To: Koul, Vinod
> Cc: Chew, Chiau Ee; Viresh Kumar; Andy Shevchenko; Williams, Dan J; dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.
> 
> On Tue, 2013-12-10 at 15:40 +0530, Vinod Koul wrote:
> > On Tue, Dec 10, 2013 at 10:06:13PM +0800, Chew Chiau Ee wrote:
> > > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > > 
> > > This is to disable/enable DW_DMAC hw during suspend/resume.
> > > 
> > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/dma/dw/pci.c |   33 +++++++++++++++++++++++++++++++++
> > >  1 files changed, 33 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index 
> > > e89fc24..97bc3a2 100644
> > > --- a/drivers/dma/dw/pci.c
> > > +++ b/drivers/dma/dw/pci.c
> > > @@ -75,6 +75,36 @@ static void dw_pci_remove(struct pci_dev *pdev)
> > >  		dev_warn(&pdev->dev, "can't remove device properly: %d\n", ret);  
> > > }
> > >  
> > > +#ifdef CONFIG_PM_SLEEP
> > > +
> > > +static int dw_pci_suspend_noirq(struct device *dev) {
> > > +	struct pci_dev *pci = to_pci_dev(dev);
> > > +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> > > +
> > > +	return dw_dma_suspend(chip);
> > > +};
> > > +
> > > +static int dw_pci_resume_noirq(struct device *dev) {
> > > +	struct pci_dev *pci = to_pci_dev(dev);
> > > +	struct dw_dma_chip *chip = pci_get_drvdata(pci);
> > > +
> > > +	return dw_dma_resume(chip);
> > > +};
> > > +
> > > +#else /* !CONFIG_PM_SLEEP */
> > > +
> > > +#define dw_pci_suspend_noirq	NULL
> > > +#define dw_pci_resume_noirq	NULL
> > > +
> > > +#endif /* !CONFIG_PM_SLEEP */
> > How about SET_SYSTEM_SLEEP_PM_OPS instead?
> 
> So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects?
> 
> > 
> > --
> > ~Vinod
> > > +
> > > +static const struct dev_pm_ops dw_pci_dev_pm_ops = {
> > > +	.suspend_noirq = dw_pci_suspend_noirq,
> > > +	.resume_noirq = dw_pci_resume_noirq, };
> > > +
> > >  static DEFINE_PCI_DEVICE_TABLE(dw_pci_id_table) = {
> > >  	/* Medfield */
> > >  	{ PCI_VDEVICE(INTEL, 0x0827), (kernel_ulong_t)&dw_pci_pdata }, @@ 
> > > -92,6 +122,9 @@ static struct pci_driver dw_pci_driver = {
> > >  	.id_table	= dw_pci_id_table,
> > >  	.probe		= dw_pci_probe,
> > >  	.remove		= dw_pci_remove,
> > > +	.driver	= {
> > > +		.pm	= &dw_pci_dev_pm_ops,
> > > +	},
> > >  };
> > >  
> > >  module_pci_driver(dw_pci_driver);
> > > --
> > > 1.7.4.4
> > > 
> > 
> 
> --
> Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy

-- 

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Andy Shevchenko]

On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote:
> On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote:

> > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > callback whereby the callbacks would be executed after IRQ handlers have been
> > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > of *_noirq suspend/PM callback? 
> 
> i dont see a reason why we need the noirq versions

Okay. I imagine the following use case.

For example we have compiled in DMA driver (dw_dmac) along with, for
example, SPI driver.

System was scheduled to go sleep.

An order of calling IIUC might be DMA first, then SPI (since they are
not in parent / child relations).

What was happened when SPI would like to do a DMA transfer and DMA is
going to sleep? I'm trying to understand if this is a case.

> > > How about SET_SYSTEM_SLEEP_PM_OPS instead?
> > 
> > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects?



-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Vinod Koul]

On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote:
> On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote:
> > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote:
> 
> > > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > > callback whereby the callbacks would be executed after IRQ handlers have been
> > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > > of *_noirq suspend/PM callback? 
> > 
> > i dont see a reason why we need the noirq versions
> 
> Okay. I imagine the following use case.
> 
> For example we have compiled in DMA driver (dw_dmac) along with, for
> example, SPI driver.
> 
> System was scheduled to go sleep.
> 
> An order of calling IIUC might be DMA first, then SPI (since they are
> not in parent / child relations).
> 
> What was happened when SPI would like to do a DMA transfer and DMA is
> going to sleep? I'm trying to understand if this is a case.
In that case how does no irq version help us?

For these cases, I have been using suspend_late. Since the dmaengine driver is
providing service to other clients (SPI), it needs to esnure that it suspends
after SPI using suspend_late and resume using resume_early. That way dma is
availble whenever the client is active

--
~Vinod

> 
> > > > How about SET_SYSTEM_SLEEP_PM_OPS instead?
> > > 
> > > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects?
> 
> 
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
> 

-- 

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Vinod Koul]

On Mon, Jan 20, 2014 at 06:17:56PM +0530, Shevchenko, Andriy wrote:
> On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote:
> > On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote:
> > > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote:
> > > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote:
> > > 
> > > > > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > > > > callback whereby the callbacks would be executed after IRQ handlers have been
> > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > > > > of *_noirq suspend/PM callback? 
> > > > 
> > > > i dont see a reason why we need the noirq versions
> > > 
> > > Okay. I imagine the following use case.
> > > 
> > > For example we have compiled in DMA driver (dw_dmac) along with, for
> > > example, SPI driver.
> > > 
> > > System was scheduled to go sleep.
> > > 
> > > An order of calling IIUC might be DMA first, then SPI (since they are
> > > not in parent / child relations).
> > > 
> > > What was happened when SPI would like to do a DMA transfer and DMA is
> > > going to sleep? I'm trying to understand if this is a case.
> > In that case how does no irq version help us?
> 
> It guarantees that we have no user of DMA anymore, since there is no
> interrupt going on.
well how is that. It will gaurantee that there wont be interrupt. User can still
submit a transaction or another transaction will be in progress...

> > For these cases, I have been using suspend_late. Since the dmaengine driver is
> > providing service to other clients (SPI), it needs to esnure that it suspends
> > after SPI using suspend_late and resume using resume_early. That way dma is
> > availble whenever the client is active
> 
> suspend_late is working in context that interrupt handler may be
> invoked. Thus, to have DMA driver be properly shut down we have to
> wait / terminate possible ongoing transfer.
Well client is already suspended via .suspend. So where is the transaction :)
> 
> It seems for me all DMA drivers that are using
> system .suspend()/.resume() are potentially buggy.
Yup!

-- 
~Vinod

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3001 bytes --]

On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote:
> On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote:
> > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote:
> > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote:
> > 
> > > > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > > > callback whereby the callbacks would be executed after IRQ handlers have been
> > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > > > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > > > of *_noirq suspend/PM callback? 
> > > 
> > > i dont see a reason why we need the noirq versions
> > 
> > Okay. I imagine the following use case.
> > 
> > For example we have compiled in DMA driver (dw_dmac) along with, for
> > example, SPI driver.
> > 
> > System was scheduled to go sleep.
> > 
> > An order of calling IIUC might be DMA first, then SPI (since they are
> > not in parent / child relations).
> > 
> > What was happened when SPI would like to do a DMA transfer and DMA is
> > going to sleep? I'm trying to understand if this is a case.
> In that case how does no irq version help us?

It guarantees that we have no user of DMA anymore, since there is no
interrupt going on.

> For these cases, I have been using suspend_late. Since the dmaengine driver is
> providing service to other clients (SPI), it needs to esnure that it suspends
> after SPI using suspend_late and resume using resume_early. That way dma is
> availble whenever the client is active

suspend_late is working in context that interrupt handler may be
invoked. Thus, to have DMA driver be properly shut down we have to
wait / terminate possible ongoing transfer.

It seems for me all DMA drivers that are using
system .suspend()/.resume() are potentially buggy.

> 
> --
> ~Vinod
> 
> > 
> > > > > How about SET_SYSTEM_SLEEP_PM_OPS instead?
> > > > 
> > > > So, we are using *_noirq versions of the functions here. What happened when we switch to normal ones? Any side effects?
> > 
> > 
> > 
> > -- 
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Intel Finland Oy
> > 
> 

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3976 bytes --]

On Mon, 2014-01-20 at 17:37 +0530, Vinod Koul wrote:
> On Mon, Jan 20, 2014 at 06:17:56PM +0530, Shevchenko, Andriy wrote:
> > On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote:
> > > On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote:
> > > > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote:
> > > > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee wrote:
> > > > 
> > > > > > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > > > > > callback whereby the callbacks would be executed after IRQ handlers have been
> > > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > > > > > of *_noirq suspend/PM callback? 
> > > > > 
> > > > > i dont see a reason why we need the noirq versions
> > > > 
> > > > Okay. I imagine the following use case.
> > > > 
> > > > For example we have compiled in DMA driver (dw_dmac) along with, for
> > > > example, SPI driver.
> > > > 
> > > > System was scheduled to go sleep.
> > > > 
> > > > An order of calling IIUC might be DMA first, then SPI (since they are
> > > > not in parent / child relations).
> > > > 
> > > > What was happened when SPI would like to do a DMA transfer and DMA is
> > > > going to sleep? I'm trying to understand if this is a case.
> > > In that case how does no irq version help us?
> > 
> > It guarantees that we have no user of DMA anymore, since there is no
> > interrupt going on.
> well how is that. It will gaurantee that there wont be interrupt. User can still
> submit a transaction or another transaction will be in progress...

This is how system suspend callback tree is called.

First it calls .suspend() for all devices, then .suspend_late(),
then .suspend_noirq().

There is set of assumptions per each callback round. After .suspend()
the device must be quiescent.

But...

> > > For these cases, I have been using suspend_late. Since the dmaengine driver is
> > > providing service to other clients (SPI), it needs to esnure that it suspends
> > > after SPI using suspend_late and resume using resume_early. That way dma is
> > > availble whenever the client is active
> > 
> > suspend_late is working in context that interrupt handler may be
> > invoked. Thus, to have DMA driver be properly shut down we have to
> > wait / terminate possible ongoing transfer.
> Well client is already suspended via .suspend. So where is the transaction :)

...as I already wrote before we have no parent-child relationship
between DMA and, for example, SPI. That means we may possible have the
case when SPI's .suspend() will be called later than DMA's one.

> > It seems for me all DMA drivers that are using
> > system .suspend()/.resume() are potentially buggy.
> Yup!

So, we have to decide what to do with them. .suspend_late() still seems
for me not the best approach. *Or* we have to check for ongoing
transaction and do something with it. *Or* just shut down the device and
rely on DMA transaction initiator that it handles the terminated
transaction properly.

What is you opinion?


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Vinod Koul]

On Mon, Jan 20, 2014 at 07:38:32PM +0530, Shevchenko, Andriy wrote:
> > > > > > > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > > > > > > callback whereby the callbacks would be executed after IRQ handlers have been
> > > > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > > > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > > > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > > > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > > > > > > of *_noirq suspend/PM callback? 
> > > > > > 
> > > > > > i dont see a reason why we need the noirq versions
> > > > > 
> > > > > Okay. I imagine the following use case.
> > > > > 
> > > > > For example we have compiled in DMA driver (dw_dmac) along with, for
> > > > > example, SPI driver.
> > > > > 
> > > > > System was scheduled to go sleep.
> > > > > 
> > > > > An order of calling IIUC might be DMA first, then SPI (since they are
> > > > > not in parent / child relations).
> > > > > 
> > > > > What was happened when SPI would like to do a DMA transfer and DMA is
> > > > > going to sleep? I'm trying to understand if this is a case.
> > > > In that case how does no irq version help us?
> > > 
> > > It guarantees that we have no user of DMA anymore, since there is no
> > > interrupt going on.
> > well how is that. It will gaurantee that there wont be interrupt. User can still
> > submit a transaction or another transaction will be in progress...
> 
> This is how system suspend callback tree is called.
> 
> First it calls .suspend() for all devices, then .suspend_late(),
> then .suspend_noirq().
> 
> There is set of assumptions per each callback round. After .suspend()
> the device must be quiescent.
> 
> But...
> 
> > > > For these cases, I have been using suspend_late. Since the dmaengine driver is
> > > > providing service to other clients (SPI), it needs to esnure that it suspends
> > > > after SPI using suspend_late and resume using resume_early. That way dma is
> > > > availble whenever the client is active
> > > 
> > > suspend_late is working in context that interrupt handler may be
> > > invoked. Thus, to have DMA driver be properly shut down we have to
> > > wait / terminate possible ongoing transfer.
> > Well client is already suspended via .suspend. So where is the transaction :)
> 
> ...as I already wrote before we have no parent-child relationship
> between DMA and, for example, SPI. That means we may possible have the
> case when SPI's .suspend() will be called later than DMA's one.
> 
> > > It seems for me all DMA drivers that are using
> > > system .suspend()/.resume() are potentially buggy.
> > Yup!
> 
> So, we have to decide what to do with them. .suspend_late() still seems
> for me not the best approach. *Or* we have to check for ongoing
> transaction and do something with it. *Or* just shut down the device and
> rely on DMA transaction initiator that it handles the terminated
> transaction properly.

As you clearly said, we dont have a parent-child relatation though we have big
dependency. I think this is true for DMA clients, i ran into similar situation
with i2c few days back!

So only think which can give us a good system behaviour would be clients getting
suspended first and then then service providing subsystems. (same reason why we
do dma driver loading and init much before others drivers)

So yes in the .suspend callback of the client, it needs to
1) abort any transactions it has
2) make the client quiscent

then the .late_suspend kicks in and suspend the core drivers like dma.

This is how it has worked reliably for me in production systems. I am all ears
if we have a better and cleaner apprach to this problem :)

-- 
~Vinod

Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Andy Shevchenko]

On Sun, 2014-01-26 at 16:47 +0530, Vinod Koul wrote:

> > > > > For these cases, I have been using suspend_late. Since the dmaengine driver is
> > > > > providing service to other clients (SPI), it needs to esnure that it suspends
> > > > > after SPI using suspend_late and resume using resume_early. That way dma is
> > > > > availble whenever the client is active
> > > > 
> > > > suspend_late is working in context that interrupt handler may be
> > > > invoked. Thus, to have DMA driver be properly shut down we have to
> > > > wait / terminate possible ongoing transfer.
> > > Well client is already suspended via .suspend. So where is the transaction :)
> > 
> > ...as I already wrote before we have no parent-child relationship
> > between DMA and, for example, SPI. That means we may possible have the
> > case when SPI's .suspend() will be called later than DMA's one.
> > 
> > > > It seems for me all DMA drivers that are using
> > > > system .suspend()/.resume() are potentially buggy.
> > > Yup!
> > 
> > So, we have to decide what to do with them. .suspend_late() still seems
> > for me not the best approach. *Or* we have to check for ongoing
> > transaction and do something with it. *Or* just shut down the device and
> > rely on DMA transaction initiator that it handles the terminated
> > transaction properly.
> 
> As you clearly said, we dont have a parent-child relatation though we have big
> dependency. I think this is true for DMA clients, i ran into similar situation
> with i2c few days back!
> 
> So only think which can give us a good system behaviour would be clients getting
> suspended first and then then service providing subsystems.

Agree.

>  (same reason why we
> do dma driver loading and init much before others drivers)

Yes, it would be done via deferred probe.

> So yes in the .suspend callback of the client, it needs to
> 1) abort any transactions it has
> 2) make the client quiscent
> 
> then the .late_suspend kicks in and suspend the core drivers like dma.
> 
> This is how it has worked reliably for me in production systems. I am all ears
> if we have a better and cleaner apprach to this problem :)

Yes, summarize everything we discussed we have to:
 - provide suspend_late, resume_early callbacks in the DMA driver
instead of *_noirq versions
 - ensure that all clients on our platforms follow the described
scenario

Chiau Ee, I think you may to change your patch accordingly.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

RE: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Chew, Chiau Ee]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3187 bytes --]



> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: Monday, January 27, 2014 6:17 PM
> To: Koul, Vinod
> Cc: Andy Shevchenko; Chew, Chiau Ee; Viresh Kumar; Williams, Dan J;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode
> DW_DMAC.
> 
> On Sun, 2014-01-26 at 16:47 +0530, Vinod Koul wrote:
> 
> > > > > > For these cases, I have been using suspend_late. Since the
> > > > > > dmaengine driver is providing service to other clients (SPI),
> > > > > > it needs to esnure that it suspends after SPI using
> > > > > > suspend_late and resume using resume_early. That way dma is
> > > > > > availble whenever the client is active
> > > > >
> > > > > suspend_late is working in context that interrupt handler may be
> > > > > invoked. Thus, to have DMA driver be properly shut down we have
> > > > > to wait / terminate possible ongoing transfer.
> > > > Well client is already suspended via .suspend. So where is the
> > > > transaction :)
> > >
> > > ...as I already wrote before we have no parent-child relationship
> > > between DMA and, for example, SPI. That means we may possible have
> > > the case when SPI's .suspend() will be called later than DMA's one.
> > >
> > > > > It seems for me all DMA drivers that are using system
> > > > > .suspend()/.resume() are potentially buggy.
> > > > Yup!
> > >
> > > So, we have to decide what to do with them. .suspend_late() still
> > > seems for me not the best approach. *Or* we have to check for
> > > ongoing transaction and do something with it. *Or* just shut down
> > > the device and rely on DMA transaction initiator that it handles the
> > > terminated transaction properly.
> >
> > As you clearly said, we dont have a parent-child relatation though we
> > have big dependency. I think this is true for DMA clients, i ran into
> > similar situation with i2c few days back!
> >
> > So only think which can give us a good system behaviour would be
> > clients getting suspended first and then then service providing subsystems.
> 
> Agree.
> 
> >  (same reason why we
> > do dma driver loading and init much before others drivers)
> 
> Yes, it would be done via deferred probe.
> 
> > So yes in the .suspend callback of the client, it needs to
> > 1) abort any transactions it has
> > 2) make the client quiscent
> >
> > then the .late_suspend kicks in and suspend the core drivers like dma.
> >
> > This is how it has worked reliably for me in production systems. I am
> > all ears if we have a better and cleaner apprach to this problem :)
> 
> Yes, summarize everything we discussed we have to:
>  - provide suspend_late, resume_early callbacks in the DMA driver instead of
> *_noirq versions
>  - ensure that all clients on our platforms follow the described scenario
> 
> Chiau Ee, I think you may to change your patch accordingly.

Ok. Noted

> 
> 
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥