Re: [PATCH] OMAP3: disable idle early in the suspend sequence
On Wed, Dec 8, 2010 at 2:11 AM, Kevin Hilman wrote: > Hi Jean, > > Jean Pihet writes: > >> Some bad interaction between the idle and the suspend paths has been >> noticed: the idle code is called during the suspend enter and exit >> sequences. This could cause corruption or lock-up of resources. >> >> The solution is to move the call to disable_hlt at the very beginning >> of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), >> and the call to enable_hlt at the very end of the suspend sequence >> (in omap3_pm_end instead of omap3_pm_finish). >> >> Tested with RET and OFF on Beagle and OMAP3EVM. >> >> Signed-off-by: Jean Pihet >> Cc: Kevin Hilman >> --- >> arch/arm/mach-omap2/pm34xx.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) > > Can you update this to do similar for OMAP2 and OMAP4? Done! It needs some testing on OMAP2 and OMAP4 boards since it has been compile tested only. > > Thanks, > > Kevin > Thanks, Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
Hi Jean, Jean Pihet writes: > Some bad interaction between the idle and the suspend paths has been > noticed: the idle code is called during the suspend enter and exit > sequences. This could cause corruption or lock-up of resources. > > The solution is to move the call to disable_hlt at the very beginning > of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), > and the call to enable_hlt at the very end of the suspend sequence > (in omap3_pm_end instead of omap3_pm_finish). > > Tested with RET and OFF on Beagle and OMAP3EVM. > > Signed-off-by: Jean Pihet > Cc: Kevin Hilman > --- > arch/arm/mach-omap2/pm34xx.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) Can you update this to do similar for OMAP2 and OMAP4? Thanks, Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
On Mon, Nov 22, 2010 at 6:51 PM, Kevin Hilman
wrote:
> Jean Pihet writes:
>
>> Some bad interaction between the idle and the suspend paths has been
>> noticed: the idle code is called during the suspend enter and exit
>> sequences. This could cause corruption or lock-up of resources.
>
>> The solution is to move the call to disable_hlt at the very beginning
>> of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare),
>> and the call to enable_hlt at the very end of the suspend sequence
>> (in omap3_pm_end instead of omap3_pm_finish).
>>
>> Tested with RET and OFF on Beagle and OMAP3EVM.
>
> I think the description could have a little more detail. Something like:
>
> Idle path should be disabled during the entire suspend/resume sequence.
> Currently it is disabled in ->prepare() and re-enabled in ->finish(),
> but the suspend sequence starts with ->begin() and ends with ->end(),
> leaving windows where the suspend/resume sequence is still underway and
> idle code could execute.
>
> To fix, move idle disable and enable into ->begin() and ->end()
> respectively to ensure idle path is disabled for the entire
> suspend/resume sequence.
>
Ok thx for the suggestion.
>>
>> @@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
>>
>> static void omap3_pm_finish(void)
>> {
>> - enable_hlt();
>> }
>
> Might as well remove these empty functions now.
>
>> /* Hooks to enable / disable UART interrupts during suspend */
>> static int omap3_pm_begin(suspend_state_t state)
>> {
>> + disable_hlt();
>> suspend_state = state;
>> omap_uart_enable_irqs(0);
>> return 0;
>> @@ -591,6 +590,7 @@ static void omap3_pm_end(void)
>> {
>> suspend_state = PM_SUSPEND_ON;
>> omap_uart_enable_irqs(1);
>> + enable_hlt();
>> return;
>> }
>
> Kevin
>
Ok resent!
Thx,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
Jean Pihet writes:
> Some bad interaction between the idle and the suspend paths has been
> noticed: the idle code is called during the suspend enter and exit
> sequences. This could cause corruption or lock-up of resources.
> The solution is to move the call to disable_hlt at the very beginning
> of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare),
> and the call to enable_hlt at the very end of the suspend sequence
> (in omap3_pm_end instead of omap3_pm_finish).
>
> Tested with RET and OFF on Beagle and OMAP3EVM.
I think the description could have a little more detail. Something like:
Idle path should be disabled during the entire suspend/resume sequence.
Currently it is disabled in ->prepare() and re-enabled in ->finish(),
but the suspend sequence starts with ->begin() and ends with ->end(),
leaving windows where the suspend/resume sequence is still underway and
idle code could execute.
To fix, move idle disable and enable into ->begin() and ->end()
respectively to ensure idle path is disabled for the entire
suspend/resume sequence.
>
> Signed-off-by: Jean Pihet
> Cc: Kevin Hilman
> ---
> arch/arm/mach-omap2/pm34xx.c |4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 75c0cd1..022fdff 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -508,7 +508,6 @@ static suspend_state_t suspend_state;
>
> static int omap3_pm_prepare(void)
> {
> - disable_hlt();
> return 0;
> }
>
> @@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
>
> static void omap3_pm_finish(void)
> {
> - enable_hlt();
> }
Might as well remove these empty functions now.
> /* Hooks to enable / disable UART interrupts during suspend */
> static int omap3_pm_begin(suspend_state_t state)
> {
> + disable_hlt();
> suspend_state = state;
> omap_uart_enable_irqs(0);
> return 0;
> @@ -591,6 +590,7 @@ static void omap3_pm_end(void)
> {
> suspend_state = PM_SUSPEND_ON;
> omap_uart_enable_irqs(1);
> + enable_hlt();
> return;
> }
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] OMAP3: disable idle early in the suspend sequence
> -Original Message- > From: Jean Pihet [mailto:jean.pi...@newoldbits.com] > Sent: Monday, November 22, 2010 5:01 PM > To: Rajendra Nayak > Cc: linux-omap@vger.kernel.org; Jean Pihet-XID; Kevin Hilman > Subject: Re: [PATCH] OMAP3: disable idle early in the suspend sequence > > On Mon, Nov 22, 2010 at 11:53 AM, Rajendra Nayak wrote: > >> -Original Message- > >> From: linux-omap-ow...@vger.kernel.org > > [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Jean Pihet > >> Sent: Monday, November 22, 2010 4:15 PM > >> To: linux-omap@vger.kernel.org > >> Cc: Jean Pihet; Kevin Hilman > >> Subject: [PATCH] OMAP3: disable idle early in the suspend sequence > >> > >> Some bad interaction between the idle and the suspend paths has been > >> noticed: the idle code is called during the suspend enter and exit > >> sequences. This could cause corruption or lock-up of resources. > > > > Can you elaborate more on what kind of issues were seen? > > Trying to get the PRCM registers dump after a suspend/resume does not > show the correct registers values, cf. Kevin's patch at > http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap- > pm.git;a=commitdiff;h=9fc4891d4a21d2b644a463d62c77ef97da55f091. > > Digging a bit further I found out that the idle routine is called >50 > times while the suspend/resume sequence still is on-going. The root > cause is because disable_hlt is called from omap3_pm_prepare which > runs after omap3_pm_begin (and the same issue in the resume sequence). > This leaves a time window for idle to kick-in while the suspend > sequence is busy saving/restoring the system state. This is a > potential bug that just waits to show up soon or later, especially if > more code is added in the suspend prepare and finish functions. Ok. Thanks. Basically we had some similar issues on OMAP4. I am yet to root cause, but the initial hunch was there was a race between idle and suspend and I did pretty much the same change in pm44xx.c. That fixed the issue but only partially, so am still in the process of debugging it further to see what exactly is the issue. So was just curious to know what issues you ran into on OMAP3. Will update once I know whats happening on OMAP4. > > Does the changelog need an update? No, The changelog looks fine to me. Thanks, Rajendra > > Regards, > Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
Hi, On Mon, Nov 22, 2010 at 12:38:21PM +0100, Jean Pihet wrote: The original patch has the ti.com address in both the From: and Signed-off-by: fields, but the From: on the ML still shows otherwise. Weird... Any idea how to fix that? how about setting sendemail.envelopesender ?? -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
Hi, On Mon, Nov 22, 2010 at 12:11 PM, Felipe Balbi wrote: > Hi, > > On Mon, Nov 22, 2010 at 11:44:57AM +0100, Jean Pihet wrote: >> >> Some bad interaction between the idle and the suspend paths has been >> noticed: the idle code is called during the suspend enter and exit >> sequences. This could cause corruption or lock-up of resources. >> >> The solution is to move the call to disable_hlt at the very beginning >> of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), >> and the call to enable_hlt at the very end of the suspend sequence >> (in omap3_pm_end instead of omap3_pm_finish). >> >> Tested with RET and OFF on Beagle and OMAP3EVM. >> >> Signed-off-by: Jean Pihet >> Cc: Kevin Hilman > > This patch will look weird on git log, it's going to be From: Jean Pihet > and Signed-off-by: Jean Pihet > . Maybe you should change user.email variable on your > .gitconfig and use envelope sender as your newoldbits.com address ? That is exactly what I am trying to achieve but it does not work ;-( git config -l shows: user.name=Jean Pihet user.email=j-pi...@ti.com sendemail.smtpuser=jean.pi...@newoldbits.com The original patch has the ti.com address in both the From: and Signed-off-by: fields, but the From: on the ML still shows otherwise. Weird... Any idea how to fix that? Thanks! Jean > > -- > balbi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
On Mon, Nov 22, 2010 at 11:53 AM, Rajendra Nayak wrote: >> -Original Message- >> From: linux-omap-ow...@vger.kernel.org > [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Jean Pihet >> Sent: Monday, November 22, 2010 4:15 PM >> To: linux-omap@vger.kernel.org >> Cc: Jean Pihet; Kevin Hilman >> Subject: [PATCH] OMAP3: disable idle early in the suspend sequence >> >> Some bad interaction between the idle and the suspend paths has been >> noticed: the idle code is called during the suspend enter and exit >> sequences. This could cause corruption or lock-up of resources. > > Can you elaborate more on what kind of issues were seen? Trying to get the PRCM registers dump after a suspend/resume does not show the correct registers values, cf. Kevin's patch at http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=9fc4891d4a21d2b644a463d62c77ef97da55f091. Digging a bit further I found out that the idle routine is called >50 times while the suspend/resume sequence still is on-going. The root cause is because disable_hlt is called from omap3_pm_prepare which runs after omap3_pm_begin (and the same issue in the resume sequence). This leaves a time window for idle to kick-in while the suspend sequence is busy saving/restoring the system state. This is a potential bug that just waits to show up soon or later, especially if more code is added in the suspend prepare and finish functions. Does the changelog need an update? Regards, Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP3: disable idle early in the suspend sequence
Hi, On Mon, Nov 22, 2010 at 11:44:57AM +0100, Jean Pihet wrote: Some bad interaction between the idle and the suspend paths has been noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources. The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish). Tested with RET and OFF on Beagle and OMAP3EVM. Signed-off-by: Jean Pihet Cc: Kevin Hilman This patch will look weird on git log, it's going to be From: Jean Pihet and Signed-off-by: Jean Pihet . Maybe you should change user.email variable on your .gitconfig and use envelope sender as your newoldbits.com address ? -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] OMAP3: disable idle early in the suspend sequence
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Jean Pihet
> Sent: Monday, November 22, 2010 4:15 PM
> To: linux-omap@vger.kernel.org
> Cc: Jean Pihet; Kevin Hilman
> Subject: [PATCH] OMAP3: disable idle early in the suspend sequence
>
> Some bad interaction between the idle and the suspend paths has been
> noticed: the idle code is called during the suspend enter and exit
> sequences. This could cause corruption or lock-up of resources.
Can you elaborate more on what kind of issues were seen?
>
> The solution is to move the call to disable_hlt at the very beginning
> of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare),
> and the call to enable_hlt at the very end of the suspend sequence
> (in omap3_pm_end instead of omap3_pm_finish).
>
> Tested with RET and OFF on Beagle and OMAP3EVM.
>
> Signed-off-by: Jean Pihet
> Cc: Kevin Hilman
> ---
> arch/arm/mach-omap2/pm34xx.c |4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 75c0cd1..022fdff 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -508,7 +508,6 @@ static suspend_state_t suspend_state;
>
> static int omap3_pm_prepare(void)
> {
> - disable_hlt();
> return 0;
> }
>
> @@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
>
> static void omap3_pm_finish(void)
> {
> - enable_hlt();
> }
>
> /* Hooks to enable / disable UART interrupts during suspend */
> static int omap3_pm_begin(suspend_state_t state)
> {
> + disable_hlt();
> suspend_state = state;
> omap_uart_enable_irqs(0);
> return 0;
> @@ -591,6 +590,7 @@ static void omap3_pm_end(void)
> {
> suspend_state = PM_SUSPEND_ON;
> omap_uart_enable_irqs(1);
> + enable_hlt();
> return;
> }
>
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
