LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix oprofile support for e500 in arch/powerpc
From: Andy Fleming @ 2006-10-27 20:06 UTC (permalink / raw)
  To: linuxppc-dev, linuxppc-embedded, Paul Mackerras, Kumar Gala; +Cc: Oprofile List

Fixed a compile error in building the 85xx support with oprofile, and in
the process cleaned up some issues with the fsl_booke performance monitor
code.

* Reorganized FSL Book-E performance monitoring code so that the 7450
  wouldn't be built if the e500 was, and cleaned it up so it was more
  self-contained.

* Added a cpu_setup function for FSL Book-E.  The original
  cpu_setup function prototype had no arguments, assuming that
  the reg_setup function would copy the required information into
  variables which represented the registers.  This was silly for
  e500, since it has 1 register per counter (rather than 3 for
  all counters), so the code has been restructured to have
  cpu_setup take the current counter config array as an argument,
  with op_powerpc_setup() invoking op_powerpc_cpu_setup() through
  on_each_cpu(), and op_powerpc_cpu_setup() invoking the
  model-specific cpu_setup function with an argument.  The
  argument is ignored on all other platforms at present.

* Fixed a confusing line where a trinary operator only had two
  arguments
Signed-off-by: Andrew Fleming <afleming@freescale.com>
---
This is a very slight respin of this patch: 
http://patchwork.ozlabs.org/linuxppc/patch?id=5531
Thanks to Vitaly Wool for noticing that this bug still existed!

 arch/powerpc/kernel/Makefile               |    1 
 arch/powerpc/kernel/perfmon_fsl_booke.c    |  221 ----------------------------
 arch/powerpc/kernel/pmc.c                  |    2 
 arch/powerpc/oprofile/Makefile             |    2 
 arch/powerpc/oprofile/common.c             |   10 +
 arch/powerpc/oprofile/op_model_7450.c      |    2 
 arch/powerpc/oprofile/op_model_fsl_booke.c |  170 +++++++++++++++++-----
 arch/powerpc/oprofile/op_model_power4.c    |    2 
 arch/powerpc/oprofile/op_model_rs64.c      |    2 
 include/asm-powerpc/oprofile_impl.h        |   87 +++++++++++
 include/asm-powerpc/pmc.h                  |   13 --
 11 files changed, 234 insertions(+), 278 deletions(-)

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 8b133af..7af23c4 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -38,7 +38,6 @@ obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6x
 obj-$(CONFIG_TAU)		+= tau_6xx.o
 obj32-$(CONFIG_SOFTWARE_SUSPEND) += swsusp_32.o
 obj32-$(CONFIG_MODULES)		+= module_32.o
-obj-$(CONFIG_E500)		+= perfmon_fsl_booke.o
 
 ifeq ($(CONFIG_PPC_MERGE),y)
 
diff --git a/arch/powerpc/kernel/perfmon_fsl_booke.c b/arch/powerpc/kernel/perfmon_fsl_booke.c
deleted file mode 100644
index e0dcf2b..0000000
--- a/arch/powerpc/kernel/perfmon_fsl_booke.c
+++ /dev/null
@@ -1,221 +0,0 @@
-/* arch/powerpc/kernel/perfmon_fsl_booke.c
- * Freescale Book-E Performance Monitor code
- *
- * Author: Andy Fleming
- * Copyright (c) 2004 Freescale Semiconductor, Inc
- *
- *  This program is free software; you can redistribute it and/or
- *  modify it under the terms of the GNU General Public License
- *  as published by the Free Software Foundation; either version
- *  2 of the License, or (at your option) any later version.
- */
-
-#include <linux/errno.h>
-#include <linux/sched.h>
-#include <linux/kernel.h>
-#include <linux/mm.h>
-#include <linux/stddef.h>
-#include <linux/unistd.h>
-#include <linux/ptrace.h>
-#include <linux/slab.h>
-#include <linux/user.h>
-#include <linux/a.out.h>
-#include <linux/interrupt.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/prctl.h>
-
-#include <asm/pgtable.h>
-#include <asm/uaccess.h>
-#include <asm/system.h>
-#include <asm/io.h>
-#include <asm/reg.h>
-#include <asm/xmon.h>
-#include <asm/pmc.h>
-
-static inline u32 get_pmlca(int ctr);
-static inline void set_pmlca(int ctr, u32 pmlca);
-
-static inline u32 get_pmlca(int ctr)
-{
-	u32 pmlca;
-
-	switch (ctr) {
-		case 0:
-			pmlca = mfpmr(PMRN_PMLCA0);
-			break;
-		case 1:
-			pmlca = mfpmr(PMRN_PMLCA1);
-			break;
-		case 2:
-			pmlca = mfpmr(PMRN_PMLCA2);
-			break;
-		case 3:
-			pmlca = mfpmr(PMRN_PMLCA3);
-			break;
-		default:
-			panic("Bad ctr number\n");
-	}
-
-	return pmlca;
-}
-
-static inline void set_pmlca(int ctr, u32 pmlca)
-{
-	switch (ctr) {
-		case 0:
-			mtpmr(PMRN_PMLCA0, pmlca);
-			break;
-		case 1:
-			mtpmr(PMRN_PMLCA1, pmlca);
-			break;
-		case 2:
-			mtpmr(PMRN_PMLCA2, pmlca);
-			break;
-		case 3:
-			mtpmr(PMRN_PMLCA3, pmlca);
-			break;
-		default:
-			panic("Bad ctr number\n");
-	}
-}
-
-void init_pmc_stop(int ctr)
-{
-	u32 pmlca = (PMLCA_FC | PMLCA_FCS | PMLCA_FCU |
-			PMLCA_FCM1 | PMLCA_FCM0);
-	u32 pmlcb = 0;
-
-	switch (ctr) {
-		case 0:
-			mtpmr(PMRN_PMLCA0, pmlca);
-			mtpmr(PMRN_PMLCB0, pmlcb);
-			break;
-		case 1:
-			mtpmr(PMRN_PMLCA1, pmlca);
-			mtpmr(PMRN_PMLCB1, pmlcb);
-			break;
-		case 2:
-			mtpmr(PMRN_PMLCA2, pmlca);
-			mtpmr(PMRN_PMLCB2, pmlcb);
-			break;
-		case 3:
-			mtpmr(PMRN_PMLCA3, pmlca);
-			mtpmr(PMRN_PMLCB3, pmlcb);
-			break;
-		default:
-			panic("Bad ctr number!\n");
-	}
-}
-
-void set_pmc_event(int ctr, int event)
-{
-	u32 pmlca;
-
-	pmlca = get_pmlca(ctr);
-
-	pmlca = (pmlca & ~PMLCA_EVENT_MASK) |
-		((event << PMLCA_EVENT_SHIFT) &
-		 PMLCA_EVENT_MASK);
-
-	set_pmlca(ctr, pmlca);
-}
-
-void set_pmc_user_kernel(int ctr, int user, int kernel)
-{
-	u32 pmlca;
-
-	pmlca = get_pmlca(ctr);
-
-	if(user)
-		pmlca &= ~PMLCA_FCU;
-	else
-		pmlca |= PMLCA_FCU;
-
-	if(kernel)
-		pmlca &= ~PMLCA_FCS;
-	else
-		pmlca |= PMLCA_FCS;
-
-	set_pmlca(ctr, pmlca);
-}
-
-void set_pmc_marked(int ctr, int mark0, int mark1)
-{
-	u32 pmlca = get_pmlca(ctr);
-
-	if(mark0)
-		pmlca &= ~PMLCA_FCM0;
-	else
-		pmlca |= PMLCA_FCM0;
-
-	if(mark1)
-		pmlca &= ~PMLCA_FCM1;
-	else
-		pmlca |= PMLCA_FCM1;
-
-	set_pmlca(ctr, pmlca);
-}
-
-void pmc_start_ctr(int ctr, int enable)
-{
-	u32 pmlca = get_pmlca(ctr);
-
-	pmlca &= ~PMLCA_FC;
-
-	if (enable)
-		pmlca |= PMLCA_CE;
-	else
-		pmlca &= ~PMLCA_CE;
-
-	set_pmlca(ctr, pmlca);
-}
-
-void pmc_start_ctrs(int enable)
-{
-	u32 pmgc0 = mfpmr(PMRN_PMGC0);
-
-	pmgc0 &= ~PMGC0_FAC;
-	pmgc0 |= PMGC0_FCECE;
-
-	if (enable)
-		pmgc0 |= PMGC0_PMIE;
-	else
-		pmgc0 &= ~PMGC0_PMIE;
-
-	mtpmr(PMRN_PMGC0, pmgc0);
-}
-
-void pmc_stop_ctrs(void)
-{
-	u32 pmgc0 = mfpmr(PMRN_PMGC0);
-
-	pmgc0 |= PMGC0_FAC;
-
-	pmgc0 &= ~(PMGC0_PMIE | PMGC0_FCECE);
-
-	mtpmr(PMRN_PMGC0, pmgc0);
-}
-
-void dump_pmcs(void)
-{
-	printk("pmgc0: %x\n", mfpmr(PMRN_PMGC0));
-	printk("pmc\t\tpmlca\t\tpmlcb\n");
-	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC0),
-			mfpmr(PMRN_PMLCA0), mfpmr(PMRN_PMLCB0));
-	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC1),
-			mfpmr(PMRN_PMLCA1), mfpmr(PMRN_PMLCB1));
-	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC2),
-			mfpmr(PMRN_PMLCA2), mfpmr(PMRN_PMLCB2));
-	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC3),
-			mfpmr(PMRN_PMLCA3), mfpmr(PMRN_PMLCB3));
-}
-
-EXPORT_SYMBOL(init_pmc_stop);
-EXPORT_SYMBOL(set_pmc_event);
-EXPORT_SYMBOL(set_pmc_user_kernel);
-EXPORT_SYMBOL(set_pmc_marked);
-EXPORT_SYMBOL(pmc_start_ctr);
-EXPORT_SYMBOL(pmc_start_ctrs);
-EXPORT_SYMBOL(pmc_stop_ctrs);
-EXPORT_SYMBOL(dump_pmcs);
diff --git a/arch/powerpc/kernel/pmc.c b/arch/powerpc/kernel/pmc.c
index a0a2efa..3d8f6f4 100644
--- a/arch/powerpc/kernel/pmc.c
+++ b/arch/powerpc/kernel/pmc.c
@@ -71,7 +71,7 @@ int reserve_pmc_hardware(perf_irq_t new_
 	}
 
 	pmc_owner_caller = __builtin_return_address(0);
-	perf_irq = new_perf_irq ? : dummy_perf;
+	perf_irq = new_perf_irq ? new_perf_irq : dummy_perf;
 
  out:
 	spin_unlock(&pmc_owner_lock);
diff --git a/arch/powerpc/oprofile/Makefile b/arch/powerpc/oprofile/Makefile
index 3145d61..0b5df9c 100644
--- a/arch/powerpc/oprofile/Makefile
+++ b/arch/powerpc/oprofile/Makefile
@@ -13,4 +13,4 @@ DRIVER_OBJS := $(addprefix ../../../driv
 oprofile-y := $(DRIVER_OBJS) common.o backtrace.o
 oprofile-$(CONFIG_PPC64) += op_model_rs64.o op_model_power4.o
 oprofile-$(CONFIG_FSL_BOOKE) += op_model_fsl_booke.o
-oprofile-$(CONFIG_PPC32) += op_model_7450.o
+oprofile-$(CONFIG_6xx) += op_model_7450.o
diff --git a/arch/powerpc/oprofile/common.c b/arch/powerpc/oprofile/common.c
index fd0bbbe..63bbef3 100644
--- a/arch/powerpc/oprofile/common.c
+++ b/arch/powerpc/oprofile/common.c
@@ -34,6 +34,11 @@ static void op_handle_interrupt(struct p
 	model->handle_interrupt(regs, ctr);
 }
 
+static void op_powerpc_cpu_setup(void *dummy)
+{
+	model->cpu_setup(ctr);
+}
+
 static int op_powerpc_setup(void)
 {
 	int err;
@@ -47,7 +52,7 @@ static int op_powerpc_setup(void)
 	model->reg_setup(ctr, &sys, model->num_counters);
 
 	/* Configure the registers on all cpus.  */
-	on_each_cpu(model->cpu_setup, NULL, 0, 1);
+	on_each_cpu(op_powerpc_cpu_setup, NULL, 0, 1);
 
 	return 0;
 }
@@ -142,7 +147,8 @@ #ifdef CONFIG_PPC64
 		case PPC_OPROFILE_POWER4:
 			model = &op_model_power4;
 			break;
-#else
+#endif
+#ifdef CONFIG_6xx
 		case PPC_OPROFILE_G4:
 			model = &op_model_7450;
 			break;
diff --git a/arch/powerpc/oprofile/op_model_7450.c b/arch/powerpc/oprofile/op_model_7450.c
index d8ee3ae..f481c0e 100644
--- a/arch/powerpc/oprofile/op_model_7450.c
+++ b/arch/powerpc/oprofile/op_model_7450.c
@@ -81,7 +81,7 @@ static void pmc_stop_ctrs(void)
 
 /* Configures the counters on this CPU based on the global
  * settings */
-static void fsl7450_cpu_setup(void *unused)
+static void fsl7450_cpu_setup(struct op_counter_config *ctr)
 {
 	/* freeze all counters */
 	pmc_stop_ctrs();
diff --git a/arch/powerpc/oprofile/op_model_fsl_booke.c b/arch/powerpc/oprofile/op_model_fsl_booke.c
index e29dede..0b3c31f 100644
--- a/arch/powerpc/oprofile/op_model_fsl_booke.c
+++ b/arch/powerpc/oprofile/op_model_fsl_booke.c
@@ -32,42 +32,152 @@ static unsigned long reset_value[OP_MAX_
 static int num_counters;
 static int oprofile_running;
 
-static inline unsigned int ctr_read(unsigned int i)
+static void init_pmc_stop(int ctr)
 {
-	switch(i) {
-		case 0:
-			return mfpmr(PMRN_PMC0);
-		case 1:
-			return mfpmr(PMRN_PMC1);
-		case 2:
-			return mfpmr(PMRN_PMC2);
-		case 3:
-			return mfpmr(PMRN_PMC3);
-		default:
-			return 0;
-	}
-}
+	u32 pmlca = (PMLCA_FC | PMLCA_FCS | PMLCA_FCU |
+			PMLCA_FCM1 | PMLCA_FCM0);
+	u32 pmlcb = 0;
 
-static inline void ctr_write(unsigned int i, unsigned int val)
-{
-	switch(i) {
+	switch (ctr) {
 		case 0:
-			mtpmr(PMRN_PMC0, val);
+			mtpmr(PMRN_PMLCA0, pmlca);
+			mtpmr(PMRN_PMLCB0, pmlcb);
 			break;
 		case 1:
-			mtpmr(PMRN_PMC1, val);
+			mtpmr(PMRN_PMLCA1, pmlca);
+			mtpmr(PMRN_PMLCB1, pmlcb);
 			break;
 		case 2:
-			mtpmr(PMRN_PMC2, val);
+			mtpmr(PMRN_PMLCA2, pmlca);
+			mtpmr(PMRN_PMLCB2, pmlcb);
 			break;
 		case 3:
-			mtpmr(PMRN_PMC3, val);
+			mtpmr(PMRN_PMLCA3, pmlca);
+			mtpmr(PMRN_PMLCB3, pmlcb);
 			break;
 		default:
-			break;
+			panic("Bad ctr number!\n");
 	}
 }
 
+static void set_pmc_event(int ctr, int event)
+{
+	u32 pmlca;
+
+	pmlca = get_pmlca(ctr);
+
+	pmlca = (pmlca & ~PMLCA_EVENT_MASK) |
+		((event << PMLCA_EVENT_SHIFT) &
+		 PMLCA_EVENT_MASK);
+
+	set_pmlca(ctr, pmlca);
+}
+
+static void set_pmc_user_kernel(int ctr, int user, int kernel)
+{
+	u32 pmlca;
+
+	pmlca = get_pmlca(ctr);
+
+	if(user)
+		pmlca &= ~PMLCA_FCU;
+	else
+		pmlca |= PMLCA_FCU;
+
+	if(kernel)
+		pmlca &= ~PMLCA_FCS;
+	else
+		pmlca |= PMLCA_FCS;
+
+	set_pmlca(ctr, pmlca);
+}
+
+static void set_pmc_marked(int ctr, int mark0, int mark1)
+{
+	u32 pmlca = get_pmlca(ctr);
+
+	if(mark0)
+		pmlca &= ~PMLCA_FCM0;
+	else
+		pmlca |= PMLCA_FCM0;
+
+	if(mark1)
+		pmlca &= ~PMLCA_FCM1;
+	else
+		pmlca |= PMLCA_FCM1;
+
+	set_pmlca(ctr, pmlca);
+}
+
+static void pmc_start_ctr(int ctr, int enable)
+{
+	u32 pmlca = get_pmlca(ctr);
+
+	pmlca &= ~PMLCA_FC;
+
+	if (enable)
+		pmlca |= PMLCA_CE;
+	else
+		pmlca &= ~PMLCA_CE;
+
+	set_pmlca(ctr, pmlca);
+}
+
+static void pmc_start_ctrs(int enable)
+{
+	u32 pmgc0 = mfpmr(PMRN_PMGC0);
+
+	pmgc0 &= ~PMGC0_FAC;
+	pmgc0 |= PMGC0_FCECE;
+
+	if (enable)
+		pmgc0 |= PMGC0_PMIE;
+	else
+		pmgc0 &= ~PMGC0_PMIE;
+
+	mtpmr(PMRN_PMGC0, pmgc0);
+}
+
+static void pmc_stop_ctrs(void)
+{
+	u32 pmgc0 = mfpmr(PMRN_PMGC0);
+
+	pmgc0 |= PMGC0_FAC;
+
+	pmgc0 &= ~(PMGC0_PMIE | PMGC0_FCECE);
+
+	mtpmr(PMRN_PMGC0, pmgc0);
+}
+
+static void dump_pmcs(void)
+{
+	printk("pmgc0: %x\n", mfpmr(PMRN_PMGC0));
+	printk("pmc\t\tpmlca\t\tpmlcb\n");
+	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC0),
+			mfpmr(PMRN_PMLCA0), mfpmr(PMRN_PMLCB0));
+	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC1),
+			mfpmr(PMRN_PMLCA1), mfpmr(PMRN_PMLCB1));
+	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC2),
+			mfpmr(PMRN_PMLCA2), mfpmr(PMRN_PMLCB2));
+	printk("%8x\t%8x\t%8x\n", mfpmr(PMRN_PMC3),
+			mfpmr(PMRN_PMLCA3), mfpmr(PMRN_PMLCB3));
+}
+
+static void fsl_booke_cpu_setup(struct op_counter_config *ctr)
+{
+	int i;
+
+	/* freeze all counters */
+	pmc_stop_ctrs();
+
+	for (i = 0;i < num_counters;i++) {
+		init_pmc_stop(i);
+
+		set_pmc_event(i, ctr[i].event);
+
+		set_pmc_user_kernel(i, ctr[i].user, ctr[i].kernel);
+	}
+}
 
 static void fsl_booke_reg_setup(struct op_counter_config *ctr,
 			     struct op_system_config *sys,
@@ -77,23 +187,14 @@ static void fsl_booke_reg_setup(struct o
 
 	num_counters = num_ctrs;
 
-	/* freeze all counters */
-	pmc_stop_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_counters; ++i) {
+	for (i = 0; i < num_counters; ++i)
 		reset_value[i] = 0x80000000UL - ctr[i].count;
 
-		init_pmc_stop(i);
-
-		set_pmc_event(i, ctr[i].event);
-
-		set_pmc_user_kernel(i, ctr[i].user, ctr[i].kernel);
-	}
 }
 
 static void fsl_booke_start(struct op_counter_config *ctr)
@@ -105,8 +206,8 @@ static void fsl_booke_start(struct op_co
 	for (i = 0; i < num_counters; ++i) {
 		if (ctr[i].enabled) {
 			ctr_write(i, reset_value[i]);
-			/* Set Each enabled counterd to only
-			 * count when the Mark bit is not set */
+			/* Set each enabled counter to only
+			 * count when the Mark bit is *not* set */
 			set_pmc_marked(i, 1, 0);
 			pmc_start_ctr(i, 1);
 		} else {
@@ -177,6 +278,7 @@ static void fsl_booke_handle_interrupt(s
 
 struct op_powerpc_model op_model_fsl_booke = {
 	.reg_setup		= fsl_booke_reg_setup,
+	.cpu_setup		= fsl_booke_cpu_setup,
 	.start			= fsl_booke_start,
 	.stop			= fsl_booke_stop,
 	.handle_interrupt	= fsl_booke_handle_interrupt,
diff --git a/arch/powerpc/oprofile/op_model_power4.c b/arch/powerpc/oprofile/op_model_power4.c
index 6a927ef..356709d 100644
--- a/arch/powerpc/oprofile/op_model_power4.c
+++ b/arch/powerpc/oprofile/op_model_power4.c
@@ -82,7 +82,7 @@ static inline int mmcra_must_set_sample(
 	return 0;
 }
 
-static void power4_cpu_setup(void *unused)
+static void power4_cpu_setup(struct op_counter_config *ctr)
 {
 	unsigned int mmcr0 = mmcr0_val;
 	unsigned long mmcra = mmcra_val;
diff --git a/arch/powerpc/oprofile/op_model_rs64.c b/arch/powerpc/oprofile/op_model_rs64.c
index 042f8f4..19c5ee0 100644
--- a/arch/powerpc/oprofile/op_model_rs64.c
+++ b/arch/powerpc/oprofile/op_model_rs64.c
@@ -102,7 +102,7 @@ static void rs64_reg_setup(struct op_cou
 	/* XXX setup user and kernel profiling */
 }
 
-static void rs64_cpu_setup(void *unused)
+static void rs64_cpu_setup(struct op_counter_config *ctr)
 {
 	unsigned int mmcr0;
 
diff --git a/include/asm-powerpc/oprofile_impl.h b/include/asm-powerpc/oprofile_impl.h
index 5b33994..07a10e5 100644
--- a/include/asm-powerpc/oprofile_impl.h
+++ b/include/asm-powerpc/oprofile_impl.h
@@ -42,7 +42,7 @@ struct op_powerpc_model {
 	void (*reg_setup) (struct op_counter_config *,
 			   struct op_system_config *,
 			   int num_counters);
-	void (*cpu_setup) (void *);
+	void (*cpu_setup) (struct op_counter_config *);
 	void (*start) (struct op_counter_config *);
 	void (*stop) (void);
 	void (*handle_interrupt) (struct pt_regs *,
@@ -121,7 +121,90 @@ #endif
 		break;
 	}
 }
-#endif /* !CONFIG_FSL_BOOKE */
+#else /* CONFIG_FSL_BOOKE */
+static inline u32 get_pmlca(int ctr)
+{
+	u32 pmlca;
+
+	switch (ctr) {
+		case 0:
+			pmlca = mfpmr(PMRN_PMLCA0);
+			break;
+		case 1:
+			pmlca = mfpmr(PMRN_PMLCA1);
+			break;
+		case 2:
+			pmlca = mfpmr(PMRN_PMLCA2);
+			break;
+		case 3:
+			pmlca = mfpmr(PMRN_PMLCA3);
+			break;
+		default:
+			panic("Bad ctr number\n");
+	}
+
+	return pmlca;
+}
+
+static inline void set_pmlca(int ctr, u32 pmlca)
+{
+	switch (ctr) {
+		case 0:
+			mtpmr(PMRN_PMLCA0, pmlca);
+			break;
+		case 1:
+			mtpmr(PMRN_PMLCA1, pmlca);
+			break;
+		case 2:
+			mtpmr(PMRN_PMLCA2, pmlca);
+			break;
+		case 3:
+			mtpmr(PMRN_PMLCA3, pmlca);
+			break;
+		default:
+			panic("Bad ctr number\n");
+	}
+}
+
+static inline unsigned int ctr_read(unsigned int i)
+{
+	switch(i) {
+		case 0:
+			return mfpmr(PMRN_PMC0);
+		case 1:
+			return mfpmr(PMRN_PMC1);
+		case 2:
+			return mfpmr(PMRN_PMC2);
+		case 3:
+			return mfpmr(PMRN_PMC3);
+		default:
+			return 0;
+	}
+}
+
+static inline void ctr_write(unsigned int i, unsigned int val)
+{
+	switch(i) {
+		case 0:
+			mtpmr(PMRN_PMC0, val);
+			break;
+		case 1:
+			mtpmr(PMRN_PMC1, val);
+			break;
+		case 2:
+			mtpmr(PMRN_PMC2, val);
+			break;
+		case 3:
+			mtpmr(PMRN_PMC3, val);
+			break;
+		default:
+			break;
+	}
+}
+
+
+#endif /* CONFIG_FSL_BOOKE */
+
 
 extern void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth);
 
diff --git a/include/asm-powerpc/pmc.h b/include/asm-powerpc/pmc.h
index 07d6a42..8588be6 100644
--- a/include/asm-powerpc/pmc.h
+++ b/include/asm-powerpc/pmc.h
@@ -32,18 +32,5 @@ #ifdef CONFIG_PPC64
 void power4_enable_pmcs(void);
 #endif
 
-#ifdef CONFIG_FSL_BOOKE
-void init_pmc_stop(int ctr);
-void set_pmc_event(int ctr, int event);
-void set_pmc_user_kernel(int ctr, int user, int kernel);
-void set_pmc_marked(int ctr, int mark0, int mark1);
-void pmc_start_ctr(int ctr, int enable);
-void pmc_start_ctrs(int enable);
-void pmc_stop_ctrs(void);
-void dump_pmcs(void);
-
-extern struct op_powerpc_model op_model_fsl_booke;
-#endif
-
 #endif /* __KERNEL__ */
 #endif /* _POWERPC_PMC_H */
-- 
1.4.2.3

^ permalink raw reply related

* Subject: [PATCH] powerpc: EEH recovery tweaks
From: Linas Vepstas @ 2006-10-27 21:12 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev, linux-kernel


Paul,

Please apply and forward upstream. This patch is not urgent,
it provides fixes for a code path that is not currently used
by any existing driver.

--linas

If one attempts to create a device driver recovery sequence that
does not depend on a hard reset of the device, but simply just
attempts to resume processing, then one discovers that the 
recovery sequence implemented on powerpc is not quite right.
This patch fixes this up.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 arch/powerpc/platforms/pseries/eeh.c        |    1 +
 arch/powerpc/platforms/pseries/eeh_driver.c |   13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.19-rc1-git11/arch/powerpc/platforms/pseries/eeh_driver.c
===================================================================
--- linux-2.6.19-rc1-git11.orig/arch/powerpc/platforms/pseries/eeh_driver.c	2006-10-26 16:21:06.000000000 -0500
+++ linux-2.6.19-rc1-git11/arch/powerpc/platforms/pseries/eeh_driver.c	2006-10-26 16:33:19.000000000 -0500
@@ -170,14 +170,19 @@ static void eeh_report_reset(struct pci_
 static void eeh_report_resume(struct pci_dev *dev, void *userdata)
 {
 	struct pci_driver *driver = dev->driver;
+	struct device_node *dn = pci_device_to_OF_node(dev);
 
 	dev->error_state = pci_channel_io_normal;
 
 	if (!driver)
 		return;
-	if (!driver->err_handler)
-		return;
-	if (!driver->err_handler->resume)
+
+	if ((PCI_DN(dn)->eeh_mode) & EEH_MODE_IRQ_DISABLED) {
+		PCI_DN(dn)->eeh_mode &= ~EEH_MODE_IRQ_DISABLED;
+		enable_irq(dev->irq);
+	}
+	if (!driver->err_handler ||
+	    !driver->err_handler->resume)
 		return;
 
 	driver->err_handler->resume(dev);
@@ -407,6 +412,8 @@ struct pci_dn * handle_eeh_events (struc
 
 		if (rc)
 			result = PCI_ERS_RESULT_NEED_RESET;
+		else
+			result = PCI_ERS_RESULT_RECOVERED;
 	}
 
 	/* If any device has a hard failure, then shut off everything. */

^ permalink raw reply

* Re: Building ELDK 3.0/3.1 from SRPMS
From: Wolfgang Denk @ 2006-10-27 21:19 UTC (permalink / raw)
  To: John W; +Cc: linuxppc-embedded
In-Reply-To: <20061027182114.24662.qmail@web50315.mail.yahoo.com>

Dear John,

in message <20061027182114.24662.qmail@web50315.mail.yahoo.com> you wrote:
> 
> I having trouble rebuilding ELDK 3.0 & ELDK 3.1 from
> the source RPMS.  I have spent a great deal of time on
> this, and I have a feeling that maybe I=92m just not
> doing something correctly (setting up the build
> environment, retrieving source/specs from CVS..
> etc..).  My build machine is Red Hat 9.0 without any
> updates (Kernel 2.4.20-8, the Shrike release).

This will not work; to build ELDK 3.x from scratch you need a  RH-7.3
box *with* all updates installed.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
e-credibility: the non-guaranteeable likelihood that  the  electronic
data you're seeing is genuine rather than somebody's made-up crap.
- Karl Lehenbauer

^ permalink raw reply

* Re: [POWERPC] Cleanup pegasos i8259 not in device tree workaround.
From: Benjamin Herrenschmidt @ 2006-10-27 21:44 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <116196732980-git-send-email-grant.likely@gmail.com>

On Fri, 2006-10-27 at 10:42 -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> Only make assumptions about i8259 presence if ppc_md.get_irq is not set.
> Previous workaround only checked chrp_mpic value.
> 
> Nicolas, please verify that this patch fixes your mpc52xx pic problems.
> Ben, I got tired of how often this question was being raised, so I just
> wrote this patch; I've compiled, but cannot test it.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
>  arch/powerpc/platforms/chrp/setup.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
> index 49b8dab..a5466ed 100644
> --- a/arch/powerpc/platforms/chrp/setup.c
> +++ b/arch/powerpc/platforms/chrp/setup.c
> @@ -451,7 +451,7 @@ static void __init chrp_find_8259(void)
>  	 * we have a pegasos that claims to be chrp but doesn't have
>  	 * a proper interrupt tree
>  	 */
> -	if (pic == NULL && chrp_mpic != NULL) {
> +	if (pic == NULL && ppc_md.get_irq != NULL) {
>  		printk(KERN_ERR "i8259: Not found in device-tree"
>  		       " assuming no legacy interrupts\n");
>  		return;

^ permalink raw reply

* Re: [PATCH] General CHRP/MPC5K2 platform support patch
From: Sylvain Munaut @ 2006-10-27 22:05 UTC (permalink / raw)
  To: Nicolas DET; +Cc: akpm, sl, linuxppc-dev, linuxppc-embedded, sha
In-Reply-To: <45421C97.80501@bplan-gmbh.de>

Hi Nicolas,

Here is a few comments. I'm not very familiar with the new irq stuff so
others
might have more insights.


Sylvain

> --- a/arch/powerpc/sysdev/mpc52xx_pic.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/arch/powerpc/sysdev/mpc52xx_pic.c	2006-10-27 15:58:29.000000000 +0200
> @@ -0,0 +1,414 @@
> +/*
> + * arch/powerpc/sysdev/mpc52xx_pic.c
>   
Looks like jdl is right, we apparently don't do that any more ... So
let's not ;)
> + * Based on (well, mostly copied from) the code from the 2.4 kernel by
> + * Dale Farnsworth <dfarnsworth@mvista.com> and Kent Borg.
>   
That can be removed ... We can't blame Dale anymore if it doesn't work ;)

> +
> +static void mpc52xx_ic_mask_and_ack(unsigned int irq)
> +{
> +	mpc52xx_ic_mask(irq);
> +	mpc52xx_ic_ack(irq);
> +}
>   
>From kernel/irq/chip.c that's done automatically if mask_and_ack is NULL.

> +
> +static struct irq_chip mpc52xx_irqchip = {
> +	.typename = " MPC52xx  ",
> +	.mask = mpc52xx_ic_mask,
> +	.unmask = mpc52xx_ic_unmask,
> +	.mask_ack = mpc52xx_ic_mask_and_ack,
> +};
Is it useful to implement set_type for IRQ[0-3] ? (Just asking ...)

> +	for (i = 0; i < NR_IRQS; i++) {
> +		irq_desc[i].chip = &mpc52xx_irqchip;
> +		irq_desc[i].status = IRQ_LEVEL;
> +
> +	}
>   
All LEVEL ?

> +
> +	/*
> +	 * As last step, add an irq host to translate the real
> +	 * hw irq information provided by the ofw to linux virq
> +	 */
> +
> +	mpc52xx_irqhost =
> +	    irq_alloc_host(IRQ_HOST_MAP_LINEAR, NR_IRQS, &mpc52xx_irqhost_ops,
> +			   -1);
> +}
>   
NR_IRQS ? Might be time to do something better.



> diff -uprN a/include/asm-powerpc/mpc52xx.h b/include/asm-powerpc/mpc52xx.h
> --- a/include/asm-powerpc/mpc52xx.h	1970-01-01 01:00:00.000000000 +0100
> +++ b/include/asm-powerpc/mpc52xx.h	2006-10-27 15:51:55.000000000 +0200
> @@ -0,0 +1,414 @@
> +/*
> + * include/asm-ppc/mpc52xx.h
> + * 
> + * Prototypes, etc. for the Freescale MPC52xx embedded cpu chips
> + * May need to be cleaned as the port goes on ...
> + *
> + *
> + * Maintainer : Sylvain Munaut <tnt@246tNt.com>
> + *
> + * Originally written by Dale Farnsworth <dfarnsworth@mvista.com> 
> + * for the 2.4 kernel.
>   
Again, remove all that (just leave the first line of the description)

> +
> +/* ======================================================================== */
> +/* IRQ mapping                                                              */
> +/* ======================================================================== */
> +
> +#define MPC52xx_IRQ_L1_CRIT	0
> +#define MPC52xx_IRQ_L1_MAIN	1
> +#define MPC52xx_IRQ_L1_PERP	2
> +#define MPC52xx_IRQ_L1_SDMA	3
> +
> +#define MPC52xx_IRQ_L1_OFFSET  (6)
> +#define MPC52xx_IRQ_L1_MASK    (0xc0)
> +
> +#define MPC52xx_IRQ_L2_OFFSET  (0)
> +#define MPC52xx_IRQ_L2_MASK    (0x3f)
>   
As benh suggested on IRC, a L1 offset of 8 might be better for
readability of the
hw irq numbers.

^ permalink raw reply

* Re: [PATCH] General CHRP/MPC5K2 platform support patch
From: Benjamin Herrenschmidt @ 2006-10-27 22:34 UTC (permalink / raw)
  To: Nicolas DET; +Cc: akpm, linuxppc-dev, sl, sha, linuxppc-embedded
In-Reply-To: <45421FFD.8070407@bplan-gmbh.de>

On Fri, 2006-10-27 at 17:04 +0200, Nicolas DET wrote:

> +static void mpc52xx_ic_mask(unsigned int virq)
> +{
> +	u32 val;
> +	int irq;
> +	int l1irq;
> +	int l2irq;
> +
> +	irq = irq_map[virq].hwirq;
> +	l1irq = (irq & MPC52xx_IRQ_L1_MASK) >> MPC52xx_IRQ_L1_OFFSET;
> +	l2irq = (irq & MPC52xx_IRQ_L2_MASK) >> MPC52xx_IRQ_L2_OFFSET;
> +
> +	pr_debug("%s: irq=%x. l1=%d, l2=%d\n", __func__, irq, l1irq, l2irq);
> +
> +	switch (l1irq) {
> +	case MPC52xx_IRQ_L1_CRIT:
> +		if (l2irq != 0)
> +			BUG();
> +
> +		val = in_be32(&intr->ctrl);
> +		val &= ~(1 << 11);
> +		out_be32(&intr->ctrl, val);
> +		break;
> +
> +	case MPC52xx_IRQ_L1_MAIN:
> +		if ( (l2irq >= 1) && (l2irq <= 3) ) {
> +			val = in_be32(&intr->ctrl);
> +			val &= ~(1 << (11 - l2irq));
> +			out_be32(&intr->ctrl, val);
> +		} else {
> +			val = in_be32(&intr->main_mask);
> +			val |= 1 << (16 - l2irq);
> +			out_be32(&intr->main_mask, val);
> +		}
> +		break;

Any reason why you do the above instead o defining two diferent levels
instead. Also, L1_CRIT would fit in the L1_MAIN case too...

I don't see the point of having also XXX-l2, just put a bit number in L2
and be done with it.

The idea is to streamling the code, such that you can index an array
with l1irq to get the register, and build a mask. No test, no switch
case, etc...

Actually... the most performant way of doing all this is to have a
different irq_chip (with a different set of ask,mask,unmask) for each
"L1" so that they get right to the point. But I would settle for an
array indexed by L1.

> +static void mpc52xx_ic_mask_and_ack(unsigned int irq)
> +{
> +	mpc52xx_ic_mask(irq);
> +	mpc52xx_ic_ack(irq);
> +}

The above is unnuecessary. The core will call mask and ack.

> +static struct irq_chip mpc52xx_irqchip = {
> +	.typename = " MPC52xx  ",
> +	.mask = mpc52xx_ic_mask,
> +	.unmask = mpc52xx_ic_unmask,
> +	.mask_ack = mpc52xx_ic_mask_and_ack,
> +};

In the above, you need to provide ack.

> +static int mpc52xx_irqhost_match(struct irq_host *h, struct device_node *node)
> +{
> +	pr_debug("%s: %p vs %p\n", __func__, find_mpc52xx_picnode(), node);
> +	return find_mpc52xx_picnode() == node;
> +}

Don't redo the whole find all the time. Put the node in your
irq_host->host_data at init time and compare it there. That way, also,
find_mpc53xx_picnode() thingy can just stay static to chrp/setup.c. The
way you did it would be a problem if we had more than one platform using
52xx built in the same kernel.

> +static int mpc52xx_islevel(int num)
> +{
> +	u32 ictl_req;
> +	int ictl_type;
> +
> +	ictl_req = in_be32(&intr->ctrl);
> +	ictl_type = (ictl_req >> 16) & 0x3;
> +
> +	switch (ictl_type) {
> +	case 0:
> +	case 3:
> +		return 1;
> +	}
> +
> +	return 0;
> +}

You only have level/edge settings, not polarity ? Also, you aren't
giving the user or device-tree the option to set the type manually....

Ideally, you should use the bits in IRQ_TYPE_SENSE_MASK to fully
define the irq type/polarity and provide a set_irq_type() callback.

Also, beware that you cannot call set_irq_chip_and_handler() from within
set_irq_type() due to a spinlock recursion issue. There is a patch
floating on the list for IPIC fixing an issue of that sort, you may want
to have a look.

In general, look at what others are doing, notably mpic and ipic.

You can also keep IRQ_LEVEL in "sync" with the other polarity bits, it's
really only useful to display the polarity in /proc/interrupts.

Note that your map code would always OR the bit, never clear it. It
should have probably cleared it before the switch/case.

> +void mpc52xx_irqhost_unmap(struct irq_host *h, unsigned int virq)
> +{
> +	pr_debug("%s: v=%x\n", __func__, virq);
> +
> +	mpc52xx_ic_mask(virq);
> +	set_irq_chip_and_handler(virq, NULL, NULL);
> +	synchronize_irq(virq);
> +}

The common code will do all of the above. Your unmap can be empty. (Or
just don't do an unmap).

> +static struct irq_host_ops mpc52xx_irqhost_ops = {
> +	.match = mpc52xx_irqhost_match,
> +	.xlate = mpc52xx_irqhost_xlate,
> +	.map = mpc52xx_irqhost_map,
> +	.unmap = mpc52xx_irqhost_unmap,
> +};
> +
> +void __init mpc52xx_init_irq(void)
> +{
> +	int i;
> +	u32 intr_ctrl;
> +
> +	/* Remap the necessary zones */
> +	intr = ioremap(MPC52xx_PA(MPC52xx_INTR_OFFSET), MPC52xx_INTR_SIZE);
> +	sdma = ioremap(MPC52xx_PA(MPC52xx_SDMA_OFFSET), MPC52xx_SDMA_SIZE);
> +
> +	if ((intr == NULL) || (sdma == NULL))
> +		panic("Can't ioremap PIC/SDMA register or init_irq !");
> +
> +	/* Disable all interrupt sources. */
> +	out_be32(&sdma->IntPend, 0xffffffff);	/* 1 means clear pending */
> +	out_be32(&sdma->IntMask, 0xffffffff);	/* 1 means disabled */
> +	out_be32(&intr->per_mask, 0x7ffffc00);	/* 1 means disabled */
> +	out_be32(&intr->main_mask, 0x00010fff);	/* 1 means disabled */
> +	intr_ctrl = in_be32(&intr->ctrl);
> +	intr_ctrl &= 0x00ff0000;	/* Keeps IRQ[0-3] config */
> +	intr_ctrl |= 0x0f000000 |	/* clear IRQ 0-3 */
> +	    0x00001000 |	/* MEE master external enable */
> +	    0x00000000 |	/* 0 means disable IRQ 0-3 */
> +	    0x00000001;		/* CEb route critical normally */
> +	out_be32(&intr->ctrl, intr_ctrl);
> +
> +	/* Zero a bunch of the priority settings.  */
> +	out_be32(&intr->per_pri1, 0);
> +	out_be32(&intr->per_pri2, 0);
> +	out_be32(&intr->per_pri3, 0);
> +	out_be32(&intr->main_pri1, 0);
> +	out_be32(&intr->main_pri2, 0);
> +	/* Initialize irq_desc[i].handler's with mpc52xx_ic. */
> +	for (i = 0; i < NR_IRQS; i++) {
> +		irq_desc[i].chip = &mpc52xx_irqchip;
> +		irq_desc[i].status = IRQ_LEVEL;
> +
> +	}

The above is completely bogus and should be just removed. You should not
touch the irq_desc array. You should only ever modify irq_desc's that
have been assigned to you by your host->map callback.

> +	/*
> +	 * As last step, add an irq host to translate the real
> +	 * hw irq information provided by the ofw to linux virq
> +	 */
> +
> +	mpc52xx_irqhost =
> +	    irq_alloc_host(IRQ_HOST_MAP_LINEAR, NR_IRQS, &mpc52xx_irqhost_ops,
> +			   -1);
> +}

As I said before, NR_IRQ is a poor choice for the size of the revmap.
You should have a constant somewhere defining what is your max HW irq
number. and use that +1, or a hw irq count.
> --- a/arch/powerpc/platforms/chrp/setup.c	2006-10-25 19:07:23.000000000 +0200
> +++ b/arch/powerpc/platforms/chrp/setup.c	2006-10-27 16:08:03.000000000 +0200
> @@ -51,6 +51,7 @@
>  #include <asm/mpic.h>
>  #include <asm/rtas.h>
>  #include <asm/xmon.h>
> +#include <asm/mpc52xx.h>
>  
>  #include "chrp.h"
>  
> @@ -435,6 +436,18 @@ static struct irqaction xmon_irqaction =
>  };
>  #endif
>  
> +static int __init chrp_find_mpc52xx_pic(void)
> +{
> +	if (find_mpc52xx_picnode()) {
> +		printk(KERN_INFO "Found MPC52xx Interrupt Controller\n");
> +		ppc_md.get_irq = mpc52xx_get_irq;
> +		mpc52xx_init_irq();
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}

Just get rif of find_mpic53xx_picnode. Do it locally and pass the device
node to mpc52xx_init_irq().

>  static void __init chrp_find_8259(void)
>  {
>  	struct device_node *np, *pic = NULL;
> @@ -496,6 +509,7 @@ void __init chrp_init_IRQ(void)
>  #endif
>  	chrp_find_openpic();
>  	chrp_find_8259();
> +	chrp_find_mpc52xx_pic();
>  
>  #ifdef CONFIG_SMP
>  	/* Pegasos has no MPIC, those ops would make it crash. It might be an
> diff -uprN a/include/asm-powerpc/mpc52xx.h b/include/asm-powerpc/mpc52xx.h
> --- a/include/asm-powerpc/mpc52xx.h	1970-01-01 01:00:00.000000000 +0100
> +++ b/include/asm-powerpc/mpc52xx.h	2006-10-27 15:51:55.000000000 +0200
> @@ -0,0 +1,414 @@
> +/*
> + * include/asm-powerpc/mpc52xx.h
> + * 
> + * Prototypes, etc. for the Freescale MPC52xx embedded cpu chips
> + * May need to be cleaned as the port goes on ...
> + *
> + *
> + * Maintainer : Sylvain Munaut <tnt@246tNt.com>
> + *
> + * Originally written by Dale Farnsworth <dfarnsworth@mvista.com> 
> + * for the 2.4 kernel.
> + *
> + * Copyright (C) 2004-2005 Sylvain Munaut <tnt@246tNt.com>
> + * Copyright (C) 2003 MontaVista, Software, Inc.
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#ifndef __ASM_POWERPC_MPC52xx_H__
> +#define __ASM_POWERPC_MPC52xx_H__
> +
> +#ifndef __ASSEMBLY__
> +#include <asm/types.h>
> +#include <asm/prom.h>
> +
> +#endif /* __ASSEMBLY__ */
> +
> +
> +#ifdef CONFIG_PCI
> +#define _IO_BASE	isa_io_base
> +#define _ISA_MEM_BASE	isa_mem_base
> +#define PCI_DRAM_OFFSET	pci_dram_offset
> +#else
> +#define _IO_BASE	0
> +#define _ISA_MEM_BASE	0
> +#define PCI_DRAM_OFFSET	0
> +#endif

Remove all of the above, it's bogus. You don't want to touch those
things when CONFIG_MULTIPLATFORM. (That is, they are variables set
programmatically, you can't just go #define them and common code already
takes care of defining them anyway).

Ben.

^ permalink raw reply

* Re: status of mpc52xx with arch=powerpc?
From: Benjamin Herrenschmidt @ 2006-10-27 22:39 UTC (permalink / raw)
  To: Sylvain Munaut; +Cc: linuxppc-embedded
In-Reply-To: <45411F14.6020308@246tNt.com>


> We would use the API we worked earlier on (Dale, Andrey, John, ...).
> To add support of fw preloaded task :
>  - The bestcomm.ko module would be used when bestcomm is completely
> controlled by the kernel
> - A different module but with the same exported function would be used
> when the fw has a part to play in the init. That allows to customize a bunch
> of the sdma init / sram mapping ...
> - All the other code (task helpers / the .h / the driver api) would be
> exactly
> the same.

No. Two different modules is the wrong approach. I want to be able to
build everything in the kernel (no modules) and still boot machines with
both types.

There should be a single implementation. I know bestcomm is fucked by
design but still.

> So the bestcomm modules maintain a list with all task loaded with a name and
> a index attribute. (The index would be used when there is multiple
> device with
> the same name, like "psc". We could use names like "psc1", "psc2". But that
> would imply string manipulation to "create" the name string ;)
> 
> In the case of the "classic" (kernel handles it all), the table of task
> is just
> initially empty. But for the "custom" case, it's filled with whatever
> the fw has
> passed as infos.

A single module should handle all cases though. If, for some reasons, it
looks like that will cause a lot of additional code, them it can be
internally split and we can have config options to enable only one way,
but it should be possible to enable both.

Ben.

^ permalink raw reply

* [PATCH] Switch rmb() to use the LWSYNC macro, instead of hardcoding it
From: Andy Fleming @ 2006-10-27 19:31 UTC (permalink / raw)
  To: linuxppc-dev, Paul Mackerras


Signed-off-by: Andrew Fleming <afleming@freescale.com>
---
e500 threw an illop when rmb() was called.  It apparently *doesn't* treat 
lwsync as an msync

 include/asm-powerpc/system.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-powerpc/system.h b/include/asm-powerpc/system.h
index 4362759..f7b1227 100644
--- a/include/asm-powerpc/system.h
+++ b/include/asm-powerpc/system.h
@@ -25,8 +25,8 @@ #include <asm/atomic.h>
  *
  * We have to use the sync instructions for mb(), since lwsync doesn't
  * order loads with respect to previous stores.  Lwsync is fine for
- * rmb(), though.  Note that lwsync is interpreted as sync by
- * 32-bit and older 64-bit CPUs.
+ * rmb(), though. Note that rmb() actually uses a sync on 32-bit
+ * architectures.
  *
  * For wmb(), we use sync since wmb is used in drivers to order
  * stores to system memory with respect to writes to the device.
@@ -34,7 +34,7 @@ #include <asm/atomic.h>
  * SMP since it is only used to order updates to system memory.
  */
 #define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ ("lwsync" : : : "memory")
+#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
 #define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define read_barrier_depends()  do { } while(0)
 
-- 
1.4.2.3

^ permalink raw reply related

* Re: [PATCH] General CHRP/MPC5K2 platform support patch
From: Stephen Rothwell @ 2006-10-28  0:27 UTC (permalink / raw)
  To: Jon Loeliger
  Cc: akpm, sl, linuxppc-dev@ozlabs.org, linuxppc-embedded@ozlabs.org,
	sha
In-Reply-To: <1161968926.2426.415.camel@cashmere.sps.mot.com>

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Fri, 27 Oct 2006 12:08:46 -0500 Jon Loeliger <jdl@freescale.com> wrote:
>
> On Fri, 2006-10-27 at 10:04, Nicolas DET wrote:
>
> > > diff -uprN a/include/asm-powerpc/mpc52xx.h b/include/asm-powerpc/mpc52xx.h
> > > --- a/include/asm-powerpc/mpc52xx.h	1970-01-01 01:00:00.000000000 +0100
> > > +++ b/include/asm-powerpc/mpc52xx.h	2006-10-27 15:51:55.000000000 +0200
> > > @@ -0,0 +1,414 @@
> > > +/*
> > > + * include/asm-ppc/mpc52xx.h
> > > + *
> >
> > Fixed to powerpc
>
> I thought we were removing the names of files from
> within the file itself, right?

Correct. They add nothing and become stale.

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* EFIKA vs. CHRP platform
From: Benjamin Herrenschmidt @ 2006-10-28  5:13 UTC (permalink / raw)
  To: Nicolas DET, Matt Sealey; +Cc: linuxppc-dev list, Paul Mackerras

Hi folks !

I've been thinking about this for a while, it's been nagging me... I
think we are doing a mistake with the Efika board (in fact, we did it
with Pegasos too).

It should just not use the CHRP platform, but instead, be a platform of
its own, part of the multiplatform support of arch/powerpc.

There are several reasons leading to that conclusion, but basically, I
can summarize it that way:

The whole _chrp_type thing is just a bad duplication of the existing
platform support. It's there for historical reasons, but we shouldn't
grow on it...

It basically ends up saying something like:

	platform (chrp) {
		if (pegasos)
			do_pegasos_stuff()
		else if (efika)
			do_efika_stuff()
		else if (motorola)
			do_motorola_stuff()
		....
		do_a_bit_of_common_stuff();
	}

Which really makes me think why do we bother making all this in platform
chrp and not have instead

	platform (pegasos) {
		do_pegasos_stuff();
		do_common_stuff(); <-- that is really common, like rtas
	}
	platform (efika) {
		do_efika_stuff();
		do_common_stuff();
	}

etc...

Basically, things like RTAS-PCI, or the various races of PCI host bridge
should just be "sysdev" code (library of devices) that are common to all
platforms and selected for compilation via "select" primitives be the
respective platforms.

There is simply no point in keeping this pseudo-generic-chrp thing, it
doesn't help, it only confuse things. right now, CHRP has to build-in
pegasos marvell bits, tomorrow, it will need some mpc52xx bits as well,
at the end of the day, why do we impose those to somebody who wants to
build a kernel for a small memory footprint old CHRP box ? So we might
end up having selectable sub-platform options at compile time ? That is
just duplicating what the platform mecanism does already.

So if we have bits in CHRP that we think might be needed by platform
efika, then we cut them out into a sysdev or lib of their own and have
CHRP and Efika "select" them.

The whole OF thing is not a reason for CHRP. PowerMacs have OF and
aren't CHRP, and we are getting more and more device-trees (wether they
are provided by a real OF interface or not is totally irrelevant to the
kernel structure) in all sorts of embedded platforms, that doesn't make
them CHRP...

In fact, we could almost think as the wole arch/powerpc is what oculd
have been CHRP nowadays... platform/chrp is support of legacy hardware
that followed precisely the old spec and HW set. Your board is different
enough from that to justify it's own platform code.

So here is your todo list:

 - change your device_type at the root of the device-tree, it's not chrp
anymore. Put whtever you want as long as you can identify your board.

 - do a platform efika. As for the location of it in the kernel source
tree, well, we have embedded6xx which is a bad name for "misc6xx". It's
pretty much a grab bag for platform code that doesn't need to spawn more
than one or two files. Feel free to create a separate platform dir if
your really want to but I doubt it's necessary. Maybe paulus can propose
a new name for "various" boards that fit in a single file... I would be
happy to move briq out of chrp too in fact. And pegasos :)

 - you can define a CONFIG_PPC_MPC52xx if you want. It would be selected
by the platform code and would include the various "base" drivers for
any MPC52xx board. That would maek the transition from arch/ppc easier
as well. Typically, that would typically include the PIC code. Maybe
bits of Bestcomm too.

 - I'm not sure using RTAS for PCI config space is a very good idea.
We'll need native mpc52xx_pci.c anyway, so we can juse use it for Efika
as well instead of taking the massive overhead of RTAS calls on PCI
config space accesses.

Cheers,
Ben

^ permalink raw reply

* Re: TEMAC drivers for kernel 2.6.17.1
From: David H. Lynch Jr. @ 2006-10-28  3:35 UTC (permalink / raw)
  To: Pradeep Sampath; +Cc: linuxppc-embedded
In-Reply-To: <20061027192025.26161.qmail@web83212.mail.mud.yahoo.com>

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

Pradeep Sampath wrote:
> Hi,
>  
> I am using linux kernel 2.6.17.1 on ML403 dev board. I am trying to 
> get the TEMAC to work but with no success. I applied TEMAC patches 
> from http://source.mvista.com/~ank/paulus-powerpc/20060309/ 
> <http://source.mvista.com/%7Eank/paulus-powerpc/20060309/>. But the 
> ethernet interface does not seem to come up. 
> Is there any other TEMAC patch out there which is known to work with 
> 2.6 kernel? 
>  
> For the BSP I tried using the ML403 EDK Embedded PowerPC reference 
> design from Xilinx. But it doesn't come with a PLB TEMAC in the 
> design. I also tried to use xapp902 which has the PLB TEMAC module. 
> But on running "Generate Libraries and BSPs" it does not generate any 
> sources code under "drivers" or "arch" folder.
I do nto beleive Xilinx has released an EDK that includes Linux 2.6 support.
The patches you linked to should work for a 2.6 kernel with a PLB TEMAC. 
But you must manually go in and force the speed to whatever you need. 
The MV driver at your link does nto autonegotiate.

I have a combined LL_TEMAC/PLB_TEMAC driver that does autonegotiate. It 
did work for the LL_TEMAC several iterations ago, but  I have not tested 
it in some time.
It does not yet  work for Linux. It is very loosely based on the MV 
driver, but does nt use the EDK, and is only for the LL_TEMAC and FIFO 
PLB_TEMAC.

There is a link to it in a previous message.

-- 
Dave Lynch 					  	    DLA Systems
Software Development:  				         Embedded Linux
717.627.3770 	       dhlii@dlasys.net 	  http://www.dlasys.net
fax: 1.253.369.9244 			           Cell: 1.717.587.7774
Over 25 years' experience in platforms, languages, and technologies too numerous to list.

"Any intelligent fool can make things bigger and more complex... It takes a touch of genius - and a lot of courage to move in the opposite direction."
Albert Einstein


[-- Attachment #2: Type: text/html, Size: 2593 bytes --]

^ permalink raw reply

* Re: [PATCH] Xilinx UART Lite 2.6.18 driver
From: David H. Lynch Jr. @ 2006-10-28  3:29 UTC (permalink / raw)
  To: Peter Korsgaard; +Cc: linuxppc-embedded
In-Reply-To: <877iylreca.fsf@sleipner.barco.com>

Peter Korsgaard wrote:
>
>  static int __init ulite_console_setup(struct console *co, char *options)
>  {
> +	int i, ret;
>  	struct uart_port *port;
> -
> +	struct platform_device *pdev;
> +	
>  	if (co->index < 0 || co->index >= ULITE_NR_UARTS)
>  		return -EINVAL;
>
>  	port = &ports[co->index];
>
>  	/* not initialized yet? */
> -	if (!port->membase)
> -		return -ENODEV;
> +	if (!port->membase) {
> +		/* We might be early console */
>
> Is this really necessary? The platform probe get's called quite early, E.G.:
>
> Breakpoint 2, ulite_probe (pdev=0xc01c84d0) at drivers/serial/uartlite.c:392
    There is a substantial time/code difference between the time  
ulite_console_setup() is called and the time the platform device is 
initiallized.
    If you are actually doing board bringup, you want output as soon as 
you can get it and you do not want to have to stuff calls to 
ppc_md.progress() to do so.

    Regardless, I think this is an issue of style and personal preference.
   
> 392     {
> (gdb) print __log_buf 
> $1 = "<5>Linux version 2.6.19-rc3 (peko@sleipner) (gcc version 3.4.5) #19 Fri Oct 27 16:39:00 CEST 2006\n<6>Barco ThinLite (V2P) <peter.korsgaard@barco.com>\n<7>Entering add_active_range(0, 0, 15360) 0 entrie"...
>
> You can always use the ppc_md.progress() stuff for really early
> debugging if needed. I would prefer to keep this workaround out of
> uartlite.c if it isn't needed.
>
>   
   
    
   I will absolutely agree that trying to resolve the the 
dissimilarities between the console_setup() call and the platform device 
init  is ugly.
    But I would have changed the platform device init code to use 
the uart_port structure in the platform device data as all the other 
drivers that have early console support do.

    At some point I suspect a cleaner more structured approach to 
modularizing the relationship between  early serial code and serial 
drivers will force this issue one way or another.

    But for now, if you are not going to bother making console_settup() 
work you might as well not put the rest of the early serial code in at all.


-- 
Dave Lynch 					  	    DLA Systems
Software Development:  				         Embedded Linux
717.627.3770 	       dhlii@dlasys.net 	  http://www.dlasys.net
fax: 1.253.369.9244 			           Cell: 1.717.587.7774
Over 25 years' experience in platforms, languages, and technologies too numerous to list.

"Any intelligent fool can make things bigger and more complex... It takes a touch of genius - and a lot of courage to move in the opposite direction."
Albert Einstein

^ permalink raw reply

* Is get_property() correct?
From: Grant Likely @ 2006-10-28  6:41 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, linuxppc-dev@ozlabs.org

Is the implementation of get_property correct?  The comment says it
returns the value of a property; but the return statement just returns
the property pointer (cast as void*) it got from of_find_property();
not the value.

Does the comment or the code need to change?

in prom.c:

/*
 * Find a property with a given name for a given node
 * and return the value.
 */
const void *get_property(const struct device_node *np, const char *name,
                         int *lenp)
{
        struct property *pp = of_find_property(np,name,lenp);
        return pp ? pp->value : NULL;
}
EXPORT_SYMBOL(get_property);

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: Is get_property() correct?
From: Michael Ellerman @ 2006-10-28  6:53 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <528646bc0610272341s3c655ebbn785b373f148f6174@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Sat, 2006-10-28 at 00:41 -0600, Grant Likely wrote:
> Is the implementation of get_property correct?  The comment says it
> returns the value of a property; but the return statement just returns
> the property pointer (cast as void*) it got from of_find_property();
> not the value.

Minor correction, it returns the property->value pointer as a void *.

> Does the comment or the code need to change?

I don't think so. get_property() can't return the actual value because
it doesn't know what type it is, so it returns a void pointer to the
value and it's up to the caller to interpret it based on the callers
knowledge of what it's expecting, and/or the lenp.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

^ permalink raw reply

* Re: Is get_property() correct?
From: Johannes Berg @ 2006-10-28  7:17 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <528646bc0610272341s3c655ebbn785b373f148f6174@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]


> The comment says it
> returns the value of a property

>  * and return the value.

>         return pp ? pp->value : NULL;

...? Looks like the value to me, unless the property doesn't exist.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply

* Re: Is get_property() correct?
From: Grant Likely @ 2006-10-28  7:23 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1162019855.14051.15.camel@johannes.berg>

Bah!  Wack me with the cluestick.  I've got temporary dyslexia tonight.

Sorry.

On 10/28/06, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > The comment says it
> > returns the value of a property
>
> >  * and return the value.
>
> >         return pp ? pp->value : NULL;
>
> ...? Looks like the value to me, unless the property doesn't exist.
>
> johannes
>
>
>


-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: Is get_property() correct?
From: Benjamin Herrenschmidt @ 2006-10-28  7:23 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <528646bc0610272341s3c655ebbn785b373f148f6174@mail.gmail.com>

On Sat, 2006-10-28 at 00:41 -0600, Grant Likely wrote:
> Is the implementation of get_property correct?  The comment says it
> returns the value of a property; but the return statement just returns
> the property pointer (cast as void*) it got from of_find_property();
> not the value.
> 
> Does the comment or the code need to change?

Well considering that we don't know the type of the property value, the
best we can do is return a pointer to it ... 

The comment might want an update, though it never stroke me as confusing
but heh :)

> in prom.c:
> 
> /*
>  * Find a property with a given name for a given node
>  * and return the value.
>  */
> const void *get_property(const struct device_node *np, const char *name,
>                          int *lenp)
> {
>         struct property *pp = of_find_property(np,name,lenp);
>         return pp ? pp->value : NULL;
> }
> EXPORT_SYMBOL(get_property);
> 
> Cheers,
> g.
> 

^ permalink raw reply

* Re: Is get_property() correct?
From: Grant Likely @ 2006-10-28  7:38 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1162020235.25682.124.camel@localhost.localdomain>

On 10/28/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sat, 2006-10-28 at 00:41 -0600, Grant Likely wrote:
>
> Well considering that we don't know the type of the property value, the
> best we can do is return a pointer to it ...
>
> The comment might want an update, though it never stroke me as confusing
> but heh :)

No, it's not confusing.  Both the comment and the code are correct.  I
just forgot how to read code.  :(

g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: Is get_property() correct?
From: Michael Ellerman @ 2006-10-28  9:08 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <528646bc0610280038x421014bdxde263c5399907fcb@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Sat, 2006-10-28 at 01:38 -0600, Grant Likely wrote:
> On 10/28/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > On Sat, 2006-10-28 at 00:41 -0600, Grant Likely wrote:
> >
> > Well considering that we don't know the type of the property value, the
> > best we can do is return a pointer to it ...
> >
> > The comment might want an update, though it never stroke me as confusing
> > but heh :)
> 
> No, it's not confusing.  Both the comment and the code are correct.  I
> just forgot how to read code.  :(

But at least now we're all _sure_ they're correct, some good still
done :)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

^ permalink raw reply

* ELDK 3.1.1 for Solaris
From: Theo Gjaltema @ 2006-10-28  9:44 UTC (permalink / raw)
  To: linuxppc-embedded

Hi there,
Has someone ELDK3.1.1 cross development tools compiled for Solaris?
I saw an impressive list of software to be installed before I can 
generate it myself.

Regards,
   Theo.

^ permalink raw reply

* Re: EFIKA vs. CHRP platform
From: Nicolas DET @ 2006-10-28 13:41 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Nicolas DET, Matt Sealey
  Cc: linuxppc-dev list, Paul Mackerras

----- original message --------

Subject: EFIKA vs. CHRP platform
Sent: Sat, 28 Oct 2006
From: "Benjamin Herrenschmidt" <benh@kernel.crashing.org>


>=20
>  - I'm not sure using RTAS for PCI config space is a very good idea.
> We'll need native mpc52xx_pci.c anyway, so we can juse use it for Efika
> as well instead of taking the massive overhead of RTAS calls on PCI
> config space accesses.
>=20

Compiling...
Efika will use as far as possible RTAS.

Bye

^ permalink raw reply

* Re: Re: [PATCH] General CHRP/MPC5K2 platform support patch
From: Nicolas DET @ 2006-10-28 13:43 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Nicolas DET
  Cc: akpm, linuxppc-dev, sl, sha, linuxppc-embedded

Subject: Re: [PATCH] General CHRP/MPC5K2 platform support patch
Sent: Sat, 28 Oct 2006
From: "Benjamin Herrenschmidt" <benh@kernel.crashing.org>

> On Fri, 2006-10-27 at 17:04 +0200, Nicolas DET wrote:
>=20
> > +static void mpc52xx_ic_mask(unsigned int virq)
> > +{
> > +=09u32 val;
> > +=09int irq;
> > +=09int l1irq;
> > +=09int l2irq;
> > +
> > +=09irq =3D irq_map[virq].hwirq;
> > +=09l1irq =3D (irq & MPC52xx_IRQ_L1_MASK) >> MPC52xx_IRQ_L1_OFFSET;
> > +=09l2irq =3D (irq & MPC52xx_IRQ_L2_MASK) >> MPC52xx_IRQ_L2_OFFSET;
> > +
> > +=09pr_debug("%s: irq=3D%x. l1=3D%d, l2=3D%dn", __func__, irq, l1irq, l=
2irq);
> > +
> > +=09switch (l1irq) {
> > +=09case MPC52xx_IRQ_L1_CRIT:
> > +=09=09if (l2irq !=3D 0)
> > +=09=09=09BUG();
> > +
> > +=09=09val =3D in_be32(&intr->ctrl);
> > +=09=09val &=3D ~(1 << 11);
> > +=09=09out_be32(&intr->ctrl, val);
> > +=09=09break;
> > +
> > +=09case MPC52xx_IRQ_L1_MAIN:
> > +=09=09if ( (l2irq >=3D 1) && (l2irq <=3D 3) ) {
> > +=09=09=09val =3D in_be32(&intr->ctrl);
> > +=09=09=09val &=3D ~(1 << (11 - l2irq));
> > +=09=09=09out_be32(&intr->ctrl, val);
> > +=09=09} else {
> > +=09=09=09val =3D in_be32(&intr->main_mask);
> > +=09=09=09val |=3D 1 << (16 - l2irq);
> > +=09=09=09out_be32(&intr->main_mask, val);
> > +=09=09}
> > +=09=09break;
>=20
> Any reason why you do the above instead o defining two diferent levels
> instead. Also, L1_CRIT would fit in the L1_MAIN case too...
>=20
> I don't see the point of having also XXX-l2, just put a bit number in L2
> and be done with it.
>=20
> The idea is to streamling the code, such that you can index an array
> with l1irq to get the register, and build a mask. No test, no switch
> case, etc...
>=20
> Actually... the most performant way of doing all this is to have a
> different irq_chip (with a different set of ask,mask,unmask) for each
> "L1" so that they get right to the point. But I would settle for an
> array indexed by L1.
>=20

Ok. Let's go for the most performant then. So 4 irq_chip for the for type. =
Each
 is very simple: just clear/set the approprite bit in the appropriate regis=
ter.



> > +static void mpc52xx_ic_mask_and_ack(unsigned int irq)
> > +{
> > +=09mpc52xx_ic_mask(irq);
> > +=09mpc52xx_ic_ack(irq);
> > +}
>=20
> The above is unnuecessary. The core will call mask and ack.

Ok. Removed.

> > +static struct irq_chip mpc52xx_irqchip =3D {
> > +=09.typename =3D " MPC52xx  ",
> > +=09.mask =3D mpc52xx_ic_mask,
> > +=09.unmask =3D mpc52xx_ic_unmask,
> > +=09.mask_ack =3D mpc52xx_ic_mask_and_ack,
> > +};
>=20
> In the above, you need to provide ack.

Done.

> > +static int mpc52xx_irqhost_match(struct irq_host *h, struct device_nod=
e
> *node)
> > +{
> > +=09pr_debug("%s: %p vs %pn", __func__, find_mpc52xx_picnode(), node);
> > +=09return find_mpc52xx_picnode() =3D=3D node;
> > +}
>=20
> Don't redo the whole find all the time. Put the node in your
> irq_host->host_data at init time and compare it there. That way, also,
> find_mpc53xx_picnode() thingy can just stay static to chrp/setup.c. The
> way you did it would be a problem if we had more than one platform using
> 52xx built in the same kernel.
>

done.

> > +static int mpc52xx_islevel(int num)
> > +{
> > +=09u32 ictl_req;
> > +=09int ictl_type;
> > +
> > +=09ictl_req =3D in_be32(&intr->ctrl);
> > +=09ictl_type =3D (ictl_req >> 16) & 0x3;
> > +
> > +=09switch (ictl_type) {
> > +=09case 0:
> > +=09case 3:
> > +=09=09return 1;
> > +=09}
> > +
> > +=09return 0;
> > +}
>=20
> You only have level/edge settings, not polarity ? Also, you aren't
> giving the user or device-tree the option to set the type manually....
>=20
> Ideally, you should use the bits in IRQ_TYPE_SENSE_MASK to fully
> define the irq type/polarity and provide a set_irq_type() callback.
>=20
> Also, beware that you cannot call set_irq_chip_and_handler() from within
> set_irq_type() due to a spinlock recursion issue. There is a patch
> floating on the list for IPIC fixing an issue of that sort, you may want
> to have a look.
>=20
> In general, look at what others are doing, notably mpic and ipic.

I looked in i8258.c, let's look into others ;-)

> You can also keep IRQ_LEVEL in "sync" with the other polarity bits, it's
> really only useful to display the polarity in /proc/interrupts.
>=20
> Note that your map code would always OR the bit, never clear it. It
> should have probably cleared it before the switch/case.


> > +void mpc52xx_irqhost_unmap(struct irq_host *h, unsigned int virq)
> > +{
> > +=09pr_debug("%s: v=3D%xn", __func__, virq);
> > +
> > +=09mpc52xx_ic_mask(virq);
> > +=09set_irq_chip_and_handler(virq, NULL, NULL);
> > +=09synchronize_irq(virq);
> > +}
>=20
> The common code will do all of the above. Your unmap can be empty. (Or
> just don't do an unmap).
>=20

Ok.

> > +static struct irq_host_ops mpc52xx_irqhost_ops =3D {
> > +=09.match =3D mpc52xx_irqhost_match,
> > +=09.xlate =3D mpc52xx_irqhost_xlate,
> > +=09.map =3D mpc52xx_irqhost_map,
> > +=09.unmap =3D mpc52xx_irqhost_unmap,
> > +};
> > +
> > +void __init mpc52xx_init_irq(void)
> > +{
> > +=09int i;
> > +=09u32 intr_ctrl;
> > +
> > +=09/* Remap the necessary zones */
> > +=09intr =3D ioremap(MPC52xx_PA(MPC52xx_INTR_OFFSET), MPC52xx_INTR_SIZE=
);
> > +=09sdma =3D ioremap(MPC52xx_PA(MPC52xx_SDMA_OFFSET), MPC52xx_SDMA_SIZE=
);
> > +
> > +=09if ((intr =3D=3D NULL) || (sdma =3D=3D NULL))
> > +=09=09panic("Can't ioremap PIC/SDMA register or init_irq !");
> > +
> > +=09/* Disable all interrupt sources. */
> > +=09out_be32(&sdma->IntPend, 0xffffffff);=09/* 1 means clear pending */
> > +=09out_be32(&sdma->IntMask, 0xffffffff);=09/* 1 means disabled */
> > +=09out_be32(&intr->per_mask, 0x7ffffc00);=09/* 1 means disabled */
> > +=09out_be32(&intr->main_mask, 0x00010fff);=09/* 1 means disabled */
> > +=09intr_ctrl =3D in_be32(&intr->ctrl);
> > +=09intr_ctrl &=3D 0x00ff0000;=09/* Keeps IRQ[0-3] config */
> > +=09intr_ctrl |=3D 0x0f000000 |=09/* clear IRQ 0-3 */
> > +=09    0x00001000 |=09/* MEE master external enable */
> > +=09    0x00000000 |=09/* 0 means disable IRQ 0-3 */
> > +=09    0x00000001;=09=09/* CEb route critical normally */
> > +=09out_be32(&intr->ctrl, intr_ctrl);
> > +
> > +=09/* Zero a bunch of the priority settings.  */
> > +=09out_be32(&intr->per_pri1, 0);
> > +=09out_be32(&intr->per_pri2, 0);
> > +=09out_be32(&intr->per_pri3, 0);
> > +=09out_be32(&intr->main_pri1, 0);
> > +=09out_be32(&intr->main_pri2, 0);
> > +=09/* Initialize irq_desc[i].handler's with mpc52xx_ic. */
> > +=09for (i =3D 0; i < NR_IRQS; i++) {
> > +=09=09irq_desc[i].chip =3D &mpc52xx_irqchip;
> > +=09=09irq_desc[i].status =3D IRQ_LEVEL;
> > +
> > +=09}
>=20
> The above is completely bogus and should be just removed. You should not
> touch the irq_desc array. You should only ever modify irq_desc's that
> have been assigned to you by your host->map callback.
>=20

Ok.

> > +=09/*
> > +=09 * As last step, add an irq host to translate the real
> > +=09 * hw irq information provided by the ofw to linux virq
> > +=09 */
> > +
> > +=09mpc52xx_irqhost =3D
> > +=09    irq_alloc_host(IRQ_HOST_MAP_LINEAR, NR_IRQS, &mpc52xx_irqhost_o=
ps,
> > +=09=09=09   -1);
> > +}
>=20
> As I said before, NR_IRQ is a poor choice for the size of the revmap.
> You should have a constant somewhere defining what is your max HW irq
> number. and use that +1, or a hw irq count.

Done: 64

Bye

^ permalink raw reply

* Re: [PATCH -rt] powerpc update
From: Sergei Shtylyov @ 2006-10-28 15:05 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Daniel Walker, tglx, linux-kernel, linuxppc-dev
In-Reply-To: <20061018072858.GA29576@elte.hu>

Hello.

Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:

>>Pay close attention to the fasteoi interrupt threading. I added usage 
>>of mask/unmask instead of using level handling, which worked well on 
>>PPC.

> this is wrong - it should be doing mask+ack.

    It's what it was doing effectively. And what was wrong was calling ack() 
which OpenPIC driver didn't (and was not obliged to) support.

> also note that you changed:

>>-		goto out_unlock;

> to:

>>+		goto out;

> and you even tried to hide your tracks:
> 
> 
>> out:
>> 	desc->chip->eoi(irq);
>>-out_unlock:
>> 	spin_unlock(&desc->lock);

> :-)

> really, the ->eoi() op should only be called for true fasteoi cases. 

    Why is that? eoi() is effectively the same as ack() in this case. I must 
note that what's the "standard" 8259 driver is doing in mask_ack() seems 
misleading since it actually masks IRQ and sends EOI there.

> What we want here is to turn the fasteoi handler into a handler that 
> does mask+ack and then unmask. Not 'mask+eoi ... unmask' as your patch 
> does.

    That's effectively the same for OpenPIC. Maybe that implemetation just 
didn't look graceful but it was *correct*. And the current one is at least 
incomplete.

    I can see 3 ways to get out of this situation now:

1. Revert this change and use mask() + eoi() approach suggested by Daniel.

2. Add the ack() handler to OpenPIC driver -- and point it to mpic_eoi().

3. Do the same as x86 APIC driver does and use level/egde flows instead of 
fasteoi for the case when IRQs are threaded -- that ensues doing (2) as well.

    Note that all three aproaches lead to the effectively the same behavior 
WRT OpenPIC (except for the edge-triggered IRQs in 3rd case). Opinions?

> 	Ingo

WBR, Sergei

^ permalink raw reply

* Re: [PATCH -rt] powerpc update
From: Sergei Shtylyov @ 2006-10-28 15:33 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Daniel Walker, Ingo Molnar, tglx, linux-kernel
In-Reply-To: <454371AC.4030902@ru.mvista.com>

Hello, I wrote:

> 3. Do the same as x86 APIC driver does and use level/egde flows instead 
> of fasteoi for the case when IRQs are threaded -- that ensues doing (2) 
> as well.

    Note that OpenPIC (as far as I could understand its h/w) is essentially
the same as IOAPIC/LAPIC couple on x86, i.e. a level-triggered IRQ remains 
effectively globally masked until CPU writes to its local EOI reg. However, 
the repetitive edge-triggered IRQ from the same line may still be detected 
while being serviced on a CPU (hm, the questions is, can it be sent to any 
other CPU in system while being handled on a certain CPU?)...
    I hope somebody corrects me if I'm wrong about both x86 and PPC. :-)

>>     Ingo

WBR, Sergei

^ permalink raw reply

* Re: Re: [PATCH] General CHRP/MPC5K2 platform support patch
From: Benjamin Herrenschmidt @ 2006-10-28 20:12 UTC (permalink / raw)
  To: Nicolas DET; +Cc: akpm, linuxppc-dev, sl, sha, linuxppc-embedded
In-Reply-To: <200610281343.k9SDhTJm020310@post.webmailer.de>


> > As I said before, NR_IRQ is a poor choice for the size of the revmap.
> > You should have a constant somewhere defining what is your max HW irq
> > number. and use that +1, or a hw irq count.
> 
> Done: 64

Even with a new numbering invloving the category in top bits and the bit
number in the bottom ?

Ben.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox