[PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Jane Wang]

McBSP fucntional clock is not disabled when suspend is triggerd which
prevents PER domain entering target low-power state. To fix the problem:

1. Disable/enable McBSP functional clock for suspend/resume.

2. In addition, reconfigure McBSP, this is needed when systerm comes out of OFF state.

Signed-off-by: Jane Wang <jwang@ti.com>
---
 arch/arm/plat-omap/include/mach/mcbsp.h |    2 ++
 arch/arm/plat-omap/mcbsp.c              |   27 +++++++++++++++++++++++++++
 sound/soc/omap/omap-mcbsp.c             |   14 ++++++++++++--
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h
index e0d6eca..94e1c66 100644
--- a/arch/arm/plat-omap/include/mach/mcbsp.h
+++ b/arch/arm/plat-omap/include/mach/mcbsp.h
@@ -440,6 +440,8 @@ static inline int omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; }
 #endif
 int omap_mcbsp_request(unsigned int id);
 void omap_mcbsp_free(unsigned int id);
+void omap_mcbsp_disable_fclk(unsigned int id);
+void omap_mcbsp_enable_fclk(unsigned int id);
 void omap_mcbsp_start(unsigned int id, int tx, int rx);
 void omap_mcbsp_stop(unsigned int id, int tx, int rx);
 void omap_mcbsp_xmit_word(unsigned int id, u32 word);
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index e664b91..bfe3c61 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -459,6 +459,33 @@ int omap_mcbsp_request(unsigned int id)
 }
 EXPORT_SYMBOL(omap_mcbsp_request);
 
+void omap_mcbsp_disable_fclk(unsigned int id)
+{
+	struct omap_mcbsp *mcbsp;
+
+	if (!omap_mcbsp_check_valid_id(id)) {
+		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
+		return;
+	}
+	mcbsp = id_to_mcbsp_ptr(id);
+	clk_disable(mcbsp->fclk);
+}
+EXPORT_SYMBOL(omap_mcbsp_disable_fclk);
+
+
+void omap_mcbsp_enable_fclk(unsigned int id)
+{
+	struct omap_mcbsp *mcbsp;
+
+	if (!omap_mcbsp_check_valid_id(id)) {
+		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
+		return;
+	}
+	mcbsp = id_to_mcbsp_ptr(id);
+	clk_enable(mcbsp->fclk);
+}
+EXPORT_SYMBOL(omap_mcbsp_enable_fclk);
+
 void omap_mcbsp_free(unsigned int id)
 {
 	struct omap_mcbsp *mcbsp;
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 45be942..e56b5ff 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -229,18 +229,28 @@ static int omap_mcbsp_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		mcbsp_data->active++;
 		omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
 		break;
+	case SNDRV_PCM_TRIGGER_RESUME:
+		mcbsp_data->active++;
+		omap_mcbsp_enable_fclk(mcbsp_data->bus_id);
+		omap_mcbsp_config(mcbsp_data->bus_id,
+					&mcbsp_data->regs);
+		omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
+		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		omap_mcbsp_stop(mcbsp_data->bus_id, play, !play);
 		mcbsp_data->active--;
 		break;
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		omap_mcbsp_stop(mcbsp_data->bus_id, play, !play);
+		omap_mcbsp_disable_fclk(mcbsp_data->bus_id);
+		mcbsp_data->active--;
+		break;
 	default:
 		err = -EINVAL;
 	}
-- 
1.5.4.3

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Mark Brown]

On 13 Nov 2009, at 17:39, Jane Wang <jwang@ti.com> wrote:

> McBSP fucntional clock is not disabled when suspend is triggerd which
> prevents PER domain entering target low-power state. To fix the  
> problem:
>
> 1. Disable/enable McBSP functional clock for suspend/resume.
>
> 2. In addition, reconfigure McBSP, this is needed when systerm comes  
> out of OFF state.
>

There is no need to resend your patches this iften - please allow at  
least a week or so for review. You only need to repost if addressing  
comments or it looks like the patch got missed.

> Signed-off-by: Jane Wang <jwang@ti.com>
> ---
> arch/arm/plat-omap/include/mach/mcbsp.h |    2 ++
> arch/arm/plat-omap/mcbsp.c              |   27 ++++++++++++++++++++++ 
> +++++
> sound/soc/omap/omap-mcbsp.c             |   14 ++++++++++++--
> 3 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat- 
> omap/include/mach/mcbsp.h
> index e0d6eca..94e1c66 100644
> --- a/arch/arm/plat-omap/include/mach/mcbsp.h
> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
> @@ -440,6 +440,8 @@ static inline int omap_mcbsp_get_dma_op_mode 
> (unsigned int id) { return 0; }
> #endif
> int omap_mcbsp_request(unsigned int id);
> void omap_mcbsp_free(unsigned int id);
> +void omap_mcbsp_disable_fclk(unsigned int id);
> +void omap_mcbsp_enable_fclk(unsigned int id);
> void omap_mcbsp_start(unsigned int id, int tx, int rx);
> void omap_mcbsp_stop(unsigned int id, int tx, int rx);
> void omap_mcbsp_xmit_word(unsigned int id, u32 word);
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index e664b91..bfe3c61 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -459,6 +459,33 @@ int omap_mcbsp_request(unsigned int id)
> }
> EXPORT_SYMBOL(omap_mcbsp_request);
>
> +void omap_mcbsp_disable_fclk(unsigned int id)
> +{
> +    struct omap_mcbsp *mcbsp;
> +
> +    if (!omap_mcbsp_check_valid_id(id)) {
> +        printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> +        return;
> +    }
> +    mcbsp = id_to_mcbsp_ptr(id);
> +    clk_disable(mcbsp->fclk);
> +}
> +EXPORT_SYMBOL(omap_mcbsp_disable_fclk);
> +
> +
> +void omap_mcbsp_enable_fclk(unsigned int id)
> +{
> +    struct omap_mcbsp *mcbsp;
> +
> +    if (!omap_mcbsp_check_valid_id(id)) {
> +        printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> +        return;
> +    }
> +    mcbsp = id_to_mcbsp_ptr(id);
> +    clk_enable(mcbsp->fclk);
> +}
> +EXPORT_SYMBOL(omap_mcbsp_enable_fclk);
> +
> void omap_mcbsp_free(unsigned int id)
> {
>    struct omap_mcbsp *mcbsp;
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> index 45be942..e56b5ff 100644
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -229,18 +229,28 @@ static int omap_mcbsp_dai_trigger(struct  
> snd_pcm_substream *substream, int cmd,
>
>    switch (cmd) {
>    case SNDRV_PCM_TRIGGER_START:
> -    case SNDRV_PCM_TRIGGER_RESUME:
>    case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>        mcbsp_data->active++;
>        omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
>        break;
> +    case SNDRV_PCM_TRIGGER_RESUME:
> +        mcbsp_data->active++;
> +        omap_mcbsp_enable_fclk(mcbsp_data->bus_id);
> +        omap_mcbsp_config(mcbsp_data->bus_id,
> +                    &mcbsp_data->regs);
> +        omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
> +        break;
>
>    case SNDRV_PCM_TRIGGER_STOP:
> -    case SNDRV_PCM_TRIGGER_SUSPEND:
>    case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>        omap_mcbsp_stop(mcbsp_data->bus_id, play, !play);
>        mcbsp_data->active--;
>        break;
> +    case SNDRV_PCM_TRIGGER_SUSPEND:
> +        omap_mcbsp_stop(mcbsp_data->bus_id, play, !play);
> +        omap_mcbsp_disable_fclk(mcbsp_data->bus_id);
> +        mcbsp_data->active--;
> +        break;
>    default:
>        err = -EINVAL;
>    }
> -- 
> 1.5.4.3
>

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Wang, Jane]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-
> project.org] On Behalf Of Mark Brown
> Sent: Friday, November 13, 2009 11:49 AM
> To: Wang, Jane
> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> peter.ujfalusi@nokia.com; Wang, Jane
> Subject: Re: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure
> fix
> 
> On 13 Nov 2009, at 17:39, Jane Wang <jwang@ti.com> wrote:
> 
> > McBSP fucntional clock is not disabled when suspend is triggerd which
> > prevents PER domain entering target low-power state. To fix the
> > problem:
> >
> > 1. Disable/enable McBSP functional clock for suspend/resume.
> >
> > 2. In addition, reconfigure McBSP, this is needed when systerm comes
> > out of OFF state.
> >
> 
> There is no need to resend your patches this iften - please allow at
> least a week or so for review. You only need to repost if addressing
> comments or it looks like the patch got missed.
> 

Sorry for sending this again. Something is not set correctly with my
git send-email. The email did not reach the mailing lists, it only reached
people in cc.

Re: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Mark Brown]

On 13 Nov 2009, at 17:52, "Wang, Jane" <jwang@ti.com> wrote:

>
>
>> -----Original Message-----
>> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel- 
>> bounces@alsa-
>> project.org] On Behalf Of Mark Brown
>> Sent: Friday, November 13, 2009 11:49 AM
>> To: Wang, Jane
>> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
>> peter.ujfalusi@nokia.com; Wang, Jane
>> Subject: Re: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume  
>> failure
>> fix
>>
>> On 13 Nov 2009, at 17:39, Jane Wang <jwang@ti.com> wrote:
>>
>>> McBSP fucntional clock is not disabled when suspend is triggerd  
>>> which
>>> prevents PER domain entering target low-power state. To fix the
>>> problem:
>>>
>>> 1. Disable/enable McBSP functional clock for suspend/resume.
>>>
>>> 2. In addition, reconfigure McBSP, this is needed when systerm comes
>>> out of OFF state.
>>>
>>
>> There is no need to resend your patches this iften - please allow at
>> least a week or so for review. You only need to repost if addressing
>> comments or it looks like the patch got missed.
>>
>
> Sorry for sending this again. Something is not set correctly with my
> git send-email. The email did not reach the mailing lists, it only  
> reached
> people in cc.

No, your posts are making it through to the lists just fine. Note that  
alsa-devel is moderated for non-subscribers

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Jarkko Nikula]

Hi

On Fri, 13 Nov 2009 11:39:47 -0600
Jane Wang <jwang@ti.com> wrote:

> McBSP fucntional clock is not disabled when suspend is triggerd which
> prevents PER domain entering target low-power state. To fix the problem:
> 
> 1. Disable/enable McBSP functional clock for suspend/resume.
> 
> 2. In addition, reconfigure McBSP, this is needed when systerm comes out of OFF state.
> 
...
> --- a/arch/arm/plat-omap/include/mach/mcbsp.h
> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
> @@ -440,6 +440,8 @@ static inline int omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; }
>  #endif
>  int omap_mcbsp_request(unsigned int id);
>  void omap_mcbsp_free(unsigned int id);
> +void omap_mcbsp_disable_fclk(unsigned int id);
> +void omap_mcbsp_enable_fclk(unsigned int id);
...
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -229,18 +229,28 @@ static int omap_mcbsp_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> -	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		mcbsp_data->active++;
>  		omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
>  		break;
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		mcbsp_data->active++;
> +		omap_mcbsp_enable_fclk(mcbsp_data->bus_id);
> +		omap_mcbsp_config(mcbsp_data->bus_id,
> +					&mcbsp_data->regs);
> +		omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
> +		break;
>  
For me this looks more like a simple workaround for the audio case than
comprehensive implementation to McBSP OFF mode context save/restore. My
thoughts:

1. McBSP clock management partially moved out of low-level driver
2. McBSP client must do the context restore

I do know that arch/arm/plat-omap/mcbsp.c enables the clocks in
omap_mcbsp_request, I don't know do some API or feature there really
require clocks (or McBSP itself) to be active just after the request
time but I would look that path instead (even it is more complicated).

If no API or feature would require active McBSP block before the
omap_mcbsp_start call, then the clock management could be done inside
the omap_mcbsp_start/-stop functions.

Also context save/restore operations belongs more to PM callbacks
of .../plat-omap/mcbsp.c. Or probably that can be done also dynamically
inside the omap_mcbsp_start/-stop for keeping the whole McBSP shutdown
whenever it is idle.

I think it's worth to look McBSP register cache concept [1] from Janusz
Krzysztofik would it help all of this context save/restore operations.


-- 
Jarkko

1. http://www.spinics.net/lists/linux-omap/msg16720.html

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Aggarwal, Anuj]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Jarkko Nikula
> Sent: Saturday, November 14, 2009 1:50 PM
> To: Wang, Jane
> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> peter.ujfalusi@nokia.com; broonie@opensource.wolfsonmicro.com
> Subject: Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix
> 
> Hi
> 
> On Fri, 13 Nov 2009 11:39:47 -0600
> Jane Wang <jwang@ti.com> wrote:
> 
> > McBSP fucntional clock is not disabled when suspend is triggerd which
> > prevents PER domain entering target low-power state. To fix the problem:
> >
> > 1. Disable/enable McBSP functional clock for suspend/resume.
> >
> > 2. In addition, reconfigure McBSP, this is needed when systerm comes out
> of OFF state.
> >
> ...
> > --- a/arch/arm/plat-omap/include/mach/mcbsp.h
> > +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
> > @@ -440,6 +440,8 @@ static inline int
> omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; }
> >  #endif
> >  int omap_mcbsp_request(unsigned int id);
> >  void omap_mcbsp_free(unsigned int id);
> > +void omap_mcbsp_disable_fclk(unsigned int id);
> > +void omap_mcbsp_enable_fclk(unsigned int id);
> ...
> > --- a/sound/soc/omap/omap-mcbsp.c
> > +++ b/sound/soc/omap/omap-mcbsp.c
> > @@ -229,18 +229,28 @@ static int omap_mcbsp_dai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> >
> >  	switch (cmd) {
> >  	case SNDRV_PCM_TRIGGER_START:
> > -	case SNDRV_PCM_TRIGGER_RESUME:
> >  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> >  		mcbsp_data->active++;
> >  		omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
> >  		break;
> > +	case SNDRV_PCM_TRIGGER_RESUME:
> > +		mcbsp_data->active++;
> > +		omap_mcbsp_enable_fclk(mcbsp_data->bus_id);
> > +		omap_mcbsp_config(mcbsp_data->bus_id,
> > +					&mcbsp_data->regs);
> > +		omap_mcbsp_start(mcbsp_data->bus_id, play, !play);
> > +		break;
> >
> For me this looks more like a simple workaround for the audio case than
> comprehensive implementation to McBSP OFF mode context save/restore. My
> thoughts:
> 
> 1. McBSP clock management partially moved out of low-level driver
> 2. McBSP client must do the context restore
> 
> I do know that arch/arm/plat-omap/mcbsp.c enables the clocks in
> omap_mcbsp_request, I don't know do some API or feature there really
> require clocks (or McBSP itself) to be active just after the request
> time but I would look that path instead (even it is more complicated).
> 
> If no API or feature would require active McBSP block before the
> omap_mcbsp_start call, then the clock management could be done inside
> the omap_mcbsp_start/-stop functions.
[Aggarwal, Anuj] I tried doing that but it didn't work because before
_start, we need to call set_hw_params which configures mcbsp, for which
clocks are required.
> 
> Also context save/restore operations belongs more to PM callbacks
> of .../plat-omap/mcbsp.c. Or probably that can be done also dynamically
> inside the omap_mcbsp_start/-stop for keeping the whole McBSP shutdown
> whenever it is idle.
[Aggarwal, Anuj] I am sorry I couldn't understand that. Did you mean that
PM hooks can be added in the mcbsp driver which can be registered and called
As and when required? Any reference driver for this, if my understanding
is correct?
> 
> I think it's worth to look McBSP register cache concept [1] from Janusz
> Krzysztofik would it help all of this context save/restore operations.
[Aggarwal, Anuj] I tried these two patches on omap3 evm but they didn't
work for me. The system doesn't go to the suspend state when I do:
echo mem > /sys/power/state
And the culprits were core (think sdma) and per domain. I tried 
disabling the mcbsp i/f clock as well but it didn't help. On mcbsp 
register dump, I couldn't find the sysconfig register getting 
configured. Could that be the root cause?
> 
> 
> --
> Jarkko
> 
> 1. http://www.spinics.net/lists/linux-omap/msg16720.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Wang, Jane]

> > Also context save/restore operations belongs more to PM callbacks
> > of .../plat-omap/mcbsp.c. Or probably that can be done also dynamically
> > inside the omap_mcbsp_start/-stop for keeping the whole McBSP shutdown
> > whenever it is idle.
> [Aggarwal, Anuj] I am sorry I couldn't understand that. Did you mean that
> PM hooks can be added in the mcbsp driver which can be registered and
> called
> As and when required? Any reference driver for this, if my understanding
> is correct?
> >
> > I think it's worth to look McBSP register cache concept [1] from Janusz
> > Krzysztofik would it help all of this context save/restore operations.
> [Aggarwal, Anuj] I tried these two patches on omap3 evm but they didn't
> work for me. The system doesn't go to the suspend state when I do:
> echo mem > /sys/power/state
> And the culprits were core (think sdma) and per domain. I tried
> disabling the mcbsp i/f clock as well but it didn't help. On mcbsp
> register dump, I couldn't find the sysconfig register getting
> configured. Could that be the root cause?

Did you include this patch?
http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=commit;h=9da65a99e5e6a074c586474961dbf560e439df50

Without this patch, omap_stop_dma() did not disable DMA channel.


Regards,

-Jane

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Jarkko Nikula]

On Wed, Nov 18, 2009 at 8:28 PM, Aggarwal, Anuj <anuj.aggarwal@ti.com>wrote:

> > If no API or feature would require active McBSP block before the
> > omap_mcbsp_start call, then the clock management could be done inside
> > the omap_mcbsp_start/-stop functions.
> [Aggarwal, Anuj] I tried doing that but it didn't work because before
> _start, we need to call set_hw_params which configures mcbsp, for which
> clocks are required.
> >
>

First modification to .../plat-omap/mcbsp.c could be just a patch which
keeps the
clocks on only when needed. I.e. keep them disabled when returning from
request
and activate them only when accessing the McBSP registers or when it is
running.

But that said, I don't know are there some use case or API which requires
active
clocks after request so that should be checked.


> > Also context save/restore operations belongs more to PM callbacks
> > of .../plat-omap/mcbsp.c. Or probably that can be done also dynamically
> > inside the omap_mcbsp_start/-stop for keeping the whole McBSP shutdown
> > whenever it is idle.
> [Aggarwal, Anuj] I am sorry I couldn't understand that. Did you mean that
> PM hooks can be added in the mcbsp driver which can be registered and
> called
> As and when required? Any reference driver for this, if my understanding
> is correct?
> >
>

Yep, the PM hooks in .../plat-omap/mcbsp.c should take care of saving the
McBSP registers before going into suspend or restore them back when
resuming.
This would be second patch after the clock change patch.

The comment about dynamic PM was an idea that if McBSP could be unpowered
whenever it's not running and do the resume only when calling
omap_mcbsp_start.
But that would be some future improvements after basic suspend/resume
functionality.

> I think it's worth to look McBSP register cache concept [1] from Janusz
> > Krzysztofik would it help all of this context save/restore operations.
> [Aggarwal, Anuj] I tried these two patches on omap3 evm but they didn't
> work for me. The system doesn't go to the suspend state when I do:
> echo mem > /sys/power/state
> And the culprits were core (think sdma) and per domain. I tried
> disabling the mcbsp i/f clock as well but it didn't help. On mcbsp
> register dump, I couldn't find the sysconfig register getting
> configured. Could that be the root cause?
>

Does it go into suspend if the audio is not running (i.e. McBSP not
requested) or
if you don't even compile the McBSP support? I haven't looked are there
checks
for active clocks in PM core but PM is easier to start with bare minimum and
to
see that system can do suspend/resume and then add more features one by one.

-- 
Jarkko

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Anuj Aggarwal]

On Thu, Nov 19, 2009 at 1:04 PM, Jarkko Nikula <jhnikula@gmail.com> wrote:
> On Wed, Nov 18, 2009 at 8:28 PM, Aggarwal, Anuj <anuj.aggarwal@ti.com>wrote:
>
>> > If no API or feature would require active McBSP block before the
>> > omap_mcbsp_start call, then the clock management could be done inside
>> > the omap_mcbsp_start/-stop functions.
>> [Aggarwal, Anuj] I tried doing that but it didn't work because before
>> _start, we need to call set_hw_params which configures mcbsp, for which
>> clocks are required.
>> >
>>
>
> First modification to .../plat-omap/mcbsp.c could be just a patch which
> keeps the
> clocks on only when needed. I.e. keep them disabled when returning from
> request
> and activate them only when accessing the McBSP registers or when it is
> running.
>
> But that said, I don't know are there some use case or API which requires
> active
> clocks after request so that should be checked.
>
>
>> > Also context save/restore operations belongs more to PM callbacks
>> > of .../plat-omap/mcbsp.c. Or probably that can be done also dynamically
>> > inside the omap_mcbsp_start/-stop for keeping the whole McBSP shutdown
>> > whenever it is idle.
>> [Aggarwal, Anuj] I am sorry I couldn't understand that. Did you mean that
>> PM hooks can be added in the mcbsp driver which can be registered and
>> called
>> As and when required? Any reference driver for this, if my understanding
>> is correct?
>> >
>>
>
> Yep, the PM hooks in .../plat-omap/mcbsp.c should take care of saving the
> McBSP registers before going into suspend or restore them back when
> resuming.
> This would be second patch after the clock change patch.
>
> The comment about dynamic PM was an idea that if McBSP could be unpowered
> whenever it's not running and do the resume only when calling
> omap_mcbsp_start.
> But that would be some future improvements after basic suspend/resume
> functionality.
>
>> I think it's worth to look McBSP register cache concept [1] from Janusz
>> > Krzysztofik would it help all of this context save/restore operations.
>> [Aggarwal, Anuj] I tried these two patches on omap3 evm but they didn't
>> work for me. The system doesn't go to the suspend state when I do:
>> echo mem > /sys/power/state
>> And the culprits were core (think sdma) and per domain. I tried
>> disabling the mcbsp i/f clock as well but it didn't help. On mcbsp
>> register dump, I couldn't find the sysconfig register getting
>> configured. Could that be the root cause?
>>
>
> Does it go into suspend if the audio is not running (i.e. McBSP not
> requested) or
> if you don't even compile the McBSP support? I haven't looked are there
> checks
> for active clocks in PM core but PM is easier to start with bare minimum and
> to
> see that system can do suspend/resume and then add more features one by one.
[Anuj] Yes, w/o audio, it goes into suspend neatly.
After going through the entire patch series "[PATCHv5 00/20] OMAP
ASoC changes in DMA utilization", which I missed earlier, I found that the
sysfs entry dma_op_mode can be toggled among element/frame/threshold,
by default which is element hence not allowing the mcbsp driver to go into
idle state. I changed that at run time and after that I am able to hit
the suspend
state while playing back audio (at least the log doesn't
show that the system is not able to hit suspend).
My question is: is sysfs the only way to choose threshold or can it be made as
default? I could not see that happening in the code so thinking of the possible
reason of choosing not to do so. Because the user who wants to hit suspend
will not like to issue driver specific commands. Any specific reason of not
doing that by default?
>
> --
> Jarkko
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



-- 
Best Regards,
Anuj Aggarwal

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Peter Ujfalusi]

On Thursday 19 November 2009 21:48:26 ext Anuj Aggarwal wrote:
> [Anuj] Yes, w/o audio, it goes into suspend neatly.
> After going through the entire patch series "[PATCHv5 00/20] OMAP
> ASoC changes in DMA utilization", which I missed earlier, I found that the
> sysfs entry dma_op_mode can be toggled among element/frame/threshold,
> by default which is element hence not allowing the mcbsp driver to go into
> idle state. I changed that at run time and after that I am able to hit
> the suspend
> state while playing back audio (at least the log doesn't
> show that the system is not able to hit suspend).
> My question is: is sysfs the only way to choose threshold or can it be made
>  as default? I could not see that happening in the code so thinking of the
>  possible reason of choosing not to do so. Because the user who wants to
>  hit suspend will not like to issue driver specific commands. Any specific
>  reason of not doing that by default?

It is not selected as default on OMAP3, since it is a new feature, and we don't 
want to change the behavior of the audio. If the reports are positive regarding 
to the threshold mode, than we can make it as default on OMAP3, at least that is 
the plan AFAIK. 
In some cases the element mode is desirable over the threshold mode, and 
probably there are additional modes can be implemented for other cases, hence 
the sysfs interface is really useful feature.

Back than there were quite a bit of discussion here about the threshold mode: in 
the original series the threshold mode was the default, but we decided to not to 
do that before others have the chance to test it. We have been using the 
threshold mode without issues internally, so it should be really stable, but it 
is still a new mode, which can break other implementations.

So please test the threshold mode in your HW, and report if it is working there 
OK, if we have enough positive feedbacks, than we can make it as default.

-- 
Péter

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Jarkko Nikula]

On Fri, 20 Nov 2009 09:51:13 +0200
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> > My question is: is sysfs the only way to choose threshold or can it be made
> >  as default? I could not see that happening in the code so thinking of the
> >  possible reason of choosing not to do so. Because the user who wants to
> >  hit suspend will not like to issue driver specific commands. Any specific
> >  reason of not doing that by default?
> 
> It is not selected as default on OMAP3, since it is a new feature, and we don't 
> want to change the behavior of the audio. If the reports are positive regarding 
> to the threshold mode, than we can make it as default on OMAP3, at least that is 
> the plan AFAIK. 

Yep. Currently we want to keep DMA behaviour consistent across the
OMAPs and McBSP ports since the large internal FIFO is found only
from McBSP2 on OMAP3.

This is good finding that you have found that it's the audio preventing
the suspend and the threshold transfer mode can make it hit better.

But anyway, I'm afraid that threshold mode doesn't help to create
robust suspend. Threshold allow the DMA and core to be mode in idle
between the FIFO fills and probably that's why suspend might work
during these idle periods. For robust implementation there must be
explicit code stopping and resuming all activity gracefully so that it
can hit the suspend also if the fill is happening or if using another
transfer mode.


-- 
Jarkko

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Eero Nurkkala]

On Fri, 2009-11-20 at 14:53 +0100, ext Jarkko Nikula wrote:
> On Fri, 20 Nov 2009 09:51:13 +0200
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> 
> > > My question is: is sysfs the only way to choose threshold or can it be made
> > >  as default? I could not see that happening in the code so thinking of the
> > >  possible reason of choosing not to do so. Because the user who wants to
> > >  hit suspend will not like to issue driver specific commands. Any specific
> > >  reason of not doing that by default?
> > 
> > It is not selected as default on OMAP3, since it is a new feature, and we don't 
> > want to change the behavior of the audio. If the reports are positive regarding 
> > to the threshold mode, than we can make it as default on OMAP3, at least that is 
> > the plan AFAIK. 
> 
> Yep. Currently we want to keep DMA behaviour consistent across the
> OMAPs and McBSP ports since the large internal FIFO is found only
> from McBSP2 on OMAP3.
> 
> This is good finding that you have found that it's the audio preventing
> the suspend and the threshold transfer mode can make it hit better.
> 
> But anyway, I'm afraid that threshold mode doesn't help to create
> robust suspend. Threshold allow the DMA and core to be mode in idle
> between the FIFO fills and probably that's why suspend might work
> during these idle periods. For robust implementation there must be
> explicit code stopping and resuming all activity gracefully so that it
> can hit the suspend also if the fill is happening or if using another
> transfer mode.
> 
> 

Looking at the very original patch, I don't know how things could get
into deep sleep by disabling the fclk only... need to disable the iclk
also. In threshold mode, HW autogates the iclk, so you may be just
"lucky".

One may want to be aware of this also:

http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commitdiff;h=72cc6d715d5b018e2cff4adb68966855850d4e77

I think it's an undocumented feature.

- Eero

Re: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Anuj Aggarwal]

On Fri, Nov 20, 2009 at 7:39 PM, Eero Nurkkala
<ext-eero.nurkkala@nokia.com> wrote:
> On Fri, 2009-11-20 at 14:53 +0100, ext Jarkko Nikula wrote:
>> On Fri, 20 Nov 2009 09:51:13 +0200
>> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
>>
>> > > My question is: is sysfs the only way to choose threshold or can it be made
>> > >  as default? I could not see that happening in the code so thinking of the
>> > >  possible reason of choosing not to do so. Because the user who wants to
>> > >  hit suspend will not like to issue driver specific commands. Any specific
>> > >  reason of not doing that by default?
>> >
>> > It is not selected as default on OMAP3, since it is a new feature, and we don't
>> > want to change the behavior of the audio. If the reports are positive regarding
>> > to the threshold mode, than we can make it as default on OMAP3, at least that is
>> > the plan AFAIK.
>>
>> Yep. Currently we want to keep DMA behaviour consistent across the
>> OMAPs and McBSP ports since the large internal FIFO is found only
>> from McBSP2 on OMAP3.
>>
>> This is good finding that you have found that it's the audio preventing
>> the suspend and the threshold transfer mode can make it hit better.
>>
>> But anyway, I'm afraid that threshold mode doesn't help to create
>> robust suspend. Threshold allow the DMA and core to be mode in idle
>> between the FIFO fills and probably that's why suspend might work
>> during these idle periods. For robust implementation there must be
>> explicit code stopping and resuming all activity gracefully so that it
>> can hit the suspend also if the fill is happening or if using another
>> transfer mode.
>>
>>
>
> Looking at the very original patch, I don't know how things could get
> into deep sleep by disabling the fclk only... need to disable the iclk
> also. In threshold mode, HW autogates the iclk, so you may be just
> "lucky".
[Anuj] No, I am not :(. I had to modify the original patch to include
the disable part for the ICLK too. Without that, I was not able to hit
retention.
I will try to do some further regression on OMAP3 EVM and post my
findings. As of now, audio is working fine with these patches + ICLK
modification.
>
> One may want to be aware of this also:
>
> http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commitdiff;h=72cc6d715d5b018e2cff4adb68966855850d4e77
>
> I think it's an undocumented feature.
>
> - Eero
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Aggarwal, Anuj]

> > Looking at the very original patch, I don't know how things could get
> > into deep sleep by disabling the fclk only... need to disable the iclk
> > also. In threshold mode, HW autogates the iclk, so you may be just
> > "lucky".
> [Anuj] No, I am not :(. I had to modify the original patch to include
> the disable part for the ICLK too. Without that, I was not able to hit
> retention.
> I will try to do some further regression on OMAP3 EVM and post my
> findings. As of now, audio is working fine with these patches + ICLK
> modification.
[Aggarwal, Anuj] After further testing, I found that there is some
problem in the capture path. While capturing in background, if I tried 
to do suspend, capture fails after a few seconds giving;
arecord: pcm_read:1617: read error: Input/output error
This I was discussing in another thread (*) for AIC23/AM3517 on NFS but now
I realized after finishing my tests that this behavior is common for 
both omap3530/twl4030 and am3517/aic23. Moreover, the problem doesn't 
depend on the underlying file system - both NFS and ramdisk produce the
same error. 
To make matter worse, if suspend/resume is tried while audio
loopback is running, system just hangs. Even telnet to the evm doesn't work.
So the audio capture path, from suspend/resume point of view, definitely
needs some more time.
I will continue debugging at my end. But pointers are always welcome.

*) http://www.mail-archive.com/linux-omap@vger.kernel.org/msg19845.html
> >
> > One may want to be aware of this also:
> >
> > http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-
> 2.6.git;a=commitdiff;h=72cc6d715d5b018e2cff4adb68966855850d4e77
> >
> > I think it's an undocumented feature.
> >

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Aggarwal, Anuj]

> -----Original Message-----
> From: Aggarwal, Anuj
> Sent: Monday, November 30, 2009 5:21 PM
> To: ext-eero.nurkkala@nokia.com
> Cc: ext Jarkko Nikula; alsa-devel@alsa-project.org; Wang, Jane;
> broonie@opensource.wolfsonmicro.com; Ujfalusi Peter (Nokia-D/Tampere);
> linux-omap@vger.kernel.org
> Subject: RE: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure
> fix
> 
> > > Looking at the very original patch, I don't know how things could get
> > > into deep sleep by disabling the fclk only... need to disable the iclk
> > > also. In threshold mode, HW autogates the iclk, so you may be just
> > > "lucky".
> > [Anuj] No, I am not :(. I had to modify the original patch to include
> > the disable part for the ICLK too. Without that, I was not able to hit
> > retention.
> > I will try to do some further regression on OMAP3 EVM and post my
> > findings. As of now, audio is working fine with these patches + ICLK
> > modification.
> [Aggarwal, Anuj] After further testing, I found that there is some
> problem in the capture path. While capturing in background, if I tried
> to do suspend, capture fails after a few seconds giving;
> arecord: pcm_read:1617: read error: Input/output error
> This I was discussing in another thread (*) for AIC23/AM3517 on NFS but
> now
> I realized after finishing my tests that this behavior is common for
> both omap3530/twl4030 and am3517/aic23. Moreover, the problem doesn't
> depend on the underlying file system - both NFS and ramdisk produce the
> same error.
> To make matter worse, if suspend/resume is tried while audio
> loopback is running, system just hangs. Even telnet to the evm doesn't
> work.
> So the audio capture path, from suspend/resume point of view, definitely
> needs some more time.
> I will continue debugging at my end. But pointers are always welcome.
[Aggarwal, Anuj] I think I found one possible root cause of the hang which 
is observed if suspend/resume is tried while audio loopback is going on.

soc_suspend() calls snd_pcm_suspend_all() which calls snd_pcm_suspend() 
for all the substreams in the pcm->streams[]. On debugging, I found that 
I am not able to come out of snd_pcm_suspend_all(). I think this API needs 
some fix as the comment also suggests:
FIXME: the open/close code should lock this as well

Could this be the root cause of this hang? Any pointers on what needs to be
fixed?

Re: [PATCH 1/2] OMAP3: MCBSP: Suspend/resume failure fix

[Aggarwal, Anuj]

> > -----Original Message-----
> > From: Aggarwal, Anuj
> > Sent: Monday, November 30, 2009 5:21 PM
> > To: ext-eero.nurkkala@nokia.com
> > Cc: ext Jarkko Nikula; alsa-devel@alsa-project.org; Wang, Jane;
> > broonie@opensource.wolfsonmicro.com; Ujfalusi Peter (Nokia-D/Tampere);
> > linux-omap@vger.kernel.org
> > Subject: RE: [alsa-devel] [PATCH 1/2] OMAP3: MCBSP: Suspend/resume
> failure
> > fix
> >
> > > > Looking at the very original patch, I don't know how things could
> get
> > > > into deep sleep by disabling the fclk only... need to disable the
> iclk
> > > > also. In threshold mode, HW autogates the iclk, so you may be just
> > > > "lucky".
> > > [Anuj] No, I am not :(. I had to modify the original patch to include
> > > the disable part for the ICLK too. Without that, I was not able to hit
> > > retention.
> > > I will try to do some further regression on OMAP3 EVM and post my
> > > findings. As of now, audio is working fine with these patches + ICLK
> > > modification.
> > [Aggarwal, Anuj] After further testing, I found that there is some
> > problem in the capture path. While capturing in background, if I tried
> > to do suspend, capture fails after a few seconds giving;
> > arecord: pcm_read:1617: read error: Input/output error
> > This I was discussing in another thread (*) for AIC23/AM3517 on NFS but
> > now
> > I realized after finishing my tests that this behavior is common for
> > both omap3530/twl4030 and am3517/aic23. Moreover, the problem doesn't
> > depend on the underlying file system - both NFS and ramdisk produce the
> > same error.
> > To make matter worse, if suspend/resume is tried while audio
> > loopback is running, system just hangs. Even telnet to the evm doesn't
> > work.
> > So the audio capture path, from suspend/resume point of view, definitely
> > needs some more time.
> > I will continue debugging at my end. But pointers are always welcome.
> [Aggarwal, Anuj] I think I found one possible root cause of the hang which
> is observed if suspend/resume is tried while audio loopback is going on.
> 
> soc_suspend() calls snd_pcm_suspend_all() which calls snd_pcm_suspend()
> for all the substreams in the pcm->streams[]. On debugging, I found that
> I am not able to come out of snd_pcm_suspend_all(). I think this API needs
> some fix as the comment also suggests:
> FIXME: the open/close code should lock this as well
> 
> Could this be the root cause of this hang? Any pointers on what needs to
> be
> fixed?
> 
[Aggarwal, Anuj] Can someone please confirm that the problem lies in this 
API? Also, any help on how that can be fixed would be great.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html