* Two variants of fixing Tegra20 suspend bug
@ 2015-01-15 10:58 Dmitry Osipenko
2015-01-15 10:58 ` [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM Dmitry Osipenko
2015-01-15 10:58 ` [PATCH] ARM: tegra: Store tegra_resume() address " Dmitry Osipenko
0 siblings, 2 replies; 21+ messages in thread
From: Dmitry Osipenko @ 2015-01-15 10:58 UTC (permalink / raw)
To: digetx, Stephen Warren, Thierry Reding, Alexandre Courbot,
Peter De Schrijver
Cc: linux-tegra, linux-kernel, stable
Hi, this is third attempt to fix Tegra20 suspend bug. First was to use other
PMC scratch register for tegra_resume() address store and second to use syscore
ops for PMC driver.
Thierry Reding proposed other solution: to use IRAM instead of PMC scratch
register. I prepared two implementation variants:
1) Store CPU "resettable" status in IRAM
2) Store tegra_resume() address in IRAM
Both variants use reset handler IRAM area for placing tegra_resume() address,
since it is reserved and guarantees avoidance of any further possible memory
conflicts.
As for me, second variant is cleaner. It removes suspend PM from PMC driver,
which can be re-added later if needed.
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-15 10:58 Two variants of fixing Tegra20 suspend bug Dmitry Osipenko @ 2015-01-15 10:58 ` Dmitry Osipenko [not found] ` <1421319545-23920-2-git-send-email-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-01-15 10:58 ` [PATCH] ARM: tegra: Store tegra_resume() address " Dmitry Osipenko 1 sibling, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-15 10:58 UTC (permalink / raw) To: digetx, Stephen Warren, Thierry Reding, Alexandre Courbot, Peter De Schrijver Cc: linux-tegra, linux-kernel, stable Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() location storing from late to early and, as a result, broke suspend on Tegra20. PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored physical memory address of common resume function and in the same time used by tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), which is storing CPU1 "resettable" status. It implies strict order of scratch register usage, otherwise resume function address is lost on Tegra20 after disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in IRAM instead of PMC scratch register. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) Cc: <stable@vger.kernel.org> # v3.17+ --- arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- arch/arm/mach-tegra/reset.h | 4 ++++ arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- arch/arm/mach-tegra/sleep.h | 4 ++++ 5 files changed, 38 insertions(+), 22 deletions(-) diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index b30bf5c..f209e9c 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -35,6 +35,7 @@ #include "iomap.h" #include "irq.h" #include "pm.h" +#include "reset.h" #include "sleep.h" #ifdef CONFIG_PM_SLEEP @@ -72,15 +73,13 @@ static struct cpuidle_driver tegra_idle_driver = { #ifdef CONFIG_PM_SLEEP #ifdef CONFIG_SMP -static void __iomem *pmc = IO_ADDRESS(TEGRA_PMC_BASE); - static int tegra20_reset_sleeping_cpu_1(void) { int ret = 0; tegra_pen_lock(); - if (readl(pmc + PMC_SCRATCH41) == CPU_RESETTABLE) + if (readb(tegra20_cpu1_resettable_status) == CPU_RESETTABLE) tegra20_cpu_shutdown(1); else ret = -EINVAL; diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S index 7b2baab..60e9add 100644 --- a/arch/arm/mach-tegra/reset-handler.S +++ b/arch/arm/mach-tegra/reset-handler.S @@ -168,10 +168,10 @@ after_errata: cmp r6, #TEGRA20 bne 1f /* If not CPU0, don't let CPU0 reset CPU1 now that CPU1 is coming up. */ - mov32 r5, TEGRA_PMC_BASE - mov r0, #0 + mov32 r5, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + mov r0, #CPU_NOT_RESETTABLE cmp r10, #0 - strne r0, [r5, #PMC_SCRATCH41] + strneb r0, [r5, #__tegra20_cpu1_resettable_status_offset] 1: #endif @@ -280,6 +280,10 @@ __tegra_cpu_reset_handler_data: .rept TEGRA_RESET_DATA_SIZE .long 0 .endr + .globl __tegra20_cpu1_resettable_status_offset + .equ __tegra20_cpu1_resettable_status_offset, \ + . - __tegra_cpu_reset_handler_start + .byte 0 .align L1_CACHE_SHIFT ENTRY(__tegra_cpu_reset_handler_end) diff --git a/arch/arm/mach-tegra/reset.h b/arch/arm/mach-tegra/reset.h index 76a9343..29c3dec 100644 --- a/arch/arm/mach-tegra/reset.h +++ b/arch/arm/mach-tegra/reset.h @@ -35,6 +35,7 @@ extern unsigned long __tegra_cpu_reset_handler_data[TEGRA_RESET_DATA_SIZE]; void __tegra_cpu_reset_handler_start(void); void __tegra_cpu_reset_handler(void); +void __tegra20_cpu1_resettable_status_offset(void); void __tegra_cpu_reset_handler_end(void); void tegra_secondary_startup(void); @@ -47,6 +48,9 @@ void tegra_secondary_startup(void); (IO_ADDRESS(TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + \ ((u32)&__tegra_cpu_reset_handler_data[TEGRA_RESET_MASK_LP2] - \ (u32)__tegra_cpu_reset_handler_start))) +#define tegra20_cpu1_resettable_status \ + (IO_ADDRESS(TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + \ + (u32)__tegra20_cpu1_resettable_status_offset)) #endif #define tegra_cpu_reset_handler_offset \ diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S index be4bc5f..e6b684e 100644 --- a/arch/arm/mach-tegra/sleep-tegra20.S +++ b/arch/arm/mach-tegra/sleep-tegra20.S @@ -97,9 +97,10 @@ ENDPROC(tegra20_hotplug_shutdown) ENTRY(tegra20_cpu_shutdown) cmp r0, #0 reteq lr @ must not be called for CPU 0 - mov32 r1, TEGRA_PMC_VIRT + PMC_SCRATCH41 + mov32 r1, TEGRA_IRAM_RESET_BASE_VIRT + ldr r2, =__tegra20_cpu1_resettable_status_offset mov r12, #CPU_RESETTABLE - str r12, [r1] + strb r12, [r1, r2] cpu_to_halt_reg r1, r0 ldr r3, =TEGRA_FLOW_CTRL_VIRT @@ -182,38 +183,41 @@ ENDPROC(tegra_pen_unlock) /* * tegra20_cpu_clear_resettable(void) * - * Called to clear the "resettable soon" flag in PMC_SCRATCH41 when + * Called to clear the "resettable soon" flag in IRAM variable when * it is expected that the secondary CPU will be idle soon. */ ENTRY(tegra20_cpu_clear_resettable) - mov32 r1, TEGRA_PMC_VIRT + PMC_SCRATCH41 + mov32 r1, TEGRA_IRAM_RESET_BASE_VIRT + ldr r2, =__tegra20_cpu1_resettable_status_offset mov r12, #CPU_NOT_RESETTABLE - str r12, [r1] + strb r12, [r1, r2] ret lr ENDPROC(tegra20_cpu_clear_resettable) /* * tegra20_cpu_set_resettable_soon(void) * - * Called to set the "resettable soon" flag in PMC_SCRATCH41 when + * Called to set the "resettable soon" flag in IRAM variable when * it is expected that the secondary CPU will be idle soon. */ ENTRY(tegra20_cpu_set_resettable_soon) - mov32 r1, TEGRA_PMC_VIRT + PMC_SCRATCH41 + mov32 r1, TEGRA_IRAM_RESET_BASE_VIRT + ldr r2, =__tegra20_cpu1_resettable_status_offset mov r12, #CPU_RESETTABLE_SOON - str r12, [r1] + strb r12, [r1, r2] ret lr ENDPROC(tegra20_cpu_set_resettable_soon) /* * tegra20_cpu_is_resettable_soon(void) * - * Returns true if the "resettable soon" flag in PMC_SCRATCH41 has been + * Returns true if the "resettable soon" flag in IRAM variable has been * set because it is expected that the secondary CPU will be idle soon. */ ENTRY(tegra20_cpu_is_resettable_soon) - mov32 r1, TEGRA_PMC_VIRT + PMC_SCRATCH41 - ldr r12, [r1] + mov32 r1, TEGRA_IRAM_RESET_BASE_VIRT + ldr r2, =__tegra20_cpu1_resettable_status_offset + ldrb r12, [r1, r2] cmp r12, #CPU_RESETTABLE_SOON moveq r0, #1 movne r0, #0 @@ -256,9 +260,10 @@ ENTRY(tegra20_sleep_cpu_secondary_finish) mov r0, #TEGRA_FLUSH_CACHE_LOUIS bl tegra_disable_clean_inv_dcache - mov32 r0, TEGRA_PMC_VIRT + PMC_SCRATCH41 + mov32 r0, TEGRA_IRAM_RESET_BASE_VIRT + ldr r4, =__tegra20_cpu1_resettable_status_offset mov r3, #CPU_RESETTABLE - str r3, [r0] + strb r3, [r0, r4] bl tegra_cpu_do_idle @@ -274,10 +279,10 @@ ENTRY(tegra20_sleep_cpu_secondary_finish) bl tegra_pen_lock - mov32 r3, TEGRA_PMC_VIRT - add r0, r3, #PMC_SCRATCH41 + mov32 r0, TEGRA_IRAM_RESET_BASE_VIRT + ldr r4, =__tegra20_cpu1_resettable_status_offset mov r3, #CPU_NOT_RESETTABLE - str r3, [r0] + strb r3, [r0, r4] bl tegra_pen_unlock diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h index 92d46ec..0d59360 100644 --- a/arch/arm/mach-tegra/sleep.h +++ b/arch/arm/mach-tegra/sleep.h @@ -18,6 +18,7 @@ #define __MACH_TEGRA_SLEEP_H #include "iomap.h" +#include "irammap.h" #define TEGRA_ARM_PERIF_VIRT (TEGRA_ARM_PERIF_BASE - IO_CPU_PHYS \ + IO_CPU_VIRT) @@ -29,6 +30,9 @@ + IO_APB_VIRT) #define TEGRA_PMC_VIRT (TEGRA_PMC_BASE - IO_APB_PHYS + IO_APB_VIRT) +#define TEGRA_IRAM_RESET_BASE_VIRT (IO_IRAM_VIRT + \ + TEGRA_IRAM_RESET_HANDLER_OFFSET) + /* PMC_SCRATCH37-39 and 41 are used for tegra_pen_lock and idle */ #define PMC_SCRATCH37 0x130 #define PMC_SCRATCH38 0x134 -- 2.2.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <1421319545-23920-2-git-send-email-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-15 10:58 ` [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM Dmitry Osipenko @ 2015-01-19 14:12 ` Thierry Reding 0 siblings, 0 replies; 21+ messages in thread From: Thierry Reding @ 2015-01-19 14:12 UTC (permalink / raw) To: Dmitry Osipenko Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 1424 bytes --] On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: > Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() > location storing from late to early and, as a result, broke suspend on Tegra20. > PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored > physical memory address of common resume function and in the same time used by > tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), > which is storing CPU1 "resettable" status. It implies strict order of scratch > register usage, otherwise resume function address is lost on Tegra20 after > disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in > IRAM instead of PMC scratch register. > > Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) > Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.17+ > --- > arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- > arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- > arch/arm/mach-tegra/reset.h | 4 ++++ > arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- > arch/arm/mach-tegra/sleep.h | 4 ++++ > 5 files changed, 38 insertions(+), 22 deletions(-) I'm leaning towards applying this. Stephen, Alex, Peter: any objections? Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM @ 2015-01-19 14:12 ` Thierry Reding 0 siblings, 0 replies; 21+ messages in thread From: Thierry Reding @ 2015-01-19 14:12 UTC (permalink / raw) To: Dmitry Osipenko Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 1370 bytes --] On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: > Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() > location storing from late to early and, as a result, broke suspend on Tegra20. > PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored > physical memory address of common resume function and in the same time used by > tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), > which is storing CPU1 "resettable" status. It implies strict order of scratch > register usage, otherwise resume function address is lost on Tegra20 after > disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in > IRAM instead of PMC scratch register. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) > Cc: <stable@vger.kernel.org> # v3.17+ > --- > arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- > arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- > arch/arm/mach-tegra/reset.h | 4 ++++ > arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- > arch/arm/mach-tegra/sleep.h | 4 ++++ > 5 files changed, 38 insertions(+), 22 deletions(-) I'm leaning towards applying this. Stephen, Alex, Peter: any objections? Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20150119141224.GF23778-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>]
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 14:12 ` Thierry Reding @ 2015-01-19 15:14 ` Dmitry Osipenko -1 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-19 15:14 UTC (permalink / raw) To: Thierry Reding Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA 19.01.2015 17:12, Thierry Reding пишет: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > I'm leaning towards applying this. Stephen, Alex, Peter: any objections? > > Thierry > Btw, I'm preparing cleanup for cpuidle and hotplug code. So this asm "ugliness" should go away. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM @ 2015-01-19 15:14 ` Dmitry Osipenko 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-19 15:14 UTC (permalink / raw) To: Thierry Reding Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable 19.01.2015 17:12, Thierry Reding пишет: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable@vger.kernel.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > I'm leaning towards applying this. Stephen, Alex, Peter: any objections? > > Thierry > Btw, I'm preparing cleanup for cpuidle and hotplug code. So this asm "ugliness" should go away. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 14:12 ` Thierry Reding @ 2015-01-19 17:26 ` Stephen Warren -1 siblings, 0 replies; 21+ messages in thread From: Stephen Warren @ 2015-01-19 17:26 UTC (permalink / raw) To: Thierry Reding, Dmitry Osipenko Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA On 01/19/2015 07:12 AM, Thierry Reding wrote: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > I'm leaning towards applying this. Stephen, Alex, Peter: any objections? Hopefully this works out. I suppose it's unlikely anyone will be running code on the AVP upstrem, so any potential conflict with AVP's usage of IRAM isn't likely to occur. __tegra20_cpu1_resettable_status_offset has a lot of _ at the start. Should the symbol be named more normally? I guess at least it's consistent with the existing very "underscory" __tegra_cpu_reset_handler_start. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM @ 2015-01-19 17:26 ` Stephen Warren 0 siblings, 0 replies; 21+ messages in thread From: Stephen Warren @ 2015-01-19 17:26 UTC (permalink / raw) To: Thierry Reding, Dmitry Osipenko Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable On 01/19/2015 07:12 AM, Thierry Reding wrote: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable@vger.kernel.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > I'm leaning towards applying this. Stephen, Alex, Peter: any objections? Hopefully this works out. I suppose it's unlikely anyone will be running code on the AVP upstrem, so any potential conflict with AVP's usage of IRAM isn't likely to occur. __tegra20_cpu1_resettable_status_offset has a lot of _ at the start. Should the symbol be named more normally? I guess at least it's consistent with the existing very "underscory" __tegra_cpu_reset_handler_start. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 17:26 ` Stephen Warren (?) @ 2015-01-19 17:41 ` Dmitry Osipenko 2015-01-19 17:45 ` Stephen Warren -1 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-19 17:41 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable 19.01.2015 20:26, Stephen Warren пишет: > Hopefully this works out. I suppose it's unlikely anyone will be running code on > the AVP upstrem, so any potential conflict with AVP's usage of IRAM isn't likely > to occur. > I don't see how it can conflict with AVP code. First KB of IRAM is reserved for reset handler. Am I missing something? From reset.h: /* The first 1K of IRAM is permanently reserved for the CPU reset handler */ > __tegra20_cpu1_resettable_status_offset has a lot of _ at the start. Should the > symbol be named more normally? I guess at least it's consistent with the > existing very "underscory" __tegra_cpu_reset_handler_start. I also wasn't happy with "underscory" :) And yes, I left it for consistency. Please feel free to rename it, if needed. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 17:41 ` Dmitry Osipenko @ 2015-01-19 17:45 ` Stephen Warren 2015-01-19 18:00 ` Dmitry Osipenko 0 siblings, 1 reply; 21+ messages in thread From: Stephen Warren @ 2015-01-19 17:45 UTC (permalink / raw) To: Dmitry Osipenko, Thierry Reding Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable On 01/19/2015 10:41 AM, Dmitry Osipenko wrote: > 19.01.2015 20:26, Stephen Warren пишет: >> Hopefully this works out. I suppose it's unlikely anyone will be >> running code on >> the AVP upstrem, so any potential conflict with AVP's usage of IRAM >> isn't likely >> to occur. >> > I don't see how it can conflict with AVP code. First KB of IRAM is > reserved for reset handler. Am I missing something? > > From reset.h: > > /* The first 1K of IRAM is permanently reserved for the CPU reset > handler */ I believe "CPU" in that context means AVP CPU. Still, I may not be correct, and to be honest it's likely not too well defined even if that comment seems clear-cut. >> __tegra20_cpu1_resettable_status_offset has a lot of _ at the start. >> Should the >> symbol be named more normally? I guess at least it's consistent with the >> existing very "underscory" __tegra_cpu_reset_handler_start. > I also wasn't happy with "underscory" :) And yes, I left it for > consistency. Please feel free to rename it, if needed. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 17:45 ` Stephen Warren @ 2015-01-19 18:00 ` Dmitry Osipenko [not found] ` <54BD4626.70902-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-19 18:00 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable 19.01.2015 20:45, Stephen Warren пишет: > On 01/19/2015 10:41 AM, Dmitry Osipenko wrote: >> 19.01.2015 20:26, Stephen Warren пишет: >>> Hopefully this works out. I suppose it's unlikely anyone will be >>> running code on >>> the AVP upstrem, so any potential conflict with AVP's usage of IRAM >>> isn't likely >>> to occur. >>> >> I don't see how it can conflict with AVP code. First KB of IRAM is >> reserved for reset handler. Am I missing something? >> >> From reset.h: >> >> /* The first 1K of IRAM is permanently reserved for the CPU reset >> handler */ > > I believe "CPU" in that context means AVP CPU. Still, I may not be correct, and > to be honest it's likely not too well defined even if that comment seems clear-cut. > Hmm... Suddenly I recalled that LP2 was always disabled in downstream kernel. I remember that I tried it once (couple years ago) and it didn't work, however I presume it was just broken. Now I don't feel good with it. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <54BD4626.70902-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 18:00 ` Dmitry Osipenko @ 2015-01-19 18:26 ` Dmitry Osipenko 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-19 18:26 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA 19.01.2015 21:00, Dmitry Osipenko пишет: > 19.01.2015 20:45, Stephen Warren пишет: >> On 01/19/2015 10:41 AM, Dmitry Osipenko wrote: >>> 19.01.2015 20:26, Stephen Warren пишет: >>>> Hopefully this works out. I suppose it's unlikely anyone will be >>>> running code on >>>> the AVP upstrem, so any potential conflict with AVP's usage of IRAM >>>> isn't likely >>>> to occur. >>>> >>> I don't see how it can conflict with AVP code. First KB of IRAM is >>> reserved for reset handler. Am I missing something? >>> >>> From reset.h: >>> >>> /* The first 1K of IRAM is permanently reserved for the CPU reset >>> handler */ >> >> I believe "CPU" in that context means AVP CPU. Still, I may not be correct, and >> to be honest it's likely not too well defined even if that comment seems >> clear-cut. >> > Hmm... Suddenly I recalled that LP2 was always disabled in downstream kernel. I > remember that I tried it once (couple years ago) and it didn't work, however I > presume it was just broken. Now I don't feel good with it. > Can't generic RAM be used for "resettable" status? Or it will be too slow?... CPU1 always come up after CPU0, so RAM is already init'ed. Given that CPU0 can't be halted with running CPU1, I suppose CPU1 can't be booted first, right? Anyway it's not the case for linux. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM @ 2015-01-19 18:26 ` Dmitry Osipenko 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-19 18:26 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable 19.01.2015 21:00, Dmitry Osipenko пишет: > 19.01.2015 20:45, Stephen Warren пишет: >> On 01/19/2015 10:41 AM, Dmitry Osipenko wrote: >>> 19.01.2015 20:26, Stephen Warren пишет: >>>> Hopefully this works out. I suppose it's unlikely anyone will be >>>> running code on >>>> the AVP upstrem, so any potential conflict with AVP's usage of IRAM >>>> isn't likely >>>> to occur. >>>> >>> I don't see how it can conflict with AVP code. First KB of IRAM is >>> reserved for reset handler. Am I missing something? >>> >>> From reset.h: >>> >>> /* The first 1K of IRAM is permanently reserved for the CPU reset >>> handler */ >> >> I believe "CPU" in that context means AVP CPU. Still, I may not be correct, and >> to be honest it's likely not too well defined even if that comment seems >> clear-cut. >> > Hmm... Suddenly I recalled that LP2 was always disabled in downstream kernel. I > remember that I tried it once (couple years ago) and it didn't work, however I > presume it was just broken. Now I don't feel good with it. > Can't generic RAM be used for "resettable" status? Or it will be too slow?... CPU1 always come up after CPU0, so RAM is already init'ed. Given that CPU0 can't be halted with running CPU1, I suppose CPU1 can't be booted first, right? Anyway it's not the case for linux. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 18:26 ` Dmitry Osipenko (?) @ 2015-01-20 16:55 ` Dmitry Osipenko -1 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-20 16:55 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable 19.01.2015 21:26, Dmitry Osipenko пишет: > 19.01.2015 21:00, Dmitry Osipenko пишет: >> 19.01.2015 20:45, Stephen Warren пишет: >>> On 01/19/2015 10:41 AM, Dmitry Osipenko wrote: >>>> 19.01.2015 20:26, Stephen Warren пишет: >>>>> Hopefully this works out. I suppose it's unlikely anyone will be >>>>> running code on >>>>> the AVP upstrem, so any potential conflict with AVP's usage of IRAM >>>>> isn't likely >>>>> to occur. >>>>> >>>> I don't see how it can conflict with AVP code. First KB of IRAM is >>>> reserved for reset handler. Am I missing something? >>>> >>>> From reset.h: >>>> >>>> /* The first 1K of IRAM is permanently reserved for the CPU reset >>>> handler */ >>> >>> I believe "CPU" in that context means AVP CPU. Still, I may not be correct, and >>> to be honest it's likely not too well defined even if that comment seems >>> clear-cut. >>> >> Hmm... Suddenly I recalled that LP2 was always disabled in downstream kernel. I >> remember that I tried it once (couple years ago) and it didn't work, however I >> presume it was just broken. Now I don't feel good with it. >> > Can't generic RAM be used for "resettable" status? Or it will be too slow?... > > CPU1 always come up after CPU0, so RAM is already init'ed. Given that CPU0 can't > be halted with running CPU1, I suppose CPU1 can't be booted first, right? Anyway > it's not the case for linux. > Correcting myself: Well, it's meaningless in case if LP2 cpuidle can't co-exist with AVP firmware. Isn't possible verify it? -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-19 14:12 ` Thierry Reding @ 2015-01-20 1:59 ` Alexandre Courbot -1 siblings, 0 replies; 21+ messages in thread From: Alexandre Courbot @ 2015-01-20 1:59 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Osipenko, Stephen Warren, Peter De Schrijver, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List, Stable On Mon, Jan 19, 2015 at 11:12 PM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > I'm leaning towards applying this. Stephen, Alex, Peter: any objections? None from me ; but I am not qualified to give an valuable opinion on this part of the code. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM @ 2015-01-20 1:59 ` Alexandre Courbot 0 siblings, 0 replies; 21+ messages in thread From: Alexandre Courbot @ 2015-01-20 1:59 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Osipenko, Stephen Warren, Peter De Schrijver, linux-tegra, Linux Kernel Mailing List, Stable On Mon, Jan 19, 2015 at 11:12 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable@vger.kernel.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > I'm leaning towards applying this. Stephen, Alex, Peter: any objections? None from me ; but I am not qualified to give an valuable opinion on this part of the code. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-01-15 10:58 ` [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM Dmitry Osipenko @ 2015-03-11 10:29 ` Thierry Reding 0 siblings, 0 replies; 21+ messages in thread From: Thierry Reding @ 2015-03-11 10:29 UTC (permalink / raw) To: Dmitry Osipenko Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 1422 bytes --] On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: > Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() > location storing from late to early and, as a result, broke suspend on Tegra20. > PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored > physical memory address of common resume function and in the same time used by > tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), > which is storing CPU1 "resettable" status. It implies strict order of scratch > register usage, otherwise resume function address is lost on Tegra20 after > disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in > IRAM instead of PMC scratch register. > > Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) > Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.17+ > --- > arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- > arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- > arch/arm/mach-tegra/reset.h | 4 ++++ > arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- > arch/arm/mach-tegra/sleep.h | 4 ++++ > 5 files changed, 38 insertions(+), 22 deletions(-) Sorry for the long delay, but I've applied this now to the Tegra tree. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM @ 2015-03-11 10:29 ` Thierry Reding 0 siblings, 0 replies; 21+ messages in thread From: Thierry Reding @ 2015-03-11 10:29 UTC (permalink / raw) To: Dmitry Osipenko Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 1368 bytes --] On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: > Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() > location storing from late to early and, as a result, broke suspend on Tegra20. > PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored > physical memory address of common resume function and in the same time used by > tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), > which is storing CPU1 "resettable" status. It implies strict order of scratch > register usage, otherwise resume function address is lost on Tegra20 after > disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in > IRAM instead of PMC scratch register. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) > Cc: <stable@vger.kernel.org> # v3.17+ > --- > arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- > arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- > arch/arm/mach-tegra/reset.h | 4 ++++ > arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- > arch/arm/mach-tegra/sleep.h | 4 ++++ > 5 files changed, 38 insertions(+), 22 deletions(-) Sorry for the long delay, but I've applied this now to the Tegra tree. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM 2015-03-11 10:29 ` Thierry Reding (?) @ 2015-03-11 13:28 ` Dmitry Osipenko -1 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2015-03-11 13:28 UTC (permalink / raw) To: Thierry Reding Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable 11.03.2015 13:29, Thierry Reding пишет: > On Thu, Jan 15, 2015 at 01:58:57PM +0300, Dmitry Osipenko wrote: >> Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() >> location storing from late to early and, as a result, broke suspend on Tegra20. >> PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored >> physical memory address of common resume function and in the same time used by >> tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), >> which is storing CPU1 "resettable" status. It implies strict order of scratch >> register usage, otherwise resume function address is lost on Tegra20 after >> disabling non-boot CPU's on suspend. Fix it by storing "resettable" status in >> IRAM instead of PMC scratch register. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) >> Cc: <stable@vger.kernel.org> # v3.17+ >> --- >> arch/arm/mach-tegra/cpuidle-tegra20.c | 5 ++--- >> arch/arm/mach-tegra/reset-handler.S | 10 +++++++--- >> arch/arm/mach-tegra/reset.h | 4 ++++ >> arch/arm/mach-tegra/sleep-tegra20.S | 37 ++++++++++++++++++++--------------- >> arch/arm/mach-tegra/sleep.h | 4 ++++ >> 5 files changed, 38 insertions(+), 22 deletions(-) > > Sorry for the long delay, but I've applied this now to the Tegra tree. > > Thierry > Thanks, now we can move forward. -- Dmitry ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] ARM: tegra: Store tegra_resume() address in IRAM 2015-01-15 10:58 Two variants of fixing Tegra20 suspend bug Dmitry Osipenko 2015-01-15 10:58 ` [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM Dmitry Osipenko @ 2015-01-15 10:58 ` Dmitry Osipenko 2015-01-19 14:01 ` Thierry Reding 1 sibling, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2015-01-15 10:58 UTC (permalink / raw) To: digetx, Stephen Warren, Thierry Reding, Alexandre Courbot, Peter De Schrijver Cc: linux-tegra, linux-kernel, stable Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() location storing from late to early and, as a result, broke suspend on Tegra20. PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored physical memory address of common resume function and in the same time used by tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), which is storing CPU1 "resettable" status. It implies strict order of scratch register usage, otherwise resume function address is lost on Tegra20 after disabling non-boot CPU's on suspend. Fix it by storing tegra_resume() physical address in IRAM instead of PMC scratch register. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) Cc: <stable@vger.kernel.org> # v3.17+ --- arch/arm/mach-tegra/pm.c | 3 +++ arch/arm/mach-tegra/reset-handler.S | 4 ++++ arch/arm/mach-tegra/reset.h | 4 ++++ arch/arm/mach-tegra/sleep-tegra20.S | 7 ++++--- arch/arm/mach-tegra/sleep-tegra30.S | 7 ++++--- drivers/soc/tegra/pmc.c | 19 ------------------- 6 files changed, 19 insertions(+), 25 deletions(-) diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c index b0f48a3..f11e129 100644 --- a/arch/arm/mach-tegra/pm.c +++ b/arch/arm/mach-tegra/pm.c @@ -395,6 +395,9 @@ void __init tegra_init_suspend(void) break; } + /* Store common resume function physical address in IRAM */ + writel_relaxed(virt_to_phys(tegra_resume), tegra_resume_func_phys_addr); + suspend_set_ops(&tegra_suspend_ops); } #endif diff --git a/arch/arm/mach-tegra/reset-handler.S b/arch/arm/mach-tegra/reset-handler.S index 7b2baab..659557b 100644 --- a/arch/arm/mach-tegra/reset-handler.S +++ b/arch/arm/mach-tegra/reset-handler.S @@ -280,6 +280,10 @@ __tegra_cpu_reset_handler_data: .rept TEGRA_RESET_DATA_SIZE .long 0 .endr + .globl __tegra_resume_func_addr_offset + .equ __tegra_resume_func_addr_offset, \ + . - __tegra_cpu_reset_handler_start + .long 0 .align L1_CACHE_SHIFT ENTRY(__tegra_cpu_reset_handler_end) diff --git a/arch/arm/mach-tegra/reset.h b/arch/arm/mach-tegra/reset.h index 76a9343..1b43f19 100644 --- a/arch/arm/mach-tegra/reset.h +++ b/arch/arm/mach-tegra/reset.h @@ -36,6 +36,7 @@ extern unsigned long __tegra_cpu_reset_handler_data[TEGRA_RESET_DATA_SIZE]; void __tegra_cpu_reset_handler_start(void); void __tegra_cpu_reset_handler(void); void __tegra_cpu_reset_handler_end(void); +void __tegra_resume_func_addr_offset(void); void tegra_secondary_startup(void); #ifdef CONFIG_PM_SLEEP @@ -47,6 +48,9 @@ void tegra_secondary_startup(void); (IO_ADDRESS(TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + \ ((u32)&__tegra_cpu_reset_handler_data[TEGRA_RESET_MASK_LP2] - \ (u32)__tegra_cpu_reset_handler_start))) +#define tegra_resume_func_phys_addr \ + (IO_ADDRESS(TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + \ + (u32)__tegra_resume_func_addr_offset)) #endif #define tegra_cpu_reset_handler_offset \ diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S index be4bc5f..bea484f8 100644 --- a/arch/arm/mach-tegra/sleep-tegra20.S +++ b/arch/arm/mach-tegra/sleep-tegra20.S @@ -327,7 +327,7 @@ tegra20_iram_start: * system clock running on the same PLL that it suspended at), and * jumps to tegra_resume to restore virtual addressing and PLLX. * The physical address of tegra_resume expected to be stored in - * PMC_SCRATCH41. + * IRAM reset handler area. * * NOTE: THIS *MUST* BE RELOCATED TO TEGRA_IRAM_LPx_RESUME_AREA. */ @@ -400,8 +400,9 @@ exit_selfrefresh_loop: mov r1, #0 @ unstall all transactions str r1, [r0, #EMC_REQ_CTRL] - mov32 r0, TEGRA_PMC_BASE - ldr r0, [r0, #PMC_SCRATCH41] + mov32 r0, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + ldr r1, =__tegra_resume_func_addr_offset + ldr r0, [r0, r1] ret r0 @ jump to tegra_resume ENDPROC(tegra20_lp1_reset) diff --git a/arch/arm/mach-tegra/sleep-tegra30.S b/arch/arm/mach-tegra/sleep-tegra30.S index 5d8d13a..b05f22c 100644 --- a/arch/arm/mach-tegra/sleep-tegra30.S +++ b/arch/arm/mach-tegra/sleep-tegra30.S @@ -314,7 +314,7 @@ tegra30_iram_start: * system clock running on the same PLL that it suspended at), and * jumps to tegra_resume to restore virtual addressing. * The physical address of tegra_resume expected to be stored in - * PMC_SCRATCH41. + * IRAM reset handler area. * * NOTE: THIS *MUST* BE RELOCATED TO TEGRA_IRAM_LPx_RESUME_AREA. */ @@ -528,8 +528,9 @@ zcal_done: bne exit_self_refresh __no_dual_emc_chanl: - mov32 r0, TEGRA_PMC_BASE - ldr r0, [r0, #PMC_SCRATCH41] + mov32 r0, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET + ldr r1, =__tegra_resume_func_addr_offset + ldr r0, [r0, r1] ret r0 @ jump to tegra_resume ENDPROC(tegra30_lp1_reset) diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index a2c0ceb..b87ead2 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -739,24 +739,6 @@ static int tegra_pmc_probe(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM_SLEEP -static int tegra_pmc_suspend(struct device *dev) -{ - tegra_pmc_writel(virt_to_phys(tegra_resume), PMC_SCRATCH41); - - return 0; -} - -static int tegra_pmc_resume(struct device *dev) -{ - tegra_pmc_writel(0x0, PMC_SCRATCH41); - - return 0; -} -#endif - -static SIMPLE_DEV_PM_OPS(tegra_pmc_pm_ops, tegra_pmc_suspend, tegra_pmc_resume); - static const char * const tegra20_powergates[] = { [TEGRA_POWERGATE_CPU] = "cpu", [TEGRA_POWERGATE_3D] = "3d", @@ -894,7 +876,6 @@ static struct platform_driver tegra_pmc_driver = { .name = "tegra-pmc", .suppress_bind_attrs = true, .of_match_table = tegra_pmc_match, - .pm = &tegra_pmc_pm_ops, }, .probe = tegra_pmc_probe, }; -- 2.2.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] ARM: tegra: Store tegra_resume() address in IRAM 2015-01-15 10:58 ` [PATCH] ARM: tegra: Store tegra_resume() address " Dmitry Osipenko @ 2015-01-19 14:01 ` Thierry Reding 0 siblings, 0 replies; 21+ messages in thread From: Thierry Reding @ 2015-01-19 14:01 UTC (permalink / raw) To: Dmitry Osipenko Cc: Stephen Warren, Alexandre Courbot, Peter De Schrijver, linux-tegra, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 1591 bytes --] On Thu, Jan 15, 2015 at 01:58:58PM +0300, Dmitry Osipenko wrote: > Commit 7232398abc6a ("ARM: tegra: Convert PMC to a driver") changed tegra_resume() > location storing from late to early and, as a result, broke suspend on Tegra20. > PMC scratch register 41 is used by tegra LP1 resume code for retrieving stored > physical memory address of common resume function and in the same time used by > tegra20_cpu_shutdown() (shared by Tegra20 cpuidle driver and platform SMP code), > which is storing CPU1 "resettable" status. It implies strict order of scratch > register usage, otherwise resume function address is lost on Tegra20 after > disabling non-boot CPU's on suspend. Fix it by storing tegra_resume() physical > address in IRAM instead of PMC scratch register. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > Fixes: 7232398abc6a (ARM: tegra: Convert PMC to a driver) > Cc: <stable@vger.kernel.org> # v3.17+ > --- > arch/arm/mach-tegra/pm.c | 3 +++ > arch/arm/mach-tegra/reset-handler.S | 4 ++++ > arch/arm/mach-tegra/reset.h | 4 ++++ > arch/arm/mach-tegra/sleep-tegra20.S | 7 ++++--- > arch/arm/mach-tegra/sleep-tegra30.S | 7 ++++--- > drivers/soc/tegra/pmc.c | 19 ------------------- > 6 files changed, 19 insertions(+), 25 deletions(-) I don't think this is a good variant. I'm sure it'll work, but storing a pointer to tegra_resume() is actually the documented purpose of the PMC scratch 41 register. That is, some software (like bootloaders and whatnot) may actually rely on this behaviour. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-03-11 13:28 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-15 10:58 Two variants of fixing Tegra20 suspend bug Dmitry Osipenko
2015-01-15 10:58 ` [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM Dmitry Osipenko
[not found] ` <1421319545-23920-2-git-send-email-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-01-19 14:12 ` Thierry Reding
2015-01-19 14:12 ` Thierry Reding
[not found] ` <20150119141224.GF23778-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
2015-01-19 15:14 ` Dmitry Osipenko
2015-01-19 15:14 ` Dmitry Osipenko
2015-01-19 17:26 ` Stephen Warren
2015-01-19 17:26 ` Stephen Warren
2015-01-19 17:41 ` Dmitry Osipenko
2015-01-19 17:45 ` Stephen Warren
2015-01-19 18:00 ` Dmitry Osipenko
[not found] ` <54BD4626.70902-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-01-19 18:26 ` Dmitry Osipenko
2015-01-19 18:26 ` Dmitry Osipenko
2015-01-20 16:55 ` Dmitry Osipenko
2015-01-20 1:59 ` Alexandre Courbot
2015-01-20 1:59 ` Alexandre Courbot
2015-03-11 10:29 ` Thierry Reding
2015-03-11 10:29 ` Thierry Reding
2015-03-11 13:28 ` Dmitry Osipenko
2015-01-15 10:58 ` [PATCH] ARM: tegra: Store tegra_resume() address " Dmitry Osipenko
2015-01-19 14:01 ` Thierry Reding
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.