[PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Peter Ujfalusi]

Rearrange the pm_runtime_get/put_sync calls so the IP will be turned off
when it is not in use.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 031c1fb44ae7..a51197ac0b92 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -528,6 +528,7 @@ static int __davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
 {
 	struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
 
+	pm_runtime_get_sync(mcasp->dev);
 	switch (div_id) {
 	case 0:		/* MCLK divider */
 		mcasp_mod_bits(mcasp, DAVINCI_MCASP_AHCLKXCTL_REG,
@@ -553,6 +554,7 @@ static int __davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
 		return -EINVAL;
 	}
 
+	pm_runtime_put_sync(mcasp->dev);
 	return 0;
 }
 
@@ -567,6 +569,7 @@ static int davinci_mcasp_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 {
 	struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
 
+	pm_runtime_get_sync(mcasp->dev);
 	if (dir == SND_SOC_CLOCK_OUT) {
 		mcasp_set_bits(mcasp, DAVINCI_MCASP_AHCLKXCTL_REG, AHCLKXE);
 		mcasp_set_bits(mcasp, DAVINCI_MCASP_AHCLKRCTL_REG, AHCLKRE);
@@ -579,6 +582,7 @@ static int davinci_mcasp_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 
 	mcasp->sysclk_freq = freq;
 
+	pm_runtime_put_sync(mcasp->dev);
 	return 0;
 }
 
@@ -1078,6 +1082,9 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
 	u32 reg;
 	int i;
 
+	if (!dai->active)
+		pm_runtime_get_sync(mcasp->dev);
+
 	for (i = 0; i < ARRAY_SIZE(context_regs); i++)
 		context->config_regs[i] = mcasp_get_reg(mcasp, context_regs[i]);
 
@@ -1094,6 +1101,8 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
 		context->xrsr_regs[i] = mcasp_get_reg(mcasp,
 						DAVINCI_MCASP_XRSRCTL_REG(i));
 
+	pm_runtime_put_sync(mcasp->dev);
+
 	return 0;
 }
 
@@ -1104,6 +1113,8 @@ static int davinci_mcasp_resume(struct snd_soc_dai *dai)
 	u32 reg;
 	int i;
 
+	pm_runtime_get_sync(mcasp->dev);
+
 	for (i = 0; i < ARRAY_SIZE(context_regs); i++)
 		mcasp_set_reg(mcasp, context_regs[i], context->config_regs[i]);
 
@@ -1120,6 +1131,9 @@ static int davinci_mcasp_resume(struct snd_soc_dai *dai)
 		mcasp_set_reg(mcasp, DAVINCI_MCASP_XRSRCTL_REG(i),
 			      context->xrsr_regs[i]);
 
+	if (!dai->active)
+		pm_runtime_put_sync(mcasp->dev);
+
 	return 0;
 }
 #else
@@ -1427,13 +1441,6 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
-	ret = pm_runtime_get_sync(&pdev->dev);
-	if (IS_ERR_VALUE(ret)) {
-		dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n");
-		pm_runtime_disable(&pdev->dev);
-		return ret;
-	}
-
 	mcasp->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
 	if (!mcasp->base) {
 		dev_err(&pdev->dev, "ioremap failed\n");
@@ -1633,14 +1640,12 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 	return 0;
 
 err:
-	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return ret;
 }
 
 static int davinci_mcasp_remove(struct platform_device *pdev)
 {
-	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
-- 
2.3.0

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1792 bytes --]

On Tue, Mar 03, 2015 at 04:26:32PM +0200, Peter Ujfalusi wrote:

> @@ -553,6 +554,7 @@ static int __davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
>  		return -EINVAL;
>  	}
>  
> +	pm_runtime_put_sync(mcasp->dev);

Why the _put_sync()s?  I know it's more symmetrical but I don't see a
need to wait and it means we may be able to avoid some unneeded power
cycles.

> @@ -1078,6 +1082,9 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
>  	u32 reg;
>  	int i;
>  
> +	if (!dai->active)
> +		pm_runtime_get_sync(mcasp->dev);
> +
>  	for (i = 0; i < ARRAY_SIZE(context_regs); i++)
>  		context->config_regs[i] = mcasp_get_reg(mcasp, context_regs[i]);
>  
> @@ -1094,6 +1101,8 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
>  		context->xrsr_regs[i] = mcasp_get_reg(mcasp,
>  						DAVINCI_MCASP_XRSRCTL_REG(i));
>  
> +	pm_runtime_put_sync(mcasp->dev);
> +

On first glance this looks unbalanced without the active check in the
put case.  I'd also need to check if runtime resume works OK in suspend
context, I guess it must but there were issues in the past (the more
common idiom is to not suspend if we're already runtime powered down but
obviously here we're actually doing extra work).

I see there's actually a balanced put in the resume path...  I see
what's going on here but I'm thinking that perhaps something more
explicit that calls the ops directly and checks pm_runtime_is_enabled()
might be clearer?  Unconditionally do the power down/up at the end with
a direct call outside of runtime PM and then make the checks at the
top/bottom be "if it's runtime suspended then...".  That's also more
direct about what's actually being checked and hence less risk that some
future changes would go wrong.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Peter Ujfalusi]

On 03/03/2015 05:07 PM, Mark Brown wrote:
> On Tue, Mar 03, 2015 at 04:26:32PM +0200, Peter Ujfalusi wrote:
> 
>> @@ -553,6 +554,7 @@ static int __davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
>>  		return -EINVAL;
>>  	}
>>  
>> +	pm_runtime_put_sync(mcasp->dev);
> 
> Why the _put_sync()s?  I know it's more symmetrical but I don't see a
> need to wait and it means we may be able to avoid some unneeded power
> cycles.

True, the _sync can be dropped. I guess I used it just for symmetry ;)

> 
>> @@ -1078,6 +1082,9 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
>>  	u32 reg;
>>  	int i;
>>  
>> +	if (!dai->active)
>> +		pm_runtime_get_sync(mcasp->dev);
>> +
>>  	for (i = 0; i < ARRAY_SIZE(context_regs); i++)
>>  		context->config_regs[i] = mcasp_get_reg(mcasp, context_regs[i]);
>>  
>> @@ -1094,6 +1101,8 @@ static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
>>  		context->xrsr_regs[i] = mcasp_get_reg(mcasp,
>>  						DAVINCI_MCASP_XRSRCTL_REG(i));
>>  
>> +	pm_runtime_put_sync(mcasp->dev);
>> +
> 
> On first glance this looks unbalanced without the active check in the
> put case.  I'd also need to check if runtime resume works OK in suspend
> context, I guess it must but there were issues in the past (the more
> common idiom is to not suspend if we're already runtime powered down but
> obviously here we're actually doing extra work).

McASP is only going to loose it's context when the whole device goes to
suspend. In this case I need to save the registers which I need to restore in
resume time.

> I see there's actually a balanced put in the resume path...  I see
> what's going on here but I'm thinking that perhaps something more
> explicit that calls the ops directly and checks pm_runtime_is_enabled()
> might be clearer?

The issue with the pm_runtime_is_enabled() is going to be in the resume path,
as I did get_sync() it will return true at the end all the time.

In order to save/restore McASP register the IP need to be enabled.

-If we suspend/resume while no audio was running:
dai->active is false, pm_runtime is disabled when the davinci_mcasp_suspend()
is called. So I need to get_sync(), save context and put_sync() so we are in
balance.
In resume, dai->active is false, pm_runtime is disabled, so I again need to
get_sync(), restore registers and put_sync() so we are in balance again and
mcasp stay disabled.

-If we suspend/resume while audio was running:
dai->active is true, pm_runtime is enabled when the davinci_mcasp_suspend() is
called. I don't need the get_sync(), save context and put_sync() so I release
all the resources needed for McASP. runtime_pm will be disabled.
In resume dai->active is true, pm_runtime is disabled. I need to get_sync(),
restore registers and since dai->active is true I should not put_sync() to
keep McASP enabled and be able to resume the audio.

I think this hassle is needed if I want to achieve what subject describes.

> Unconditionally do the power down/up at the end with
> a direct call outside of runtime PM and then make the checks at the
> top/bottom be "if it's runtime suspended then...".  That's also more
> direct about what's actually being checked and hence less risk that some
> future changes would go wrong.
> 


-- 
Péter

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 554 bytes --]

On Wed, Mar 04, 2015 at 12:12:36PM +0200, Peter Ujfalusi wrote:
> On 03/03/2015 05:07 PM, Mark Brown wrote:

> > I see there's actually a balanced put in the resume path...  I see
> > what's going on here but I'm thinking that perhaps something more
> > explicit that calls the ops directly and checks pm_runtime_is_enabled()
> > might be clearer?

> The issue with the pm_runtime_is_enabled() is going to be in the resume path,
> as I did get_sync() it will return true at the end all the time.

You can just store the state before you do the enable...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Peter Ujfalusi]

On 03/04/2015 01:30 PM, Mark Brown wrote:
> On Wed, Mar 04, 2015 at 12:12:36PM +0200, Peter Ujfalusi wrote:
>> On 03/03/2015 05:07 PM, Mark Brown wrote:
> 
>>> I see there's actually a balanced put in the resume path...  I see
>>> what's going on here but I'm thinking that perhaps something more
>>> explicit that calls the ops directly and checks pm_runtime_is_enabled()
>>> might be clearer?
> 
>> The issue with the pm_runtime_is_enabled() is going to be in the resume path,
>> as I did get_sync() it will return true at the end all the time.
> 
> You can just store the state before you do the enable...

When we come out from suspend the pm_runtime status is disabled all the time.

-- 
Péter

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 325 bytes --]

On Wed, Mar 04, 2015 at 01:31:48PM +0200, Peter Ujfalusi wrote:
> On 03/04/2015 01:30 PM, Mark Brown wrote:

> > You can just store the state before you do the enable...

> When we come out from suspend the pm_runtime status is disabled all the time.

It's fine at the start of the suspend though, you can just record state?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Peter Ujfalusi]

On 03/04/2015 07:13 PM, Mark Brown wrote:
> On Wed, Mar 04, 2015 at 01:31:48PM +0200, Peter Ujfalusi wrote:
>> On 03/04/2015 01:30 PM, Mark Brown wrote:
> 
>>> You can just store the state before you do the enable...
> 
>> When we come out from suspend the pm_runtime status is disabled all the time.
> 
> It's fine at the start of the suspend though, you can just record state?

Yes, it is possible to add a bool for this, but I don't really see the point
of duplicating the dai->active within the mcasp private data.

-- 
Péter

Re: [PATCH] ASoC: davinci-mcasp: Allow complete shutdown of McASP when not in use

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 389 bytes --]

On Thu, Mar 05, 2015 at 12:51:02PM +0200, Peter Ujfalusi wrote:

> Yes, it is possible to add a bool for this, but I don't really see the point
> of duplicating the dai->active within the mcasp private data.

That's assuming that the runtime PM state maps completely onto the DAI
being active.  That may be true right now but it seems like it'd be more
robust to not make that assumption.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]