[PATCH 3/6] MPC5121 Add generic board support

[Grant Likely]

Mostly looks good, but a few comments below.

On Fri, Jun 20, 2008 at 10:58:36AM -0600, John Rigby wrote:
> Move shared code from mpc5121_ads.c to mpc512x_shared.c.
> Add new generic board setup mpc5121_generic.c
> 
> Signed-off-by: John Rigby <jrigby at freescale.com>
> ---
>  arch/powerpc/platforms/512x/Kconfig           |   15 ++++-
>  arch/powerpc/platforms/512x/Makefile          |    3 +-
>  arch/powerpc/platforms/512x/mpc5121_ads.c     |   45 +---------------
>  arch/powerpc/platforms/512x/mpc5121_generic.c |   72 +++++++++++++++++++++++++
>  arch/powerpc/platforms/512x/mpc512x.h         |   14 +++++
>  arch/powerpc/platforms/512x/mpc512x_shared.c  |   66 ++++++++++++++++++++++
>  6 files changed, 168 insertions(+), 47 deletions(-)
>  create mode 100644 arch/powerpc/platforms/512x/mpc5121_generic.c
>  create mode 100644 arch/powerpc/platforms/512x/mpc512x.h
>  create mode 100644 arch/powerpc/platforms/512x/mpc512x_shared.c
> 
> diff --git a/arch/powerpc/platforms/512x/Kconfig b/arch/powerpc/platforms/512x/Kconfig
> index 4c0da0c..f9a04da 100644
> --- a/arch/powerpc/platforms/512x/Kconfig
> +++ b/arch/powerpc/platforms/512x/Kconfig
> @@ -2,12 +2,10 @@ config PPC_MPC512x
>  	bool
>  	select FSL_SOC
>  	select IPIC
> -	default n
>  
>  config PPC_MPC5121
>  	bool
>  	select PPC_MPC512x
> -	default n
>  
>  config MPC5121_ADS
>  	bool "Freescale MPC5121E ADS"
> @@ -16,4 +14,15 @@ config MPC5121_ADS
>  	select PPC_MPC5121
>  	help
>  	  This option enables support for the MPC5121E ADS board.
> -	default n
> +
> +config MPC5121_GENERIC
> +	bool "Generic support for simple MPC5121 based boards"
> +	depends on PPC_MULTIPLATFORM && PPC32
> +	select DEFAULT_UIMAGE
> +	select PPC_MPC5121
> +	help
> +	  This option enables support for simple MPC5121 based boards
> +	  which do not need custome platform specific setup.

typo

> +
> +	  Compatible boards include:  Protonic LVT base boards (ZANMCU
> +	  and VICVT2).
> diff --git a/arch/powerpc/platforms/512x/Makefile b/arch/powerpc/platforms/512x/Makefile
> index ef6c925..e6674c8 100644
> --- a/arch/powerpc/platforms/512x/Makefile
> +++ b/arch/powerpc/platforms/512x/Makefile
> @@ -1,5 +1,6 @@
>  #
>  # Makefile for the Freescale PowerPC 512x linux kernel.
>  #
> -obj-y				:= clock.o
> +obj-y				:= clock.o mpc512x_shared.o
>  obj-$(CONFIG_MPC5121_ADS)	+= mpc5121_ads.o
> +obj-$(CONFIG_MPC5121_GENERIC)	+= mpc5121_generic.o
> diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c
> index 50bd3a3..45bb2ef 100644
> --- a/arch/powerpc/platforms/512x/mpc5121_ads.c
> +++ b/arch/powerpc/platforms/512x/mpc5121_ads.c
> @@ -68,20 +40,7 @@ static void __init mpc5121_ads_declare_of_platform_devices(void)
>  
>  static void __init mpc5121_ads_init_IRQ(void)
>  {
> -	struct device_node *np;
> -
> -	np = of_find_compatible_node(NULL, NULL, "fsl,ipic");
> -	if (!np)
> -		return;
> -
> -	ipic_init(np, 0);
> -	of_node_put(np);
> -
> -	/*
> -	 * Initialize the default interrupt mapping priorities,
> -	 * in case the boot rom changed something on us.
> -	 */
> -	ipic_set_default_priority();
> +	mpc512x_init_IRQ();
>  }

Why not just put mpc512x_init_IRQ directly into the machine structure?
>  
>  /*
> diff --git a/arch/powerpc/platforms/512x/mpc5121_generic.c b/arch/powerpc/platforms/512x/mpc5121_generic.c
> new file mode 100644
> index 0000000..0111a98
> --- /dev/null
> +++ b/arch/powerpc/platforms/512x/mpc5121_generic.c
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: John Rigby, <jrigby at freescale.com>
> + *
> + * Description:
> + * MPC5121 SoC setup
> + *
> + * This 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/kernel.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/machdep.h>
> +#include <asm/ipic.h>
> +#include <asm/prom.h>
> +#include <asm/time.h>
> +
> +#include "mpc512x.h"
> +
> +static struct of_device_id __initdata of_bus_ids[] = {
> +	{ .name = "soc", },
> +	{ .name = "localbus", },

Ugh.  Not good.  Bind against compatible, not name or type.

> +	{},
> +};
> +
> +static void __init mpc5121_generic_declare_of_platform_devices(void)
> +{
> +	/* Find every child of the SOC node and add it to of_platform */
> +	if (of_platform_bus_probe(NULL, of_bus_ids, NULL))
> +		printk(KERN_ERR __FILE__ ": "
> +			"Error while probing of_platform bus\n");
> +}
> +
> +/*
> + * list of supported boards
> + */
> +static char *board[] __initdata = {
> +	"prt,prtlvt",
> +	NULL
> +};
> +
> +/*
> + * Called very early, MMU is off, device-tree isn't unflattened
> + */
> +static int __init mpc5121_generic_probe(void)
> +{
> +	unsigned long node = of_get_flat_dt_root();
> +	int i = 0;
> +
> +	while (board[i]) {
> +		if (of_flat_dt_is_compatible(node, board[i]))
> +			break;
> +		i++;
> +	}
> +
> +	return board[i] != NULL;
> +}
> +
> +define_machine(mpc5121_generic) {
> +	.name			= "MPC5121 generic",
> +	.probe			= mpc5121_generic_probe,
> +	.init			= mpc5121_generic_declare_of_platform_devices,
> +	.init_IRQ		= mpc512x_init_IRQ,
> +	.get_irq		= ipic_get_irq,
> +	.calibrate_decr		= generic_calibrate_decr,
> +};
> diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platforms/512x/mpc512x.h
> new file mode 100644
> index 0000000..789b817
> --- /dev/null
> +++ b/arch/powerpc/platforms/512x/mpc512x.h
> @@ -0,0 +1,14 @@
> +/*
> + * Copyright (C) 2007 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * 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.
> + */

Should probably state what this file is for in the header block.

> diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
> new file mode 100644
> index 0000000..4b8fe6a
> --- /dev/null
> +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: John Rigby <jrigby at freescale.com>
> + *
> + * Description:
> + * MPC512x Shared code
> + *
> + * This 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/kernel.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/machdep.h>
> +#include <asm/ipic.h>
> +#include <asm/prom.h>
> +#include <asm/time.h>
> +
> +#include "mpc512x.h"
> +
> +unsigned long
> +mpc512x_find_ips_freq(struct device_node *node)
> +{
> +	struct device_node *np;
> +	const unsigned int *p_ips_freq = NULL;
> +
> +	of_node_get(node);
> +	while (node) {
> +		p_ips_freq = of_get_property(node, "bus-frequency", NULL);
> +		if (p_ips_freq)
> +			break;
> +
> +		np = of_get_parent(node);
> +		of_node_put(node);
> +		node = np;
> +	}
> +	if (node)
> +		of_node_put(node);
> +
> +	return p_ips_freq ? *p_ips_freq : 0;
> +}
> +EXPORT_SYMBOL(mpc512x_find_ips_freq);

This looks identical to the 52xx version.  Perhaps they should be
merged... I wouldn't bother for getting this mainlined, but it is
something to think about for the future.

g.