Device tree review
Stephen Warren
swarren at wwwdotorg.org
Tue May 29 23:25:26 EDT 2012
Chris, Simon, everyone,
To set the background, I'm interested in helping out with Raspberry Pi
kernel upstreaming. I've been doing quite a bit of work w.r.t. device
tree (for Tegra) recently, so am fairly familiar with many of the
requirements for a good DT binding. As such, I read through the DT files
in the rpi-linear branch and wanted to give some feedback.
The aim here is to give you a heads up on some of the review comments
you'll get when you attempt to upstream this; hopefully this will speed
up the process a bit since you will have fixed up a lot of the issues
before posting the first patches. To re-iterate, although I have a lot
of picky comments below, please be assured I really am trying to help
out here; just wanted to make that clear since this is probably about
the first interaction many people here will have had with me.
bcm2835.dtsi:
Should this be bcm2708.dtsi to match the driver names and base
compatible values? If there are differences between 2708 and 2835, the
bcm2835.dtsi could include bcm2708.dtsi in order to achieve that.
> /include/ "skeleton.dtsi"
>
> / {
> model = "BCM2835";
> #address-cells = <1>;
> #size-cells = <1>;
Those two are already in skeleton.dtsi.
> compatible = "broadcom,bcm2835", "broadcom,bcm2708";
> system-rev = <0>; /* Set by VideoCore */
> system-serial = <0 0>; /* Set by VideoCore */
I'm not entirely sure if / is the right place for those, or if they
should have a vendor prefix to differentiate them from standardized
property names. Still, I can't really find any precedent to say either
way...
> chosen {
> bootargs = ""; /* Set by VideoCore */
That property would usually be left out, and the bootloader expected to
create the property if it wasn't already present, as well as fill in the
value of the property.
> };
>
> cpus {
> cpu at 0 {
> compatible = "broadcom,bcm2835-cpu", "broadcom,bcm2708-cpu", "arm,1176jz-f";
I'm not sure that I would include the first two compatible values. I
agree it is standard practice to do this for any other type of HW
module, but I can't see any precedent for CPUs.
> };
> };
> memory {
> device_type = "memory";
> reg = <0 0>; /* Set by VideoCore */
> };
That whole node and set of properties is in skeleton.dtsi.
> display {
> compatible = "broadcom,bcm2835-fb", "broadcom,bcm2708-fb";
> #address-cells = <0>;
> #size-cells = <0>;
If the node doesn't have children, there's no need to include any
#address-cells/#size-cells properties.
> vc_mailbox = <&vc_mbox>;
> vc_channel = <1>;
Properties that are custom to a particular binding should contain the
vendor prefix. Also, dash/minus is typically used rather than
underscore. so "broadcom,vc-mailbox".
> width = <0>; /* Set by VideoCore */
> height = <0>; /* Set by VideoCore */
> depth = <0>; /* Set by VideoCore */
> };
I'm not sure if the display stuff is upstreamable in the near term or
not (must it rely on binary-only user-space, or can a simple KMS driver
be provided that doesn't rely on one?) But, if so, there is some very
early discusion on DT bindings for display happening re: the Tegra DRM
driver. It might be interesting to try and keep any and all
display-related DT bindings as similar as possible.
> power: power {
...
> /* Devices
> * 0: SD Card
> * 3: USB
> */
> valid = <0x9>;
>
> default-off = <0x9>;
I'm not sure exactly what those last two properties represent. (Related:
there should be a full description of each binding in
Documentation/devicetree/bindings/...) It might be best to have the
driver turn off everything all the time, and have each client node
contain a property that identifies which bit in the power manager
register to enable.
Actually, once I put it that way, the power node/driver probably should
be a regulator provider, and the other nodes should regulator consumers,
and everything use the standardized bindings in
Documentation/devicetree/bindings/regulator/ for this purpose. That
would also allow you to specify some regulators as
always-on/enabled-at-boot if needed.
> axi {
> compatible = "broadcom,bcm2835-axi", "broadcom,bcm2708-axi", "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0x20000000 0x01000000>;
> ranges = <0 0x20000000 0x01000000>;
This ranges is probably fine as-is, but data-sheets usually document all
addresses using absolute values and so allowing all reg properties of
the children to map 1:1 to the documentation might be better. As you've
done elsewhere, that would mean just "ranges;", instead of defining a
value for the property.
Although I admit BCM2835-ARM-Peripherals.pdf is a little odd in that it
documents assumed virtual addresses rather than physical addresses, so
there won't ever be a 1:1 correlation between that document and the DT
content.
> st {
Can you explain more what this st bus is; I'm guessing that "st" means
"system timer", and that there isn't really an explicit bus in the HW.
If there isn't a bus in HW, there shouldn't be a node in DT, since the
DT is supposed to be purely a representation of HW.
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0x3000 0x1000>;
> ranges = <0 0x3000 0x1000>;
>
> stc {
As such, I'd expect this node to exist directly under axi.
Nodes are typically named after the type of object they implement, using
fairly generic names, so this should probably be "timer" rather than "stc".
> compatible = "broadcom,bcm2835-clock", "broadcom,bcm2708-clock", "mmio-clock", "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0x004 0x4 0x000 0x4>;
I don't see the benefit of representing each timer as separate node in
DT. The documentation describes a single HW module that happens to
implement 4 timers within it. I'd expect to see the stc node contain
just a single compatible, reg, and interrupts property.
> reg-names = "counter", "control";
> ranges;
>
> clock-outputs = "sys";
> clock-frequency = <1000000>;
> rating = <300>;
What is rating? If needed, this should have a vendor prefix. Unless the
rating varies between SoCs in the 2708 family, or per-board, I'd expect
to encode whatever this (and min-delta below) directly in a data
structure in the DT, rather than having to parse this static data from
the DT each time the system boots.
I guess some of this is related to trying to create a generic
mmio-clock/mmio-timer binding?
>
> /* DO NOT USE (it doesn't work) */
It'd probably be best to add status="disabled" to this node then.
> timer0 {
This node should also be named just "timer". But then you get multiple
nodes with the same name, so you'd need to include a "unit address"
("@nnn", where nnn is the first reg address) in the node name, giving
"timer at c".
> compatible = "broadcom,bcm2835-timer", "broadcom,bcm2708-clock", "mmio-timer";
> reg = <0x00c 0x4>;
> reg-names = "compare";
> interrupt-parent = <&vic1>;
> interrupts = <0>;
>
> index = <0>;
There shouldn't be a need for a separate index property; that's what the
reg property is for. If you want, you can make the reg value be the
index, so reg=<0>, reg=<1> etc., and map from index to address in the
driver for the parent node, at least with a custom driver.
> rating = <0>;
> min-delta = <0xf>;
> };
> dma: dma {
> compatible = "broadcom,bcm2835-dma", "broadcom,bcm2708-dma";
> reg = <0x7000 0x1000>;
>
> interrupt-parent = <&vic1>;
This is probably the same value everywhere. In which case, you can set
the property in /, and it will "trickle down".
> interrupts = <16>, <17>, <18>, <19>, <20>, <21>, <22>, <23>, <24>, <25>, <26>, <27>, <28>;
>
> channels = <0x10>; /* Set by VideoCore */
There are only 12 interrupts defined above; that seems inconsistent with
16 channels. Why does the VideoCore need to change the number of
channels? Custom property -> add the vendor prefix (I guess I'll just
stop saying that, since it applies everywhere)
> };
>
> armctrl {
Is this a true separate bus in HW?
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0xB000 0x1000>;
> ranges = <0 0xB000 0x1000>;
>
> intc {
Again, I'm not sure this is a true separate bus in HW?
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0x200 0x200>;
> ranges = <0 0x200 0x200>;
>
> vic0: bank0 {
"intc at 0" would probably be a more typical node name, or
"interrupt-controller at 0".
> compatible = "broadcom,bcm2835-armctrl-ic", "broadcom,bcm2708-armctrl-ic";
> reg = <0x000 0x4 0x018 0x4 0x024 0x4>;
> reg-names = "pending", "enable", "disable";
Uggh. The register ranges for the 3 banks are all
overlapping/interleaved? Again I'd be inclined to just create a single
controller for the entire 3-bank controller, and hooking up the
cascading between the banks in a static/hard-coded fashion within that
one driver. You'd probably want #ingterrupt-cells=<2>, so that cell 0 is
the bank and cell 1 is the interrupt bit within the bank., or you could
have 1 cell with value "(bank << 16) | bit".
> interrupt-controller;
> #interrupt-cells = <1>;
>
> interrupt-base = <64>;
Is that the Linux IRQ number base? Linux-internal information shouldn't
exist in DT - instead, use the new IRQ domains feature to dynamically
allocate the Linux IRQ numbers. The IRQ core will map from the values in
DT into Linux IRQ numbers automatically, so this won't complicate the drive.
> /* IRQs
> * 0: ARM_TIMER
> * 1: ARM_MAILBOX
> * 2: ARM_DOORBELL_0
> * 3: ARM_DOORBELL_1
> * 4: VPU0_HALTED
> * 5: VPU1_HALTED
> * 6: ILLEGAL_TYPE0
> * 7: ILLEGAL_TYPE1
> */
> source-mask = <0x000000ff>;
>
> /* Banks
> * 8: PENDING1
> * 9: PENDING2
> */
> bank-mask = <0x00000300>;
>
> /* Shortcuts */
> shortcut-mask = <0x001ffc00>;
> shortcut-map = <
> 8 7 /* 10: VC_JPEG */
> 8 9 /* 11: VC_USB */
> 8 10 /* 12: VC_3D */
> 8 18 /* 13: VC_DMA2 */
> 8 19 /* 14: VC_DMA3 */
> 9 21 /* 15: VC_I2C */
> 9 22 /* 16: VC_SPI */
> 9 23 /* 17: VC_I2SPCM */
> 9 24 /* 18: VC_SDIO */
> 9 25 /* 19: VC_UART */
> 9 30 /* 20: VC_ARASANSDIO */
> >;
(Somewhat ignoring the comments on the reg property above):
Those last 3 properties appear to be describing which IRQ status bits
are cascaded interrupt controllers vs. leaf interrupts? That shouldn't
be needed; child interrupt controllers should simply request their
interrupts just like any other driver would request a leaf interrupt,
and the Linux IRQ code should handle the cascading of the interrupt
handling through the top-level controller and into the child controllers.
> };
>
> vic1: bank1 {
> compatible = "broadcom,bcm2835-armctrl-ic", "broadcom,bcm2708-armctrl-ic";
> reg = <0x004 0x4 0x010 0x4 0x01c 0x4>;
> reg-names = "pending", "enable", "disable";
> interrupt-parent = <&vic0>;
> interrupt-controller;
> #interrupt-cells = <1>;
> #address-cells = <0>;
> bank-interrupt = <8>;
> interrupt-base = <0>;
>
> /* IRQs
> * 0: TIMER0 16: DMA0
> * 1: TIMER1 17: DMA1
> * 2: TIMER2 18: VC_DMA2
> * 3: TIMER3 19: VC_DMA3
> * 4: CODEC0 20: DMA4
> * 5: CODEC1 21: DMA5
> * 6: CODEC2 22: DMA6
> * 7: VC_JPEG 23: DMA7
> * 8: ISP 24: DMA8
> * 9: VC_USB 25: DMA9
> * 10: VC_3D 26: DMA10
> * 11: TRANSPOSER 27: DMA11
> * 12: MULTICORESYNC0 28: DMA12
> * 13: MULTICORESYNC1 29: AUX
> * 14: MULTICORESYNC2 30: ARM
> * 15: MULTICORESYNC3 31: VPUDMA
> */
> source-mask = <0xffffffff>;
So those 3 properties should be:
interrupt-parent = <&vic0>;
interrupts = <8>;
> };
>
> vic2: bank2 {
> compatible = "broadcom,bcm2835-armctrl-ic", "broadcom,bcm2708-armctrl-ic";
> reg = <0x008 0x4 0x014 0x4 0x020 0x4>;
> reg-names = "pending", "enable", "disable";
> interrupt-parent = <&vic0>;
> interrupt-controller;
> #interrupt-cells = <1>;
> #address-cells = <0>;
>
> bank-interrupt = <9>;
> interrupt-base = <32>;
>
> /* IRQs
> * 0: HOSTPORT 16: SMI
> * 1: VIDEOSCALER 17: GPIO0
> * 2: CCP2TX 18: GPIO1
> * 3: SDC 19: GPIO2
> * 4: DSI0 20: GPIO3
> * 5: AVE 21: VC_I2C
> * 6: CAM0 22: VC_SPI
> * 7: CAM1 23: VC_I2SPCM
> * 8: HDMI0 24: VC_SDIO
> * 9: HDMI1 25: VC_UART
> * 10: PIXELVALVE1 26: SLIMBUS
> * 11: I2CSPISLV 27: VEC
> * 12: DSI1 28: CPG
> * 13: PWA0 29: RNG
> * 14: PWA1 30: VC_ARASANSDIO
> * 15: CPR 31: AVSPMON
> */
> source-mask = <0xffffffff>;
> };
>
> fiq {
> compatible = "broadcom,bcm2708-armctrl-ic-fiq", "broadcom,bcm2835-armctrl-ic-fiq";
> /* Bits 0-6: IRQ (index in order of banks below)
> * Bit 7: Enable FIQ generation
> * Bits 8+: Unused
> *
> * An interrupt must be disabled before
> * configuring it for FIQ generation
> * otherwise both handlers will fire at
> * the same time!
> */
I assume this is what makes each interrupt signal generate a FIQ or
regular interrupt? This again leads me to want to see a single interupt
controller driver that hides all this inside it, rather than spliting
out a couple registers at a time into separate drivers. Do we even
expect to configure interrupts to generate FIQ in Linux anyway?
> reg = <0x00c 0x4>;
> reg-names = "control";
> interrupt-parent = <&vic0>;
> interrupt-controller;
> #interrupt-cells = <0>;
>
> banks = <8 9 0>;
> };
> };
>
> armtimer {
> /* Not AMBA compatible */
> compatible = "broadcom,bcm2835-sp804", "arm,sp804";
> reg = <0x400 0x24>;
>
> interrupt-parent = <&vic0>;
> interrupts = <0>;
> };
>
> vc_bell0: bell0 {
> compatible = "broadcom,bcm2835-bell", "broadcom,bcm2708-bell";
> reg = <0x840 0x4>;
> interrupt-parent = <&vic0>;
> interrupts = <2>;
>
> access = "r-";
That property looks a little suspect...
> watchdog {
> compatible = "broadcom,bcm2835-pm-wdog", "broadcom,bcm2708-pm-wdog";
> reg = <0x100000 0x1000>;
> };
>
> pins: pinctrl {
> compatible = "broadcom,bcm2835-pinctrl", "broadcom,bcm2708-pinctrl";
> reg = <0x200000 0x1c 0x200094 0x1c>;
> reg-names = "fsel", "pull";
The HW contains a single module that implements GPIO and pinmux
functionality. There should be a single node in DT for that one HW module.
This will make it much easier to implement the driver too, since you
won't have multiple drivers needing to interact with each-other.
Then the reg property can have a single contiguous entry here too, and
you won't need reg-names.
> functions = <6>;
> count = <54>;
> base = <0>;
>
> /* Don't change this. They're connected internally and you'll cause a short circuit. */
> input-only = <46 47 48 49 50 51 52 53>;
>
> gpio = /* ALT0 ALT1 ALT2 ALT3 ALT4 ALT5 */
> /* 0 */ "SDA0", "SA5", "", "", "", "",
...
> pull = <
> /* 0 */ 2 /* 1 */ 2 /* 2 */ 2 /* 3 */ 2
...
All of those properties most likely represent completely static data
that is completely determined by the SoC HW. As such, I would strongly
recommend placing it into the pinctrl driver, rather than forcing it to
parse the data out of DT each time the system boots, only to end up with
the same data in the end. At least in the case of this chip, the size of
the tables will be very small, so this isn't going to bloat the driver
too much. For example the Tegra driver has far far larger tables in the
kernel already.
BTW, I have a lot of experience with the pinctrl subsystem (I
contributed to the design a lot and did much of the core DT
implementation). I'd be happy to write the pinctrl/GPIO driver for
upstream if you wanted.
> amba {
> compatible = "arm,amba-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
There's an AMBA-compatible bus with just one single peripheral on it?
That seems like a waste in HW. Are you sure that the AMBA bus doesn't
cover a bunch more peripherals, it's just that not all the peripherals
are AMBA compliant?
> uart0 {
> compatible = "broadcom,bcm2835-uart", "broadcom,bcm2708-uart", "arm,pl011", "arm,primecell", "broadcom,bcm2835-pinctrl-device", "broadcom,bcm2708-pinctrl-device";
The compatible flag isn't intended to document services that nodes use.
To that end, drop the pinctrl-related compatible values here. In 3.5 or
linux-next, see
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt, or feel
free to ask me more questions here!
> reg = <0x201000 0x1000>;
> interrupt-parent = <&vic2>;
> interrupts = <25>;
>
> pinctrl = <&pins>;
Per the documentation above, this should point to a node inside the pin
controller's node that indicates which pins to set to which mux settings.
However, since this is the SoC .dtsi file not the board .dt file, all
the pinctrl properties should really be omitted here, since pinctrl
configuration is board-specific.
I can provide a lot more information here if you want; just ask:-)
> pinmap = "UART0";
and delete that property.
> usb {
...
> vc_power = <&power>;
> vc_index = <3>;
Probably used standard regulator properties here instead.
bcm2835-rpi-b-dts:
> /dts-v1/;
> /include/ "bcm2835.dtsi"
>
> / {
> model = "Raspberry Pi Model B (BCM2835)";
I'd personally be inclined to drop " (BCM2835)" at the end, but not a
big deal.
> #address-cells = <1>;
> #size-cells = <1>;
Those are in skeleton.dtsi (and currently in bcm2835.dtsi too).
> leds {
This node was in bcm2835.dtsi. It's board-specific, so should be removed
from there.
> axi {
> pins: pinctrl {
> pins =
> /* 0 */ "P1-03", /* 1 */ "P1-05", /* 2 */ "S5-14", /* 3 */ "S5-13",
I'm not sure what this property is intended to represent. Is it the
static SoC data that should be in either bcm2835.dtsi or the driver, or
is it the selected pinmux configuration for this board?
Either way, the format doesn't look right to me. Perhaps we should start
a separate more detailed thread to talk through pinmux to resolve this?
> groups = /* GPIO_I GPIO_O ALT0 ALT1 ALT2 ALT3 ALT4 ALT5 */
> /* 0 */ "P1", "", "BSC0", "", "", "", "", "",
Since the BCM2708 HW is completely programmable per-pin rather than
per-group, I wouldn't expect the pinctrl driver to expose any groups at
all. And group definitions would be part of bcm2835.dtsi not the
per-board file too?
(although note that the current pinctrl core only supports mux selection
on groups not individual pins, so you do need to define a group for
every pin, but that's an implementation detail that shouldn't leak into
the DT format, and will hopefully be resolved soon enough anyway)
> usb {
> hub {
> compatible = "usb,hub", "usb,device";
> port = <1>;
>
> ethernet {
> compatible = "net,ethernet", "usb,device";
> port = <1>;
>
> mac-address = [00 00 00 00 00 00]; /* Set by VideoCore */
Hmmm. That's a very interesting problem. This definitely needs more
discussion upstream. The issue is that one doesn't usually put nodes
into DT for HW that can be probed at run-time, but rather only for
non-probe-able fixed stuff.
Still, a similar requirement exists for WiFi RF kill on some boards
(Toshiba AC100 for example), where there's an RF kill switch that
affects a USB-based device, but the USB device isn't represented in in
DT because it's probe-able, and even irrespective of that the device
couldn't be in DT, since it's a replaceable device on a mini-PCIe slot
anyway).
So, I don't have much advice here other than expect significant
discussion to try and work out the correct solution for this.
I think that's it for now. I hope this helps!
More information about the linux-rpi-kernel
mailing list