On Fri, Jun 05, 2009 at 07:44:31PM +0100, Byron Bradley wrote:
> On Fri, 5 Jun 2009, David Gibson wrote:
>
> > On Thu, Jun 04, 2009 at 09:59:04PM +0100, Byron Bradley wrote:
> > > The Thecus N1200 is a NAS device with a single internal SATA disk and
> > > an eSATA port based on an MPC8347 SoC.
> >
> > Comments on a number of fairly minor device tree nits below:
>
> Hi David, Peter. Thanks for the comments, replies inline below.
>
> >
> > [snip]
> > > + soc8...@e0000000 {
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + device_type = "soc";
> > > + compatible = "simple-bus";
> >
> > The compatible value should have something more specific
> > (e.g. "fsl,mpc8340-soc") before listing "simple-bus".
>
> Added "fsl,mpc8347-soc" and changed soc8349 to soc8347.
Ok. Strictly speaking, I think new conventions say it should be just
"soc@" but I think that might confuse u-boot.
> > > + ranges = <0x0 0xe0000000 0x00100000
> > > + 0xfe000000 0xfe000000 0x0800000>;
> > > + reg = <0xe0000000 0x00000200>;
> > > + bus-frequency = <0>; // from bootloader
> > > +
> > > + physmap-fl...@fe000000 {
> >
> > Calling this just "flash" would be more normal.
>
> Done.
>
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + compatible = "cfi-flash";
> >
> > Ideally this should list the actual model of flash chip, before the
> > generic "cfi-flash".
>
> Added "numonyx,28f640j3d"
Ok.
[snip]
> > > + w...@200 {
> > > + device_type = "watchdog";
> >
> > No device_type here.
>
> Removed all device_type entries that you said shouldn't be there.
Excellent.
[snip]
> > > + ipic: p...@700 {
> > > + interrupt-controller;
> > > + #address-cells = <0>;
> > > + #interrupt-cells = <2>;
> > > + reg = <0x700 0x100>;
> > > + device_type = "ipic";
> >
> > This should have a compatible property. It shouldn't really have
> > device_type, but I suspect that's a bug in the ipic binding, rather
> > than your tree per-se.
>
> The binding for this seems to be done in the setup file,
> arch/powerpc/platforms/83xx/thecus_n1200.c in my case. At the moment it
> does:
> np = of_find_node_by_type(NULL, "ipic");
> ...
> ipic_init(np, 0);
> so making it look for something like "fsl,mpc8347-ipic" should be no
> problem if that's how it should be done.
Ok, sounds good. device_type is a hangover from real OF - where it
advertises what method interface the node supports. As such it has
little place in flattened trees and should not be used for driver
binding.
> > > + };
> > > +
> > > + gpio1: gpio-control...@c00 {
> > > + #gpio-cells = <2>;
> > > + compatible = "fsl,mpc8347-gpio", "fsl,mpc8349-gpio";
> >
> > This actually is an 8349 board, yes? Generally compatible should be
> > listed from most specific to least specific, so the 8349 entry should
> > go first.
>
> No, as Peter pointed out this is an 8347. The soc8349 at the top was
> probably a combination of basing this dts on an 8349 one and the fact that
> most of the freescale docs for 8347 just point to 8349 ones. I've made
> sure the only place 8349 is referenced is in compatible fields and it's
> always after the 8347 version.
Ok, good.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
