[PATCH] Revert "dmaengine: dw: Enable runtime PM"

[PATCH] Revert "dmaengine: dw: Enable runtime PM"

[Cezary Rojewski]

This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
and DMA1 are part of a single entity) rather than being a standalone
one. Driver for said device may enlist DMA to transfer data during
suspend or resume sequences.

Manipulating RPM explicitly in dw's DMA request and release channel
functions causes suspend() to also invoke resume() for the exact same
device. Similar situation occurs for resume() sequence. Effectively
renders device dysfunctional after first suspend() attempt. Revert the
change to address the problem.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 drivers/dma/dw/core.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 19a23767533a..7ab83fe601ed 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -982,11 +982,8 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 
 	dev_vdbg(chan2dev(chan), "%s\n", __func__);
 
-	pm_runtime_get_sync(dw->dma.dev);
-
 	/* ASSERT:  channel is idle */
 	if (dma_readl(dw, CH_EN) & dwc->mask) {
-		pm_runtime_put_sync_suspend(dw->dma.dev);
 		dev_dbg(chan2dev(chan), "DMA channel not idle?\n");
 		return -EIO;
 	}
@@ -1003,7 +1000,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * We need controller-specific data to set up slave transfers.
 	 */
 	if (chan->private && !dw_dma_filter(chan, chan->private)) {
-		pm_runtime_put_sync_suspend(dw->dma.dev);
 		dev_warn(chan2dev(chan), "Wrong controller-specific data\n");
 		return -EINVAL;
 	}
@@ -1047,8 +1043,6 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	if (!dw->in_use)
 		do_dw_dma_off(dw);
 
-	pm_runtime_put_sync_suspend(dw->dma.dev);
-
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
 }
 
-- 
2.17.1

Re: [PATCH] Revert "dmaengine: dw: Enable runtime PM"

[Andy Shevchenko]

On Wed, Feb 3, 2021 at 5:53 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
> For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
> compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
> and DMA1 are part of a single entity) rather than being a standalone
> one. Driver for said device may enlist DMA to transfer data during
> suspend or resume sequences.
>
> Manipulating RPM explicitly in dw's DMA request and release channel
> functions causes suspend() to also invoke resume() for the exact same
> device. Similar situation occurs for resume() sequence. Effectively
> renders device dysfunctional after first suspend() attempt. Revert the
> change to address the problem.

I kinda had the mixed feelings about this, thanks for the report.
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Fixes tag?

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  drivers/dma/dw/core.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 19a23767533a..7ab83fe601ed 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -982,11 +982,8 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
>
>         dev_vdbg(chan2dev(chan), "%s\n", __func__);
>
> -       pm_runtime_get_sync(dw->dma.dev);
> -
>         /* ASSERT:  channel is idle */
>         if (dma_readl(dw, CH_EN) & dwc->mask) {
> -               pm_runtime_put_sync_suspend(dw->dma.dev);
>                 dev_dbg(chan2dev(chan), "DMA channel not idle?\n");
>                 return -EIO;
>         }
> @@ -1003,7 +1000,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
>          * We need controller-specific data to set up slave transfers.
>          */
>         if (chan->private && !dw_dma_filter(chan, chan->private)) {
> -               pm_runtime_put_sync_suspend(dw->dma.dev);
>                 dev_warn(chan2dev(chan), "Wrong controller-specific data\n");
>                 return -EINVAL;
>         }
> @@ -1047,8 +1043,6 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>         if (!dw->in_use)
>                 do_dw_dma_off(dw);
>
> -       pm_runtime_put_sync_suspend(dw->dma.dev);
> -
>         dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
>  }
>
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Revert "dmaengine: dw: Enable runtime PM"

[Andy Shevchenko]

On Wed, Feb 3, 2021 at 7:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Feb 3, 2021 at 5:53 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
> >
> > This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
> > For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
> > compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
> > and DMA1 are part of a single entity) rather than being a standalone
> > one. Driver for said device may enlist DMA to transfer data during
> > suspend or resume sequences.
> >
> > Manipulating RPM explicitly in dw's DMA request and release channel
> > functions causes suspend() to also invoke resume() for the exact same
> > device. Similar situation occurs for resume() sequence. Effectively
> > renders device dysfunctional after first suspend() attempt. Revert the
> > change to address the problem.
>
> I kinda had the mixed feelings about this, thanks for the report.

Side note: the better solution in general seems to have a specific
power domain for the ASoC multi-function devices (if ever you move to
use auxiliary bus, it may be done easier I think).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Revert "dmaengine: dw: Enable runtime PM"

[Cezary Rojewski]

On 2021-02-03 6:06 PM, Andy Shevchenko wrote:
> On Wed, Feb 3, 2021 at 5:53 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
>>
>> This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
>> For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
>> compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
>> and DMA1 are part of a single entity) rather than being a standalone
>> one. Driver for said device may enlist DMA to transfer data during
>> suspend or resume sequences.
>>
>> Manipulating RPM explicitly in dw's DMA request and release channel
>> functions causes suspend() to also invoke resume() for the exact same
>> device. Similar situation occurs for resume() sequence. Effectively
>> renders device dysfunctional after first suspend() attempt. Revert the
>> change to address the problem.
> 
> I kinda had the mixed feelings about this, thanks for the report.
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Fixes tag?

Noted, sent v2 with updated tag area.

Thanks,
Czarek

Re: [PATCH] Revert "dmaengine: dw: Enable runtime PM"

[Cezary Rojewski]

On 2021-02-03 6:08 PM, Andy Shevchenko wrote:
> On Wed, Feb 3, 2021 at 7:06 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>> On Wed, Feb 3, 2021 at 5:53 PM Cezary Rojewski
>> <cezary.rojewski@intel.com> wrote:
>>>
>>> This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
>>> For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
>>> compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
>>> and DMA1 are part of a single entity) rather than being a standalone
>>> one. Driver for said device may enlist DMA to transfer data during
>>> suspend or resume sequences.
>>>
>>> Manipulating RPM explicitly in dw's DMA request and release channel
>>> functions causes suspend() to also invoke resume() for the exact same
>>> device. Similar situation occurs for resume() sequence. Effectively
>>> renders device dysfunctional after first suspend() attempt. Revert the
>>> change to address the problem.
>>
>> I kinda had the mixed feelings about this, thanks for the report.
> 
> Side note: the better solution in general seems to have a specific
> power domain for the ASoC multi-function devices (if ever you move to
> use auxiliary bus, it may be done easier I think).

This is an area I haven't touched yet. Will definitely check it out.

Thanks for the recommendations, Andy. Much appreciated.

Regards,
Czarek