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

From: Scott Wood <scottwood@freescale.com>
To: Li Yang-R58472 <r58472@freescale.com>
Cc: Wood Scott-B07421 <B07421@freescale.com>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	Zhao Chenhui-B35336 <B35336@freescale.com>
Subject: Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch
Date: Tue, 8 Nov 2011 11:28:55 -0600	[thread overview]
Message-ID: <4EB966D7.2060609@freescale.com> (raw)
In-Reply-To: <3F607A5180246847A760FD34122A1E052BC5F8@039-SN1MPN1-004.039d.mgd.msft.net>

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