arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
We have an soc check to ensure that the scu and certain A9 specific registers are not accessed on Exynos5250 (which is A15 based). Rather than adding another soc specific check for 5420 let us test for the Cortex A9 primary part number.
This resolves the below crash seen on exynos5420 during core switching after the CPUIdle consolidation series was merged.
[ 155.975589] [<c0013174>] (scu_enable) from [<c001b0dc>] (exynos_cpu_pm_notifier+0x80/0xc4) [ 155.983833] [<c001b0dc>] (exynos_cpu_pm_notifier) from [<c003c1b0>] (notifier_call_chain+0x44/0x84) [ 155.992851] [<c003c1b0>] (notifier_call_chain) from [<c007a49c>] (cpu_pm_notify+0x20/0x3c) [ 156.001089] [<c007a49c>] (cpu_pm_notify) from [<c007a564>] (cpu_pm_exit+0x20/0x38) [ 156.008635] [<c007a564>] (cpu_pm_exit) from [<c0019e98>] (bL_switcher_thread+0x298/0x40c) [ 156.016788] [<c0019e98>] (bL_switcher_thread) from [<c003842c>] (kthread+0xcc/0xe8) [ 156.024426] [<c003842c>] (kthread) from [<c000e438>] (ret_from_fork+0x14/0x3c) [ 156.031621] Code: ea017fec c0530a00 c052e3f8 c0012dcc (e5903000
--- arch/arm/mach-exynos/pm.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
arch/arm/mach-exynos/pm.c +9 -6
@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();return 0 ;
@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup; -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();/* For release retention */
@@ -353,7 +353,7 @@ static void exynos_pm_resume(void)s3c_pm_do_restore_core (exynos_core_save,ARRAY_SIZE (exynos_core_save)); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:
@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () == +ARM_CPU_PART_CORTEX_A9 ) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ;
Signature · 1 line
1.7.9.5
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Hi Kukjin,
Quoted 17 lines · Abhilash Kesavan
We have an soc check to ensure that the scu and certain A9 specific registers are not accessed on Exynos5250 (which is A15 based). Rather than adding another soc specific check for 5420 let us test for the Cortex A9 primary part number.
This resolves the below crash seen on exynos5420 during core switching after the CPUIdle consolidation series was merged.
[ 155.975589] [<c0013174>] (scu_enable) from [<c001b0dc>] (exynos_cpu_pm_notifier+0x80/0xc4) [ 155.983833] [<c001b0dc>] (exynos_cpu_pm_notifier) from [<c003c1b0>] (notifier_call_chain+0x44/0x84) [ 155.992851] [<c003c1b0>] (notifier_call_chain) from [<c007a49c>] (cpu_pm_notify+0x20/0x3c) [ 156.001089] [<c007a49c>] (cpu_pm_notify) from [<c007a564>] (cpu_pm_exit+0x20/0x38) [ 156.008635] [<c007a564>] (cpu_pm_exit) from [<c0019e98>] (bL_switcher_thread+0x298/0x40c) [ 156.016788] [<c0019e98>] (bL_switcher_thread) from [<c003842c>] (kthread+0xcc/0xe8) [ 156.024426] [<c003842c>] (kthread) from [<c000e438>] (ret_from_fork+0x14/0x3c) [ 156.031621] Code: ea017fec c0530a00 c052e3f8 c0012dcc (e5903000
Quoted 1 line
Signed-off-by Abhilash Kesavan <[email protected]>
Do you have any comments on this patch ?
Regards,
Abhilash
Quoted 65 lines
--- arch/arm/mach-exynos/pm.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)arch/arm/mach-exynos/pm.c +9
-7@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();return 0 ;@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup; -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();/* For release retention */ @@ -353,7 +353,7 @@ static void exynos_pm_resume(void)s3c_pm_do_restore_core (exynos_core_save,ARRAY_SIZE (exynos_core_save)); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () == +ARM_CPU_PART_CORTEX_A9 ) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ; --1.7.9.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Quoted 6 lines · Abhilash Kesavan
Hi Kukjin,
Quoted 1 line · Abhilash Kesavan
Signed-off-by Abhilash Kesavan <[email protected]>Do you have any comments on this patch ?
I do.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();
...
Quoted 7 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 338
Quoted 7 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup; -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();
It is invalid to check just the part number. The part number on its own is meaningless without taking account of the implementor. Both the implementor and the part number should be checked at each of these sites.
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in proc-v7.S ?
Quoted 19 lines
Quoted 19 lines
arch/arm/mach-exynos/pm.c +3
-2@@ -353,7 +353,7 @@ static void exynos_pm_resume(void)s3c_pm_do_restore_core (exynos_core_save,ARRAY_SIZE (exynos_core_save)); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;
Quoted 12 lines
arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct notifier_block *self, · line 457
Quoted 12 lines
arch/arm/mach-exynos/pm.c +4
-2case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () == +ARM_CPU_PART_CORTEX_A9 ) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ;
And presumably with the CPU level code dealing with those registers, you don't need the calls to save and restore these registers in this notifier.
Which, by the way, is probably illegal to run as it runs in a read- lock code path and with the SCU disabled. As you're calling scu_enable() that means you're non-coherent with the other CPUs, and therefore locks don't work.
I think this code is very broken and wrongly architected, and shows that we're continuing to make the same mistakes that we made all through the 2000s with platforms doing their own crap rather than properly thinking about this stuff.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Quoted 7 lines · Russell King - ARM Linux
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in
proc-v7.S?
BTW, Shawn Guo recently posted a patch to add the diagnostic register save/restore to proc-v7.S.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
In reply to Russell King - ARM LinuxHi Russell,
Quoted 26 lines · Russell King - ARM Linux
Quoted 6 lines · Abhilash Kesavan
Hi Kukjin,
Quoted 1 line · Abhilash Kesavan
Signed-off-by Abhilash Kesavan <[email protected]>Do you have any comments on this patch ?
I do.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();...
Quoted 4 lines
Quoted 4 lines
arch/arm/mach-exynos/pm.c +0
-0@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup;
Quoted 8 lines
Quoted 3 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 338
Quoted 3 lines
arch/arm/mach-exynos/pm.c +1
-1-if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();It is invalid to check just the part number. The part number on its own is meaningless without taking account of the implementor. Both the implementor and the part number should be checked at each of these sites.
Just out of curiosity, are you aware of more than one implementor of Cortex A9 on Exynos SoCs that would differ in having the need for save and restore of those registers?
Quoted 8 lines
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in
proc-v7.S?
I agree that there is nothing platform specific in saving and restoring those registers and that this should be probably handled by generic code.
However, when running in non-secure world, the only way to restore this is to call a firmware operation, which is platform specific. Is there a way to do something like this from proc-v7.S?
Quoted 37 lines
Quoted 20 lines
Quoted 20 lines
arch/arm/mach-exynos/pm.c +3
-2@@ -353,7 +353,7 @@ static void exynos_pm_resume(void)s3c_pm_do_restore_core (exynos_core_save,ARRAY_SIZE (exynos_core_save)); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;Quoted 12 lines
arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct notifier_block *self, · line 457
Quoted 12 lines
arch/arm/mach-exynos/pm.c +4
-2case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () == +ARM_CPU_PART_CORTEX_A9 ) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ;And presumably with the CPU level code dealing with those registers, you don't need the calls to save and restore these registers in this notifier.
Quoted 4 lines
Which, by the way, is probably illegal to run as it runs in a read- lock code path and with the SCU disabled. As you're calling
scu_enable()that means you're non-coherent with the other CPUs, and therefore locks don't work.
I don't see the read lock code path you mention. Could you elaborate on this? By the way, other CPUs are still offline at this point.
Best regards,
Tomasz
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Quoted 38 lines · Tomasz Figa
Hi Russell,
Quoted 26 lines · Russell King - ARM Linux
Quoted 6 lines · Abhilash Kesavan
Hi Kukjin,
Quoted 1 line · Abhilash Kesavan
Signed-off-by Abhilash Kesavan <[email protected]>Do you have any comments on this patch ?
I do.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();...
Quoted 4 lines
Quoted 4 lines
arch/arm/mach-exynos/pm.c +0
-0@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup;Quoted 8 lines
Quoted 3 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 338
Quoted 3 lines
arch/arm/mach-exynos/pm.c +1
-1-if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();It is invalid to check just the part number. The part number on its own is meaningless without taking account of the implementor. Both the implementor and the part number should be checked at each of these sites.
Quoted 3 lines
Just out of curiosity, are you aware of more than one implementor of Cortex A9 on Exynos SoCs that would differ in having the need for save and restore of those registers?
That doesn't stop stuff changing in the future. We've been here before. Let's do the job properly if we're going to do the job at all.
If people still whine about this, I will force a change to make it harder to do the wrong thing - I will get rid of the _part_number interface replacing it with one which always returns the implementor as well as the part number so both have to be checked.
Quoted 8 lines
Quoted 7 lines
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in
proc-v7.S?
Quoted 6 lines
I agree that there is nothing platform specific in saving and restoring those registers and that this should be probably handled by generic code.
However, when running in non-secure world, the only way to restore this is to call a firmware operation, which is platform specific. Is there a way to do something like this from
proc-v7.S?
We never call firmware operations from assembly code. However, in exynos' case, it's not running in non-secure mode because it's happily reading and writing these registers with no issue.
Platforms running in non-secure mode already have to ensure that various work-arounds are implemented in their firmware/boot loader, and this really is no different (we wouldn't need this code in the kernel in the first place if the firmware/boot loader would get its act together.)
Quoted 11 lines
Quoted 8 lines
And presumably with the CPU level code dealing with those registers, you don't need the calls to save and restore these registers in this notifier.
Which, by the way, is probably illegal to run as it runs in a read- lock code path and with the SCU disabled. As you're calling
scu_enable()that means you're non-coherent with the other CPUs, and therefore locks don't work.I don't see the read lock code path you mention. Could you elaborate on this? By the way, other CPUs are still offline at this point.
We get there from kernel/cpu_pm.c, when the notifier chain is called. The notifier chain is called while taking a read lock on cpu_pm_notifier_lock.
If this is about the last CPU going down, then why the notifier? Why not put the code in exynos_suspend_enter() ? Why add this needless complexity?
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Quoted 39 lines · Russell King - ARM Linux
Quoted 38 lines · Tomasz Figa
Hi Russell,
Quoted 34 lines · Russell King - ARM Linux
Quoted 6 lines · Abhilash Kesavan
Hi Kukjin,
Quoted 1 line · Abhilash Kesavan
Signed-off-by Abhilash Kesavan <[email protected]>Do you have any comments on this patch ?
I do.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();...
Quoted 7 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 338
Quoted 7 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup; -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();It is invalid to check just the part number. The part number on its own is meaningless without taking account of the implementor. Both the implementor and the part number should be checked at each of these sites.
Quoted 6 lines
Quoted 3 lines
Just out of curiosity, are you aware of more than one implementor of Cortex A9 on Exynos SoCs that would differ in having the need for save and restore of those registers?
That doesn't stop stuff changing in the future. We've been here before. Let's do the job properly if we're going to do the job at all.
I tend to disagree. The chance of a new Cortex A9 based SoC with different implementor code showing up is so close to zero that I don't see increasing of code complexity by adding yet another check justified.
Quoted 5 lines
If people still whine about this, I will force a change to make it harder to do the wrong thing - I will get rid of the
_part_numberinterface replacing it with one which always returns the implementor as well as the part number so both have to be checked.
That might be actually a better option. Something like
if (read_cpuid_impl_and_part() == ARM_CPU(impl, part))
or even
if (ARM_CPU_IS(impl, part))
would keep the checks simple, while still checking both values.
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in
proc-v7.S?I agree that there is nothing platform specific in saving and restoring those registers and that this should be probably handled by generic code.
Quoted 7 lines
Quoted 3 lines
However, when running in non-secure world, the only way to restore this is to call a firmware operation, which is platform specific. Is there a way to do something like this from
proc-v7.S?We never call firmware operations from assembly code. However, in exynos' case, it's not running in non-secure mode because it's happily reading and writing these registers with no issue.
Current version of code, yes. However it handles only Exynos-based boards running in secure mode. For those running in non-secure mode, mainline does not support sleep yet.
I already have patches to change this, which I was planning to post after eliminating last issue. The change set includes making this save/restore being executed only in secure mode.
Quoted 5 lines
Platforms running in non-secure mode already have to ensure that various work-arounds are implemented in their
firmware/bootloader, and this really is no different (we wouldn't need this code in the kernel in the first place if thefirmware/bootloader would get its act together.)
In ideal world (which I would be glad to be living in)...
We both know that we can't fully rely on firmware. Firmware bugs are common and in many cases we can't do anything about it, because it already comes with the device and in many cases it is undesirable to change it or it can't be done at all.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
And presumably with the CPU level code dealing with those registers, you don't need the calls to save and restore these registers in this notifier.
Which, by the way, is probably illegal to run as it runs in a read- lock code path and with the SCU disabled. As you're calling
scu_enable()that means you're non-coherent with the other CPUs, and therefore locks don't work.
Quoted 6 lines
Quoted 2 lines
I don't see the read lock code path you mention. Could you elaborate on this? By the way, other CPUs are still offline at this point.
We get there from
kernel/cpu_pm.c, when the notifier chain is called. The notifier chain is called while taking a read lock oncpu_pm_notifier_lock.
Your point. Thanks for explaining this. However this will be still running with just one CPU online.
Quoted 5 lines
If this is about the last CPU going down, then why the notifier? Why not put the code in
exynos_suspend_enter()? Why add this needless complexity?
This code is used by both system-wide suspend/resume and cpuidle paths. Daniel has moved this code to CPU PM notifier as a part of his Exynos cpuidle consolidation series to avoid code duplication, as this is the common point of both paths.
To clarify, on suspend/resume path, the notifier is being called from syscore_ops registered in kernel/cpu_pm.c, while on cpuidle path, this is invoked from exynos_enter_core0_aftr() in drivers/cpuidle/cpuidle-exynos.c, which calls cpu_pm_enter().
Best regards,
Tomasz
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Quoted 3 lines · Tomasz Figa
I tend to disagree. The chance of a new Cortex A9 based SoC with different implementor code showing up is so close to zero that I don't see increasing of code complexity by adding yet another check justified.
That's your opinion which I strongly disagree with.
Quoted 7 lines
Quoted 4 lines
If people still whine about this, I will force a change to make it harder to do the wrong thing - I will get rid of the
_part_numberinterface replacing it with one which always returns the implementor as well as the part number so both have to be checked.That might be actually a better option. Something like
Quoted 7 lines
if (read_cpuid_impl_and_part() == ARM_CPU(impl, part))or even
if (ARM_CPU_IS(impl, part))would keep the checks simple, while still checking both values.
Indeed, but... Cortex is an ARM Ltd trademark, so I really doubt that we'll see a Cortex processor implemented by another party other than ARM. So, there's no need for ARM_CPU(impl, part).
Quoted 11 lines
Quoted 3 lines
We never call firmware operations from assembly code. However, in exynos' case, it's not running in non-secure mode because it's happily reading and writing these registers with no issue.
Current version of code, yes. However it handles only Exynos-based boards running in secure mode. For those running in non-secure mode, mainline does not support sleep yet.
I already have patches to change this, which I was planning to post after eliminating last issue. The change set includes making this save/restore being executed only in secure mode.
As Will has already pointed out, writing to the diagnostic register becomes a no-op in non-secure mode - it doesn't fault. So moving the saving and restoring of this register into generic code, where other platforms already require it, makes total sense.
Of course, when you have to deal with it in non-secure mode, that's something that you have to deal with, but in secure mode, platform code should not have to worry about this level of detail.
Quoted 6 lines
In ideal world (which I would be glad to be living in)...
We both know that we can't fully rely on firmware. Firmware bugs are common and in many cases we can't do anything about it, because it already comes with the device and in many cases it is undesirable to change it or it can't be done at all.
Yes, I'm aware of the crap situation there. That doesn't stop us talking about these aspects though and setting what we expect in the future - and changing the code to a better structure.
Quoted 18 lines
Quoted 3 lines
We get there from
kernel/cpu_pm.c, when the notifier chain is called. The notifier chain is called while taking a read lock oncpu_pm_notifier_lock.Your point. Thanks for explaining this. However this will be still running with just one CPU online.
Quoted 5 lines
If this is about the last CPU going down, then why the notifier? Why not put the code in
exynos_suspend_enter()? Why add this needless complexity?This code is used by both system-wide suspend/resume and cpuidle paths. Daniel has moved this code to CPU PM notifier as a part of his Exynos cpuidle consolidation series to avoid code duplication, as this is the common point of both paths.
Quoted 4 lines
To clarify, on suspend/resume path, the notifier is being called from
syscore_opsregistered inkernel/cpu_pm.c, while on cpuidle path, this is invoked fromexynos_enter_core0_aftr()indrivers/cpuidle/cpuidle-exynos.c, which callscpu_pm_enter().
... which then goes on to call cpu_suspend(). So moving this stuff into the CPU level makes total sense rather than having every platform running in secure mode duplicating this functionality. Thanks for pointing that out and adding further justification to my assertion.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
In reply to Russell King - ARM LinuxHi Russell and Tomasz,
+Arnd
Quoted 26 lines · Russell King - ARM Linux
Quoted 6 lines · Abhilash Kesavan
Hi Kukjin,
Quoted 1 line · Abhilash Kesavan
Signed-off-by Abhilash Kesavan <[email protected]>Do you have any comments on this patch ?
I do.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();...
Quoted 4 lines
Quoted 4 lines
arch/arm/mach-exynos/pm.c +0
-0@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup;
Quoted 8 lines
Quoted 3 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 338
Quoted 3 lines
arch/arm/mach-exynos/pm.c +1
-1-if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();It is invalid to check just the part number. The part number on its own is meaningless without taking account of the implementor. Both the implementor and the part number should be checked at each of these sites.
Thanks for pointing this out. I was not aware of the implementor id check requirement.
Quoted 28 lines
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in
proc-v7.S?Quoted 19 lines
Quoted 19 lines
arch/arm/mach-exynos/pm.c +3
-2@@ -353,7 +353,7 @@ static void exynos_pm_resume(void)s3c_pm_do_restore_core (exynos_core_save,ARRAY_SIZE (exynos_core_save)); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;
Quoted 16 lines
Quoted 4 lines
Quoted 4 lines
arch/arm/mach-exynos/pm.c +1
-1case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) + if (read_cpuid_part_number() ==Quoted 8 lines
arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct notifier_block *self, · line 457
Quoted 8 lines
arch/arm/mach-exynos/pm.c +3
-1+ ARM_CPU_PART_CORTEX_A9) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ;And presumably with the CPU level code dealing with those registers, you don't need the calls to save and restore these registers in this notifier.
Regarding save/restore of these registers, I could send out a patch cleaning these out once Shawn's patch gets merged. I'd need some help testing it out on Exynos4 boards though. For now, is it OK if I just update to the new function ?
Quoted 10 lines
Which, by the way, is probably illegal to run as it runs in a read- lock code path and with the SCU disabled. As you're calling
scu_enable()that means you're non-coherent with the other CPUs, and therefore locks don't work.I think this code is very broken and wrongly architected, and shows that we're continuing to make the same mistakes that we made all through the 2000s with platforms doing their own crap rather than properly thinking about this stuff.
I see that you have sent a patch out that ensures both part and implementor number are checked. Currently, my patch has been applied to the fixes branch of the arm-soc tree and I wanted to know how to proceed (without it there is a crash on the 5420):
Should I request Arnd to drop it (if possible) and send out a new patch using your updated function ?
Regards,
Abhilash
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
Quoted 41 lines · Abhilash Kesavan
Hi Russell and Tomasz,
+Arnd
Quoted 4 lines · Russell King - ARM Linux
Quoted 3 lines · Abhilash Kesavan
Hi Kukjin,
On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <[email protected]>
Quoted 22 lines · wrote:
Quoted 3 lines
Quoted 1 line
Signed-off-by Abhilash Kesavan <[email protected]>Do you have any comments on this patch ?
I do.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1
-1@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();...
Quoted 4 lines
Quoted 4 lines
arch/arm/mach-exynos/pm.c +0
-0@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup;Quoted 8 lines
Quoted 3 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 338
Quoted 3 lines
arch/arm/mach-exynos/pm.c +1
-1-if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();It is invalid to check just the part number. The part number on its own is meaningless without taking account of the implementor. Both the implementor and the part number should be checked at each of these sites.
Quoted 11 lines
Thanks for pointing this out. I was not aware of the implementor id check requirement.
Quoted 9 lines
Another point: exynos have taken it upon themselves to add code which saves various ARM core registers. This is a bad idea, it brings us back to the days where every platform did their own suspend implementations.
CPU level registers should be handled by CPU level code, not by platform code. Is there a reason why this can't be added to the Cortex-A9 support code in
proc-v7.S?
Got it. Thanks for pointing out.
Quoted 51 lines
Quoted 3 lines
Quoted 3 lines
arch/arm/mach-exynos/pm.c static void exynos_pm_resume(void) · line 354
Quoted 3 lines
arch/arm/mach-exynos/pm.c +0
-0@@ -353,7 +353,7 @@ static void exynos_pm_resume(void) s3c_pm_do_restore_core(exynos_core_save,
ARRAY_SIZE(exynos_core_save));Quoted 7 lines
Quoted 7 lines
arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct
Quoted 7 lines
arch/arm/mach-exynos/pm.c +1
-1-if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct
notifier_block*self,Quoted 22 lines
Quoted 9 lines
Quoted 9 lines
arch/arm/mach-exynos/pm.c +2
-1case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;Quoted 12 lines
arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct notifier_block *self, · line 457
Quoted 12 lines
arch/arm/mach-exynos/pm.c +4
-2case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () == +ARM_CPU_PART_CORTEX_A9 ) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ;Quoted 3 lines
And presumably with the CPU level code dealing with those registers, you don't need the calls to save and restore these registers in this notifier.
Regarding save/restore of these registers, I could send out a patch cleaning these out once Shawn's patch gets merged. I'd need some help testing it out on Exynos4 boards though. For now, is it OK if I just update to the new function ?
Quoted 10 lines
Which, by the way, is probably illegal to run as it runs in a read- lock code path and with the SCU disabled. As you're calling
scu_enable()that means you're non-coherent with the other CPUs, and therefore locks don't work.I think this code is very broken and wrongly architected, and shows that we're continuing to make the same mistakes that we made all through the 2000s with platforms doing their own crap rather than properly thinking about this stuff.
Quoted 7 lines
I see that you have sent a patch out that ensures both part and implementor number are checked. Currently, my patch has been applied to the fixes branch of the arm-soc tree and I wanted to know how to proceed (without it there is a crash on the 5420):
Should I request Arnd to drop it (if possible) and send out a new patch using your updated function ?
Oops, Abhilash please send new fix on top of the current patch.
Thanks,
Kukjin
PATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
In reply to Abhilash KesavanQuoted 6 lines · Abhilash Kesavan
I see that you have sent a patch out that ensures both part and implementor number are checked. Currently, my patch has been applied to the fixes branch of the arm-soc tree and I wanted to know how to proceed (without it there is a crash on the 5420):
Should I request Arnd to drop it (if possible) and send out a new patch using your updated function ?
I'll hold my patch off - as yours is in the fixes branch, it should end up in a -rc release. Mine isn't -rc material.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
Kukjin Kim
AppliedPATCH 1/1 arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
In reply to Abhilash KesavanQuoted 81 lines · Abhilash Kesavan
We have an soc check to ensure that the scu and certain A9 specific registers are not accessed on Exynos5250 (which is A15 based). Rather than adding another soc specific check for 5420 let us test for the Cortex A9 primary part number.
This resolves the below crash seen on exynos5420 during core switching after the CPUIdle consolidation series was merged.
[ 155.975589] [<c0013174>] (scu_enable) from [<c001b0dc>] (exynos_cpu_pm_notifier+0x80/0xc4) [ 155.983833] [<c001b0dc>] (exynos_cpu_pm_notifier) from [<c003c1b0>] (notifier_call_chain+0x44/0x84) [ 155.992851] [<c003c1b0>] (notifier_call_chain) from [<c007a49c>] (cpu_pm_notify+0x20/0x3c) [ 156.001089] [<c007a49c>] (cpu_pm_notify) from [<c007a564>] (cpu_pm_exit+0x20/0x38) [ 156.008635] [<c007a564>] (cpu_pm_exit) from [<c0019e98>] (bL_switcher_thread+0x298/0x40c) [ 156.016788] [<c0019e98>] (bL_switcher_thread) from [<c003842c>] (kthread+0xcc/0xe8) [ 156.024426] [<c003842c>] (kthread) from [<c000e438>] (ret_from_fork+0x14/0x3c) [ 156.031621] Code: ea017fec c0530a00 c052e3f8 c0012dcc (e5903000Signed-off-by Abhilash Kesavan <[email protected]>--- arch/arm/mach-exynos/pm.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)arch/arm/mach-exynos/pm.c +9
-7@@ -300,7 +300,7 @@ static int exynos_pm_suspend(void) tmp = (S5P_USE_STANDBY_WFI0 |S5P_USE_STANDBY_WFE0 );__raw_writel (tmp,S5P_CENTRAL_SEQ_OPTION ); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_save_register ();return 0 ;@@ -334,7 +334,7 @@ static void exynos_pm_resume(void)if (exynos_pm_central_resume ()) goto early_wakeup; -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )exynos_cpu_restore_register ();/* For release retention */ @@ -353,7 +353,7 @@ static void exynos_pm_resume(void)s3c_pm_do_restore_core (exynos_core_save,ARRAY_SIZE (exynos_core_save)); -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (S5P_VA_SCU ); early_wakeup:@@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,case CPU_PM_ENTER :if (cpu ==0 ) {exynos_pm_central_suspend (); -exynos_cpu_save_register (); +if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 ) +exynos_cpu_save_register (); }break ;case CPU_PM_EXIT :if (cpu ==0 ) { -if (!soc_is_exynos5250 ()) +if (read_cpuid_part_number () == +ARM_CPU_PART_CORTEX_A9 ) {scu_enable (S5P_VA_SCU ); -exynos_cpu_restore_register (); +exynos_cpu_restore_register (); + }exynos_pm_central_resume (); }break ; --
Quoted 1 line
1.7.9.5
Yes, looks good to me.
I've applied this into fixes for 3.16.
Thanks,
Kukjin