Re: [PATCH 1/2] powerpc: Board support for GE Fanuc SBC610

[Jon Loeliger <jdl@freescale.com>]

Martyn Welch wrote:
> Support for the SBC610 VPX Single Board Computer from GE Fanuc (PowerPC
> MPC8641D).
> 
> This is the basic board support for GE Fanuc's SBC610, a 6U single board
> computer, based on Freescale's MPC8641D.
> 
> Signed-off-by: Martyn Welch <martyn.welch@gefanuc.com>
> ---
> 
>  arch/powerpc/boot/dts/gef_sbc610.dts     |  268 ++++++++++++++++++++++++++++++
>  arch/powerpc/platforms/86xx/Kconfig      |    9 +
>  arch/powerpc/platforms/86xx/Makefile     |    1 
>  arch/powerpc/platforms/86xx/gef_sbc610.c |  187 +++++++++++++++++++++
>  4 files changed, 464 insertions(+), 1 deletions(-)
>  create mode 100644 arch/powerpc/boot/dts/gef_sbc610.dts
>  create mode 100644 arch/powerpc/platforms/86xx/gef_sbc610.c
> 
> 

> +	soc8641@fef00000 {

No "8641" in this name, please.

Oh, and drop the ""32-bit" in the CPU sections too.

> +		ecc@2000 {
> +			device_type = "dram-controller";

Hmmm, I suspect that should be dropped.

> +			compatible = "mpc86xx";

And that changed to indicate some form of controller thing.
Using "mpc86xx" here is just not right at all.

> +
> +                serial0: serial@4500 {
> +                        cell-index = <0>;
> +                        device_type = "serial";
> +                        compatible = "ns16550";
> +                        reg = <0x00004500 0x00000100>;
> +			clock-frequency = <0>;
> +                        interrupts = <0x2a 0x2>;
> +                        interrupt-parent = <&mpic>;
> +                };
> +
> +                serial1: serial@4600 {
> +                        cell-index = <1>;
> +                        device_type = "serial";
> +                        compatible = "ns16550";
> +                        reg = <0x00004600 0x00000100>;
> +			clock-frequency = <0>;
> +                        interrupts = <0x1c 0x2>;
> +                        interrupt-parent = <&mpic>;
> +                };

There's some form of indenting issue there...

> +		mpic: pic@40000 {
> +			clock-frequency = <0>;
> +			interrupt-controller;
> +			#address-cells = <0>;
> +			#interrupt-cells = <2>;
> +			reg = <0x00040000 0x00040000>;
> +			built-in;
> +			compatible = "chrp,open-pic";
> +			device_type = "open-pic";
> +                        big-endian;

IIRC, we dropped "big-endian" too. (?)



> diff --git a/arch/powerpc/platforms/86xx/gef_sbc610.c b/arch/powerpc/platforms/86xx/gef_sbc610.c
> new file mode 100644
> index 0000000..6b92876
> --- /dev/null
> +++ b/arch/powerpc/platforms/86xx/gef_sbc610.c
> @@ -0,0 +1,187 @@

> +
> +/*
> + * Based on: mpc86xx_hpcn.c
> + *
> + * MPC86xx HPCN board specific routines
> + *
> + * Recode: ZHANG WEI <wei.zhang@freescale.com>
> + * Initial author: Xianghua Xiao <x.xiao@freescale.com>
> + *
> + * Copyright 2006 Freescale Semiconductor Inc.
> + *
> + */

This seems misleading some.  Sure, state your attributions
and derivation sources, but this also still looks like it
is stating that it *is* the MPC86xx HPCN board code.

Thanks,
jdl