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

[David Daney <ddaney@caviumnetworks.com>]

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@caviumnetworks.com>
>> ---
>>   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