Re: [RFC PATCH 02/10] MIPS: Octeon: Add device tree source files.

[David Daney]

On 02/22/2011 04:07 PM, David Gibson wrote:
> On Tue, Feb 22, 2011 at 12:57:46PM -0800, David Daney wrote:
> > Signed-off-by: David Daney<ddaney@xxxxxxxxxxxxxxxxxx>
> > ---
> >   arch/mips/cavium-octeon/.gitignore      |    2 +
> >   arch/mips/cavium-octeon/Makefile        |   13 ++
> >   arch/mips/cavium-octeon/octeon_3xxx.dts |  314 +++++++++++++++++++++++++++++++
> >   arch/mips/cavium-octeon/octeon_68xx.dts |   99 ++++++++++
> >   4 files changed, 428 insertions(+), 0 deletions(-)
> >   create mode 100644 arch/mips/cavium-octeon/.gitignore
> >   create mode 100644 arch/mips/cavium-octeon/octeon_3xxx.dts
> >   create mode 100644 arch/mips/cavium-octeon/octeon_68xx.dts
> >
> > diff --git a/arch/mips/cavium-octeon/.gitignore b/arch/mips/cavium-octeon/.gitignore
> > new file mode 100644
> > index 0000000..39c9686
> > --- /dev/null
> > +++ b/arch/mips/cavium-octeon/.gitignore
> > @@ -0,0 +1,2 @@
> > +*.dtb.S
>
> .dtb.S?

I think I have the correct .gitignore syntax.

> [snip]
> > +/dts-v1/;
> > +/* OCTEON 3XXX, 5XXX, 63XX device tree skeleton. */
> > +/ {
> > +  model = "OCTEON";
>
> 1 tab indents are the usual convention for device trees.

OK.

> > +  compatible = "octeon,octeon";
>
> There's no model number at all for this board?


I think it should be:

	compatible = "octeon,octeon-3860";

> > +  #address-cells =<2>;
> > +  #size-cells =<2>;
> > +
> > +  soc@0 {
> > +    device_type = "soc";
>
> Drop this device_type.

OK.

> > +    compatible = "simple-bus";
> > +    #address-cells =<2>;
> > +    #size-cells =<2>;
> > +    ranges; /* Direct mapping */
> > +
> > +    ciu: ciu-3xxx@1070000000000 {
> > +      compatible = "octeon,ciu-3xxx";
>
> So, names or compatible values with "wildcards" like 3xxx should be
> avoided.  Instead, use the specific model number of this device, then
> future devices can claim compatibility with the earlier one.
>
> But, in addition the generic names convention means that the node name
> should be "interrupt-controller" rather than something model specific.

Let's try:

ciu: interrupt-controller@1070000000000 {
      compatible = "octeon,octeon-3860-ciu";


> > +      interrupt-controller;
> > +      #address-cells =<0>;
> > +      #interrupt-cells =<2>;
> > +      reg =<0x10700 0x00000000 0x0 0x7000>;
> > +    };
> > +
> > +    /* SMI0 */
> > +    mdio0: mdio@1180000001800 {
>
> If SMI0 is the name generally used in the documentation, using that in
> the label instead of mdio0 might be more useful.
>
> > +      compatible = "octeon,mdio";
>
> No model or revision number?
Let's try:

	smi0: mdio@1180000001800 {
		compatible = "octeon,octeon-3860-mdio";

> > +      #address-cells =<1>;
> > +      #size-cells =<0>;
> > +      reg =<0x11800 0x00001800 0x0 0x40>;
> > +      device_type = "mdio";
>
> Drop this device_type.

OK.

>
[...]
> > +    mgmt0: ethernet@1070000100000 {
> > +      compatible = "octeon,mgmt";

This becomes:

	mgmt0: ethernet@1070000100000 {
		compatible = "octeon,octeon-5230-mii";


> > +      device_type = "network";
> > +      model = "mgmt";
> > +      reg =<0x10700 0x00100000 0x0 0x100>, /* MIX */
> > +<0x11800 0xE0000000 0x0 0x300>, /* AGL */
> > +<0x11800 0xE0000400 0x0 0x400>, /* AGL_SHARED  */
> > +<0x11800 0xE0002000 0x0 0x8>;   /* AGL_PRT_CTL */
> > +      unit-number =<0>;
>
> What is this 'unit-number' property for?


The AGL_SHARED register bank is shared among all the octeon-5230-mii 
devices.  the 'unit-number' indicates the bit-field index that this 
device should use within those registers.


> > +      interrupt-parent =<&ciu>;
> > +      interrupts =<0 62>,<1 46>;
> > +      local-mac-address = [ 00 00 00 00 00 00 ];
>
> That's not a valid MAC address of course.  If this has to be patched
> in by the bootloader / later processing, you should add a comment to
> that effect.

Right.

> > +      phy-handle =<&phy0>;
> > +    };
> > +
> > +    mgmt1: ethernet@1070000100800 {
> > +      compatible = "octeon,mgmt";
> > +      device_type = "network";
> > +      model = "mgmt";
> > +      reg =<0x10700 0x00100800 0x0 0x100>, /* MIX */
> > +<0x11800 0xE0000800 0x0 0x300>, /* AGL */
> > +<0x11800 0xE0000400 0x0 0x400>, /* AGL_SHARED  */
> > +<0x11800 0xE0002008 0x0 0x8>;   /* AGL_PRT_CTL */
> > +      unit-number =<1>;
> > +      interrupt-parent =<&ciu>;
> > +      interrupts =<1 18>,<  1 46>;
> > +      local-mac-address = [ 00 00 00 00 00 00 ];
> > +      phy-handle =<&phy1>;
> > +    };
> > +
> > +    pip: pip@11800a0000000 {
> > +      compatible = "octeon,pip";
> > +      #address-cells =<1>;
> > +      #size-cells =<0>;
> > +      reg =<0x11800 0xa0000000 0x0 0x2000>;
> > +
> > +      interface@0 {
>
> These subnodes and subsubnodes should have compatible values too, even
> if it's just "octeon,pip-interface" and "octeon,pip-ethernet".

OK.

> > +        #address-cells =<1>;
> > +        #size-cells =<0>;
> > +        reg =<0>; /* interface */
> > +
> > +        ethernet@0 {
> > +          device_type = "network";
> > +          model = "pip";
>
> This model property doesn't look very useful.

I will remove it.

> [snip]

> Uh.. where are the CPUs?

Answered in other e-mail.

Thanks,
David Daney
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/