From: d-gerlach@ti•com (Dave Gerlach)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx
Date: Mon, 11 Apr 2016 18:16:16 -0500 [thread overview]
Message-ID: <570C3040.3030001@ti.com> (raw)
In-Reply-To: <20160411211339.GI5995@atomide.com>
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
>
next prev parent reply other threads:[~2016-04-11 23:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-10 21:35 [PATCH] ARM: OMAP3: Fix imprecise external abort for off mode on 36xx Tony Lindgren
2016-04-07 20:17 ` Dave Gerlach
2016-04-07 23:16 ` Tony Lindgren
2016-04-11 18:20 ` Dave Gerlach
2016-04-11 21:13 ` Tony Lindgren
2016-04-11 23:16 ` Dave Gerlach [this message]
2016-04-12 0:01 ` Tony Lindgren
2016-04-13 15:45 ` Dave Gerlach
2016-04-13 15:50 ` Tony Lindgren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=570C3040.3030001@ti.com \
--to=d-gerlach@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox