diff mbox

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

Message ID 1321437344-19253-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive)
State Changes Requested
Delegated to: Kumar Gala
Headers show

Commit Message

chenhui zhao Nov. 16, 2011, 9:55 a.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. 16, 2011, 6:42 p.m. UTC | #1 
On 11/16/2011 03:55 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.

Again, no it should not be used by normal boot up (whether KEXEC support
is enabled or not).  We should only do timebase sync when we actually
need to (when we've actually just reset a core), and we should do it the
way U-Boot does rather than with smp-tbsync.c.

-Scott
chenhui zhao Nov. 18, 2011, 10:12 a.m. UTC | #2 
On Wed, Nov 16, 2011 at 12:42:14PM -0600, Scott Wood wrote:
> On 11/16/2011 03:55 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.
> 
> Again, no it should not be used by normal boot up (whether KEXEC support
> is enabled or not).  We should only do timebase sync when we actually
> need to (when we've actually just reset a core), and we should do it the
> way U-Boot does rather than with smp-tbsync.c.
> 
> -Scott

OK. I'll do it the way U-Boot does.
Thanks.

-chenhui
Kumar Gala Nov. 18, 2011, 2:35 p.m. UTC | #3 
On Nov 16, 2011, at 12:42 PM, Scott Wood wrote:

> On 11/16/2011 03:55 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.
> 
> Again, no it should not be used by normal boot up (whether KEXEC support
> is enabled or not).  We should only do timebase sync when we actually
> need to (when we've actually just reset a core), and we should do it the
> way U-Boot does rather than with smp-tbsync.c.


How can we do u-boot bases timebase sync after the system us up and running?  For example would we losing some ticks of time in the case that one core is up and we bring a second core online?

- k
Scott Wood Nov. 18, 2011, 6:01 p.m. UTC | #4 
On Fri, Nov 18, 2011 at 08:35:02AM -0600, Kumar Gala wrote:
> 
> On Nov 16, 2011, at 12:42 PM, Scott Wood wrote:
> 
> > On 11/16/2011 03:55 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.
> > 
> > Again, no it should not be used by normal boot up (whether KEXEC support
> > is enabled or not).  We should only do timebase sync when we actually
> > need to (when we've actually just reset a core), and we should do it the
> > way U-Boot does rather than with smp-tbsync.c.
> 
> 
> How can we do u-boot bases timebase sync after the system us up and
> running?  For example would we losing some ticks of time in the case
> that one core is up and we bring a second core online?

Yes, we'll lose a small handful of ticks relative to wall clock time --
but it'll be the same loss on all cores.  It's better than possibly
having the timebase be imperfectly synchronized, and should complete more
quickly.

This is only during intrusive events such as kexec or deep sleep (we only
need to reset the core for jog on mpc8536 which has only one core). 
During deep sleep all cores' timebases will be stopped.  Kexec is
resetting the kernel; it's not going to care what the old timebase was,
and should resync from RTC.

Even if we end up using this for some future power management mode where
we take down some CPUs to the point their timebase stops, but never take
down others, the time loss should be negligible (for comparison, what's
the error tolerance on the crystal frequency?) and acceptable for what is
still a fairly intrusive and rare event.

-Scott
Kumar Gala Nov. 20, 2011, 4:46 p.m. UTC | #5 
On Nov 18, 2011, at 12:01 PM, Scott Wood wrote:

> On Fri, Nov 18, 2011 at 08:35:02AM -0600, Kumar Gala wrote:
>> 
>> On Nov 16, 2011, at 12:42 PM, Scott Wood wrote:
>> 
>>> On 11/16/2011 03:55 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.
>>> 
>>> Again, no it should not be used by normal boot up (whether KEXEC support
>>> is enabled or not).  We should only do timebase sync when we actually
>>> need to (when we've actually just reset a core), and we should do it the
>>> way U-Boot does rather than with smp-tbsync.c.
>> 
>> 
>> How can we do u-boot bases timebase sync after the system us up and
>> running?  For example would we losing some ticks of time in the case
>> that one core is up and we bring a second core online?
> 
> Yes, we'll lose a small handful of ticks relative to wall clock time --
> but it'll be the same loss on all cores.  It's better than possibly
> having the timebase be imperfectly synchronized, and should complete more
> quickly.

Hmm, I wondering how many ticks it really is.

> This is only during intrusive events such as kexec or deep sleep (we only
> need to reset the core for jog on mpc8536 which has only one core). 
> During deep sleep all cores' timebases will be stopped.  Kexec is
> resetting the kernel; it's not going to care what the old timebase was,
> and should resync from RTC.
> 
> Even if we end up using this for some future power management mode where
> we take down some CPUs to the point their timebase stops, but never take
> down others, the time loss should be negligible (for comparison, what's
> the error tolerance on the crystal frequency?) and acceptable for what is
> still a fairly intrusive and rare event.

I'm also concerned about how this ends up working for p4080 when we have to sync up to 8 cores or more in the future.

- k
Li Yang-R58472 Nov. 22, 2011, 9:29 a.m. UTC | #6 
>Subject: Re: [PATCH v2 1/7] powerpc/85xx: re-enable timebase sync disabled
>by KEXEC patch
>
>On Fri, Nov 18, 2011 at 08:35:02AM -0600, Kumar Gala wrote:
>>
>> On Nov 16, 2011, at 12:42 PM, Scott Wood wrote:
>>
>> > On 11/16/2011 03:55 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.
>> >
>> > Again, no it should not be used by normal boot up (whether KEXEC
>support
>> > is enabled or not).  We should only do timebase sync when we actually
>> > need to (when we've actually just reset a core), and we should do it
>the
>> > way U-Boot does rather than with smp-tbsync.c.

While looking into the timebase sync code in u-boot, I have a few questions.

        /* enable time base at the platform */
        if (whoami)
                devdisr |= MPC85xx_DEVDISR_TB1;
        else
                devdisr |= MPC85xx_DEVDISR_TB0;
        out_be32(&gur->devdisr, devdisr);

        /* readback to sync write */
        in_be32(&gur->devdisr);

        mtspr(SPRN_TBWU, 0);
        mtspr(SPRN_TBWL, 0);

What are the TBWU/TBWL registers?  I can't find the definition of them in either e500 RM or booke RM.  Are they valid to be used?  What is the result of writing to them?  Aren't the SPR registers core specific?  How can we set the TB for the other cores?

        devdisr &= ~(MPC85xx_DEVDISR_TB0 | MPC85xx_DEVDISR_TB1);
        out_be32(&gur->devdisr, devdisr);

Also in the UM, I read "Blocks disabled by DEVDISR must not be re-enabled without a hard reset."  Is it safe we enable it here?

- Leo
Kumar Gala Nov. 22, 2011, 4:05 p.m. UTC | #7 
On Nov 22, 2011, at 3:29 AM, Li Yang-R58472 wrote:

>> Subject: Re: [PATCH v2 1/7] powerpc/85xx: re-enable timebase sync disabled
>> by KEXEC patch
>> 
>> On Fri, Nov 18, 2011 at 08:35:02AM -0600, Kumar Gala wrote:
>>> 
>>> On Nov 16, 2011, at 12:42 PM, Scott Wood wrote:
>>> 
>>>> On 11/16/2011 03:55 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.
>>>> 
>>>> Again, no it should not be used by normal boot up (whether KEXEC
>> support
>>>> is enabled or not).  We should only do timebase sync when we actually
>>>> need to (when we've actually just reset a core), and we should do it
>> the
>>>> way U-Boot does rather than with smp-tbsync.c.
> 
> While looking into the timebase sync code in u-boot, I have a few questions.
> 
>        /* enable time base at the platform */
>        if (whoami)
>                devdisr |= MPC85xx_DEVDISR_TB1;
>        else
>                devdisr |= MPC85xx_DEVDISR_TB0;
>        out_be32(&gur->devdisr, devdisr);
> 
>        /* readback to sync write */
>        in_be32(&gur->devdisr);
> 
>        mtspr(SPRN_TBWU, 0);
>        mtspr(SPRN_TBWL, 0);
> 
> What are the TBWU/TBWL registers?  I can't find the definition of them in either e500 RM or booke RM.  Are they valid to be used?  What is the result of writing to them?  Aren't the SPR registers core specific?  How can we set the TB for the other cores?

TBWU/TBWL are SPR 284/285 (they might be called something a little different in the manual).

You can only set the TB on the core itself.  The way the u-boot code works is it sets TBL/TBU (TB) to 0 in release.S for secondary cores.  In the code you reference we are setting TB on the 'master' core to 0 since we turned TB on earlier for the master core and now are resetting it to 0 to sync all the cores.

> 
>        devdisr &= ~(MPC85xx_DEVDISR_TB0 | MPC85xx_DEVDISR_TB1);
>        out_be32(&gur->devdisr, devdisr);
> 
> Also in the UM, I read "Blocks disabled by DEVDISR must not be re-enabled without a hard reset."  Is it safe we enable it here?

Yes this is safe, TB isn't truly a block.  Manual should technically be updated to exclude TB.

- k
diff mbox

Patch

diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 2df4785..6834447 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -114,10 +114,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