lkml.org 
[lkml]   [2011]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 02/10] MIPS: Octeon: Add device tree source files.
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


\
 
 \ /
  Last update: 2011-02-23 20:09    [W:0.129 / U:1.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site