diff mbox

[1/2] powerpc/85xx: add hardware automatically enter altivec idle state

Message ID ABB05CD9C9F68C46A5CEDC7F1543925901027BD3@039-SN2MPN1-021.039d.mgd.msft.net (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Wang Dongsheng-B40534 Aug. 22, 2013, 3:13 a.m. UTC 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, August 20, 2013 8:39 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Kumar Gala; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically enter
> altivec idle state
> 
> On Sun, 2013-08-18 at 21:53 -0500, Wang Dongsheng-B40534 wrote:
> > Thanks for your feedback.
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Saturday, August 17, 2013 12:51 AM
> > > To: Kumar Gala
> > > Cc: Wang Dongsheng-B40534; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically
> enter
> > > altivec idle state
> > >
> > > On Fri, 2013-08-16 at 06:02 -0500, Kumar Gala wrote:
> > > > On Aug 16, 2013, at 2:23 AM, Dongsheng Wang wrote:
> > > >
> > > > > From: Wang Dongsheng <dongsheng.wang@freescale.com>
> > > > >
> > > > > Each core's AltiVec unit may be placed into a power savings mode
> > > > > by turning off power to the unit. Core hardware will
> automatically
> > > > > power down the AltiVec unit after no AltiVec instructions have
> > > > > executed in N cycles. The AltiVec power-control is triggered by
> > > hardware.
> > > > >
> > > > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > > >
> > > > Why treat this as a idle HW governor vs just some one time setup at
> > > boot of the time delay?
> > >
> > > It is being done as one-time setup, despite the function name.
> > >
> > > Maybe it should be moved into __setup/restore_cpu_e6500 (BTW, we
> really
> > > should refactor those to reduce duplication) with the timebase bit
> > > number hardcoded rather than a time in us.
> > >
> > The frequency of the different platforms timebase is not the same.
> 
> It's close enough.  Remember, this number is a vague initial guess, not
> something that's been carefully calibrated.  Though, it would make it
> harder to substitute the number with one that's been more carefully
> calibrated...  but wouldn't different chips possibly have different
> optimal delays anyway?
> 
> > If we use it, we need to set different timebase bit on each platform.
> > That is why I did not put the code in __setup/restore_cpu_e6500, I need
> > use tb_ticks_per_usec to get timebase bit. So we only need to set a
> time
> > for each platform, and not set different timebase bit.
> 
> It just seems wrong to have an ad-hoc mechanism for running
> core-specific code when we have cputable...  If we really need this,
> maybe we should add a "cpu_setup_late" function pointer.
> 
> With your patch, when does the power management register get set when
> hot plugging a cpu?
> 
Um.. I don't deal with this situation. I will fix it.
__setup/restore_cpu_e6500 looks good. But only bootcpu call __setup_cpu_e6500, not on each cpu.
I think this is a bug.

Fix code:


> > > As for the PVR check, the upstream kernel doesn't need to care about
> > > rev1, so knowing it's an e6500 is good enough.
> > >
> > But AltiVec idle & PW20 cannot work on rev1 platform.
> > We didn't have to deal with it?
> 
> Upstream does not run on rev1.
> 
:), But already have customers in the use of rev1.
Why we don't need to care about that?

Comments

Scott Wood Aug. 22, 2013, 3:19 p.m. UTC | #1 
On Wed, 2013-08-21 at 22:13 -0500, Wang Dongsheng-B40534 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, August 20, 2013 8:39 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Kumar Gala; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically enter
> > altivec idle state
> > 
> > It just seems wrong to have an ad-hoc mechanism for running
> > core-specific code when we have cputable...  If we really need this,
> > maybe we should add a "cpu_setup_late" function pointer.
> > 
> > With your patch, when does the power management register get set when
> > hot plugging a cpu?
> > 
> Um.. I don't deal with this situation. I will fix it.
> __setup/restore_cpu_e6500 looks good. But only bootcpu call __setup_cpu_e6500, not on each cpu.
> I think this is a bug.

Other CPUs call __restore_cpu_e6500.

> > > > As for the PVR check, the upstream kernel doesn't need to care about
> > > > rev1, so knowing it's an e6500 is good enough.
> > > >
> > > But AltiVec idle & PW20 cannot work on rev1 platform.
> > > We didn't have to deal with it?
> > 
> > Upstream does not run on rev1.
> > 
> :), But already have customers in the use of rev1.
> Why we don't need to care about that?

rev1 is not production-qualified.  Those customers are supposed to only
be using rev1 for evaluation and early development.  It's not that we
don't care about rev1 now (we have the SDK for that) but that we won't
care about it long-term and don't want to have to carry around a bunch
of baggage for it.  Some of the workarounds are pretty nasty (especially
A-006198).

-Scott
Wang Dongsheng-B40534 Aug. 23, 2013, 2:52 a.m. UTC | #2 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, August 22, 2013 11:19 PM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Kumar Gala; Zhao Chenhui-B35336; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically enter
> altivec idle state
> 
> On Wed, 2013-08-21 at 22:13 -0500, Wang Dongsheng-B40534 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, August 20, 2013 8:39 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Kumar Gala; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically
> > > enter altivec idle state
> > >
> > > It just seems wrong to have an ad-hoc mechanism for running
> > > core-specific code when we have cputable...  If we really need this,
> > > maybe we should add a "cpu_setup_late" function pointer.
> > >
> > > With your patch, when does the power management register get set
> > > when hot plugging a cpu?
> > >
> > Um.. I don't deal with this situation. I will fix it.
> > __setup/restore_cpu_e6500 looks good. But only bootcpu call
> __setup_cpu_e6500, not on each cpu.
> > I think this is a bug.
> 
> Other CPUs call __restore_cpu_e6500.
> 
No, there is bootcore of secondary thread, and other cores of first thread call __restore_cpu_e6500.
This flow is correct?

> > > > > As for the PVR check, the upstream kernel doesn't need to care
> > > > > about rev1, so knowing it's an e6500 is good enough.
> > > > >
> > > > But AltiVec idle & PW20 cannot work on rev1 platform.
> > > > We didn't have to deal with it?
> > >
> > > Upstream does not run on rev1.
> > >
> > :), But already have customers in the use of rev1.
> > Why we don't need to care about that?
> 
> rev1 is not production-qualified.  Those customers are supposed to only
> be using rev1 for evaluation and early development.  It's not that we
> don't care about rev1 now (we have the SDK for that) but that we won't
> care about it long-term and don't want to have to carry around a bunch of
> baggage for it.  Some of the workarounds are pretty nasty (especially A-
> 006198).
> 
Thanks.
Scott Wood Aug. 23, 2013, 3:31 p.m. UTC | #3 
On Thu, 2013-08-22 at 21:52 -0500, Wang Dongsheng-B40534 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, August 22, 2013 11:19 PM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Kumar Gala; Zhao Chenhui-B35336; linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically enter
> > altivec idle state
> > 
> > On Wed, 2013-08-21 at 22:13 -0500, Wang Dongsheng-B40534 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, August 20, 2013 8:39 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Kumar Gala; linuxppc-dev@lists.ozlabs.org
> > > > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically
> > > > enter altivec idle state
> > > >
> > > > It just seems wrong to have an ad-hoc mechanism for running
> > > > core-specific code when we have cputable...  If we really need this,
> > > > maybe we should add a "cpu_setup_late" function pointer.
> > > >
> > > > With your patch, when does the power management register get set
> > > > when hot plugging a cpu?
> > > >
> > > Um.. I don't deal with this situation. I will fix it.
> > > __setup/restore_cpu_e6500 looks good. But only bootcpu call
> > __setup_cpu_e6500, not on each cpu.
> > > I think this is a bug.
> > 
> > Other CPUs call __restore_cpu_e6500.
> > 
> No, there is bootcore of secondary thread, and other cores of first thread call __restore_cpu_e6500.

This is the upstream list -- there is no e6500 thread support yet. :-)

But in the SDK I do see generic_secondary_common_init being called from
generic_secondary_thread_init, which means __restore_cpu_e6500 will be
called.

-Scott
Wang Dongsheng-B40534 Aug. 26, 2013, 2:34 a.m. UTC | #4 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, August 23, 2013 11:31 PM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Kumar Gala; Zhao Chenhui-B35336; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically enter
> altivec idle state
> 
> On Thu, 2013-08-22 at 21:52 -0500, Wang Dongsheng-B40534 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, August 22, 2013 11:19 PM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Kumar Gala; Zhao Chenhui-B35336; linuxppc-
> > > dev@lists.ozlabs.org
> > > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically
> enter
> > > altivec idle state
> > >
> > > On Wed, 2013-08-21 at 22:13 -0500, Wang Dongsheng-B40534 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, August 20, 2013 8:39 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Wood Scott-B07421; Kumar Gala; linuxppc-dev@lists.ozlabs.org
> > > > > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically
> > > > > enter altivec idle state
> > > > >
> > > > > It just seems wrong to have an ad-hoc mechanism for running
> > > > > core-specific code when we have cputable...  If we really need
> this,
> > > > > maybe we should add a "cpu_setup_late" function pointer.
> > > > >
> > > > > With your patch, when does the power management register get set
> > > > > when hot plugging a cpu?
> > > > >
> > > > Um.. I don't deal with this situation. I will fix it.
> > > > __setup/restore_cpu_e6500 looks good. But only bootcpu call
> > > __setup_cpu_e6500, not on each cpu.
> > > > I think this is a bug.
> > >
> > > Other CPUs call __restore_cpu_e6500.
> > >
> > No, there is bootcore of secondary thread, and other cores of first
> thread call __restore_cpu_e6500.
> 
> This is the upstream list -- there is no e6500 thread support yet. :-)
> 
> But in the SDK I do see generic_secondary_common_init being called from
> generic_secondary_thread_init, which means __restore_cpu_e6500 will be
> called.

Thanks.
diff mbox

Patch

diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 2cfed9e..2a9a718 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -603,6 +603,9 @@  __secondary_start:
        /* Set thread priority to MEDIUM */
        HMT_MEDIUM

+#ifdef CONFIG_PPC_BOOK3E
+       bl      call_setup_cpu          /* Call setup_cpu for this CPU */
+#endif
        /* Initialize the kernel stack */
        LOAD_REG_ADDR(r3, current_set)
        sldi    r28,r24,3               /* get current_set[cpu#]         */
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index b863b87..7d84bf4 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -55,6 +55,17 @@  _GLOBAL(call_handle_irq)
        mtlr    r0
        blr

+_GLOBAL(call_setup_cpu)
+       LOAD_REG_ADDR(r4, cur_cpu_spec)
+       ld      r4, 0(r4)
+       ld      r5, CPU_SPEC_SETUP(r4)
+
+       cmpwi   0, r5, 0
+       beqlr
+       ld      r5, 0(r5)
+       mtctr   r5
+       bctr
+
        .section        ".toc","aw"
 PPC64_CACHES:
        .tc             ppc64_caches[TC],ppc64_caches