* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-24 16:11 ` Russell King - ARM Linux
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 16:11 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
> Hi Kukjin,
>
> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Do you have any comments on this patch ?
I do.
> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> > index d10c351..6dd4a11 100644
> > --- a/arch/arm/mach-exynos/pm.c
> > +++ b/arch/arm/mach-exynos/pm.c
> > @@ -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();
...
> > @@ -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 ?
> > @@ -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;
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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-24 16:11 ` Russell King - ARM Linux
@ 2014-06-24 16:20 ` Russell King - ARM Linux
-1 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 16:20 UTC (permalink / raw)
To: Abhilash Kesavan
Cc: Kukjin Kim, Tomasz Figa, Daniel Lezcano, linux-samsung-soc,
Abhilash Kesavan, linux-arm-kernel
On Tue, Jun 24, 2014 at 05:11:14PM +0100, Russell King - ARM Linux wrote:
> 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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-24 16:20 ` Russell King - ARM Linux
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 16:20 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 24, 2014 at 05:11:14PM +0100, Russell King - ARM Linux wrote:
> 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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-24 16:11 ` Russell King - ARM Linux
@ 2014-06-24 16:20 ` Tomasz Figa
-1 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2014-06-24 16:20 UTC (permalink / raw)
To: Russell King - ARM Linux, Abhilash Kesavan
Cc: linux-samsung-soc, linux-arm-kernel, Kukjin Kim,
Abhilash Kesavan, Daniel Lezcano
Hi Russell,
On 24.06.2014 18:11, Russell King - ARM Linux wrote:
> On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
>> Hi Kukjin,
>>
>> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
>>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>
>> Do you have any comments on this patch ?
>
> I do.
>
>>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>>> index d10c351..6dd4a11 100644
>>> --- a/arch/arm/mach-exynos/pm.c
>>> +++ b/arch/arm/mach-exynos/pm.c
>>> @@ -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();
> ...
>>> @@ -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.
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?
>
> 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?
>
>>> @@ -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;
>
> 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.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-24 16:20 ` Tomasz Figa
0 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2014-06-24 16:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Russell,
On 24.06.2014 18:11, Russell King - ARM Linux wrote:
> On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
>> Hi Kukjin,
>>
>> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
>>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>
>> Do you have any comments on this patch ?
>
> I do.
>
>>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>>> index d10c351..6dd4a11 100644
>>> --- a/arch/arm/mach-exynos/pm.c
>>> +++ b/arch/arm/mach-exynos/pm.c
>>> @@ -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();
> ...
>>> @@ -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.
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?
>
> 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?
>
>>> @@ -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;
>
> 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.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-24 16:20 ` Tomasz Figa
@ 2014-06-24 16:30 ` Russell King - ARM Linux
-1 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 16:30 UTC (permalink / raw)
To: Tomasz Figa
Cc: Abhilash Kesavan, linux-samsung-soc, linux-arm-kernel,
Kukjin Kim, Abhilash Kesavan, Daniel Lezcano
On Tue, Jun 24, 2014 at 06:20:56PM +0200, Tomasz Figa wrote:
> Hi Russell,
>
> On 24.06.2014 18:11, Russell King - ARM Linux wrote:
> > On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
> >> Hi Kukjin,
> >>
> >> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
> >>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>
> >> Do you have any comments on this patch ?
> >
> > I do.
> >
> >>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> >>> index d10c351..6dd4a11 100644
> >>> --- a/arch/arm/mach-exynos/pm.c
> >>> +++ b/arch/arm/mach-exynos/pm.c
> >>> @@ -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();
> > ...
> >>> @@ -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.
>
> 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.
> > 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?
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.)
> > 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?
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-24 16:30 ` Russell King - ARM Linux
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 16:30 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 24, 2014 at 06:20:56PM +0200, Tomasz Figa wrote:
> Hi Russell,
>
> On 24.06.2014 18:11, Russell King - ARM Linux wrote:
> > On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
> >> Hi Kukjin,
> >>
> >> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
> >>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>
> >> Do you have any comments on this patch ?
> >
> > I do.
> >
> >>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> >>> index d10c351..6dd4a11 100644
> >>> --- a/arch/arm/mach-exynos/pm.c
> >>> +++ b/arch/arm/mach-exynos/pm.c
> >>> @@ -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();
> > ...
> >>> @@ -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.
>
> 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.
> > 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?
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.)
> > 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?
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-24 16:30 ` Russell King - ARM Linux
@ 2014-06-24 17:16 ` Tomasz Figa
-1 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2014-06-24 17:16 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Abhilash Kesavan, linux-samsung-soc, linux-arm-kernel,
Kukjin Kim, Abhilash Kesavan, Daniel Lezcano
On 24.06.2014 18:30, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 06:20:56PM +0200, Tomasz Figa wrote:
>> Hi Russell,
>>
>> On 24.06.2014 18:11, Russell King - ARM Linux wrote:
>>> On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
>>>> Hi Kukjin,
>>>>
>>>> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
>>>>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>>>
>>>> Do you have any comments on this patch ?
>>>
>>> I do.
>>>
>>>>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>>>>> index d10c351..6dd4a11 100644
>>>>> --- a/arch/arm/mach-exynos/pm.c
>>>>> +++ b/arch/arm/mach-exynos/pm.c
>>>>> @@ -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();
>>> ...
>>>>> @@ -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.
>>
>> 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.
>
> 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.
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.
>
>>> 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?
>
> 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.
>
> 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.)
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.
>
>>> 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.
Your point. Thanks for explaining this. However this will be still
running with just one CPU online.
>
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-24 17:16 ` Tomasz Figa
0 siblings, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2014-06-24 17:16 UTC (permalink / raw)
To: linux-arm-kernel
On 24.06.2014 18:30, Russell King - ARM Linux wrote:
> On Tue, Jun 24, 2014 at 06:20:56PM +0200, Tomasz Figa wrote:
>> Hi Russell,
>>
>> On 24.06.2014 18:11, Russell King - ARM Linux wrote:
>>> On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
>>>> Hi Kukjin,
>>>>
>>>> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
>>>>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>>>
>>>> Do you have any comments on this patch ?
>>>
>>> I do.
>>>
>>>>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>>>>> index d10c351..6dd4a11 100644
>>>>> --- a/arch/arm/mach-exynos/pm.c
>>>>> +++ b/arch/arm/mach-exynos/pm.c
>>>>> @@ -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();
>>> ...
>>>>> @@ -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.
>>
>> 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.
>
> 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.
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.
>
>>> 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?
>
> 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.
>
> 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.)
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.
>
>>> 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.
Your point. Thanks for explaining this. However this will be still
running with just one CPU online.
>
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-24 17:16 ` Tomasz Figa
@ 2014-06-24 18:12 ` Russell King - ARM Linux
-1 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 18:12 UTC (permalink / raw)
To: Tomasz Figa
Cc: Abhilash Kesavan, linux-samsung-soc, linux-arm-kernel,
Kukjin Kim, Abhilash Kesavan, Daniel Lezcano
On Tue, Jun 24, 2014 at 07:16:47PM +0200, Tomasz Figa wrote:
> 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.
> > 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.
>
> 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.
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).
> > 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.
> 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.
> > 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.
>
> Your point. Thanks for explaining this. However this will be still
> running with just one CPU online.
>
> >
> > 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().
... 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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-24 18:12 ` Russell King - ARM Linux
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-24 18:12 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 24, 2014 at 07:16:47PM +0200, Tomasz Figa wrote:
> 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.
> > 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.
>
> 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.
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).
> > 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.
> 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.
> > 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.
>
> Your point. Thanks for explaining this. However this will be still
> running with just one CPU online.
>
> >
> > 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().
... 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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-24 16:11 ` Russell King - ARM Linux
@ 2014-06-25 5:00 ` Abhilash Kesavan
-1 siblings, 0 replies; 24+ messages in thread
From: Abhilash Kesavan @ 2014-06-25 5:00 UTC (permalink / raw)
To: Russell King - ARM Linux, Arnd Bergmann
Cc: linux-samsung-soc, linux-arm-kernel, Kukjin Kim, Tomasz Figa,
Daniel Lezcano
Hi Russell and Tomasz,
+Arnd
On Tue, Jun 24, 2014 at 9:41 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
>> Hi Kukjin,
>>
>> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
>> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>
>> Do you have any comments on this patch ?
>
> I do.
>
>> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> > index d10c351..6dd4a11 100644
>> > --- a/arch/arm/mach-exynos/pm.c
>> > +++ b/arch/arm/mach-exynos/pm.c
>> > @@ -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();
> ...
>> > @@ -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.
Thanks for pointing this out. I was not aware of the implementor id
check requirement.
>
> 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 ?
>
>> > @@ -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;
>
> 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 ?
>
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-25 5:00 ` Abhilash Kesavan
0 siblings, 0 replies; 24+ messages in thread
From: Abhilash Kesavan @ 2014-06-25 5:00 UTC (permalink / raw)
To: linux-arm-kernel
Hi Russell and Tomasz,
+Arnd
On Tue, Jun 24, 2014 at 9:41 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
>> Hi Kukjin,
>>
>> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
>> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>
>> Do you have any comments on this patch ?
>
> I do.
>
>> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> > index d10c351..6dd4a11 100644
>> > --- a/arch/arm/mach-exynos/pm.c
>> > +++ b/arch/arm/mach-exynos/pm.c
>> > @@ -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();
> ...
>> > @@ -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.
Thanks for pointing this out. I was not aware of the implementor id
check requirement.
>
> 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 ?
>
>> > @@ -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;
>
> 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 ?
>
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread* RE: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-25 5:00 ` Abhilash Kesavan
@ 2014-06-25 11:02 ` Kukjin Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Kukjin Kim @ 2014-06-25 11:02 UTC (permalink / raw)
To: 'Abhilash Kesavan', 'Russell King - ARM Linux',
'Arnd Bergmann'
Cc: 'linux-samsung-soc', 'linux-arm-kernel',
'Tomasz Figa', 'Daniel Lezcano'
Abhilash Kesavan wrote:
>
> Hi Russell and Tomasz,
>
> +Arnd
> On Tue, Jun 24, 2014 at 9:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
> >> Hi Kukjin,
> >>
> >> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com>
> wrote:
> >> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>
> >> Do you have any comments on this patch ?
> >
> > I do.
> >
> >> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> >> > index d10c351..6dd4a11 100644
> >> > --- a/arch/arm/mach-exynos/pm.c
> >> > +++ b/arch/arm/mach-exynos/pm.c
> >> > @@ -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();
> > ...
> >> > @@ -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.
> Thanks for pointing this out. I was not aware of the implementor id
> check requirement.
> >
> > 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.
> >> > @@ -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;
> >
> > 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 ?
> >
> > 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 ?
>
Oops, Abhilash please send new fix on top of the current patch.
Thanks,
Kukjin
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-25 11:02 ` Kukjin Kim
0 siblings, 0 replies; 24+ messages in thread
From: Kukjin Kim @ 2014-06-25 11:02 UTC (permalink / raw)
To: linux-arm-kernel
Abhilash Kesavan wrote:
>
> Hi Russell and Tomasz,
>
> +Arnd
> On Tue, Jun 24, 2014 at 9:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
> >> Hi Kukjin,
> >>
> >> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@samsung.com>
> wrote:
> >> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>
> >> Do you have any comments on this patch ?
> >
> > I do.
> >
> >> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> >> > index d10c351..6dd4a11 100644
> >> > --- a/arch/arm/mach-exynos/pm.c
> >> > +++ b/arch/arm/mach-exynos/pm.c
> >> > @@ -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();
> > ...
> >> > @@ -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.
> Thanks for pointing this out. I was not aware of the implementor id
> check requirement.
> >
> > 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.
> >> > @@ -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;
> >
> > 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 ?
> >
> > 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 ?
>
Oops, Abhilash please send new fix on top of the current patch.
Thanks,
Kukjin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
2014-06-25 5:00 ` Abhilash Kesavan
@ 2014-06-25 11:18 ` Russell King - ARM Linux
-1 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-25 11:18 UTC (permalink / raw)
To: Abhilash Kesavan
Cc: Arnd Bergmann, linux-samsung-soc, linux-arm-kernel, Kukjin Kim,
Tomasz Figa, Daniel Lezcano
On Wed, Jun 25, 2014 at 10:30:46AM +0530, Abhilash Kesavan wrote:
> 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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC
@ 2014-06-25 11:18 ` Russell King - ARM Linux
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2014-06-25 11:18 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 25, 2014 at 10:30:46AM +0530, Abhilash Kesavan wrote:
> 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.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply [flat|nested] 24+ messages in thread