Subject: Re: Freescale MPC5554 device tree (was: cross-compiling Linux for PowerPC e200 core?)

From: Grant Likely <[email-masked]>
Date: Fri, 12 Mar 2010 05:14:56 -0700

2010/3/11 Németh Márton <[email-masked]>:
> Hi,
>
> thank you for the comments, I reworked the Freescale MPC5554 device tree
> accordingly. I'm listening for comments on this draft.
>
> Regards,
>
>        Márton Németh
>
> ---
> From: Márton Németh <[email-masked]>
>
> Add device tree for Freescale MPC5554.
>
> Signed-off-by: Márton Németh <[email-masked]>
> ---
> diff -uprN linux-2.6.33.orig/arch/powerpc/boot/dts/mpc5554.dts linux/arch/powerpc/boot/dts/mpc5554.dts
> --- linux-2.6.33.orig/arch/powerpc/boot/dts/mpc5554.dts 1970-01-01 01:00:00.000000000 +0100
> +++ linux/arch/powerpc/boot/dts/mpc5554.dts     2010-03-12 07:22:37.000000000 +0100
> @@ -0,0 +1,189 @@
> +/*
> + * Freescale MPC5554 Device Tree Source
> + *
> + * Based on MPC5553/5554 Microcontroller Reference Manual, Rev. 4.0, 04/2007
> + * http://www.freescale.com/files/32bit/doc/ref_manual/MPC5553_MPC5554_RM.pdf
> + *
> + * Copyright 2010 Márton Németh
> + * Márton Németh <[email-masked]>
> + *
> + * 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.
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +       model = "MPC5554";
> +       compatible = "fsl,MPC5554EVB";          // Freescale MPC5554 Evaluation Board
> +       #address-cells = <1>;
> +       #size-cells = <1>;

also need: interrupt-parent = <&intc>;

I describe why later...

> +
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       device_type = "cpu";
> +                       compatible = "PowerPC,5554";

I'd rather see the same convention used here as for all the other
compatible values in this file.  ie:

compatible = "fsl,mpc5554-e200z6", "fsl,powerpc-e200z6";

Dave, what do you think?

> +                       reg = <0>;
> +                       d-cache-line-size = <32>;
> +                       i-cache-line-size = <32>;
> +                       d-cache-size = <0x8000>;        // L1, 32KiB
> +                       i-cache-size = <0x8000>;        // L1, 32KiB
> +                       timebase-frequency = <0>;       // from bootloader
> +                       bus-frequency = <0>;            // from bootloader
> +                       clock-frequency = <0>;          // from bootloader
> +               };
> +       };
> +
> +       memory@40000000 {
> +               device_type = "memory";
> +               reg = <0x40000000 0x10000>;     // 32KiB internal SRAM
> +       };
> +
> +       xbar@fff04000 {         // System Bus Crossbar Switch (XBAR)
> +               compatible = "fsl,mpc5554-xbar";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               // The full memory range is covered by XBAR
> +               ranges = <>;

An empty ranges property looks like this:

ranges;

> +               bridge@fff00000 {
> +                       compatible = "fsl,mpc5554-pbridge-b";
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       ranges = <0 0xe0000000 0x20000000>;
> +                       reg = <0xfff00000 0x4000>;
> +
> +                       ecsm@fff40000 {         // Error Correction Status Module (ECSM)
> +                               compatible = "fsl,mpc5554-ecsm";
> +                               reg = <0xfff40000 0x4000>;
> +                       };
> +
> +                       edma@fff44000 {         // Enhanced DMA Controller (eDMA)
> +                               compatible = "fsl,mpc5554-edma";
> +                               reg = <0xfff44000 0x4000>;
> +                       };
> +
> +                       intc@fff48000 {         // Interrupt Controller (INTC)
> +                               compatible = "fsl,mpc5554-intc";
> +                               reg = <0xfff48000 0x4000>;
> +                       };

Need a label on this node so that the rest of the tree can find it,
and it needs the interrupt-controller and #interrupt-cells properties:

                       intc: intc@fff48000 {         // Interrupt
Controller (INTC)
                               compatible = "fsl,mpc5554-intc";
                               interrupt-controller;
                               #interrupt-cells = <2>;
                               reg = <0xfff48000 0x4000>;
                       };

I've set #interrupt-cells to 2 which is fairly typical on fsl parts,
but that may or may not make sense.  You need to decide how each
device is going to specify it's interrupt line.  Often the first cell
is the hardware interrupt number, and the second cell encodes the
sense (high, low, edge).  Conversely, PCI interrupts only use
#interrupt-cells = <1> because all PCI irqs use the active low sense.

Then, each device in the tree should have an 'interrupts = < [hwirq#]
[sense]>;' property.

Otherwise, starting to look pretty good.

g.