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

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 <[email protected]>

Cc Nishanth Menon <[email protected]>

Cc Richard Woodruff <[email protected]>

Cc Tero Kristo <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---

Anybody else seen this issue before?

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

arch/arm/mach-omap2/sleep34xx.S +6 -0

index 1b9f052..0fbaa08 100644
@@ -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:

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].

Quoted 12 lines · Dave Gerlach

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.

Quoted 6 lines

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 <[email protected]>
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".

arch/arm/mach-omap2/pm34xx.c +4 -3

@@ -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)

Tony,

Quoted 25 lines · Tony Lindgren

Quoted 12 lines · Dave Gerlach

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.

Quoted 6 lines

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.

Quoted 5 lines

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?

Quoted 31 lines · Dave Gerlach

Tony,

Quoted 29 lines · Tony Lindgren

Quoted 12 lines · Dave Gerlach

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.

Quoted 6 lines

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).

Quoted 4 lines

Quoted 1 line

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..

Quoted 9 lines

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

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

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):

arch/arm/mach-omap2/pm34xx.c +2 -4

index fcf975eb5e9d..8d39b44ba3a3 100644
@@ -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();

Quoted 41 lines · Dave Gerlach

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

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):

arch/arm/mach-omap2/pm34xx.c +2 -4

index fcf975eb5e9d..8d39b44ba3a3 100644
@@ -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();

arch/arm/mach-omap2/pm34xx.c void omap_sram_idle(void) · line 362

Quoted 10 lines

arch/arm/mach-omap2/pm34xx.c +3 -3

                         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

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/