diff mbox

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

Message ID 1320409787-14360-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive)
State Superseded
Headers show

Commit Message

chenhui zhao Nov. 4, 2011, 12:29 p.m. UTC 
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(-)

Comments

Scott Wood Nov. 4, 2011, 5:33 p.m. UTC | #1 
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
Kumar Gala Nov. 4, 2011, 7:33 p.m. UTC | #2 
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
Scott Wood Nov. 4, 2011, 7:38 p.m. UTC | #3 
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
Li Yang-R58472 Nov. 8, 2011, 9:06 a.m. UTC | #4 
>-----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.

>
>> @@ -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?  Other architectures don't have this dependency, 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
Scott Wood Nov. 8, 2011, 5:28 p.m. UTC | #5 
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
diff mbox

Patch

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