[RFC PATCH 13/14] ARM: vexpress: Definition of vexpress dts specification
Lorenzo Pieralisi
Lorenzo.Pieralisi at arm.com
Sat Aug 21 03:51:08 EST 2010
> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of Grant
> Likely
> Sent: 18 August 2010 22:55
> To: Lorenzo Pieralisi
> Cc: devicetree-discuss at lists.ozlabs.org; Philippe Robin; nico at fluxnic.net;
> linux at arm.linux.org.uk; Catalin Marinas; jeremy.kerr at canonical.com
> 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 at arm.com> 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 at arm.com>
> > ---
> > 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 at 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 at 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 at 4f000000 {
> > + compatible = "nxp,usb-isp1761";
> > + reg = <0x4f000000 0x20000>;
> > + interrupts = <48>;
> > + port1_otg;
> > + };
> > +
> > + ddc at 10016000 {
> > + compatible = "arm,versatile-i2c";
> > + reg = <0x10016000 0x1000>;
> > + };
> > +
> > + pmu at 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 at 10011000 {
> > + compatible = "arm,arm-sp804";
> > + reg = <0x10011000 0x1000>;
> > + interrupts = <34>;
> > + };
> > +
> > + timer at 10012000 {
> > + compatible = "arm,arm-sp804";
> > + reg = <0x10012000 0x1000>;
> > + };
> > +
> > + };
> > +
> > + amba {
> > + compatible = "arm,amba";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + uart at 0 {
>
> uart at 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 at 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 at 0 {
> > + compatible = "arm,amba-device";
> > + reg = <0x10006000 0x1000>;
> > + arm,amba-deviceid = <0x00041050>;
> > + interrupts = <44>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + aaci at 0 {
> > + compatible = "arm,amba-device";
> > + reg = <0x10004000 0x1000>;
> > + arm,amba-deviceid = <0x00041041>;
> > + interrupts = <43>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + ps2 at 0 {
> > + compatible = "arm,amba-device";
> > + reg = <0x10007000 0x1000>;
> > + arm,amba-deviceid = <0x00041050>;
> > + interrupts = <45>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + rtc at 0 {
> > + compatible = "arm,amba-device";
> > + reg = <0x10017000 0x1000>;
> > + arm,amba-deviceid = <0x00041031>;
> > + interrupts = <36>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + wdt at 0 {
> > + compatible = "arm,amba-device";
> > + reg = <0x1000f000 0x1000>;
> > + arm,amba-deviceid = <0x00041085>;
> > + interrupts = <32>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + clcd at 0 {
> > + compatible = "arm,clcd-pl11x","arm,amba-device";
> > + reg = <0x10020000 0x1000>;
> > + arm,amba-deviceid = <0x00041111>;
> > + interrupts = <76>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + mmci at 0 {
> > + compatible = "arm,mmc-pl18x","arm,amba-device";
> > + reg = <0x10005000 0x1000>;
> > + interrupts = <41 42>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + dmc at 0 {
> > + compatible = "arm,dmc","arm,amba-device";
> > + reg = <0x100e0000 0x1000>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + smc at 0 {
> > + compatible = "arm,smc","arm,amba-device";
> > + reg = <0x100e1000 0x1000>;
> > + interrupts = <77 78>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > +
> > + gpio at 0 {
> > + compatible = "arm,gpio","arm,amba-device";
> > + reg = <0x100e8000 0x1000>;
> > + interrupts = <82>;
> > + clock-map = <&ref24_clk 0>;
> > + };
> > + };
> > +
> > + clocks {
> > + ref24_clk: clock at 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
More information about the devicetree-discuss
mailing list