| Message ID | 1378879004-2446-2-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2013-09-11 at 13:56 +0800, 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> > --- > *v3: > Assembly code instead of C code. > > *v2: > Remove: > delete setup_idle_hw_governor function. > delete "Fix erratum" for rev1. > > Move: > move setup_* into __setup/restore_cpu_e6500. > > arch/powerpc/kernel/cpu_setup_fsl_booke.S | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kernel/cpu_setup_fsl_booke.S > index bfb18c7..3c03c109 100644 > --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S > +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S > @@ -53,11 +53,30 @@ _GLOBAL(__e500_dcache_setup) > isync > blr > > +/* > + * FIXME - We don't know the AltiVec application scenarios. > + */ > +#define AV_WAIT_IDLE_BIT 50 /* 1ms, TB frequency is 41.66MHZ */ > +_GLOBAL(setup_altivec_idle) > + mfspr r3, SPRN_PWRMGTCR0 > + > + /* Enable Altivec Idle */ > + oris r3, r3, PWRMGTCR0_AV_IDLE_PD_EN@h > + li r4, AV_WAIT_IDLE_BIT > + > + /* Set Automatic AltiVec Idle Count */ > + rlwimi r3, r4, PWRMGTCR0_AV_IDLE_CNT_SHIFT, PWRMGTCR0_AV_IDLE_CNT > + > + mtspr SPRN_PWRMGTCR0, r3 > + > + blr The FIXME comment is not clear. If you mean that we haven't yet done testing to determine a reasonable default value for AV_WAIT_IDLE_BIT, then just say that. Likewise with the FIXME comment in the pw20 patch -- the uncertainty is due to a lack of investigation, not "because we don't know the current state of the cpu load". While some workloads may want a different value than whatever default we set, that's what the sysfs interface is for. The only thing missing here is benchmarking to determine a good overall default. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, September 12, 2013 6:43 AM > To: Wang Dongsheng-B40534 > Cc: galak@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH v3 2/4] powerpc/85xx: add hardware automatically > enter altivec idle state > > On Wed, 2013-09-11 at 13:56 +0800, 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> > > --- > > *v3: > > Assembly code instead of C code. > > > > *v2: > > Remove: > > delete setup_idle_hw_governor function. > > delete "Fix erratum" for rev1. > > > > Move: > > move setup_* into __setup/restore_cpu_e6500. > > > > arch/powerpc/kernel/cpu_setup_fsl_booke.S | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S > b/arch/powerpc/kernel/cpu_setup_fsl_booke.S > > index bfb18c7..3c03c109 100644 > > --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S > > +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S > > @@ -53,11 +53,30 @@ _GLOBAL(__e500_dcache_setup) > > isync > > blr > > > > +/* > > + * FIXME - We don't know the AltiVec application scenarios. > > + */ > > +#define AV_WAIT_IDLE_BIT 50 /* 1ms, TB frequency is 41.66MHZ > */ > > +_GLOBAL(setup_altivec_idle) > > + mfspr r3, SPRN_PWRMGTCR0 > > + > > + /* Enable Altivec Idle */ > > + oris r3, r3, PWRMGTCR0_AV_IDLE_PD_EN@h > > + li r4, AV_WAIT_IDLE_BIT > > + > > + /* Set Automatic AltiVec Idle Count */ > > + rlwimi r3, r4, PWRMGTCR0_AV_IDLE_CNT_SHIFT, > PWRMGTCR0_AV_IDLE_CNT > > + > > + mtspr SPRN_PWRMGTCR0, r3 > > + > > + blr > > The FIXME comment is not clear. If you mean that we haven't yet done > testing to determine a reasonable default value for AV_WAIT_IDLE_BIT, > then just say that. Likewise with the FIXME comment in the pw20 patch > -- the uncertainty is due to a lack of investigation, not "because we > don't know the current state of the cpu load". > > While some workloads may want a different value than whatever default we > set, that's what the sysfs interface is for. The only thing missing > here is benchmarking to determine a good overall default. > Thanks.
diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kernel/cpu_setup_fsl_booke.S index bfb18c7..3c03c109 100644 --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S @@ -53,11 +53,30 @@ _GLOBAL(__e500_dcache_setup) isync blr +/* + * FIXME - We don't know the AltiVec application scenarios. + */ +#define AV_WAIT_IDLE_BIT 50 /* 1ms, TB frequency is 41.66MHZ */ +_GLOBAL(setup_altivec_idle) + mfspr r3, SPRN_PWRMGTCR0 + + /* Enable Altivec Idle */ + oris r3, r3, PWRMGTCR0_AV_IDLE_PD_EN@h + li r4, AV_WAIT_IDLE_BIT + + /* Set Automatic AltiVec Idle Count */ + rlwimi r3, r4, PWRMGTCR0_AV_IDLE_CNT_SHIFT, PWRMGTCR0_AV_IDLE_CNT + + mtspr SPRN_PWRMGTCR0, r3 + + blr + _GLOBAL(__setup_cpu_e6500) mflr r6 #ifdef CONFIG_PPC64 bl .setup_altivec_ivors #endif + bl .setup_altivec_idle bl __setup_cpu_e5500 mtlr r6 blr @@ -119,6 +138,7 @@ _GLOBAL(__setup_cpu_e5500) _GLOBAL(__restore_cpu_e6500) mflr r5 bl .setup_altivec_ivors + bl .setup_altivec_idle bl __restore_cpu_e5500 mtlr r5 blr