Device tree review
Simon Arlott
simon at fire.lp0.eu
Wed May 30 15:06:43 EDT 2012
On Wed, May 30, 2012 04:25, Stephen Warren wrote:
> 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.
It's unclear what the future intentions of Broadcom will be and they
currently state that 2835 is a model within the 2708 family that supports
Linux and that there won't be any more. There's no reason to believe
that another 2708 device would have the same selection of devices.
>> /include/ "skeleton.dtsi"
>>
>> / {
>> model = "BCM2835";
>
>> #address-cells = <1>;
>> #size-cells = <1>;
>
> Those two are already in skeleton.dtsi.
Ok
>> 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...
Linux on arm has these two properties, do you want to call them linux,*?
>> 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.
As long as libfdt can do this otherwise it should stay in.
>> };
>>
>> 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.
Ok
>> };
>> };
>
>> memory {
>> device_type = "memory";
>> reg = <0 0>; /* Set by VideoCore */
>> };
>
> That whole node and set of properties is in skeleton.dtsi.
Ok
>> 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.
Ok
>> 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".
Ok
>> 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
Have you looked at the framebuffer driver? It doesn't require anything
from userspace. I didn't even have a userspace until I started using an
initrd.
> 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.
It needs some initial information on what display properties to use, which
it gets via the GPU bootloader.
>> 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.
I couldn't see anything simple in Linux for attaching a power management
device to other arbitrary devices.
>> 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.
Is it possible to sort this out so that it only ever refers to 0x7e..
or 0x20..?
>> 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.
Because there are a dozen slightly different drivers for N, N+1, N+2
I wanted to make it work for any number of timers.
>> 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?
Yes, so it's really linux,rating.
>>
>> /* 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".
I don't see why the node name matters that much...
>> 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.
It's the index into the timer control register.
>> 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".
It's not because there are three interrupt banks in total.
>> 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)
This driver is incomplete. The channels value is a mask. I don't know
why the VideoCore needs to change the channels. The default is 0x10 and
it then sets it to 0x3c.
>> };
>>
>> armctrl {
>
> Is this a true separate bus in HW?
>
No
>> 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
Maybe
> have 1 cell with value "(bank << 16) | bit".
No
>
>> 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.
This is redundant and can be removed. It used to be using the legacy domain
which meant IRQ 0 never ever worked and it started blocking GPIO IRQs, so I
converted it to use linear domains.
>> /* 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.
They are not true cascaded interrupts, they're shortcuts. You can't mask
them. There's a comment in the IRQ handler driver that explains their
quirks.
There are also several different very very similiar IRQ handlers already
in arm for 2, 3 or 4 banks.
>> };
>>
>> 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?
There's not currently a FIQ driver because Linux has practically no support
for it.
>> 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...
The bell device appears to be read-only/read-write/write-only and there's no
documentation.
>> 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.
They don't currently need to interact except by the standard interfaces
for requesting a GPIO (and that wouldn't change if it was a combined
driver registering two devices).
> 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.
No, because the connections aren't that static. New models could have
different connections using the same driver - isn't that the whole point
of DT?
> 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.
I already wrote a driver for both of those...
>> 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?
Only the uart0 is 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!
When this was written there was no way to do a pinctrl mapping in DT.
This was the only sensible way to do it without adding it to every client
driver.
>> 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.
The data in the .dtsi is for each GPIO on the 2835 so it won't change, and
the rpi .dts file contains which pin these are physically connected to.
> 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.
Ok
>> 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?
The format is just compact. It's a list of the pins for all the GPIOs in
order 0-53.
>> 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?
No, because the 2835 allows for more combinations than make sense with
the specific board. You can output the uarts in many ways but only one of
them maps to real pins that can be used. This is simply listing all the
valid groups so that the drivers can make use of them with pinmux.
> (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)
We don't need a group for every pin. There's also a lack of proper support
for the 2835 in pinctrl. It assumes that a group will want to set all pins
in the group to the same function which isn't always the case.
>> 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.
This looks like the best way to express a fixed usb device that needs
some configuration from DT. You can move the mac-address to the board
but there still needs to be a way to state which device gets that mac.
> 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!
--
Simon Arlott
More information about the linux-rpi-kernel
mailing list