ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
---
Changes since v1:
- Add Cortex-A9 specific processor function for suspend and resume to handle diagnostic register across suspend/resume cycle
arch/arm/include/asm/glue-proc.h | 18 +++++++++--------- arch/arm/mm/proc-v7.S | 31 ++++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 10 deletions(-)
arch/arm/include/asm/glue-proc.h +9 -9
@@ -221,15 +221,6 @@# endif #endif -#ifdef CONFIG_CPU_V7 -# ifdef CPU_NAME -# undef MULTI_CPU -# define MULTI_CPU -# else -# define CPU_NAME cpu_v7 -# endif -#endif -#ifdef CONFIG_CPU_V7M # ifdef CPU_NAME # undef MULTI_CPU
@@ -248,6 +239,15 @@# endif #endif +#ifdef CONFIG_CPU_V7 +/* +* Cortex-A9 needs a different suspend/resume function, so we need +* multiple CPU support for ARMv7 anyway. +*/ +# undef MULTI_CPU +# define MULTI_CPU +#endif +#ifndef MULTI_CPU #define cpu_proc_init __glue(CPU_NAME,_proc_init)#define cpu_proc_fin __glue(CPU_NAME,_proc_fin)
arch/arm/mm/proc-v7.S +30 -1
@@ -152,6 +152,34 @@ ENTRY(cpu_v7_do_resume) ENDPROC(cpu_v7_do_resume) #endif +/* + * Cortex-A9 processor functions + */ + globl_equ cpu_ca9mp_proc_init, cpu_v7_proc_init + globl_equ cpu_ca9mp_proc_fin, cpu_v7_proc_fin + globl_equ cpu_ca9mp_reset, cpu_v7_reset + globl_equ cpu_ca9mp_do_idle, cpu_v7_do_idle + globl_equ cpu_ca9mp_dcache_clean_area, cpu_v7_dcache_clean_area + globl_equ cpu_ca9mp_switch_mm, cpu_v7_switch_mm + globl_equ cpu_ca9mp_set_pte_ext, cpu_v7_set_pte_ext +.globl cpu_ca9mp_suspend_size +.equ cpu_ca9mp_suspend_size, cpu_v7_suspend_size + 1 * 4 +#ifdef CONFIG_ARM_CPU_SUSPEND +ENTRY(cpu_ca9mp_do_suspend) + stmfd sp!, {r4} + mrc p15, 0, r4, c15, c0, 1 @ Diagnostic register + stmia r0!, {r4} + ldmfd sp!, {r4} + b cpu_v7_do_suspend +ENDPROC(cpu_ca9mp_do_suspend) + +ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic register + b cpu_v7_do_resume +ENDPROC(cpu_ca9mp_do_resume) +#endif + #ifdef CONFIG_CPU_PJ4B globl_equ cpu_pj4b_switch_mm, cpu_v7_switch_mm globl_equ cpu_pj4b_set_pte_ext, cpu_v7_set_pte_ext
@@ -418,6 +446,7 @@ __v7_setup_stack: @ define struct processor (see <asm/proc-fns.h> and proc-macros.S) define_processor_functions v7, dabort=v7_early_abort, pabort=v7_pabort, suspend=1 + define_processor_functions ca9mp, dabort=v7_early_abort, pabort=v7_pabort, suspend=1 #ifdef CONFIG_CPU_PJ4B define_processor_functions pj4b, dabort=v7_early_abort, pabort=v7_pabort, suspend=1 #endif
@@ -470,7 +499,7 @@ __v7_ca5mp_proc_info: __v7_ca9mp_proc_info: .long 0x410fc090 .long 0xff0ffff0 - __v7_proc __v7_ca9mp_setup + __v7_proc __v7_ca9mp_setup, proc_fns = ca9mp_processor_functions .size __v7_ca9mp_proc_info, . - __v7_ca9mp_proc_info #endif /* CONFIG_ARM_LPAE */
Signature · 1 line
1.8.3.2
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Hi Shawn,
Quoted 7 lines · Shawn Guo
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
[snip]
arch/arm/mm/proc-v7.S ENTRY(cpu_v7_do_resume) · line 178
Quoted 3 lines
arch/arm/mm/proc-v7.S +3
-0+ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic register
What about platforms running in non-secure mode in which the register is read-only?
Best regards,
Tomasz
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Quoted 11 lines · Tomasz Figa
Hi Shawn,
Quoted 7 lines · Shawn Guo
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
Quoted 8 lines
[snip]
arch/arm/mm/proc-v7.S ENTRY(cpu_v7_do_resume) · line 178
Quoted 3 lines
arch/arm/mm/proc-v7.S +3
-0+ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic registerWhat about platforms running in non-secure mode in which the register is read-only?
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
Will
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Hi Will,
Quoted 14 lines · Will Deacon
Quoted 13 lines · Tomasz Figa
Hi Shawn,
Quoted 7 lines · Shawn Guo
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
[snip]
Quoted 8 lines
Quoted 6 lines
arch/arm/mm/proc-v7.S ENTRY(cpu_v7_do_resume) · line 178
Quoted 3 lines
arch/arm/mm/proc-v7.S +3
-0+ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic registerWhat about platforms running in non-secure mode in which the register is read-only?
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
I'm observing a complete system hang on Exynos4412-based Trats2 board if I try to write this register in resume from system-wide sleep.
Note that the board is running under secure firmware, but there is no support for suspend/resume of such boards in mainline yet, so I'm testing on a work in progress (but mostly finished) series that is yet to be sent.
Best regards,
Tomasz
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Quoted 1 line · Tomasz Figa
Hi Will,
Hello,
Quoted 22 lines · Will Deacon
Quoted 21 lines · Tomasz Figa
Quoted 19 lines
Hi Shawn,
Quoted 7 lines · Shawn Guo
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
[snip]
arch/arm/mm/proc-v7.S ENTRY(cpu_v7_do_resume) · line 178
Quoted 3 lines
arch/arm/mm/proc-v7.S +3
-0+ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic registerWhat about platforms running in non-secure mode in which the register is read-only?
Quoted 4 lines
Quoted 1 line
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
I'm observing a complete system hang on Exynos4412-based Trats2 board if I try to write this register in resume from system-wide sleep.
That's certainly unexpected; I just double-checked that non-secure accesses are treated as read-only/write-ignore.
Quoted 4 lines
Note that the board is running under secure firmware, but there is no support for suspend/resume of such boards in mainline yet, so I'm testing on a work in progress (but mostly finished) series that is yet to be sent.
Maybe it could be unrelated to this register. What happens if you change the code to read/write a different (architected) cp15 register?
Will
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
In reply to Tomasz FigaQuoted 26 lines · Tomasz Figa
Hi Will,
Quoted 14 lines · Will Deacon
Quoted 13 lines · Tomasz Figa
Hi Shawn,
Quoted 7 lines · Shawn Guo
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
[snip]
Quoted 8 lines
Quoted 6 lines
arch/arm/mm/proc-v7.S ENTRY(cpu_v7_do_resume) · line 178
Quoted 3 lines
arch/arm/mm/proc-v7.S +3
-0+ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic registerWhat about platforms running in non-secure mode in which the register is read-only?
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
Quoted 7 lines
I'm observing a complete system hang on Exynos4412-based Trats2 board if I try to write this register in resume from system-wide sleep.
Note that the board is running under secure firmware, but there is no support for suspend/resume of such boards in mainline yet, so I'm testing on a work in progress (but mostly finished) series that is yet to be sent.
Do we have any progress on this? There's patches being generated by the Exynos people which depend on this, so we need to decide whether we want this patch or not.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Quoted 30 lines · Russell King - ARM Linux
Quoted 29 lines · Tomasz Figa
Hi Will,
Quoted 14 lines · Will Deacon
Quoted 13 lines · Tomasz Figa
Hi Shawn,
Quoted 7 lines · Shawn Guo
The CP15 diagnostic register holds ARM errata bits on Cortex-A9, so it needs to be saved/restored on suspend/resume. Otherwise, the effectiveness of errata workaround gets lost together with diagnostic register bit across suspend/resume cycle.
The patch adds a couple of Cortex-A9 specific suspend and resume functions to handle the diagnostic register across suspend/resume cycle.
[snip]
Quoted 8 lines
Quoted 6 lines
arch/arm/mm/proc-v7.S ENTRY(cpu_v7_do_resume) · line 178
Quoted 3 lines
arch/arm/mm/proc-v7.S +3
-0+ENTRY(cpu_ca9mp_do_resume) + ldmia r0!, {r4} + mcr p15, 0, r4, c15, c0, 1 @ Diagnostic registerWhat about platforms running in non-secure mode in which the register is read-only?
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
I'm observing a complete system hang on Exynos4412-based Trats2 board if I try to write this register in resume from system-wide sleep.
Quoted 9 lines
Quoted 4 lines
Note that the board is running under secure firmware, but there is no support for suspend/resume of such boards in mainline yet, so I'm testing on a work in progress (but mostly finished) series that is yet to be sent.
Do we have any progress on this? There's patches being generated by the Exynos people which depend on this, so we need to decide whether we want this patch or not.
I need to handle some other duties right now, but I'll get back to this as soon as possible and carry on with further testing. However I've done few more tests checking non-secure access to both diagnostic and power control registers and both always crash on Exynos4412.
Best regards,
Tomasz
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Hi Tomasz,
I asked the hardware guys to take a closer look at your issue and they've uncovered the problem with trying to document an undocumented register.
Quoted 7 lines · Tomasz Figa
Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Tomasz Figa
Quoted 1 line · Will Deacon
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
I'm observing a complete system hang on Exynos4412-based Trats2 board if I try to write this register in resume from system-wide sleep.
In actual fact, this register causes an undefined instruction exception if you try to write it from a non-secure mode (although reads work fine, for whatever that's worth). That nicely explains what you're seeing, but it leaves us in a right old two and eight with respect to the suspend/resume code.
Given that we don't know whether we're running secure or non-secure, I'm not sure about the best approach to solve this. For arm64, we simply mandate booting non-secure and require firmware to do everything that requires secure access, but we're not in a position to attempt imposing these sorts of restrictions for arch/arm/.
Does anybody have any ideas? We could try `handling' the undef, but it's ugly and fragile.
Anyway, apologies for the mistake earlier on -- I assumed the documentation I had was correct.
Will
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Quoted 20 lines · Will Deacon
Hi Tomasz,
I asked the hardware guys to take a closer look at your issue and they've uncovered the problem with trying to document an undocumented register.
Quoted 7 lines · Tomasz Figa
Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Tomasz Figa
Quoted 1 line · Will Deacon
On A9, it should be write-ignore. Are you seeing problems on a real SoC?
I'm observing a complete system hang on Exynos4412-based Trats2 board if I try to write this register in resume from system-wide sleep.
In actual fact, this register causes an undefined instruction exception if you try to write it from a non-secure mode (although reads work fine, for whatever that's worth). That nicely explains what you're seeing, but it leaves us in a right old two and eight with respect to the suspend/resume code.
Quoted 8 lines
Given that we don't know whether we're running secure or non-secure, I'm not sure about the best approach to solve this. For arm64, we simply mandate booting non-secure and require firmware to do everything that requires secure access, but we're not in a position to attempt imposing these sorts of restrictions for
arch/arm/.Does anybody have any ideas? We could try `handling' the undef, but it's ugly and fragile.
You could try to treat it the same way as ACTLR (see cpu_v7_do_resume, where the ACTLR is first read, if it is already set, ie equal to the saved value, skip the write); ACTLR suffers from the same issue and IIRC that's why Russell added the "teq" then "write" test at the time cpu_suspend was consolidated.
Lorenzo
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Quoted 9 lines · Lorenzo Pieralisi
Quoted 2 lines · Will Deacon
Does anybody have any ideas? We could try `handling' the undef, but it's ugly and fragile.
You could try to treat it the same way as ACTLR (see
cpu_v7_do_resume, where the ACTLR is first read, if it is already set, ie equal to the saved value, skip the write); ACTLR suffers from the same issue and IIRC that's why Russell added the "teq" then "write" test at the timecpu_suspendwas consolidated.
We can't do that now for these, because if they were already correct, then by definition we wouldn't need to restore them except on secure parts.
Quoted 1 line
From the comments that have been made on the subject of these registers,
it is already the case that non-secure platforms don't restore this register, instead relying on the platform to jump through hoops to update it _after_ the MMU has been enabled.
Will Deacon raised a point when I discussed this that once the MMU is enabled, the hardware doesn't expect the diagnostic register to be changed. So, having platform code (which runs after the MMU has been enabled) sounds dodgy to me.
Of course, prior to DT and single zImage, this wouldn't be that big a deal, we _could_ work around it via the kernel config or similar, insisting that we have a secure and a non-secure build mode. We don't have that "luxury" anymore though.
What this all means is that we can't even augment the suspend/resume paths to deal with this register even for secure mode parts.
I, too, don't know what the answer is here. This is really a totally fubar'd situation where there is no solution which will work for everyone.
I guess what this ultimately comes down to is that we _did_ accept the work-arounds into the kernel source for secure parts - had we not done that and set a clear message like ARM64 does that work-arounds must be done prior to calling the kernel (irrespective of how that happens, iow, whether boot or resume) then we wouldn't have this problem right now. Such a statement would have raised lots of objections though, but with hindsight, it would have been the right thing to do to overrule those objections and just mandate it. It would've been a pain for some people, but we would not be in this situation now where there is no proper solution which works for everyone.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
PATCH v2 1/1 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
Quoted 20 lines · Russell King - ARM Linux
Quoted 9 lines · Lorenzo Pieralisi
Quoted 2 lines · Will Deacon
Does anybody have any ideas? We could try `handling' the undef, but it's ugly and fragile.
You could try to treat it the same way as ACTLR (see
cpu_v7_do_resume, where the ACTLR is first read, if it is already set, ie equal to the saved value, skip the write); ACTLR suffers from the same issue and IIRC that's why Russell added the "teq" then "write" test at the timecpu_suspendwas consolidated.We can't do that now for these, because if they were already correct, then by definition we wouldn't need to restore them except on secure parts.
From the comments that have been made on the subject of these registers, it is already the case that non-secure platforms don't restore this register, instead relying on the platform to jump through hoops to update it _after_ the MMU has been enabled.
Quoted 4 lines
Will Deacon raised a point when I discussed this that once the MMU is enabled, the hardware doesn't expect the diagnostic register to be changed. So, having platform code (which runs after the MMU has been enabled) sounds dodgy to me.
Though it sounds dodgy, restoring the registers at platform level is the only choice we have now to get the workaround effectiveness back, due to this secure vs. non-secure pain. Also it seems Exynos has been doing this for years (see exynos_cpu_restore_register()).
Shawn