[PATCH] powerpc/mpc52xx: add Phytec phyCORE-MPC5200B-IO board (pcm032)

[Grant Likely]

Thanks for the patch Wolfram.  Comments below.

On Wed, Feb 25, 2009 at 8:32 AM, Wolfram Sang <w.sang at pengutronix.de> wrote:
> Signed-off-by: Wolfram Sang <w.sang at pengutronix.de>
> ---
>  arch/powerpc/boot/dts/pcm032.dts             |  391 +++++++
>  arch/powerpc/configs/52xx/pcm032_defconfig   | 1394 ++++++++++++++++++++++++++

Do you really need a separate defconfig for this board?  Can it be
merged with an existing defconfig?

>  arch/powerpc/platforms/52xx/Kconfig          |    1 +
>  arch/powerpc/platforms/52xx/mpc5200_simple.c |    3 +-
>  4 files changed, 1788 insertions(+), 1 deletions(-)
>  create mode 100644 arch/powerpc/boot/dts/pcm032.dts
>  create mode 100644 arch/powerpc/configs/52xx/pcm032_defconfig
>
> diff --git a/arch/powerpc/boot/dts/pcm032.dts b/arch/powerpc/boot/dts/pcm032.dts
> new file mode 100644
> index 0000000..ebaf660
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/pcm032.dts
> @@ -0,0 +1,391 @@
> [...]
> +       lpb at e4000000 {

As per generic names recommended practice, this node name should
really be something like "localbus {", and you should drop the node
address since the local bus doesn't really have an address.

> +               compatible = "fsl,lpb";

Ideally should be: compatible =
"fsl,mpc5200b-lpb","fsl,mpc5200-lpb","simple-bus";
I know that 'fsl,lpb' has been used in the past, but it is far to
generic and isn't good practice.  The 'simple-bus' property ensures
that the bus will get probed.

> +               ranges = <0x0 0xe4000000 0x08000000>;
> +               #size-cells = <1>;
> +               #address-cells = <1>;

Should be #address-cells = <2>;.  First cell is CS#, second cell is
offset from base of CS.  This is particularly important if the LPB
FIFO is ever used with any of this hardware.

motionpro.dts is a good example of what it should look like.

> +
> +               /* free chipselect */
> +               cs4 at 00000000 {
> +                       compatible = "free";

Very bad!  If you want to show available chip selects, then add some
commented out sections as examples.  Creating nodes with the extremely
generic compatible value of "free" is just asking for trouble
(especially when someone new sees the example and decides to use the
same thing in their driver).

> +       lpb at f7000000 {

Another localbus node?  Why can't a single localbus node hold all the
child nodes?

> +               compatible = "fsl,lpb";
> +               ranges = <0x0 0xf7000000 0x09000000>;
> +               #size-cells = <1>;
> +               #address-cells = <1>;
> +
> +               fpga1 at 00e00000 {
> +                       compatible = "fpga1";

Too generic.  Be board specific.

> +                       reg = <0x00e00000 0x02000000>;
> +                       bank-width = <4>;
> +               };
> +
> +               fpga2 at 02e00000 {
> +                       compatible = "fpga2";

Ditto

Otherwise, the rest of the patch looks good to me.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.