[PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Tony Lindgren]

With CONFIG_DEBUG_RODATA enabled I started noticing imprecise external
aborts on a dm3730 when hitting off idle. These don't seem to happen
on 34xx.

Pretty much changing anything in the code made these go away, like
changing .config options. At first I though it might be an alignment
issue in the 36xx specific assembly code in sleep34xx.S, or something
related to the recent rodata fixes. But that does not seem to be
the case. It seems to be a timing issue instead.

Adding few extra nop instructions after the wfi seems to fix the
issue. When adding 5 nops, the errors showed up less often. With
add ed 6 nops, I don't seem to get them at all any longer.

Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Anybody else seen this issue before?

---
 arch/arm/mach-omap2/sleep34xx.S | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 1b9f052..0fbaa08 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -264,6 +264,12 @@ ENTRY(omap3_do_wfi)
 	nop
 	nop
 	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
 
 /*
  * This function implements the erratum ID i581 WA:
-- 
2.7.0

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Dave Gerlach]

Hi,
On 02/10/2016 03:35 PM, Tony Lindgren wrote:
> With CONFIG_DEBUG_RODATA enabled I started noticing imprecise external
> aborts on a dm3730 when hitting off idle. These don't seem to happen
> on 34xx.
>
> Pretty much changing anything in the code made these go away, like
> changing .config options. At first I though it might be an alignment
> issue in the 36xx specific assembly code in sleep34xx.S, or something
> related to the recent rodata fixes. But that does not seem to be
> the case. It seems to be a timing issue instead.
>
> Adding few extra nop instructions after the wfi seems to fix the
> issue. When adding 5 nops, the errors showed up less often. With
> add ed 6 nops, I don't seem to get them at all any longer.
>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Richard Woodruff <r-woodruff2@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>
> Anybody else seen this issue before?

I have a series to convert omap3 PM code to using generic SRAM driver 
but when testing I see an external abort on BBXM off mode resume very 
similar to this that seems to be timing related caused by using generic 
SRAM driver to re-copy PM code rather than omap3_sram_restore_context.

By tracing the resume path I believe the abort is caused by 
omap3_intc_resume_idle in pm34xx.c, which writes to two registers in the 
INTC. Removing this call makes the abort unreproducible in my 
experiments and changing the writes to reads causes a bus lock, so I'm 
pretty confident it's coming from this call attempting to write to an 
idled INTC.

Advisory 1.106 in DM3730 Silicon Errata Document [1] describes an issue 
with "MPU Leaves MSTANDBY State Before IDLEREQ of Interrupt Controller 
is Released" which sounds like a very similar failure situation to what 
we are seeing even though the timing of INTC access is a bit further 
from WFI exit than it describes. Perhaps there are more conditions where 
it can occur. Pushed my WIP branch for SRAM series that shows the same 
failure here [2].

Regards,
Dave

[1] http://www.ti.com/lit/er/sprz319f/sprz319f.pdf
[2] https://github.com/dgerlach/linux-pm/tree/beagle-sram-wip-v4.5

>
> ---
>   arch/arm/mach-omap2/sleep34xx.S | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index 1b9f052..0fbaa08 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -264,6 +264,12 @@ ENTRY(omap3_do_wfi)
>   	nop
>   	nop
>   	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
>
>   /*
>    * This function implements the erratum ID i581 WA:
>

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Tony Lindgren]

* Dave Gerlach <d-gerlach@ti.com> [160407 13:18]:
> 
> I have a series to convert omap3 PM code to using generic SRAM driver but
> when testing I see an external abort on BBXM off mode resume very similar to
> this that seems to be timing related caused by using generic SRAM driver to
> re-copy PM code rather than omap3_sram_restore_context.
> 
> By tracing the resume path I believe the abort is caused by
> omap3_intc_resume_idle in pm34xx.c, which writes to two registers in the
> INTC. Removing this call makes the abort unreproducible in my experiments
> and changing the writes to reads causes a bus lock, so I'm pretty confident
> it's coming from this call attempting to write to an idled INTC.
> 
> Advisory 1.106 in DM3730 Silicon Errata Document [1] describes an issue with
> "MPU Leaves MSTANDBY State Before IDLEREQ of Interrupt Controller is
> Released" which sounds like a very similar failure situation to what we are
> seeing even though the timing of INTC access is a bit further from WFI exit
> than it describes. Perhaps there are more conditions where it can occur.
> Pushed my WIP branch for SRAM series that shows the same failure here [2].

Interesting, I think you may have something with the errata 1.106.. And
looks like we also need still errata 1.62 handled also on 36xx in the
same pdf. And need to disable intc autoidle early at least for HS omaps
as save_secure_ram context supposedly also can do WFI.

Maybe something like the following would make sense? It seems to work
here without external aborts with your test branch on dm3730, and boots
fine on omap3430 hs (n900).

Or do you have some better ideas for a fix?

Regards,

Tony

8< ------------------------
From: Tony Lindgren <tony@atomide.com>
Date: Thu, 7 Apr 2016 16:06:35 -0700
Subject: [PATCH] ARM: OMAP3: Fix external abort on 36xx waking from off mode
 idle

Depending on the timings affected by .config options, we may get
external aborts with 36xx. These seem to be caused by the following:

- omap3 errata 1.62 "MPU Cannot Exit from Standby" says we need to
  disable disable intc autoidle before WFI

- dm3730 errata 1.106 "MPU Leaves MSTANDBY State Before IDLEREQ of
  Interrupt Controller is Released" says we need to wait before
  accessing intc

Looks like we're currently disabling intc autoidle after we save
the intc context. And then we attempt to re-enable intc autoidle
after we restore intc context.

This means we're trying to restore intc autoidle twice, and it
occasionally fails as the intc restore may take a while for
things to get configured?

So we should either completely leave out omap3_intc_resume_idle()
assuming that the context restore really also enables the intc
autoidle.

Or we can disable intc autoidle before saving context, and then
reenable intc autoidle only after the context restore. This pairs
the calls, can we can call omap3_intc_resume_idle() later to
allow some time for intc to settle down.

Note that supposedly save_secure_ram() also can cause a WFI
in the secure ram code, so we need to disable intc autoidle
early. There's some information about that in the old thread
"[PATCH 09/13] OMAP3: PM: Apply errata i540 before save
secure ram".

--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -303,6 +303,8 @@ void omap_sram_idle(void)
 		omap2_gpio_prepare_for_idle(per_going_off);
 	}
 
+	omap3_intc_prepare_idle();
+
 	/* CORE */
 	if (core_next_state < PWRDM_POWER_ON) {
 		if (core_next_state == PWRDM_POWER_OFF) {
@@ -314,8 +316,6 @@ void omap_sram_idle(void)
 	/* Configure PMIC signaling for I2C4 or sys_off_mode */
 	omap3_vc_set_pmic_signaling(core_next_state);
 
-	omap3_intc_prepare_idle();
-
 	/*
 	 * On EMU/HS devices ROM code restores a SRDC value
 	 * from scratchpad which has automatic self refresh on timeout
@@ -358,13 +358,14 @@ void omap_sram_idle(void)
 			omap2_sms_restore_context();
 		}
 	}
-	omap3_intc_resume_idle();
 
 	pwrdm_post_transition(NULL);
 
 	/* PER */
 	if (per_next_state < PWRDM_POWER_ON)
 		omap2_gpio_resume_after_idle();
+
+	omap3_intc_resume_idle();
 }
 
 static void omap3_pm_idle(void)

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Dave Gerlach]

Tony,
On 04/07/2016 06:16 PM, Tony Lindgren wrote:
> * Dave Gerlach <d-gerlach@ti.com> [160407 13:18]:
>>
>> I have a series to convert omap3 PM code to using generic SRAM driver but
>> when testing I see an external abort on BBXM off mode resume very similar to
>> this that seems to be timing related caused by using generic SRAM driver to
>> re-copy PM code rather than omap3_sram_restore_context.
>>
>> By tracing the resume path I believe the abort is caused by
>> omap3_intc_resume_idle in pm34xx.c, which writes to two registers in the
>> INTC. Removing this call makes the abort unreproducible in my experiments
>> and changing the writes to reads causes a bus lock, so I'm pretty confident
>> it's coming from this call attempting to write to an idled INTC.
>>
>> Advisory 1.106 in DM3730 Silicon Errata Document [1] describes an issue with
>> "MPU Leaves MSTANDBY State Before IDLEREQ of Interrupt Controller is
>> Released" which sounds like a very similar failure situation to what we are
>> seeing even though the timing of INTC access is a bit further from WFI exit
>> than it describes. Perhaps there are more conditions where it can occur.
>> Pushed my WIP branch for SRAM series that shows the same failure here [2].
>
> Interesting, I think you may have something with the errata 1.106.. And
> looks like we also need still errata 1.62 handled also on 36xx in the
> same pdf. And need to disable intc autoidle early at least for HS omaps
> as save_secure_ram context supposedly also can do WFI.
>
> Maybe something like the following would make sense? It seems to work
> here without external aborts with your test branch on dm3730, and boots
> fine on omap3430 hs (n900).
>
> Or do you have some better ideas for a fix?

I can also confirm that this fixes the external abort, I can not cause 
it with your attached patch.

I would be ok with the solution you have proposed and I was unable to 
come up with anything better while trying to debug the issue originally.

We still need the call to omap3_intc_resume_idle because the intc 
restore context only gets called on resume from off mode. Perhaps we 
only need to call omap3_intc_resume_idle when coming back from non-off 
modes, otherwise let the context restore handle the reconfig of the INTC 
idle/sysconfig registers?

Regards,
Dave


>
> Regards,
>
> Tony
>
> 8< ------------------------
> From: Tony Lindgren <tony@atomide.com>
> Date: Thu, 7 Apr 2016 16:06:35 -0700
> Subject: [PATCH] ARM: OMAP3: Fix external abort on 36xx waking from off mode
>   idle
>
> Depending on the timings affected by .config options, we may get
> external aborts with 36xx. These seem to be caused by the following:
>
> - omap3 errata 1.62 "MPU Cannot Exit from Standby" says we need to
>    disable disable intc autoidle before WFI
>
> - dm3730 errata 1.106 "MPU Leaves MSTANDBY State Before IDLEREQ of
>    Interrupt Controller is Released" says we need to wait before
>    accessing intc
>
> Looks like we're currently disabling intc autoidle after we save
> the intc context. And then we attempt to re-enable intc autoidle
> after we restore intc context.
>
> This means we're trying to restore intc autoidle twice, and it
> occasionally fails as the intc restore may take a while for
> things to get configured?
>
> So we should either completely leave out omap3_intc_resume_idle()
> assuming that the context restore really also enables the intc
> autoidle.
>
> Or we can disable intc autoidle before saving context, and then
> reenable intc autoidle only after the context restore. This pairs
> the calls, can we can call omap3_intc_resume_idle() later to
> allow some time for intc to settle down.
>
> Note that supposedly save_secure_ram() also can cause a WFI
> in the secure ram code, so we need to disable intc autoidle
> early. There's some information about that in the old thread
> "[PATCH 09/13] OMAP3: PM: Apply errata i540 before save
> secure ram".
>
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -303,6 +303,8 @@ void omap_sram_idle(void)
>   		omap2_gpio_prepare_for_idle(per_going_off);
>   	}
>
> +	omap3_intc_prepare_idle();
> +
>   	/* CORE */
>   	if (core_next_state < PWRDM_POWER_ON) {
>   		if (core_next_state == PWRDM_POWER_OFF) {
> @@ -314,8 +316,6 @@ void omap_sram_idle(void)
>   	/* Configure PMIC signaling for I2C4 or sys_off_mode */
>   	omap3_vc_set_pmic_signaling(core_next_state);
>
> -	omap3_intc_prepare_idle();
> -
>   	/*
>   	 * On EMU/HS devices ROM code restores a SRDC value
>   	 * from scratchpad which has automatic self refresh on timeout
> @@ -358,13 +358,14 @@ void omap_sram_idle(void)
>   			omap2_sms_restore_context();
>   		}
>   	}
> -	omap3_intc_resume_idle();
>
>   	pwrdm_post_transition(NULL);
>
>   	/* PER */
>   	if (per_next_state < PWRDM_POWER_ON)
>   		omap2_gpio_resume_after_idle();
> +
> +	omap3_intc_resume_idle();
>   }
>
>   static void omap3_pm_idle(void)
>

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Tony Lindgren]

* Dave Gerlach <d-gerlach@ti.com> [160411 11:21]:
> Tony,
> On 04/07/2016 06:16 PM, Tony Lindgren wrote:
> >* Dave Gerlach <d-gerlach@ti.com> [160407 13:18]:
> >>
> >>I have a series to convert omap3 PM code to using generic SRAM driver but
> >>when testing I see an external abort on BBXM off mode resume very similar to
> >>this that seems to be timing related caused by using generic SRAM driver to
> >>re-copy PM code rather than omap3_sram_restore_context.
> >>
> >>By tracing the resume path I believe the abort is caused by
> >>omap3_intc_resume_idle in pm34xx.c, which writes to two registers in the
> >>INTC. Removing this call makes the abort unreproducible in my experiments
> >>and changing the writes to reads causes a bus lock, so I'm pretty confident
> >>it's coming from this call attempting to write to an idled INTC.
> >>
> >>Advisory 1.106 in DM3730 Silicon Errata Document [1] describes an issue with
> >>"MPU Leaves MSTANDBY State Before IDLEREQ of Interrupt Controller is
> >>Released" which sounds like a very similar failure situation to what we are
> >>seeing even though the timing of INTC access is a bit further from WFI exit
> >>than it describes. Perhaps there are more conditions where it can occur.
> >>Pushed my WIP branch for SRAM series that shows the same failure here [2].
> >
> >Interesting, I think you may have something with the errata 1.106.. And
> >looks like we also need still errata 1.62 handled also on 36xx in the
> >same pdf. And need to disable intc autoidle early at least for HS omaps
> >as save_secure_ram context supposedly also can do WFI.
> >
> >Maybe something like the following would make sense? It seems to work
> >here without external aborts with your test branch on dm3730, and boots
> >fine on omap3430 hs (n900).
> >
> >Or do you have some better ideas for a fix?
> 
> I can also confirm that this fixes the external abort, I can not cause it
> with your attached patch.

OK. I still can't quite see why exactly this patch fixes things. So
I'm afraid it might be just hiding the problem..

> I would be ok with the solution you have proposed and I was unable to come
> up with anything better while trying to debug the issue originally.
> 
> We still need the call to omap3_intc_resume_idle because the intc restore
> context only gets called on resume from off mode. Perhaps we only need to
> call omap3_intc_resume_idle when coming back from non-off modes, otherwise
> let the context restore handle the reconfig of the INTC idle/sysconfig
> registers?

OK. Did you actually test by commenting out omap3_intc_resume_idle()?

Yeah sounds like we can optimize out the restore there for non-off
modes.

Tony

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Dave Gerlach]

On 04/11/2016 04:13 PM, Tony Lindgren wrote:
> * Dave Gerlach <d-gerlach@ti.com> [160411 11:21]:
>> Tony,
>> On 04/07/2016 06:16 PM, Tony Lindgren wrote:
>>> * Dave Gerlach <d-gerlach@ti.com> [160407 13:18]:
>>>>
>>>> I have a series to convert omap3 PM code to using generic SRAM driver but
>>>> when testing I see an external abort on BBXM off mode resume very similar to
>>>> this that seems to be timing related caused by using generic SRAM driver to
>>>> re-copy PM code rather than omap3_sram_restore_context.
>>>>
>>>> By tracing the resume path I believe the abort is caused by
>>>> omap3_intc_resume_idle in pm34xx.c, which writes to two registers in the
>>>> INTC. Removing this call makes the abort unreproducible in my experiments
>>>> and changing the writes to reads causes a bus lock, so I'm pretty confident
>>>> it's coming from this call attempting to write to an idled INTC.
>>>>
>>>> Advisory 1.106 in DM3730 Silicon Errata Document [1] describes an issue with
>>>> "MPU Leaves MSTANDBY State Before IDLEREQ of Interrupt Controller is
>>>> Released" which sounds like a very similar failure situation to what we are
>>>> seeing even though the timing of INTC access is a bit further from WFI exit
>>>> than it describes. Perhaps there are more conditions where it can occur.
>>>> Pushed my WIP branch for SRAM series that shows the same failure here [2].
>>>
>>> Interesting, I think you may have something with the errata 1.106.. And
>>> looks like we also need still errata 1.62 handled also on 36xx in the
>>> same pdf. And need to disable intc autoidle early at least for HS omaps
>>> as save_secure_ram context supposedly also can do WFI.
>>>
>>> Maybe something like the following would make sense? It seems to work
>>> here without external aborts with your test branch on dm3730, and boots
>>> fine on omap3430 hs (n900).
>>>
>>> Or do you have some better ideas for a fix?
>>
>> I can also confirm that this fixes the external abort, I can not cause it
>> with your attached patch.
> 
> OK. I still can't quite see why exactly this patch fixes things. So
> I'm afraid it might be just hiding the problem..

I agree, moving the omap3_intc_resume_idle may just be masking the issue for
that exact situation, but I think we can get rid of it for off mode...

> 
>> I would be ok with the solution you have proposed and I was unable to come
>> up with anything better while trying to debug the issue originally.
>>
>> We still need the call to omap3_intc_resume_idle because the intc restore
>> context only gets called on resume from off mode. Perhaps we only need to
>> call omap3_intc_resume_idle when coming back from non-off modes, otherwise
>> let the context restore handle the reconfig of the INTC idle/sysconfig
>> registers?
> 
> OK. Did you actually test by commenting out omap3_intc_resume_idle()?
> 
> Yeah sounds like we can optimize out the restore there for non-off
> modes.

Yes I removed it entirely for testing, and I also tried something like this for
a possible workable solution (without your patch applied):

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index fcf975eb5e9d..8d39b44ba3a3 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -268,7 +268,6 @@ void omap_sram_idle(void)
        int per_next_state = PWRDM_POWER_ON;
        int core_next_state = PWRDM_POWER_ON;
        int per_going_off;
-       int core_prev_state;
        u32 sdrc_pwr = 0;

        mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
@@ -348,17 +347,16 @@ void omap_sram_idle(void)
                sdrc_write_reg(sdrc_pwr, SDRC_POWER);

        /* CORE */
-       if (core_next_state < PWRDM_POWER_ON) {
-               core_prev_state = pwrdm_read_prev_pwrst(core_pwrdm);
-               if (core_prev_state == PWRDM_POWER_OFF) {
+       if (core_next_state < PWRDM_POWER_ON &&
+           pwrdm_read_prev_pwrst(core_pwrdm) == PWRDM_POWER_OFF) {
                        omap3_core_restore_context();
                        omap3_cm_restore_context();
                        omap3_push_sram_idle();
                        omap3_push_sram_secure_idle();
                        omap2_sms_restore_context();
-               }
-       }
-       omap3_intc_resume_idle();
+       } else
+               omap3_intc_resume_idle();
+

        pwrdm_post_transition(NULL);


Regards,
Dave


> 
> Tony
> 

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Tony Lindgren]

* Dave Gerlach <d-gerlach@ti.com> [160411 16:17]:
> On 04/11/2016 04:13 PM, Tony Lindgren wrote:
> >> We still need the call to omap3_intc_resume_idle because the intc restore
> >> context only gets called on resume from off mode. Perhaps we only need to
> >> call omap3_intc_resume_idle when coming back from non-off modes, otherwise
> >> let the context restore handle the reconfig of the INTC idle/sysconfig
> >> registers?
> > 
> > OK. Did you actually test by commenting out omap3_intc_resume_idle()?
> > 
> > Yeah sounds like we can optimize out the restore there for non-off
> > modes.
> 
> Yes I removed it entirely for testing, and I also tried something like this for
> a possible workable solution (without your patch applied):
> 
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index fcf975eb5e9d..8d39b44ba3a3 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -268,7 +268,6 @@ void omap_sram_idle(void)
>         int per_next_state = PWRDM_POWER_ON;
>         int core_next_state = PWRDM_POWER_ON;
>         int per_going_off;
> -       int core_prev_state;
>         u32 sdrc_pwr = 0;
> 
>         mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
> @@ -348,17 +347,16 @@ void omap_sram_idle(void)
>                 sdrc_write_reg(sdrc_pwr, SDRC_POWER);
> 
>         /* CORE */
> -       if (core_next_state < PWRDM_POWER_ON) {
> -               core_prev_state = pwrdm_read_prev_pwrst(core_pwrdm);
> -               if (core_prev_state == PWRDM_POWER_OFF) {
> +       if (core_next_state < PWRDM_POWER_ON &&
> +           pwrdm_read_prev_pwrst(core_pwrdm) == PWRDM_POWER_OFF) {
>                         omap3_core_restore_context();
>                         omap3_cm_restore_context();
>                         omap3_push_sram_idle();
>                         omap3_push_sram_secure_idle();
>                         omap2_sms_restore_context();
> -               }
> -       }
> -       omap3_intc_resume_idle();
> +       } else
> +               omap3_intc_resume_idle();
> +
> 
>         pwrdm_post_transition(NULL);
> 
> 

OK yeah that works for me.

Can you post a proper patch with few minor changes:

- Add a comment to the code somewhere saying that for off mode,
  omap3_core_restore_context() also restores intc and we don't
  need to omap3_intc_resume_idle().

- Add the brackets to the one line else statement for checkpatch.

Cheers,

Tony

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Dave Gerlach]

On 04/11/2016 07:01 PM, Tony Lindgren wrote:
> * Dave Gerlach <d-gerlach@ti.com> [160411 16:17]:
>> On 04/11/2016 04:13 PM, Tony Lindgren wrote:
>>>> We still need the call to omap3_intc_resume_idle because the intc restore
>>>> context only gets called on resume from off mode. Perhaps we only need to
>>>> call omap3_intc_resume_idle when coming back from non-off modes, otherwise
>>>> let the context restore handle the reconfig of the INTC idle/sysconfig
>>>> registers?
>>>
>>> OK. Did you actually test by commenting out omap3_intc_resume_idle()?
>>>
>>> Yeah sounds like we can optimize out the restore there for non-off
>>> modes.
>>
>> Yes I removed it entirely for testing, and I also tried something like this for
>> a possible workable solution (without your patch applied):
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index fcf975eb5e9d..8d39b44ba3a3 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -268,7 +268,6 @@ void omap_sram_idle(void)
>>          int per_next_state = PWRDM_POWER_ON;
>>          int core_next_state = PWRDM_POWER_ON;
>>          int per_going_off;
>> -       int core_prev_state;
>>          u32 sdrc_pwr = 0;
>>
>>          mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
>> @@ -348,17 +347,16 @@ void omap_sram_idle(void)
>>                  sdrc_write_reg(sdrc_pwr, SDRC_POWER);
>>
>>          /* CORE */
>> -       if (core_next_state < PWRDM_POWER_ON) {
>> -               core_prev_state = pwrdm_read_prev_pwrst(core_pwrdm);
>> -               if (core_prev_state == PWRDM_POWER_OFF) {
>> +       if (core_next_state < PWRDM_POWER_ON &&
>> +           pwrdm_read_prev_pwrst(core_pwrdm) == PWRDM_POWER_OFF) {
>>                          omap3_core_restore_context();
>>                          omap3_cm_restore_context();
>>                          omap3_push_sram_idle();
>>                          omap3_push_sram_secure_idle();
>>                          omap2_sms_restore_context();
>> -               }
>> -       }
>> -       omap3_intc_resume_idle();
>> +       } else
>> +               omap3_intc_resume_idle();
>> +
>>
>>          pwrdm_post_transition(NULL);
>>
>>
>
> OK yeah that works for me.
>
> Can you post a proper patch with few minor changes:
>
> - Add a comment to the code somewhere saying that for off mode,
>    omap3_core_restore_context() also restores intc and we don't
>    need to omap3_intc_resume_idle().
>
> - Add the brackets to the one line else statement for checkpatch.

Thanks for your comments, I'm sure you've seen it but I sent a patch 
here with the fixes you recommended here: 
https://patchwork.kernel.org/patch/8811951/

Regards,
Dave

>
> Cheers,
>
> Tony
>

Re: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx

[Tony Lindgren]

* Dave Gerlach <d-gerlach@ti.com> [160413 08:47]:
> 
> Thanks for your comments, I'm sure you've seen it but I sent a patch here
> with the fixes you recommended here:
> https://patchwork.kernel.org/patch/8811951/

Yes looks good to me thanks!

Tony