[PATCH 0/2] oprofile support for ppc750

Adds oprofile support for ppc750. ppc750 performance counters
interface is similar with G4, but it only has 4 performance counters
instead of 6.

I tested this on a PPC750GX and it seems to work fine. 

tavi

For ppc750 processors which use 4 performance counters instead of the
6 G4 uses but otherwise is compatible with G4.

Signed-off-by: Octavian Purdila <redacted>
---
 arch/powerpc/oprofile/op_model_7450.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/oprofile/op_model_7450.c b/arch/powerpc/oprofile/op_model_7450.c
index cc599eb..97348f5 100644
--- a/arch/powerpc/oprofile/op_model_7450.c
+++ b/arch/powerpc/oprofile/op_model_7450.c

@@ -29,7 +29,7 @@
 static unsigned long reset_value[OP_MAX_COUNTER];
 
 static int oprofile_running;
-static u32 mmcr0_val, mmcr1_val, mmcr2_val;
+static u32 mmcr0_val, mmcr1_val, mmcr2_val, ctrs;
 
 #define MMCR0_PMC1_SHIFT	6
 #define MMCR0_PMC2_SHIFT	0

@@ -88,13 +88,12 @@ static int fsl7450_cpu_setup(struct op_counter_config *ctr)
 
 	mtspr(SPRN_MMCR0, mmcr0_val);
 	mtspr(SPRN_MMCR1, mmcr1_val);
-	mtspr(SPRN_MMCR2, mmcr2_val);
+	if (ctrs > 4)
+		mtspr(SPRN_MMCR2, mmcr2_val);
 
 	return 0;
 }
 
-#define NUM_CTRS 6
-
 /* Configures the global settings for the countes on all CPUs. */
 static int fsl7450_reg_setup(struct op_counter_config *ctr,
 			     struct op_system_config *sys,

@@ -102,12 +101,13 @@ static int fsl7450_reg_setup(struct op_counter_config *ctr,
 {
 	int i;
 
+	ctrs = num_ctrs;
 	/* Our counters count up, and "count" refers to
 	 * how much before the next interrupt, and we interrupt
 	 * on overflow.  So we calculate the starting value
 	 * which will give us "count" until overflow.
 	 * Then we set the events on the enabled counters */
-	for (i = 0; i < NUM_CTRS; ++i)
+	for (i = 0; i < num_ctrs; ++i)
 		reset_value[i] = 0x80000000UL - ctr[i].count;
 
 	/* Set events for Counters 1 & 2 */

@@ -123,9 +123,10 @@ static int fsl7450_reg_setup(struct op_counter_config *ctr,
 
 	/* Set events for Counters 3-6 */
 	mmcr1_val = mmcr1_event3(ctr[2].event)
-		| mmcr1_event4(ctr[3].event)
-		| mmcr1_event5(ctr[4].event)
-		| mmcr1_event6(ctr[5].event);
+		| mmcr1_event4(ctr[3].event);
+	if (num_ctrs > 4)
+		mmcr1_val |= mmcr1_event5(ctr[4].event)
+			| mmcr1_event6(ctr[5].event);
 
 	mmcr2_val = 0;
 

@@ -139,7 +140,7 @@ static int fsl7450_start(struct op_counter_config *ctr)
 
 	mtmsr(mfmsr() | MSR_PMM);
 
-	for (i = 0; i < NUM_CTRS; ++i) {
+	for (i = 0; i < ctrs; ++i) {
 		if (ctr[i].enabled)
 			classic_ctr_write(i, reset_value[i]);
 		else

@@ -184,7 +185,7 @@ static void fsl7450_handle_interrupt(struct pt_regs *regs,
 	pc = mfspr(SPRN_SIAR);
 	is_kernel = is_kernel_addr(pc);
 
-	for (i = 0; i < NUM_CTRS; ++i) {
+	for (i = 0; i < ctrs; ++i) {
 		val = classic_ctr_read(i);
 		if (val < 0) {
 			if (oprofile_running && ctr[i].enabled) {

-- 
1.5.6.5

Signed-off-by: Octavian Purdila <redacted>
---
 arch/powerpc/kernel/cputable.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 923f87a..4e20cfb 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c

@@ -726,6 +726,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/7450",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750FX rev 2.0 must disable HID0[DPM] */
 		.pvr_mask		= 0xffffffff,

@@ -741,6 +743,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/7450",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750FX (All revs except 2.0) */
 		.pvr_mask		= 0xffff0000,

@@ -756,6 +760,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750fx,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/7450",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750GX */
 		.pvr_mask		= 0xffff0000,

-- 
1.5.6.5

On Tue, 2009-01-06 at 14:55 +0200, Octavian Purdila wrote:

Adds oprofile support for ppc750. ppc750 performance counters
interface is similar with G4, but it only has 4 performance counters
instead of 6.

I tested this on a PPC750GX and it seems to work fine. 

I'm not too sure about using the same model as 74xx... I have some
patches that haven't been released yet from Jack Miller to implement the
full OP support for 750 but they appear to cause crashes.

There is a known HW bug in various 750's that basically make the
performance interrupt unuseable. I -think- it may have been fixed in the
very latest revision of the 750CL though but I'm not sure.

Cheers,
Ben.

=46rom: Benjamin Herrenschmidt [off-list ref]

On Tue, 2009-01-06 at 14:55 +0200, Octavian Purdila wrote:

quoted

Adds oprofile support for ppc750. ppc750 performance counters
interface is similar with G4, but it only has 4 performance counters
instead of 6.

I tested this on a PPC750GX and it seems to work fine.

I'm not too sure about using the same model as 74xx...=20

I've double checked the bits, and it looks to me that what is implemented i=
n=20
op_model_7450.c maps with my User=E2=80=99s Manual IBM PowerPC 750GX and 75=
0GL RISC=20
Microprocessor copy :)

I have some
patches that haven't been released yet from Jack Miller to implement the
full OP support for 750 but they appear to cause crashes.

Were they posted somewhere?

There is a known HW bug in various 750's that basically make the
performance interrupt unuseable. I -think- it may have been fixed in the
very latest revision of the 750CL though but I'm not sure.

Yeah, I've read that somewhere, but I did not notice any issues with 750GX =
I=20
am using - so far.

Thanks,
tavi

On Tue, 2009-01-06 at 14:55 +0200, Octavian Purdila wrote:

This may be static but it's still a global scope as far as kernel
symbols are concerned. Care to give it a slightly better name ? num_pmcs
would probably be already more telling.

Cheers,
Ben.

For ppc750 processors which use 4 performance counters instead of the
6 G4 uses but otherwise is compatible with G4.

Signed-off-by: Octavian Purdila <redacted>
---
 arch/powerpc/oprofile/op_model_7450.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/oprofile/op_model_7450.c b/arch/powerpc/oprofile/op_model_7450.c
index cc599eb..f8d36f9 100644
--- a/arch/powerpc/oprofile/op_model_7450.c
+++ b/arch/powerpc/oprofile/op_model_7450.c

@@ -29,7 +29,7 @@
 static unsigned long reset_value[OP_MAX_COUNTER];
 
 static int oprofile_running;
-static u32 mmcr0_val, mmcr1_val, mmcr2_val;
+static u32 mmcr0_val, mmcr1_val, mmcr2_val, num_pmcs;
 
 #define MMCR0_PMC1_SHIFT	6
 #define MMCR0_PMC2_SHIFT	0

@@ -88,13 +88,12 @@ static int fsl7450_cpu_setup(struct op_counter_config *ctr)
 
 	mtspr(SPRN_MMCR0, mmcr0_val);
 	mtspr(SPRN_MMCR1, mmcr1_val);
-	mtspr(SPRN_MMCR2, mmcr2_val);
+	if (num_pmcs > 4)
+		mtspr(SPRN_MMCR2, mmcr2_val);
 
 	return 0;
 }
 
-#define NUM_CTRS 6
-
 /* Configures the global settings for the countes on all CPUs. */
 static int fsl7450_reg_setup(struct op_counter_config *ctr,
 			     struct op_system_config *sys,

@@ -102,12 +101,13 @@ static int fsl7450_reg_setup(struct op_counter_config *ctr,
 {
 	int i;
 
+	num_pmcs = num_ctrs;
 	/* Our counters count up, and "count" refers to
 	 * how much before the next interrupt, and we interrupt
 	 * on overflow.  So we calculate the starting value
 	 * which will give us "count" until overflow.
 	 * Then we set the events on the enabled counters */
-	for (i = 0; i < NUM_CTRS; ++i)
+	for (i = 0; i < num_ctrs; ++i)
 		reset_value[i] = 0x80000000UL - ctr[i].count;
 
 	/* Set events for Counters 1 & 2 */

@@ -123,9 +123,10 @@ static int fsl7450_reg_setup(struct op_counter_config *ctr,
 
 	/* Set events for Counters 3-6 */
 	mmcr1_val = mmcr1_event3(ctr[2].event)
-		| mmcr1_event4(ctr[3].event)
-		| mmcr1_event5(ctr[4].event)
-		| mmcr1_event6(ctr[5].event);
+		| mmcr1_event4(ctr[3].event);
+	if (num_ctrs > 4)
+		mmcr1_val |= mmcr1_event5(ctr[4].event)
+			| mmcr1_event6(ctr[5].event);
 
 	mmcr2_val = 0;
 

@@ -139,7 +140,7 @@ static int fsl7450_start(struct op_counter_config *ctr)
 
 	mtmsr(mfmsr() | MSR_PMM);
 
-	for (i = 0; i < NUM_CTRS; ++i) {
+	for (i = 0; i < num_pmcs; ++i) {
 		if (ctr[i].enabled)
 			classic_ctr_write(i, reset_value[i]);
 		else

@@ -184,7 +185,7 @@ static void fsl7450_handle_interrupt(struct pt_regs *regs,
 	pc = mfspr(SPRN_SIAR);
 	is_kernel = is_kernel_addr(pc);
 
-	for (i = 0; i < NUM_CTRS; ++i) {
+	for (i = 0; i < num_pmcs; ++i) {
 		val = classic_ctr_read(i);
 		if (val < 0) {
 			if (oprofile_running && ctr[i].enabled) {

-- 
1.5.6.5

Signed-off-by: Octavian Purdila <redacted>
---
 arch/powerpc/kernel/cputable.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 923f87a..4e20cfb 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c

@@ -726,6 +726,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/7450",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750FX rev 2.0 must disable HID0[DPM] */
 		.pvr_mask		= 0xffffffff,

@@ -741,6 +743,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/7450",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750FX (All revs except 2.0) */
 		.pvr_mask		= 0xffff0000,

@@ -756,6 +760,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750fx,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/7450",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750GX */
 		.pvr_mask		= 0xffff0000,

-- 
1.5.6.5

quoted

 static int oprofile_running;
-static u32 mmcr0_val, mmcr1_val, mmcr2_val;
+static u32 mmcr0_val, mmcr1_val, mmcr2_val, ctrs;

This may be static but it's still a global scope as far as kernel
symbols are concerned. Care to give it a slightly better name ? num_pmcs
would probably be already more telling.

Sure. v2 will follow, with s/ctrs/num_pmcs/.

Thanks,
tavi

On Tue, Feb 24, 2009 at 6:09 AM, Octavian Purdila [off-list ref] wr=
ote:

e.c

__setup_cpu_750,

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.machine_check =A0 =A0 =A0 =A0 =A0=3D mach=

ine_check_generic,

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.platform =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D =

"ppc750",

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 .oprofile_cpu_type =A0 =A0 =A0=3D "ppc/7450=

",

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 .oprofile_type =A0 =A0 =A0 =A0 =A0=3D PPC_O=

PROFILE_G4,

=A0 =A0 =A0 =A0},

I know this saves you some code, but it seems hacky.  It would be
better to modify oprofile to detect the proper cpu type.  Also, this
will screw things up if you try to use the different event set that
the 750 has.

Also, one more concern is the long-standing errata which makes this
quite dangerous.  All of the versions of the 750 I'm aware of have a
bug where if a Performance Monitor exception occurs within one cycle
of the Decrementer exception, the cpu will lose the ability to return
from the interrupt (SRR0/SRR1 become corrupted).  It's possible the
750s you have modified to support oprofile don't have this errata.
Alternatively, we can decide we don't care, as you have to be root to
use oprofile.  But this is why I didn't add support for anything
before the 7450.

Andy

On Thu, 2009-02-26 at 16:30 -0600, Andy Fleming wrote:

I know this saves you some code, but it seems hacky.  It would be
better to modify oprofile to detect the proper cpu type.  Also, this
will screw things up if you try to use the different event set that
the 750 has.

Agreed. Note that Jack Miller (CC) has some patches for the oprofile
side.

Also, one more concern is the long-standing errata which makes this
quite dangerous.  All of the versions of the 750 I'm aware of have a
bug where if a Performance Monitor exception occurs within one cycle
of the Decrementer exception, the cpu will lose the ability to return
from the interrupt (SRR0/SRR1 become corrupted).  It's possible the
750s you have modified to support oprofile don't have this errata.
Alternatively, we can decide we don't care, as you have to be root to
use oprofile.  But this is why I didn't add support for anything
before the 7450.

I think we need to advertise it as broken in some way... I -think- the
latest batch of IBM 750CL have that bug fixed but I'm not 100% certain.

Cheers,
Ben.

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

On Thu, 2009-02-26 at 16:30 -0600, Andy Fleming wrote:

quoted

I know this saves you some code, but it seems hacky.  It would be
better to modify oprofile to detect the proper cpu type.  Also, this
will screw things up if you try to use the different event set that
the 750 has.

Agreed. Note that Jack Miller (CC) has some patches for the oprofile
side.

Not sure I understand, but the oprofile userspace  tool sees it as new 
processor (ppc750). I had to patch it to add the events for the new processor.

quoted

Also, one more concern is the long-standing errata which makes this
quite dangerous.  All of the versions of the 750 I'm aware of have a
bug where if a Performance Monitor exception occurs within one cycle
of the Decrementer exception, the cpu will lose the ability to return
from the interrupt (SRR0/SRR1 become corrupted).  It's possible the
750s you have modified to support oprofile don't have this errata.
Alternatively, we can decide we don't care, as you have to be root to
use oprofile.  But this is why I didn't add support for anything
before the 7450.

Yes, I understand. We knew about the errata and we used the timer interrupt 
for profiling for some time, but then we run into some issue were we had to use 
advanced counters as L1 cache misses / TLB misses to diagnose some issues and 
we decided to give it a try. And _seems_ to work fine. Although we didn't 
stress it too hard (around 10-20 minutes of continuous run).

So maybe it would be useful for other people in the same situation as ours. 
BTW, this is the out of cat /proc/cpuinfo on our hw:

processor       : 0
cpu             : 750GX
temperature     : 10-12 C (uncalibrated)
revision        : 1.2 (pvr 7002 0102)
bogomips        : 1597.44
vendor          : Ixia
machine         : TCPX [0x6b]

 

I think we need to advertise it as broken in some way... 

Perhaps set it to use the timer by default, and allow the user to switch to 
the hardware performance counter + printing a message that this mode is 
dangerous?

Thanks,
tavi

On Tue, 2009-01-06 at 14:55 +0200, Octavian Purdila wrote:

Signed-off-by: Octavian Purdila <redacted>

So I'm going to merge 1/2 but this one should really be changed to
advertise ppc/750 in oprofile_cpu_type (ie. to userspace).

Cheers,
Ben.

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

On Tue, 2009-01-06 at 14:55 +0200, Octavian Purdila wrote:

quoted

Signed-off-by: Octavian Purdila <redacted>

So I'm going to merge 1/2 but this one should really be changed to
advertise ppc/750 in oprofile_cpu_type (ie. to userspace).

Sure. Here is the new patch which uses ppc/750. It enables oprofile for all 3 FX variants and GX as well.

Thanks!
tavi


commit 70f4865a614e9b0ff4594ebd52b95f78e998b79f
Author: Octavian Purdila [off-list ref]
Date:   Tue Jan 6 12:51:43 2009 +0200

    powerpc: oprofile: enable support for ppc750 processors
    
    Signed-off-by: Octavian Purdila [off-list ref]

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 923f87a..c3ea72b 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c

@@ -726,6 +726,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/750",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750FX rev 2.0 must disable HID0[DPM] */
 		.pvr_mask		= 0xffffffff,

@@ -741,6 +743,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/750",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750FX (All revs except 2.0) */
 		.pvr_mask		= 0xffff0000,

@@ -756,6 +760,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750fx,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/750",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 750GX */
 		.pvr_mask		= 0xffff0000,

@@ -771,6 +777,8 @@ static struct cpu_spec __initdata cpu_specs[] = {
 		.cpu_setup		= __setup_cpu_750fx,
 		.machine_check		= machine_check_generic,
 		.platform		= "ppc750",
+		.oprofile_cpu_type      = "ppc/750",
+		.oprofile_type		= PPC_OPROFILE_G4,
 	},
 	{	/* 740/750 (L2CR bit need fixup for 740) */
 		.pvr_mask		= 0xffff0000,