Two variants of fixing Tegra20 suspend bug
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.
PATCH 1/1 ARM: tegra20: Store CPU "resettable" status in IRAM
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.
--- 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(-)
arch/arm/mach-tegra/cpuidle-tegra20.c +2 -3
@@ -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;
arch/arm/mach-tegra/reset-handler.S +7 -3
@@ -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)
arch/arm/mach-tegra/reset.h +4 -0
@@ -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 \
arch/arm/mach-tegra/sleep-tegra20.S +21 -16
@@ -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
arch/arm/mach-tegra/sleep.h +4 -0
@@ -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
Signature · 1 line
2.2.1
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
Quoted 13 lines · Dmitry Osipenko
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+
Quoted 7 lines
--- 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
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
19.01.2015 17:12, Thierry Reding пишет:
Quoted 22 lines · Dmitry Osipenko
Quoted 13 lines
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+Quoted 7 lines
--- 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(-)
Quoted 4 lines
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.
Signature · 1 line
Dmitry
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
In reply to Thierry RedingQuoted 22 lines · Thierry Reding
Quoted 13 lines · Dmitry Osipenko
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+Quoted 7 lines
--- 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(-)
Quoted 1 line
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.
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
19.01.2015 20:26, Stephen Warren пишет:
Quoted 4 lines
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 */
Quoted 3 lines
__tegra20_cpu1_resettable_status_offsethas 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.
Signature · 1 line
Dmitry
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
Quoted 7 lines · Dmitry Osipenko
19.01.2015 20:26, Stephen Warren пишет:
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 6 lines
Quoted 4 lines
__tegra20_cpu1_resettable_status_offsethas 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.
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
19.01.2015 20:45, Stephen Warren пишет:
Quoted 11 lines · Dmitry Osipenko
Quoted 10 lines
19.01.2015 20:26, Stephen Warren пишет:
Quoted 6 lines
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?
Quoted 8 lines
Quoted 4 lines
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.
Signature · 1 line
Dmitry
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
19.01.2015 21:00, Dmitry Osipenko пишет:
Quoted 17 lines
19.01.2015 20:45, Stephen Warren пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 14 lines
19.01.2015 20:26, Stephen Warren пишет:
Quoted 6 lines
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 */
Quoted 8 lines
Quoted 4 lines
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.
Signature · 1 line
Dmitry
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
19.01.2015 21:26, Dmitry Osipenko пишет:
Quoted 26 lines
19.01.2015 21:00, Dmitry Osipenko пишет:
Quoted 17 lines
19.01.2015 20:45, Stephen Warren пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 14 lines
19.01.2015 20:26, Stephen Warren пишет:
Quoted 6 lines
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 */
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
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?
Signature · 1 line
Dmitry
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
In reply to Thierry RedingQuoted 22 lines · Thierry Reding
Quoted 13 lines · Dmitry Osipenko
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+Quoted 7 lines
--- 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(-)
Quoted 1 line
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.
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
In reply to Dmitry OsipenkoQuoted 13 lines · Dmitry Osipenko
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+
Quoted 7 lines
--- 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
Re: [PATCH] ARM: tegra20: Store CPU "resettable" status in IRAM
11.03.2015 13:29, Thierry Reding пишет:
Quoted 22 lines · Dmitry Osipenko
Quoted 13 lines
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+Quoted 7 lines
--- 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(-)
Quoted 4 lines
Sorry for the long delay, but I've applied this now to the Tegra tree.
Thierry
Thanks, now we can move forward.
Signature · 1 line
Dmitry
PATCH 1/1 ARM: tegra: Store tegra_resume() address in IRAM
In reply to Dmitry OsipenkoCommit 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.
--- 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(-)
arch/arm/mach-tegra/pm.c +3 -0
@@ -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
arch/arm/mach-tegra/reset-handler.S +4 -0
@@ -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)
arch/arm/mach-tegra/reset.h +4 -0
@@ -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 \
arch/arm/mach-tegra/sleep-tegra20.S +4 -3
@@ -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)
arch/arm/mach-tegra/sleep-tegra30.S +4 -3
@@ -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)
drivers/soc/tegra/pmc.c +0 -19
@@ -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, };
Signature · 1 line
2.2.1
Re: [PATCH] ARM: tegra: Store tegra_resume() address in IRAM
Quoted 13 lines · Dmitry Osipenko
Commit
7232398abc6a("ARM: tegra: Convert PMC to a driver") changedtegra_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 bytegra20_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 storingtegra_resume()physical address in IRAM instead of PMC scratch register.Signed-off-by Dmitry Osipenko <[email protected]>Fixes 7232398abc6a (ARM: tegra: Convert PMC to a driver)Cc <[email protected]> # v3.17+
Quoted 8 lines
--- 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