[PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Zhao Chenhui]

From: Li Yang <leoli@freescale.com>

The timebase sync is not only necessary when using KEXEC. It should also
be used by normal boot up and cpu hotplug. Remove the ifdef added by
the KEXEC patch.

Signed-off-by: Jin Qing <b24347@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/platforms/85xx/smp.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 5b9b901..9b0de9c 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -113,10 +113,8 @@ smp_85xx_kick_cpu(int nr)
 
 struct smp_ops_t smp_85xx_ops = {
 	.kick_cpu = smp_85xx_kick_cpu,
-#ifdef CONFIG_KEXEC
 	.give_timebase	= smp_generic_give_timebase,
 	.take_timebase	= smp_generic_take_timebase,
-#endif
 };
 
 #ifdef CONFIG_KEXEC
-- 
1.6.4.1

Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Scott Wood]

On 11/04/2011 07:29 AM, Zhao Chenhui wrote:
> From: Li Yang <leoli@freescale.com>
> 
> The timebase sync is not only necessary when using KEXEC. It should also
> be used by normal boot up and cpu hotplug. Remove the ifdef added by
> the KEXEC patch.

The KEXEC patch didn't just add the ifdef, it also added the initializers:

> @@ -105,8 +107,64 @@ smp_85xx_setup_cpu(int cpu_nr)
>  
>  struct smp_ops_t smp_85xx_ops = {
>         .kick_cpu = smp_85xx_kick_cpu,
> +#ifdef CONFIG_KEXEC
> +       .give_timebase  = smp_generic_give_timebase,
> +       .take_timebase  = smp_generic_take_timebase,
> +#endif
>  };

U-Boot synchronizes the timebase on 85xx.  With what chip and U-Boot
version are you seeing this not happen?

If you are seeing only a small (around one tick) difference, make sure
you're running a U-Boot that has this commit:

> commit 7afc45ad7d9493208d89072cbb78a5bfc8034b59
> Author: Kumar Gala <galak@kernel.crashing.org>
> Date:   Sun Mar 13 10:55:53 2011 -0500
> 
>     powerpc/85xx: Fix synchronization of timebase on MP boot
>     
>     There is a small ordering issue in the master core in that we need to
>     make sure the disabling of the timebase in the SoC is visible before we
>     set the value to 0.  We can simply just read back the value to
>     synchronizatize the write, before we set TB to 0.
>     
>     Reported-by: Dan Hettena
>     Tested-by: Dan Hettena
>     Signed-off-by: Kumar Gala <galak@kernel.crashing.org>


-Scott

Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Kumar Gala]

On Nov 4, 2011, at 12:33 PM, Scott Wood wrote:

> On 11/04/2011 07:29 AM, Zhao Chenhui wrote:
>> From: Li Yang <leoli@freescale.com>
>> 
>> The timebase sync is not only necessary when using KEXEC. It should also
>> be used by normal boot up and cpu hotplug. Remove the ifdef added by
>> the KEXEC patch.
> 
> The KEXEC patch didn't just add the ifdef, it also added the initializers:
> 
>> @@ -105,8 +107,64 @@ smp_85xx_setup_cpu(int cpu_nr)
>> 
>> struct smp_ops_t smp_85xx_ops = {
>>        .kick_cpu = smp_85xx_kick_cpu,
>> +#ifdef CONFIG_KEXEC
>> +       .give_timebase  = smp_generic_give_timebase,
>> +       .take_timebase  = smp_generic_take_timebase,
>> +#endif
>> };
> 
> U-Boot synchronizes the timebase on 85xx.  With what chip and U-Boot
> version are you seeing this not happen?
> 
> If you are seeing only a small (around one tick) difference, make sure
> you're running a U-Boot that has this commit:
> 
>> commit 7afc45ad7d9493208d89072cbb78a5bfc8034b59
>> Author: Kumar Gala <galak@kernel.crashing.org>
>> Date:   Sun Mar 13 10:55:53 2011 -0500
>> 
>>    powerpc/85xx: Fix synchronization of timebase on MP boot
>> 
>>    There is a small ordering issue in the master core in that we need to
>>    make sure the disabling of the timebase in the SoC is visible before we
>>    set the value to 0.  We can simply just read back the value to
>>    synchronizatize the write, before we set TB to 0.
>> 
>>    Reported-by: Dan Hettena
>>    Tested-by: Dan Hettena
>>    Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

Scott,

Aren't we going to need this when a core is woken back up w/o any state?

- k

Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Scott Wood]

On 11/04/2011 02:33 PM, Kumar Gala wrote:
> 
> On Nov 4, 2011, at 12:33 PM, Scott Wood wrote:
> 
>> On 11/04/2011 07:29 AM, Zhao Chenhui wrote:
>>> From: Li Yang <leoli@freescale.com>
>>>
>>> The timebase sync is not only necessary when using KEXEC. It should also
>>> be used by normal boot up and cpu hotplug. Remove the ifdef added by
>>> the KEXEC patch.
>>
>> The KEXEC patch didn't just add the ifdef, it also added the initializers:
>>
>>> @@ -105,8 +107,64 @@ smp_85xx_setup_cpu(int cpu_nr)
>>>
>>> struct smp_ops_t smp_85xx_ops = {
>>>        .kick_cpu = smp_85xx_kick_cpu,
>>> +#ifdef CONFIG_KEXEC
>>> +       .give_timebase  = smp_generic_give_timebase,
>>> +       .take_timebase  = smp_generic_take_timebase,
>>> +#endif
>>> };
>>
>> U-Boot synchronizes the timebase on 85xx.  With what chip and U-Boot
>> version are you seeing this not happen?
>>
>> If you are seeing only a small (around one tick) difference, make sure
>> you're running a U-Boot that has this commit:
[snip]
> 
> Scott,
> 
> Aren't we going to need this when a core is woken back up w/o any state?

We'll need some form of timebase resync if a core is individually
hard-reset -- I was responding to the "should also be used by normal
boot up" bit.

For kexec/hotplug, if we must reset the core (for deep sleep we must),
any reason not to do the sync the same way U-Boot does?

-Scott

RE: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Li Yang-R58472]

>-----Original Message-----
>From: linuxppc-dev-bounces+leoli=3Dfreescale.com@lists.ozlabs.org
>[mailto:linuxppc-dev-bounces+leoli=3Dfreescale.com@lists.ozlabs.org] On
>Behalf Of Scott Wood
>Sent: Saturday, November 05, 2011 1:34 AM
>To: Zhao Chenhui-B35336
>Cc: linuxppc-dev@lists.ozlabs.org
>Subject: Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by
>KEXEC patch
>
>On 11/04/2011 07:29 AM, Zhao Chenhui wrote:
>> From: Li Yang <leoli@freescale.com>
>>
>> The timebase sync is not only necessary when using KEXEC. It should also
>> be used by normal boot up and cpu hotplug. Remove the ifdef added by
>> the KEXEC patch.
>
>The KEXEC patch didn't just add the ifdef, it also added the initializers:

Yes.  But the code suggests that the timebase synchronization is only neces=
sary for KEXEC, but it turns out that sleep/wakeup also need it.  Maybe the=
 description of the patch need to be changed as KEXEC is not to be blamed.

>
>> @@ -105,8 +107,64 @@ smp_85xx_setup_cpu(int cpu_nr)
>>
>>  struct smp_ops_t smp_85xx_ops =3D {
>>         .kick_cpu =3D smp_85xx_kick_cpu,
>> +#ifdef CONFIG_KEXEC
>> +       .give_timebase  =3D smp_generic_give_timebase,
>> +       .take_timebase  =3D smp_generic_take_timebase,
>> +#endif
>>  };
>
>U-Boot synchronizes the timebase on 85xx.  With what chip and U-Boot
>version are you seeing this not happen?

I'm curious why don't we make it happen in kernel as we are against adding =
dependency to the bootloader?  Other architectures don't have this dependen=
cy, it will be better if we don't add this dependency either IMO.


>
>If you are seeing only a small (around one tick) difference, make sure
>you're running a U-Boot that has this commit:
>
>> commit 7afc45ad7d9493208d89072cbb78a5bfc8034b59
>> Author: Kumar Gala <galak@kernel.crashing.org>
>> Date:   Sun Mar 13 10:55:53 2011 -0500
>>
>>     powerpc/85xx: Fix synchronization of timebase on MP boot
>>
>>     There is a small ordering issue in the master core in that we need
>to
>>     make sure the disabling of the timebase in the SoC is visible before
>we
>>     set the value to 0.  We can simply just read back the value to
>>     synchronizatize the write, before we set TB to 0.
>>
>>     Reported-by: Dan Hettena
>>     Tested-by: Dan Hettena
>>     Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>
>
>-Scott
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Scott Wood]

On 11/08/2011 03:06 AM, Li Yang-R58472 wrote:
> 
> 
>> -----Original Message-----
>> From: linuxppc-dev-bounces+leoli=freescale.com@lists.ozlabs.org
>> [mailto:linuxppc-dev-bounces+leoli=freescale.com@lists.ozlabs.org] On
>> Behalf Of Scott Wood
>> Sent: Saturday, November 05, 2011 1:34 AM
>> To: Zhao Chenhui-B35336
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Subject: Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by
>> KEXEC patch
>>
>> On 11/04/2011 07:29 AM, Zhao Chenhui wrote:
>>> From: Li Yang <leoli@freescale.com>
>>>
>>> The timebase sync is not only necessary when using KEXEC. It should also
>>> be used by normal boot up and cpu hotplug. Remove the ifdef added by
>>> the KEXEC patch.
>>
>> The KEXEC patch didn't just add the ifdef, it also added the initializers:
> 
> Yes.  But the code suggests that the timebase synchronization is only necessary for KEXEC, but it turns out that sleep/wakeup also need it.  Maybe the description of the patch need to be changed as KEXEC is not to be blamed.

It is needed when you hard reset a core.  This was something we never
did on SMP before kexec.  Now you're adding a second thing that does it,
so it'll need the sync as well, but that doesn't mean we should do it on
normal boot.

>>> @@ -105,8 +107,64 @@ smp_85xx_setup_cpu(int cpu_nr)
>>>
>>>  struct smp_ops_t smp_85xx_ops = {
>>>         .kick_cpu = smp_85xx_kick_cpu,
>>> +#ifdef CONFIG_KEXEC
>>> +       .give_timebase  = smp_generic_give_timebase,
>>> +       .take_timebase  = smp_generic_take_timebase,
>>> +#endif
>>>  };
>>
>> U-Boot synchronizes the timebase on 85xx.  With what chip and U-Boot
>> version are you seeing this not happen?
> 
> I'm curious why don't we make it happen in kernel as we are against
> adding dependency to the bootloader?

We are against adding gratuitous dependencies on the bootloader, but
some things are just a lot easier to do in that context.  Nobody
complains about Linux expecting RAM to be working on entry. :-)

While it's certainly possible to do this in Linux (and should be done
the way U-Boot does instead of the software sync, in the cases where we
need to), it's easier to do in U-Boot, before the cores are running.

It would be impossible for Linux to do this (or any other tb
modifications) when running on top of a hypervisor.

In http://lists.ozlabs.org/pipermail/linuxppc-dev/2011-June/091321.html,
Ben Herrenschmidt said, "smp-tbsync.c is and has always been a
'workaround' for broken HW."

> Other architectures don't have this dependency, 

Which "other architectures" are you referring to?

On PPC server this is handled with a firmware call to freeze the
timebase.  On x86 this is handled by the BIOS by the time the OS starts.

-Scott

[PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Li Yang]

The timebase sync is not only necessary when using KEXEC.  It should also
be used by normal boot up and cpu hotplug.  Remove the ifdef added by
the KEXEC patch.  Fix a problem that cpu hotplugging freezes the whole system.

Signed-off-by: Jin Qing <b24347@freescale.com>
Singed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/platforms/85xx/smp.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 5c91a99..1e8aec8 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -2,7 +2,7 @@
  * Author: Andy Fleming <afleming@freescale.com>
  * 	   Kumar Gala <galak@kernel.crashing.org>
  *
- * Copyright 2006-2008 Freescale Semiconductor Inc.
+ * Copyright 2006-2010 Freescale Semiconductor Inc.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -115,10 +115,8 @@ smp_85xx_setup_cpu(int cpu_nr)
 
 struct smp_ops_t smp_85xx_ops = {
 	.kick_cpu = smp_85xx_kick_cpu,
-#ifdef CONFIG_KEXEC
 	.give_timebase	= smp_generic_give_timebase,
 	.take_timebase	= smp_generic_take_timebase,
-#endif
 };
 
 #ifdef CONFIG_KEXEC
-- 
1.6.6-rc1.GIT

Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Kumar Gala]

On Dec 3, 2010, at 6:34 AM, Li Yang wrote:

> The timebase sync is not only necessary when using KEXEC.  It should =
also
> be used by normal boot up and cpu hotplug.  Remove the ifdef added by
> the KEXEC patch.  Fix a problem that cpu hotplugging freezes the whole =
system.
>=20
> Signed-off-by: Jin Qing <b24347@freescale.com>
> Singed-off-by: Li Yang <leoli@freescale.com>
> ---
> arch/powerpc/platforms/85xx/smp.c |    4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)

But we have problems with KEXEC w/o this?  What is the issue with =
hotplugging and the generic timebase code?  When do we freeze?

>=20
> diff --git a/arch/powerpc/platforms/85xx/smp.c =
b/arch/powerpc/platforms/85xx/smp.c
> index 5c91a99..1e8aec8 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -2,7 +2,7 @@
>  * Author: Andy Fleming <afleming@freescale.com>
>  * 	   Kumar Gala <galak@kernel.crashing.org>
>  *
> - * Copyright 2006-2008 Freescale Semiconductor Inc.
> + * Copyright 2006-2010 Freescale Semiconductor Inc.
>  *
>  * This program is free software; you can redistribute  it and/or =
modify it
>  * under  the terms of  the GNU General  Public License as published =
by the
> @@ -115,10 +115,8 @@ smp_85xx_setup_cpu(int cpu_nr)
>=20
> struct smp_ops_t smp_85xx_ops =3D {
> 	.kick_cpu =3D smp_85xx_kick_cpu,
> -#ifdef CONFIG_KEXEC
> 	.give_timebase	=3D smp_generic_give_timebase,
> 	.take_timebase	=3D smp_generic_take_timebase,
> -#endif
> };
>=20
> #ifdef CONFIG_KEXEC
> --=20
> 1.6.6-rc1.GIT
>=20
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch

[Li Yang]

On Sat, Dec 4, 2010 at 12:40 AM, Kumar Gala <galak@kernel.crashing.org> wro=
te:
>
> On Dec 3, 2010, at 6:34 AM, Li Yang wrote:
>
>> The timebase sync is not only necessary when using KEXEC. =C2=A0It shoul=
d also
>> be used by normal boot up and cpu hotplug. =C2=A0Remove the ifdef added =
by
>> the KEXEC patch. =C2=A0Fix a problem that cpu hotplugging freezes the wh=
ole system.
>>
>> Signed-off-by: Jin Qing <b24347@freescale.com>
>> Singed-off-by: Li Yang <leoli@freescale.com>
>> ---
>> arch/powerpc/platforms/85xx/smp.c | =C2=A0 =C2=A04 +---
>> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> But we have problems with KEXEC w/o this? =C2=A0What is the issue with ho=
tplugging and the generic timebase code? =C2=A0When do we freeze?

Actually the KEXEC patch disables timebase sync when KEXEC is not
defined.  If the timebase sync is disabled, the timebase on non-boot
cpu will become non-consistent.  And thus ruins the scheduler when
hot-plugged.

>
>>
>> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/=
85xx/smp.c
>> index 5c91a99..1e8aec8 100644
>> --- a/arch/powerpc/platforms/85xx/smp.c
>> +++ b/arch/powerpc/platforms/85xx/smp.c
>> @@ -2,7 +2,7 @@
>> =C2=A0* Author: Andy Fleming <afleming@freescale.com>
>> =C2=A0* =C2=A0 =C2=A0 =C2=A0 Kumar Gala <galak@kernel.crashing.org>
>> =C2=A0*
>> - * Copyright 2006-2008 Freescale Semiconductor Inc.
>> + * Copyright 2006-2010 Freescale Semiconductor Inc.
>> =C2=A0*
>> =C2=A0* This program is free software; you can redistribute =C2=A0it and=
/or modify it
>> =C2=A0* under =C2=A0the terms of =C2=A0the GNU General =C2=A0Public Lice=
nse as published by the
>> @@ -115,10 +115,8 @@ smp_85xx_setup_cpu(int cpu_nr)
>>
>> struct smp_ops_t smp_85xx_ops =3D {
>> =C2=A0 =C2=A0 =C2=A0 .kick_cpu =3D smp_85xx_kick_cpu,
>> -#ifdef CONFIG_KEXEC
>> =C2=A0 =C2=A0 =C2=A0 .give_timebase =C2=A0=3D smp_generic_give_timebase,
>> =C2=A0 =C2=A0 =C2=A0 .take_timebase =C2=A0=3D smp_generic_take_timebase,
>> -#endif
>> };
>>
>> #ifdef CONFIG_KEXEC
>> --
>> 1.6.6-rc1.GIT
>>
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>



--=20
- Leo