Re: [PATCH] General CHRP/MPC5K2 platform support patch
From: Grant Likely <hidden>
Date: 2006-10-26 16:02:04
(minor) Whitespace is still inconsistent; mostly spaces used where they should be tabs and a few lines longer than 80 chars. Running the source through scripts/Lindent will help. On 10/26/06, Nicolas DET [off-list ref] wrote:
quoted hunk ↗ jump to hunk
--- a/arch/powerpc/sysdev/mpc52xx_pic.c 2006-10-25 19:07:24.000000000 +0200 +++ b/arch/powerpc/sysdev/mpc52xx_pic.c 2006-10-26 10:55:44.000000000 +0200@@ -0,0 +1,371 @@ +/* + * arch/powerpc/sysdev/mpc52xx_pic.c + * + * Programmable Interrupt Controller functions for the Freescale MPC52xx + * embedded CPU. + * Modified for CHRP Efika 5K2
Don't really need to add the "Modified for.." line; That's what git is for. Besides this code is modified for more than just the Efika now. :)
+ * + * Maintainer : Sylvain Munaut [off-list ref] + * + * Based on (well, mostly copied from) the code from the 2.4 kernel by + * Dale Farnsworth [off-list ref] and Kent Borg.
Ditto for this; but I know you didn't add it. :) Although this kind of comment is more justified because it captures information not maintained in git. (Namely where the original file came from.)
+ * + * Copyright (C) 2004 Sylvain Munaut [off-list ref] + * Copyright (C) 2003 Montavista Software, Inc
You should add a bplan copyright line too.
+ * + * 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. + */ + +//#define DEBUG
C++ style comment bad. :)
+ +#include <linux/stddef.h> +#include <linux/init.h> +#include <linux/sched.h> +#include <linux/signal.h> +#include <linux/stddef.h> +#include <linux/delay.h> +#include <linux/irq.h> + +#include <asm/io.h> +#include <asm/processor.h> +#include <asm/system.h> +#include <asm/irq.h> +#include <asm/prom.h> + +#include <asm/mpc52xx.h> + +static struct mpc52xx_intr __iomem *intr; +static struct mpc52xx_sdma __iomem *sdma; + +static struct irq_host *mpc52xx_irqhost = NULL; +
(snip)
quoted hunk ↗ jump to hunk
--- a/arch/powerpc/Kconfig 2006-10-25 19:07:23.000000000 +0200 +++ b/arch/powerpc/Kconfig 2006-10-26 11:32:54.000000000 +0200@@ -384,6 +384,12 @@ config PPC_CHRP select PPC_RTAS select PPC_MPC106 select PPC_UDBG_16550 + select PPC_MPC52xx_PIC + default y + +config PPC_MPC52xx_PIC + bool + depends on PPC_CHRP default y
This isn't good for embedded 52xx boards. Anything u-boot based cannot be CHRP. Please drop the "depends on" line
quoted hunk ↗ jump to hunk
--- a/arch/powerpc/platforms/chrp/setup.c 2006-10-25 19:07:23.000000000 +0200 +++ b/arch/powerpc/platforms/chrp/setup.c 2006-10-26 10:59:39.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,51 @@ static struct irqaction xmon_irqaction = }; #endif + +struct device_node *find_mpc52xx_pic(void) +{ + struct device_node *dev; + const char *piccompatible_list[] = + { + "mpc5200-interrupt-controller", + "mpc52xx-interrupt-controller", + "mpc52xx-pic", + "mpc5200-pic", + "5200-interrupt-controller", + "52xx-interrupt-controller", + "52xx-pic", + "5200-pic", + NULL + };
Considering Efika is the *only* CHRP 52xx board out there at the moment, and only a handful of embedded folks trying to move it to arch/powerpc, surely we can come to an agreement now on what this thing is named. :) Seriously, just recommend a name to use and everyone will use it. Providing a list of names means that every name in your list will be used.
quoted hunk ↗ jump to hunk
+ + /* Look for an MPC52xx interrupt controller */ + for_each_node_by_type(dev, "interrupt-controller") + { + const char **piccompatible_entry = piccompatible_list; + + for(piccompatible_entry = piccompatible_list; *piccompatible_entry; piccompatible_entry++ ) + { + if (device_is_compatible(dev, *piccompatible_entry )) + return dev; + } + } + + return NULL; +} + +static int __init chrp_find_mpc52xx_pic(void) +{ + if (find_mpc52xx_pic()) + { + printk(KERN_INFO "Found MPC52xx Interrupt Controller\n"); + ppc_md.get_irq = mpc52xx_get_irq; + mpc52xx_init_irq(); + return 0; + } + + return -ENODEV; +} + static void __init chrp_find_8259(void) { struct device_node *np, *pic = NULL;@@ -494,8 +540,12 @@ void __init chrp_init_IRQ(void) #if defined(CONFIG_VT) && defined(CONFIG_INPUT_ADBHID) && defined(XMON) struct device_node *kbd; #endif - chrp_find_openpic(); - chrp_find_8259(); + + if ( chrp_find_mpc52xx_pic() < 0) + { + chrp_find_openpic(); + chrp_find_8259(); + }
Why? Why not just add the call to chrp_find_mpc52xx_pic() before or after the other two chrp_find_ calls? Will calling them cause things to break? Keep the code simple. Cheers, g. -- Grant Likely, B.Sc. P.Eng. Secret Lab Technologies Ltd. grant.likely@secretlab.ca (403) 399-0195