RE: [RFC PATCH 13/14] ARM: vexpress: Definition of vexpress dts specification

["Lorenzo Pieralisi" <Lorenzo.Pieralisi-5wv7dgnIgG8@public.gmane.org>]

> -----Original Message-----
> From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On Behalf Of Grant
> Likely
> Sent: 18 August 2010 22:55
> To: Lorenzo Pieralisi
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; Philippe Robin; nico-vtqb6HGKxmzR7s880joybQ@public.gmane.org;
> linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org; Catalin Marinas; jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org
> Subject: Re: [RFC PATCH 13/14] ARM: vexpress: Definition of vexpress dts
> specification
> 
> On Wed, Aug 18, 2010 at 12:59 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> > The device tree methodology requires a dts file to be defined per
> > platform in order to describe the platform address space and topology,
> > in short the device tree in source format.
> >
> > This patch adds the dts file for the Versatile Express board.
> > Some device names are still temporary and non-compliant waiting for the
> > definition of proper clock bindings.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/vexpress.dts |  199
> ++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 199 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/boot/dts/vexpress.dts
> >
> > diff --git a/arch/arm/boot/dts/vexpress.dts
> b/arch/arm/boot/dts/vexpress.dts
> > new file mode 100644
> > index 0000000..cc58603
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/vexpress.dts
> > @@ -0,0 +1,199 @@
> > +/dts-v1/;
> > +
> > +/ {
> > +       model = "versatile-express";
> > +       compatible = "arm,versatile-express";
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +       #interrupt-cells = <1>;
> 
> #interrupt-cells does not belong in the root node.  You can remove this.
> 
> > +       interrupt-parent = <&intc>;
> > +
> > +       memory {
> > +               name = "memory";
> > +               device_type = "memory";
> > +               reg = <0x60000000 0x20000000>;
> > +       };
> > +
> > +       chosen {
> > +               bootargs = "rdinit=/bin/ash console=ttyAMA0 vga=0x311
> mem=512M debug earlyprintk";
> > +       };
> > +
> > +       soc {
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               device_type = "soc";
> 
> Remove device_type property.  It only has meaning when real
> OpenFirmware is present on the board.

OK.

> 
> > +               compatible = "simple-bus";
> > +               ranges = <0x00000000 0x00000000 0xFFFFFFFF>;
> > +
> > +               gic-cpu {
> > +                       #address-cells = <0>;
> > +                       #interrupt-cells = <1>;
> > +                       interrupt-controller;
> > +                       reg = <0x1e000100 0x1000>;
> > +                       compatible = "arm,gic-cpu-interrupt-controller";
> > +                       device_type = "gic";
> 
> Ditto on device_type
>

OK.
 
> > +               };
> > +
> > +               intc: gic-dist {
> > +                       #address-cells = <0>;
> > +                       #interrupt-cells = <1>;
> > +                       interrupt-controller;
> > +                       reg = <0x1e001000 0x1000>;
> > +                       compatible = "arm,gic-dist-interrupt-controller";
> > +                       device_type = "gic";
> 
> Ditto
> 
> > +                       irq-start = <29>;
> 
> This ends up encoding Linux-kernel implementation details into the
> device tree.  Ideally the Linux irq numbers are to be dynamically
> assigned by the kernel for each interrupt controller, but I haven't
> ported the support code for that yet.  Shouldn't be too hard to do
> though.
> 

Ok, let's keep in sync on this, see below.

> > +               };
> > +
> > +               smsc@4e000000 {
> > +                       compatible = "smc,smsc-911";
> > +                        reg = <0x4e000000 0x1000>;
> > +                       interrupts = <47>;
> 
> I suspect this interrupts property is wrong.  An interrupts specifier
> is specific to the interrupt controller node; so this should always
> specify the interrupt input on a specific controller (either gic-cpu
> or gic-dist in this system).  It looks like '47' is the global irq
> number.  Am I correct?
>

Strictly related to the previous point.

When you say input Grant you mean input HW pin or interrupt HW ID ?

I think you mean the irq ID specific to a given interrupt controller, 
because that is what SW sees and controls.

47 is the eth interrupt ID on the gic-dist, that in this particular
case becomes the global IRQ number as well if I am not mistaken. 
(it is retrieved through the platform device resource).
I think it is correct as it is, but I will countercheck.

> > +                       flags = <2>;
> > +                       irq-polarity = <1>;
> > +                       irq-type = <1>;
> > +                       phy-interface = <0>;
> 
> As mentioned in my comments on the smsc911x driver changes, there
> already is a binding for the phy which should be used, and the irq
> flags should be specified as a second cell in the 'interrupts'
> property.  This also means that the interrupt controller nodes should
> probably use #interrupt-cells = <2>;
> 

Ok, I will patch the dts accordingly.

> > +               };
> > +
> > +               flash@40000000 {
> > +                       compatible = "arm,arm-flash";
> 
> Use "cfi-flash" which the physmap_of.c driver will bind against.
> 

Ok, I will follow Catalin's reply up.

> > +                       reg = <0x40000000 0x4000000
> > +                              0x44000000 0x4000000>;
> > +               };
> > +
> > +               isp@4f000000 {
> > +                       compatible = "nxp,usb-isp1761";
> > +                       reg = <0x4f000000 0x20000>;
> > +                       interrupts = <48>;
> > +                       port1_otg;
> > +               };
> > +
> > +               ddc@10016000 {
> > +                       compatible = "arm,versatile-i2c";
> > +                       reg = <0x10016000 0x1000>;
> > +               };
> > +
> > +               pmu@0 {
> 
> If there is no 'reg' property, then there should not be a @0 component
> to the node name.
>

Ok.
 
> > +                       compatible = "arm,arm-pmu";
> > +                       interrupts = <92 93 94 95>;
> > +               };
> > +
> > +               timer@10011000 {
> > +                       compatible = "arm,arm-sp804";
> > +                       reg = <0x10011000 0x1000>;
> > +                       interrupts = <34>;
> > +               };
> > +
> > +               timer@10012000 {
> > +                       compatible = "arm,arm-sp804";
> > +                       reg = <0x10012000 0x1000>;
> > +               };
> > +
> > +       };
> > +
> > +       amba {
> > +               compatible = "arm,amba";
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +
> > +               uart@0 {
> 
> uart@10009000
> 
> The @<id> portion of the node name consists of the first
> #address-cells values of the reg property separated by commas.  If
> #address-cells = <1>, then it is just a single number like in this
> case.
>

Yes Grant, consider it done, I left them like that because the clock 
look-up uses the name (before merging your changes) and I avoided recoding
the clock LUT before posting the patchset.
 
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x10009000 0x1000>;
> > +                       arm,amba-deviceid = <0x00141011>;
> > +                       interrupts = <37>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               uart@1 {
> 
> ditto (and so on down the file)
> 
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x1000a000 0x1000>;
> > +                       arm,amba-deviceid = <0x00141011>;
> > +                       interrupts = <38>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               kbd@0 {
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x10006000 0x1000>;
> > +                       arm,amba-deviceid = <0x00041050>;
> > +                       interrupts = <44>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               aaci@0 {
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x10004000 0x1000>;
> > +                       arm,amba-deviceid = <0x00041041>;
> > +                       interrupts = <43>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               ps2@0 {
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x10007000 0x1000>;
> > +                       arm,amba-deviceid = <0x00041050>;
> > +                       interrupts = <45>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               rtc@0 {
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x10017000 0x1000>;
> > +                       arm,amba-deviceid = <0x00041031>;
> > +                       interrupts = <36>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               wdt@0 {
> > +                       compatible = "arm,amba-device";
> > +                       reg = <0x1000f000 0x1000>;
> > +                       arm,amba-deviceid = <0x00041085>;
> > +                       interrupts = <32>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               clcd@0 {
> > +                       compatible = "arm,clcd-pl11x","arm,amba-device";
> > +                       reg = <0x10020000 0x1000>;
> > +                       arm,amba-deviceid = <0x00041111>;
> > +                       interrupts = <76>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               mmci@0 {
> > +                       compatible = "arm,mmc-pl18x","arm,amba-device";
> > +                       reg = <0x10005000 0x1000>;
> > +                       interrupts = <41 42>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               dmc@0 {
> > +                       compatible = "arm,dmc","arm,amba-device";
> > +                       reg = <0x100e0000 0x1000>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               smc@0 {
> > +                       compatible = "arm,smc","arm,amba-device";
> > +                       reg = <0x100e1000 0x1000>;
> > +                       interrupts = <77 78>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +
> > +               gpio@0 {
> > +                       compatible = "arm,gpio","arm,amba-device";
> > +                       reg = <0x100e8000 0x1000>;
> > +                       interrupts = <82>;
> > +                       clock-map = <&ref24_clk 0>;
> > +               };
> > +       };
> > +
> > +       clocks {
> > +               ref24_clk: clock@0 {
> > +                       device_type = "clock";
> > +                       compatible = "fixed-clock";
> > +                       clock-frequency = <24000000>;
> > +                       clock-output-names = "ref";
> 
> This will of course need to be updated to the clock binding that
> Jeremy and I have settled on, but you already knew that.  :-)
> 

Yes, again, you are right I knew that but I wanted to post a version 
I completely tested before applying the new clock bindings when rebasing.
I will update the code for the next respin.

> Cheers,
> g.

Thanks a lot,
Lorenzo