[PATCH] powerpc/85xx: Add back condition for smp

[PATCH] powerpc/85xx: Add back condition for smp

[York Sun]

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
-- 
1.7.0.4

Re: [PATCH] powerpc/85xx: Add back condition for smp

[Scott Wood]

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.

Re: [PATCH] powerpc/85xx: Add back condition for smp

[Kumar Gala]

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
>=20
>        .give_timebase  =3D smp_generic_give_timebase,
>        .take_timebase  =3D smp_generic_take_timebase,

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

>=20
> 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(-)
>=20
> 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 =3D {
> 	.cpu_disable	=3D generic_cpu_disable,
> 	.cpu_die	=3D generic_cpu_die,
> #endif
> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
> 	.give_timebase	=3D smp_generic_give_timebase,
> 	.take_timebase	=3D smp_generic_take_timebase,
> +#endif
> };
>=20
> #ifdef CONFIG_KEXEC
> --=20
> 1.7.0.4
>=20
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/85xx: Add back condition for smp

[Kumar Gala]

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
>>=20
>>        .give_timebase  =3D smp_generic_give_timebase,
>>        .take_timebase  =3D smp_generic_take_timebase,
>>=20
>> 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(-)
>>=20
>> 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 =3D {
>> 	.cpu_disable	=3D generic_cpu_disable,
>> 	.cpu_die	=3D generic_cpu_die,
>> #endif
>> +#if defined(CONFIG_KEXEC) || defined(CONFIG_HOTPLUG_CPU)
>> 	.give_timebase	=3D smp_generic_give_timebase,
>> 	.take_timebase	=3D smp_generic_take_timebase,
>> +#endif
>> };
>>=20
>> #ifdef CONFIG_KEXEC
>=20
> 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=

Re: [PATCH] powerpc/85xx: Add back condition for smp

[Scott Wood]

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