[PATCH v2] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[PATCH v2] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

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

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
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(-)

diff --git a/arch/arm/include/asm/glue-proc.h b/arch/arm/include/asm/glue-proc.h
index 74a8b84f3cb1..74be7c22035a 100644
--- a/arch/arm/include/asm/glue-proc.h
+++ b/arch/arm/include/asm/glue-proc.h
@@ -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)
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index 3db2c2f04a30..e26bfaa419ee 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -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 */
-- 
1.8.3.2

[PATCH v2] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Tomasz Figa]

Hi Shawn,

On 22.06.2014 12:15, Shawn Guo wrote:
> 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]

> +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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Will Deacon]

On Tue, Jun 24, 2014 at 05:28:54PM +0100, Tomasz Figa wrote:
> Hi Shawn,
> 
> On 22.06.2014 12:15, Shawn Guo wrote:
> > 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]
> 
> > +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?

On A9, it should be write-ignore. Are you seeing problems on a real SoC?

Will

[PATCH v2] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Tomasz Figa]

Hi Will,

On 24.06.2014 18:33, Will Deacon wrote:
> On Tue, Jun 24, 2014 at 05:28:54PM +0100, Tomasz Figa wrote:
>> Hi Shawn,
>>
>> On 22.06.2014 12:15, Shawn Guo wrote:
>>> 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]
>>
>>> +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?
> 
> 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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Will Deacon]

On Tue, Jun 24, 2014 at 06:40:27PM +0100, Tomasz Figa wrote:
> Hi Will,

Hello,

> On 24.06.2014 18:33, Will Deacon wrote:
> > On Tue, Jun 24, 2014 at 05:28:54PM +0100, Tomasz Figa wrote:
> >> Hi Shawn,
> >>
> >> On 22.06.2014 12:15, Shawn Guo wrote:
> >>> 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]
> >>
> >>> +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?
> > 
> > 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.

> 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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Russell King - ARM Linux]

On Tue, Jun 24, 2014 at 07:40:27PM +0200, Tomasz Figa wrote:
> Hi Will,
> 
> On 24.06.2014 18:33, Will Deacon wrote:
> > On Tue, Jun 24, 2014 at 05:28:54PM +0100, Tomasz Figa wrote:
> >> Hi Shawn,
> >>
> >> On 22.06.2014 12:15, Shawn Guo wrote:
> >>> 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]
> >>
> >>> +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?
> > 
> > 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.

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.

-- 
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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Tomasz Figa]

On 01.07.2014 16:44, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 07:40:27PM +0200, Tomasz Figa wrote:
>> Hi Will,
>>
>> On 24.06.2014 18:33, Will Deacon wrote:
>>> On Tue, Jun 24, 2014 at 05:28:54PM +0100, Tomasz Figa wrote:
>>>> Hi Shawn,
>>>>
>>>> On 22.06.2014 12:15, Shawn Guo wrote:
>>>>> 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]
>>>>
>>>>> +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?
>>>
>>> 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.
> 
> 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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

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

On Tue, Jul 01, 2014 at 05:26:05PM +0100, Tomasz Figa wrote:
> On 01.07.2014 16:44, Russell King - ARM Linux wrote:
> > On Tue, Jun 24, 2014 at 07:40:27PM +0200, Tomasz Figa wrote:
> >> On 24.06.2014 18:33, Will Deacon wrote:
> >>> 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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Lorenzo Pieralisi]

On Wed, Jul 02, 2014 at 01:34:44PM +0100, Will Deacon wrote:
> 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.
> 
> On Tue, Jul 01, 2014 at 05:26:05PM +0100, Tomasz Figa wrote:
> > On 01.07.2014 16:44, Russell King - ARM Linux wrote:
> > > On Tue, Jun 24, 2014 at 07:40:27PM +0200, Tomasz Figa wrote:
> > >> On 24.06.2014 18:33, Will Deacon wrote:
> > >>> 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.

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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Russell King - ARM Linux]

On Wed, Jul 02, 2014 at 03:01:34PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Jul 02, 2014 at 01:34:44PM +0100, Will Deacon wrote:
> > 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.

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.

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.

-- 
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] ARM: save/restore diagnostic register on Cortex-A9 suspend/resume

[Shawn Guo]

On Wed, Jul 02, 2014 at 03:26:21PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 02, 2014 at 03:01:34PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Jul 02, 2014 at 01:34:44PM +0100, Will Deacon wrote:
> > > 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.
> 
> 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.
> 
> 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