| Message ID | 1426069206-13667-1-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ?ro, 2015-03-11 at 11:20 +0100, Krzysztof Kozlowski wrote: > On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in > 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache > controller") the second suspend to RAM failed. First suspend worked fine > but the next one hang just after powering down of secondary CPUs (system > consumed energy as it would be running but was not responsive). > > The issue was caused by enabling delayed reset assertion for CPU0 just > after issuing power down of cores. This was introduced for Exynos4 in > 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). > > The whole behavior is not well documented but after checking with vendor > code this should be done like this (on Exynos4): > 1. Enable delayed reset assertion when system is running (for all CPUs). > 2. Disable delayed reset assertion before suspending the system. > This can be done after powering off secondary CPUs. > 3. Re-enable the delayed reset assertion when system is resumed. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") > Cc: <stable@vger.kernel.org> > Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Tested-by: Chanwoo Choi <cw00.choi@samsung.com> Dear Kukjin, This patch was first sent on 3rd of February. It could enter before opening 4.0 merge window. I did not receive any response from you in that time. So let me point next steps: 1. The Exynos4412 suspend on 4.0 is broken and now this patch applies as a fix. 2. I resent it on 18th of February. 3. I received tested-by from Bartlomiej and Chanwoo. 4. Bartlomiej pinged you on 3rd March. Still no response. If the patch does not look good then please share your comments. I'll fix it. If this patch looks good, why does it take so much time? Dear Arnd and Olof, Could you pick it up to your arm-soc? Best regards, Krzysztof
On 03/11/15 19:29, Krzysztof Kozlowski wrote: > On ?ro, 2015-03-11 at 11:20 +0100, Krzysztof Kozlowski wrote: >> On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in >> 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache >> controller") the second suspend to RAM failed. First suspend worked fine >> but the next one hang just after powering down of secondary CPUs (system >> consumed energy as it would be running but was not responsive). >> >> The issue was caused by enabling delayed reset assertion for CPU0 just >> after issuing power down of cores. This was introduced for Exynos4 in >> 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). >> >> The whole behavior is not well documented but after checking with vendor >> code this should be done like this (on Exynos4): >> 1. Enable delayed reset assertion when system is running (for all CPUs). >> 2. Disable delayed reset assertion before suspending the system. >> This can be done after powering off secondary CPUs. >> 3. Re-enable the delayed reset assertion when system is resumed. >> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") >> Cc: <stable@vger.kernel.org> >> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> Tested-by: Chanwoo Choi <cw00.choi@samsung.com> > > Dear Kukjin, > > This patch was first sent on 3rd of February. It could enter before > opening 4.0 merge window. I did not receive any response from you in > that time. > > So let me point next steps: > 1. The Exynos4412 suspend on 4.0 is broken and now this patch applies as > a fix. > 2. I resent it on 18th of February. > 3. I received tested-by from Bartlomiej and Chanwoo. > 4. Bartlomiej pinged you on 3rd March. > > Still no response. If the patch does not look good then please share > your comments. I'll fix it. > If this patch looks good, why does it take so much time? > Please use another way something like check ARM core rather than use 'soc_is_xxx()', as you know it is not acceptable now even it is just moving/modifying exist function though. And please make sure your updates don't hurt other exynos5 stuff. Any tests on exynos5 platforms would be helpful. And I don't think the fix should be sent to 'stable' because I can't see the 'add node for L2$ controller' in v3.19...looks applied from v4.0-rc... One more if you have any doubts, I'd like to ask you to contact S.LSI guys who have created the vendor codes not assume with the code because maybe the vendor code you mentioned cannot cover all exynos stuff I think. Then we could make more clear pm codes in mainline. To be honest I'm not a Power Management hardware guy so I don't know every regarding PM stuff in exynos SoCs, I can contact them easier though...I mean please don't assume any hardware behavior with just vendor code. Please ask, you have an access in Samsung intranet before posting something like this...Hope let's make a better fix together during -rc. Thanks, Kukjin
On ?ro, 2015-03-18 at 03:05 +0900, Kukjin Kim wrote: > On 03/11/15 19:29, Krzysztof Kozlowski wrote: > > On ?ro, 2015-03-11 at 11:20 +0100, Krzysztof Kozlowski wrote: > >> On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in > >> 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache > >> controller") the second suspend to RAM failed. First suspend worked fine > >> but the next one hang just after powering down of secondary CPUs (system > >> consumed energy as it would be running but was not responsive). > >> > >> The issue was caused by enabling delayed reset assertion for CPU0 just > >> after issuing power down of cores. This was introduced for Exynos4 in > >> 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). > >> > >> The whole behavior is not well documented but after checking with vendor > >> code this should be done like this (on Exynos4): > >> 1. Enable delayed reset assertion when system is running (for all CPUs). > >> 2. Disable delayed reset assertion before suspending the system. > >> This can be done after powering off secondary CPUs. > >> 3. Re-enable the delayed reset assertion when system is resumed. > >> > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >> Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") > >> Cc: <stable@vger.kernel.org> > >> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > >> Tested-by: Chanwoo Choi <cw00.choi@samsung.com> > > > > Dear Kukjin, > > > > This patch was first sent on 3rd of February. It could enter before > > opening 4.0 merge window. I did not receive any response from you in > > that time. > > > > So let me point next steps: > > 1. The Exynos4412 suspend on 4.0 is broken and now this patch applies as > > a fix. > > 2. I resent it on 18th of February. > > 3. I received tested-by from Bartlomiej and Chanwoo. > > 4. Bartlomiej pinged you on 3rd March. > > > > Still no response. If the patch does not look good then please share > > your comments. I'll fix it. > > If this patch looks good, why does it take so much time? > > > > Please use another way something like check ARM core rather than use > 'soc_is_xxx()', as you know it is not acceptable now even it is just > moving/modifying exist function though. Probably of_machine_is_compatible() could be used here but such change should be done in separate patch. This is fix for wrong usage of use_delayed_assertion so it should not mix with other changes in the code. This fixes one thing at a time. Fixing many things in one patch often leads to new errors or difficulties in debugging. I can prepare a separate patch for changing this to of_machine_is_compatible(). > > And please make sure your updates don't hurt other exynos5 stuff. Any > tests on exynos5 platforms would be helpful. > > And I don't think the fix should be sent to 'stable' because I can't see > the 'add node for L2$ controller' in v3.19...looks applied from v4.0-rc... You're right. git-describe gave me 3.19-rc1 but this was tag for the specific commit, not for merge-commit. The stable can be removed if this comes during this RC-cycle. > > One more if you have any doubts, I'd like to ask you to contact S.LSI > guys who have created the vendor codes not assume with the code because > maybe the vendor code you mentioned cannot cover all exynos stuff I > think. Then we could make more clear pm codes in mainline. To be honest > I'm not a Power Management hardware guy so I don't know every regarding > PM stuff in exynos SoCs, I can contact them easier though...I mean > please don't assume any hardware behavior with just vendor code. Please > ask, you have an access in Samsung intranet before posting something > like this...Hope let's make a better fix together during -rc. As you probably know I work in completely different company within Samsung Electronics than System LSI. I don't have access to the LSI intranet. I don't have access to guys from LSI. I'll try contacting them through my HQ partners. Thanks for feedback! Best regards, Krzysztof
On ?ro, 2015-03-18 at 09:57 +0100, Krzysztof Kozlowski wrote: > On ?ro, 2015-03-18 at 03:05 +0900, Kukjin Kim wrote: > > On 03/11/15 19:29, Krzysztof Kozlowski wrote: > > > On ?ro, 2015-03-11 at 11:20 +0100, Krzysztof Kozlowski wrote: > > >> On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in > > >> 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache > > >> controller") the second suspend to RAM failed. First suspend worked fine > > >> but the next one hang just after powering down of secondary CPUs (system > > >> consumed energy as it would be running but was not responsive). > > >> > > >> The issue was caused by enabling delayed reset assertion for CPU0 just > > >> after issuing power down of cores. This was introduced for Exynos4 in > > >> 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). > > >> > > >> The whole behavior is not well documented but after checking with vendor > > >> code this should be done like this (on Exynos4): > > >> 1. Enable delayed reset assertion when system is running (for all CPUs). > > >> 2. Disable delayed reset assertion before suspending the system. > > >> This can be done after powering off secondary CPUs. > > >> 3. Re-enable the delayed reset assertion when system is resumed. > > >> > > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > >> Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") > > >> Cc: <stable@vger.kernel.org> > > >> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > >> Tested-by: Chanwoo Choi <cw00.choi@samsung.com> (...) > > > > > And please make sure your updates don't hurt other exynos5 stuff. Any > > tests on exynos5 platforms would be helpful. > > > > And I don't think the fix should be sent to 'stable' because I can't see > > the 'add node for L2$ controller' in v3.19...looks applied from v4.0-rc... > > You're right. git-describe gave me 3.19-rc1 but this was tag for the > specific commit, not for merge-commit. The stable can be removed if this > comes during this RC-cycle. Actually the "fixes" tag is for commit introducing wrong usage of "use_delayed_reset_assertion" which was merged for 3.19. Although mentioned bug (failed second to RAM) is observable only after enabling L2 cache, the fix is for original commit. I have also other proposal: what about reverting the commit 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off")? It will introduce minor issue (CPU idle clock down will stop to work after CPU hot unplug) but the main problem with suspend to RAM will be fixed. Best regards, Krzysztof
Hi, On Wednesday, March 18, 2015 09:57:27 AM Krzysztof Kozlowski wrote: > On ?ro, 2015-03-18 at 03:05 +0900, Kukjin Kim wrote: > > On 03/11/15 19:29, Krzysztof Kozlowski wrote: > > > On ?ro, 2015-03-11 at 11:20 +0100, Krzysztof Kozlowski wrote: > > >> On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in > > >> 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache > > >> controller") the second suspend to RAM failed. First suspend worked fine > > >> but the next one hang just after powering down of secondary CPUs (system > > >> consumed energy as it would be running but was not responsive). > > >> > > >> The issue was caused by enabling delayed reset assertion for CPU0 just > > >> after issuing power down of cores. This was introduced for Exynos4 in > > >> 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). > > >> > > >> The whole behavior is not well documented but after checking with vendor > > >> code this should be done like this (on Exynos4): > > >> 1. Enable delayed reset assertion when system is running (for all CPUs). > > >> 2. Disable delayed reset assertion before suspending the system. > > >> This can be done after powering off secondary CPUs. > > >> 3. Re-enable the delayed reset assertion when system is resumed. > > >> > > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > >> Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") > > >> Cc: <stable@vger.kernel.org> > > >> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > >> Tested-by: Chanwoo Choi <cw00.choi@samsung.com> > > > > > > Dear Kukjin, > > > > > > This patch was first sent on 3rd of February. It could enter before > > > opening 4.0 merge window. I did not receive any response from you in > > > that time. > > > > > > So let me point next steps: > > > 1. The Exynos4412 suspend on 4.0 is broken and now this patch applies as > > > a fix. > > > 2. I resent it on 18th of February. > > > 3. I received tested-by from Bartlomiej and Chanwoo. > > > 4. Bartlomiej pinged you on 3rd March. > > > > > > Still no response. If the patch does not look good then please share > > > your comments. I'll fix it. > > > If this patch looks good, why does it take so much time? > > > > > > > Please use another way something like check ARM core rather than use > > 'soc_is_xxx()', as you know it is not acceptable now even it is just > > moving/modifying exist function though. Kukjin, could you please explain why 'soc_is_xxx()' usage inside arch/arm/mach-exynos/ code is not acceptable? I know that it should not be used outisde of this directory because of multiplatform support but what is wrong with using it for arch/arm/mach-exynos/ code? I'm also not sure if -rc4 is a desirable time to be doing such changes (especially given that the patch in question is moving an existing code, not adding new 'soc_is_xxx()' users). [ Moreover of_machine_is_compatible() can be sometimes harmful as could have been seen in commit ca489c58ef0b81 ("ARM: EXYNOS: Don't use LDREX and STREX after disabling cache coherency" fix. ] > Probably of_machine_is_compatible() could be used here but such change > should be done in separate patch. This is fix for wrong usage of > use_delayed_assertion so it should not mix with other changes in the > code. This fixes one thing at a time. Fixing many things in one patch > often leads to new errors or difficulties in debugging. > > I can prepare a separate patch for changing this to > of_machine_is_compatible(). IMHO this would be the best solution if there is an agreement on 'soc_is_xxx()' removal. > > > > And please make sure your updates don't hurt other exynos5 stuff. Any > > tests on exynos5 platforms would be helpful. The patch is quite obvious and only affects Exynos4 SoCs. Extra testing on Exynos5 SoCs won't hurt but they should not be required for merge. > > And I don't think the fix should be sent to 'stable' because I can't see > > the 'add node for L2$ controller' in v3.19...looks applied from v4.0-rc... > > You're right. git-describe gave me 3.19-rc1 but this was tag for the > specific commit, not for merge-commit. The stable can be removed if this > comes during this RC-cycle. Yes, the stable tag was a mistake and should be removed. > > One more if you have any doubts, I'd like to ask you to contact S.LSI > > guys who have created the vendor codes not assume with the code because > > maybe the vendor code you mentioned cannot cover all exynos stuff I > > think. Then we could make more clear pm codes in mainline. To be honest > > I'm not a Power Management hardware guy so I don't know every regarding > > PM stuff in exynos SoCs, I can contact them easier though...I mean > > please don't assume any hardware behavior with just vendor code. Please > > ask, you have an access in Samsung intranet before posting something > > like this...Hope let's make a better fix together during -rc. FWIW I think that -rc4 is too late to be doing 'perfect patch' (we've waited for 6 weeks on any feedback on this patch from you). The current solution is quite simple and has been tested to fix the regression without introducing other problems. > As you probably know I work in completely different company within > Samsung Electronics than System LSI. I don't have access to the LSI > intranet. I don't have access to guys from LSI. I'll try contacting them > through my HQ partners. > > Thanks for feedback! > > Best regards, > Krzysztof Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Hi, On Wednesday, March 18, 2015 10:47:44 AM Krzysztof Kozlowski wrote: > On ?ro, 2015-03-18 at 09:57 +0100, Krzysztof Kozlowski wrote: > > On ?ro, 2015-03-18 at 03:05 +0900, Kukjin Kim wrote: > > > On 03/11/15 19:29, Krzysztof Kozlowski wrote: > > > > On ?ro, 2015-03-11 at 11:20 +0100, Krzysztof Kozlowski wrote: > > > >> On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in > > > >> 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache > > > >> controller") the second suspend to RAM failed. First suspend worked fine > > > >> but the next one hang just after powering down of secondary CPUs (system > > > >> consumed energy as it would be running but was not responsive). > > > >> > > > >> The issue was caused by enabling delayed reset assertion for CPU0 just > > > >> after issuing power down of cores. This was introduced for Exynos4 in > > > >> 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). > > > >> > > > >> The whole behavior is not well documented but after checking with vendor > > > >> code this should be done like this (on Exynos4): > > > >> 1. Enable delayed reset assertion when system is running (for all CPUs). > > > >> 2. Disable delayed reset assertion before suspending the system. > > > >> This can be done after powering off secondary CPUs. > > > >> 3. Re-enable the delayed reset assertion when system is resumed. > > > >> > > > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > > >> Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") > > > >> Cc: <stable@vger.kernel.org> > > > >> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > > >> Tested-by: Chanwoo Choi <cw00.choi@samsung.com> > > (...) > > > > > > > > > And please make sure your updates don't hurt other exynos5 stuff. Any > > > tests on exynos5 platforms would be helpful. > > > > > > And I don't think the fix should be sent to 'stable' because I can't see > > > the 'add node for L2$ controller' in v3.19...looks applied from v4.0-rc... > > > > You're right. git-describe gave me 3.19-rc1 but this was tag for the > > specific commit, not for merge-commit. The stable can be removed if this > > comes during this RC-cycle. > > Actually the "fixes" tag is for commit introducing wrong usage of > "use_delayed_reset_assertion" which was merged for 3.19. Although > mentioned bug (failed second to RAM) is observable only after enabling > L2 cache, the fix is for original commit. IMHO "fixes" tag should be for "L2 cache" commit not for "use delayed reset assertion" one. Without L2 cache being enabled the delayed reset assertion code worked fine (even though in theory it was wrong). > I have also other proposal: what about reverting the commit 13cfa6c4f7fa > ("ARM: EXYNOS: Fix CPU idle clock down after CPU off")? It will > introduce minor issue (CPU idle clock down will stop to work after CPU > hot unplug) but the main problem with suspend to RAM will be fixed. Please don't do that. We should not replace one regression with the other one. If you want to revert something it would be better to revert the patch adding L2 cache nodes for Exynos4 SoCs for now. However we should not be reverting anything IMO. The regression fix patch is simple and is working. I would really prefer to have it applied now (to fix suspend and cpuidle v4.0-rc1 regressions finally and not keep code broken for months as we do now). We can always revisit it to something more 'perfect' later when we have more data. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Dear Kukjin, How would you like to proceed? You did not respond to my email nor to Bartlomiej's questions. You questioned the "soc_is_exynos()". I replied but there was no answer from you. My last reply: 2015-03-18 9:57 GMT+01:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>: > Probably of_machine_is_compatible() could be used here but such change > should be done in separate patch. This is fix for wrong usage of > use_delayed_assertion so it should not mix with other changes in the > code. This fixes one thing at a time. Fixing many things in one patch > often leads to new errors or difficulties in debugging. Bartlomiej's justification for it: 2015-03-18 11:29 GMT+01:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>: >> > >> > Please use another way something like check ARM core rather than use >> > 'soc_is_xxx()', as you know it is not acceptable now even it is just >> > moving/modifying exist function though. > > Kukjin, could you please explain why 'soc_is_xxx()' usage inside > arch/arm/mach-exynos/ code is not acceptable? I know that it should > not be used outisde of this directory because of multiplatform support > but what is wrong with using it for arch/arm/mach-exynos/ code? > > I'm also not sure if -rc4 is a desirable time to be doing such changes > (especially given that the patch in question is moving an existing code, > not adding new 'soc_is_xxx()' users). > > [ Moreover of_machine_is_compatible() can be sometimes harmful as could > have been seen in commit ca489c58ef0b81 ("ARM: EXYNOS: Don't use LDREX > and STREX after disabling cache coherency" fix. ] ... and more. As for the checking in LSI, I did not receive any answer. To summarize: Fixing this with this patch is an optimal way in my humble opinion because: 1. Fixes the problem. 2. Does not introduce regression. 3. Does not remove other features (reverting commits would do). 4. It was tested. 5. We are in late RC and second suspend-to-RAM still does not work. Reverting commits or developing some kind of new patch (which details you did not propose) is asking for some other troubles. Best regards, Krzysztof
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h index f70eca7ee705..0ef8d4b47102 100644 --- a/arch/arm/mach-exynos/common.h +++ b/arch/arm/mach-exynos/common.h @@ -153,6 +153,8 @@ extern void exynos_enter_aftr(void); extern struct cpuidle_exynos_data cpuidle_coupled_exynos_data; +extern void exynos_set_delayed_reset_assertion(bool enable); + extern void s5p_init_cpu(void __iomem *cpuid_addr); extern unsigned int samsung_rev(void); extern void __iomem *cpu_boot_reg_base(void); diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c index 9e9dfdfad9d7..1081ff1f03c6 100644 --- a/arch/arm/mach-exynos/exynos.c +++ b/arch/arm/mach-exynos/exynos.c @@ -166,6 +166,33 @@ static void __init exynos_init_io(void) exynos_map_io(); } +/* + * Set or clear the USE_DELAYED_RESET_ASSERTION option. Used by smp code + * and suspend. + * + * This is necessary only on Exynos4 SoCs. When system is running + * USE_DELAYED_RESET_ASSERTION should be set so the ARM CLK clock down + * feature could properly detect global idle state when secondary CPU is + * powered down. + * + * However this should not be set when such system is going into suspend. + */ +void exynos_set_delayed_reset_assertion(bool enable) +{ + if (soc_is_exynos4()) { + unsigned int tmp, core_id; + + for (core_id = 0; core_id < num_possible_cpus(); core_id++) { + tmp = pmu_raw_readl(EXYNOS_ARM_CORE_OPTION(core_id)); + if (enable) + tmp |= S5P_USE_DELAYED_RESET_ASSERTION; + else + tmp &= ~(S5P_USE_DELAYED_RESET_ASSERTION); + pmu_raw_writel(tmp, EXYNOS_ARM_CORE_OPTION(core_id)); + } + } +} + static const struct of_device_id exynos_dt_pmu_match[] = { { .compatible = "samsung,exynos3250-pmu" }, { .compatible = "samsung,exynos4210-pmu" }, diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c index d2e9f12d12f1..d45e8cd23925 100644 --- a/arch/arm/mach-exynos/platsmp.c +++ b/arch/arm/mach-exynos/platsmp.c @@ -34,30 +34,6 @@ extern void exynos4_secondary_startup(void); -/* - * Set or clear the USE_DELAYED_RESET_ASSERTION option, set on Exynos4 SoCs - * during hot-(un)plugging CPUx. - * - * The feature can be cleared safely during first boot of secondary CPU. - * - * Exynos4 SoCs require setting USE_DELAYED_RESET_ASSERTION during powering - * down a CPU so the CPU idle clock down feature could properly detect global - * idle state when CPUx is off. - */ -static void exynos_set_delayed_reset_assertion(u32 core_id, bool enable) -{ - if (soc_is_exynos4()) { - unsigned int tmp; - - tmp = pmu_raw_readl(EXYNOS_ARM_CORE_OPTION(core_id)); - if (enable) - tmp |= S5P_USE_DELAYED_RESET_ASSERTION; - else - tmp &= ~(S5P_USE_DELAYED_RESET_ASSERTION); - pmu_raw_writel(tmp, EXYNOS_ARM_CORE_OPTION(core_id)); - } -} - #ifdef CONFIG_HOTPLUG_CPU static inline void cpu_leave_lowpower(u32 core_id) { @@ -73,8 +49,6 @@ static inline void cpu_leave_lowpower(u32 core_id) : "=&r" (v) : "Ir" (CR_C), "Ir" (0x40) : "cc"); - - exynos_set_delayed_reset_assertion(core_id, false); } static inline void platform_do_lowpower(unsigned int cpu, int *spurious) @@ -87,14 +61,6 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious) /* Turn the CPU off on next WFI instruction. */ exynos_cpu_power_down(core_id); - /* - * Exynos4 SoCs require setting - * USE_DELAYED_RESET_ASSERTION so the CPU idle - * clock down feature could properly detect - * global idle state when CPUx is off. - */ - exynos_set_delayed_reset_assertion(core_id, true); - wfi(); if (pen_release == core_id) { @@ -354,9 +320,6 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) udelay(10); } - /* No harm if this is called during first boot of secondary CPU */ - exynos_set_delayed_reset_assertion(core_id, false); - /* * now the secondary core is starting up let it run its * calibrations, then wait for it to finish @@ -403,6 +366,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) exynos_sysram_init(); + exynos_set_delayed_reset_assertion(true); + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) scu_enable(scu_base_addr()); diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c index 318d127df147..582ef2df960d 100644 --- a/arch/arm/mach-exynos/suspend.c +++ b/arch/arm/mach-exynos/suspend.c @@ -235,6 +235,8 @@ static void exynos_pm_enter_sleep_mode(void) static void exynos_pm_prepare(void) { + exynos_set_delayed_reset_assertion(false); + /* Set wake-up mask registers */ exynos_pm_set_wakeup_mask(); @@ -383,6 +385,7 @@ early_wakeup: /* Clear SLEEP mode set in INFORM1 */ pmu_raw_writel(0x0, S5P_INFORM1); + exynos_set_delayed_reset_assertion(true); } static void exynos3250_pm_resume(void)
On Exynos4412 boards (Trats2, Odroid U3) after enabling L2 cache in 56b60b8bce4a ("ARM: 8265/1: dts: exynos4: Add nodes for L2 cache controller") the second suspend to RAM failed. First suspend worked fine but the next one hang just after powering down of secondary CPUs (system consumed energy as it would be running but was not responsive). The issue was caused by enabling delayed reset assertion for CPU0 just after issuing power down of cores. This was introduced for Exynos4 in 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off"). The whole behavior is not well documented but after checking with vendor code this should be done like this (on Exynos4): 1. Enable delayed reset assertion when system is running (for all CPUs). 2. Disable delayed reset assertion before suspending the system. This can be done after powering off secondary CPUs. 3. Re-enable the delayed reset assertion when system is resumed. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Fixes: 13cfa6c4f7fa ("ARM: EXYNOS: Fix CPU idle clock down after CPU off") Cc: <stable@vger.kernel.org> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Tested-by: Chanwoo Choi <cw00.choi@samsung.com> --- Notes: Changes since v1: 1. Add Bartlomiej's and Chanwoo's tested-by. arch/arm/mach-exynos/common.h | 2 ++ arch/arm/mach-exynos/exynos.c | 27 +++++++++++++++++++++++++++ arch/arm/mach-exynos/platsmp.c | 39 ++------------------------------------- arch/arm/mach-exynos/suspend.c | 3 +++ 4 files changed, 34 insertions(+), 37 deletions(-)