Patchwork powerpc/85xx: Add back condition for smp

login
register
mail settings
Submitter York Sun
Date April 17, 2012, 9:39 p.m.
Message ID <1334698746-11529-1-git-send-email-yorksun@freescale.com>
Download mbox | patch
Permalink /patch/153326/
State Changes Requested
Delegated to: Kumar Gala
Headers show

Comments

York Sun - April 17, 2012, 9:39 p.m.
The timebase synchronization is only necessary if we need to reset a
separate core.  Currently only KEXEC and CPU hotplug require resetting
a single core. The following code should be in the condition of
CONFIG_KEXEC or CONFIG_HOTPLUG_CPU

        .give_timebase  = smp_generic_give_timebase,
        .take_timebase  = smp_generic_take_timebase,

Signed-off-by: York Sun <yorksun@freescale.com>
Acked-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/platforms/85xx/smp.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Scott Wood - April 17, 2012, 10:17 p.m.
On 04/17/2012 04:39 PM, York Sun wrote:
> The timebase synchronization is only necessary if we need to reset a
> separate core.  Currently only KEXEC and CPU hotplug require resetting
> a single core. The following code should be in the condition of
> CONFIG_KEXEC or CONFIG_HOTPLUG_CPU
> 
>         .give_timebase  = smp_generic_give_timebase,
>         .take_timebase  = smp_generic_take_timebase,
> 
> Signed-off-by: York Sun <yorksun@freescale.com>
> Acked-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/platforms/85xx/smp.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
> index 56942af..868c6d7 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -192,8 +192,10 @@ struct smp_ops_t smp_85xx_ops = {
>  	.cpu_disable	= generic_cpu_disable,
>  	.cpu_die	= generic_cpu_die,
>  #endif
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
>  	.give_timebase	= smp_generic_give_timebase,
>  	.take_timebase	= smp_generic_take_timebase,
> +#endif
>  };
>  
>  #ifdef CONFIG_KEXEC

Note that this is only a temporary fix, that assumes the environments
where tbsync is problematic[1] (virtualization and simulation) do not
enable CONFIG_KEXEC or CONFIG_HOTPLUG_CPU.  Eventually the sync should
be done via CCSR like in U-Boot, and the decision on whether to do it
should be runtime.

-Scott

[1] More so than usual.
Kumar Gala - April 18, 2012, 1:37 p.m.
On Apr 17, 2012, at 4:39 PM, York Sun wrote:

> The timebase synchronization is only necessary if we need to reset a
> separate core.  Currently only KEXEC and CPU hotplug require resetting
> a single core. The following code should be in the condition of
> CONFIG_KEXEC or CONFIG_HOTPLUG_CPU
> 
>        .give_timebase  = smp_generic_give_timebase,
>        .take_timebase  = smp_generic_take_timebase,

This doesn't explain why you are putting the #ifdef back, only under what conditions it applies.

> 
> Signed-off-by: York Sun <yorksun@freescale.com>
> Acked-by: Li Yang <leoli@freescale.com>
> ---
> arch/powerpc/platforms/85xx/smp.c |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
> index 56942af..868c6d7 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -192,8 +192,10 @@ struct smp_ops_t smp_85xx_ops = {
> 	.cpu_disable	= generic_cpu_disable,
> 	.cpu_die	= generic_cpu_die,
> #endif
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
> 	.give_timebase	= smp_generic_give_timebase,
> 	.take_timebase	= smp_generic_take_timebase,
> +#endif
> };
> 
> #ifdef CONFIG_KEXEC
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
Kumar Gala - April 18, 2012, 2:15 p.m.
On Apr 17, 2012, at 5:17 PM, Scott Wood wrote:

> On 04/17/2012 04:39 PM, York Sun wrote:
>> The timebase synchronization is only necessary if we need to reset a
>> separate core.  Currently only KEXEC and CPU hotplug require resetting
>> a single core. The following code should be in the condition of
>> CONFIG_KEXEC or CONFIG_HOTPLUG_CPU
>> 
>>        .give_timebase  = smp_generic_give_timebase,
>>        .take_timebase  = smp_generic_take_timebase,
>> 
>> Signed-off-by: York Sun <yorksun@freescale.com>
>> Acked-by: Li Yang <leoli@freescale.com>
>> ---
>> arch/powerpc/platforms/85xx/smp.c |    2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
>> index 56942af..868c6d7 100644
>> --- a/arch/powerpc/platforms/85xx/smp.c
>> +++ b/arch/powerpc/platforms/85xx/smp.c
>> @@ -192,8 +192,10 @@ struct smp_ops_t smp_85xx_ops = {
>> 	.cpu_disable	= generic_cpu_disable,
>> 	.cpu_die	= generic_cpu_die,
>> #endif
>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
>> 	.give_timebase	= smp_generic_give_timebase,
>> 	.take_timebase	= smp_generic_take_timebase,
>> +#endif
>> };
>> 
>> #ifdef CONFIG_KEXEC
> 
> Note that this is only a temporary fix, that assumes the environments
> where tbsync is problematic[1] (virtualization and simulation) do not
> enable CONFIG_KEXEC or CONFIG_HOTPLUG_CPU.  Eventually the sync should
> be done via CCSR like in U-Boot, and the decision on whether to do it
> should be runtime.

Is the thinking with the CCSR like u-boot sync after boot would resync to some non-zero TB value?

I'm guessing the idea is doing such a rsync w/TB stopped in the system is quick enough that the freezing of it will not be noticed.  One should measure this and see what it looks like in core cycles and how it scales based on # of cores.  (could use alternate TB as a counter to measure).

- k
Scott Wood - April 18, 2012, 4:23 p.m.
On 04/18/2012 09:15 AM, Kumar Gala wrote:
> 
> On Apr 17, 2012, at 5:17 PM, Scott Wood wrote:
> 
>> On 04/17/2012 04:39 PM, York Sun wrote:
>>> The timebase synchronization is only necessary if we need to reset a
>>> separate core.  Currently only KEXEC and CPU hotplug require resetting
>>> a single core. The following code should be in the condition of
>>> CONFIG_KEXEC or CONFIG_HOTPLUG_CPU
>>>
>>>        .give_timebase  = smp_generic_give_timebase,
>>>        .take_timebase  = smp_generic_take_timebase,
>>>
>>> Signed-off-by: York Sun <yorksun@freescale.com>
>>> Acked-by: Li Yang <leoli@freescale.com>
>>> ---
>>> arch/powerpc/platforms/85xx/smp.c |    2 ++
>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
>>> index 56942af..868c6d7 100644
>>> --- a/arch/powerpc/platforms/85xx/smp.c
>>> +++ b/arch/powerpc/platforms/85xx/smp.c
>>> @@ -192,8 +192,10 @@ struct smp_ops_t smp_85xx_ops = {
>>> 	.cpu_disable	= generic_cpu_disable,
>>> 	.cpu_die	= generic_cpu_die,
>>> #endif
>>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
>>> 	.give_timebase	= smp_generic_give_timebase,
>>> 	.take_timebase	= smp_generic_take_timebase,
>>> +#endif
>>> };
>>>
>>> #ifdef CONFIG_KEXEC
>>
>> Note that this is only a temporary fix, that assumes the environments
>> where tbsync is problematic[1] (virtualization and simulation) do not
>> enable CONFIG_KEXEC or CONFIG_HOTPLUG_CPU.  Eventually the sync should
>> be done via CCSR like in U-Boot, and the decision on whether to do it
>> should be runtime.
> 
> Is the thinking with the CCSR like u-boot sync after boot would resync to some non-zero TB value?

Yes, while all timebases are stopped, the core coming out of reset will
be set to the timebase value of the other cores.

> I'm guessing the idea is doing such a rsync w/TB stopped in the
> system is quick enough that the freezing of it will not be noticed.

Quick enough is relative, but remember that the context is things like
kexec and deep sleep, which are already quite timing-intrusive.

> One should measure this and see what it looks like in core cycles and
> how it scales based on # of cores.  (could use alternate TB as a
> counter to measure).

I have a hard time imagining it being slower than the generic tbsync,
but in any case it's really the only sane option we have in cases where
we must reset individual cores -- I'm not sure how relevant such
benchmarking would be, other than "nice to know".

-Scott

Patch

diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 56942af..868c6d7 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -192,8 +192,10 @@  struct smp_ops_t smp_85xx_ops = {
 	.cpu_disable	= generic_cpu_disable,
 	.cpu_die	= generic_cpu_die,
 #endif
+#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
 	.give_timebase	= smp_generic_give_timebase,
 	.take_timebase	= smp_generic_take_timebase,
+#endif
 };
 
 #ifdef CONFIG_KEXEC