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

login
register
mail settings
Submitter Dongsheng Wang
Date Aug. 16, 2013, 7:23 a.m.
Message ID <1376637789-27330-1-git-send-email-dongsheng.wang@freescale.com>
Download mbox | patch
Permalink /patch/267592/
State Superseded
Headers show

Comments

Dongsheng Wang - Aug. 16, 2013, 7:23 a.m.
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>
Kumar Gala - Aug. 16, 2013, 11:02 a.m.
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?

- k
Scott Wood - Aug. 16, 2013, 4:50 p.m.
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.

As for the PVR check, the upstream kernel doesn't need to care about
rev1, so knowing it's an e6500 is good enough.

-Scott
Wang Dongsheng-B40534 - Aug. 19, 2013, 2:53 a.m.
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.
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.

> 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?

-dongsheng
Scott Wood - Aug. 20, 2013, 12:38 a.m.
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?

> > 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.

-Scott

Patch

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 5d7d9c2..5c7a7ba 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1053,6 +1053,8 @@ 
 #define PVR_8560	0x80200000
 #define PVR_VER_E500V1	0x8020
 #define PVR_VER_E500V2	0x8021
+#define PVR_VER_E6500	0x8040
+
 /*
  * For the 8xx processors, all of them report the same PVR family for
  * the PowerPC core. The various versions of these processors must be
diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
index b417de3..c047e08 100644
--- a/arch/powerpc/include/asm/reg_booke.h
+++ b/arch/powerpc/include/asm/reg_booke.h
@@ -170,6 +170,7 @@ 
 #define SPRN_L2CSR1	0x3FA	/* L2 Data Cache Control and Status Register 1 */
 #define SPRN_DCCR	0x3FA	/* Data Cache Cacheability Register */
 #define SPRN_ICCR	0x3FB	/* Instruction Cache Cacheability Register */
+#define SPRN_PWRMGTCR0	0x3FB	/* Power management control register 0 */
 #define SPRN_SVR	0x3FF	/* System Version Register */
 
 /*
@@ -216,6 +217,9 @@ 
 #define	CCR1_DPC	0x00000100 /* Disable L1 I-Cache/D-Cache parity checking */
 #define	CCR1_TCS	0x00000080 /* Timer Clock Select */
 
+/* Bit definitions for PWRMGTCR0. */
+#define PWRMGTCR0_ALTIVEC_IDLE	(1 << 22) /* Altivec idle enable */
+
 /* Bit definitions for the MCSR. */
 #define MCSR_MCS	0x80000000 /* Machine Check Summary */
 #define MCSR_IB		0x40000000 /* Instruction PLB Error */
diff --git a/arch/powerpc/platforms/85xx/common.c b/arch/powerpc/platforms/85xx/common.c
index d0861a0..dbbbc24 100644
--- a/arch/powerpc/platforms/85xx/common.c
+++ b/arch/powerpc/platforms/85xx/common.c
@@ -7,10 +7,22 @@ 
  */
 #include <linux/of_platform.h>
 
+#include <asm/time.h>
+
 #include <sysdev/cpm2_pic.h>
 
 #include "mpc85xx.h"
 
+#define MAX_BIT				64
+
+#define ALTIVEC_COUNT_OFFSET		16
+#define ALTIVEC_IDLE_COUNT_MASK		0x003f0000
+
+/*
+ * FIXME - We don't know the AltiVec application scenarios.
+ */
+#define ALTIVEC_IDLE_TIME	1000 /* 1ms */
+
 static struct of_device_id __initdata mpc85xx_common_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
@@ -80,3 +92,63 @@  void __init mpc85xx_cpm2_pic_init(void)
 	irq_set_chained_handler(irq, cpm2_cascade);
 }
 #endif
+
+static bool has_pw20_altivec_idle(void)
+{
+	u32 pvr;
+
+	pvr = mfspr(SPRN_PVR);
+
+	/* PW20 & AltiVec idle feature only exists for E6500 */
+	if (PVR_VER(pvr) != PVR_VER_E6500)
+		return false;
+
+	/* Fix erratum, e6500 rev1 does not support PW20 & AltiVec idle */
+	if (PVR_REV(pvr) < 0x20)
+		return false;
+
+	return true;
+}
+
+static unsigned int get_idle_ticks_bit(unsigned int us)
+{
+	unsigned int cycle;
+
+	/*
+	 * The time control by TB turn over bit, so we need
+	 * to be divided by 2.
+	 */
+	cycle = (us / 2) * tb_ticks_per_usec;
+
+	return ilog2(cycle) + 1;
+}
+
+static void setup_altivec_idle(void *unused)
+{
+	u32 altivec_idle, bit;
+
+	if (!has_pw20_altivec_idle())
+		return;
+
+	/* Enable Altivec Idle */
+	altivec_idle = mfspr(SPRN_PWRMGTCR0);
+	altivec_idle |= PWRMGTCR0_ALTIVEC_IDLE;
+
+	/* Set Automatic AltiVec Idle Count */
+	/* clear count */
+	altivec_idle &= ~ALTIVEC_IDLE_COUNT_MASK;
+
+	/* set count */
+	bit = get_idle_ticks_bit(ALTIVEC_IDLE_TIME);
+	altivec_idle |= ((MAX_BIT - bit) << ALTIVEC_COUNT_OFFSET);
+
+	mtspr(SPRN_PWRMGTCR0, altivec_idle);
+}
+
+static int __init setup_idle_hw_governor(void)
+{
+	on_each_cpu(setup_altivec_idle, NULL, 1);
+
+	return 0;
+}
+late_initcall(setup_idle_hw_governor);