Device tree review #2

[Simon Arlott]

On 20/06/12 04:48, Stephen Warren wrote:
>> /*
>>  * The "-vc" for the compatible value indicates that VideoCore is
>>  * booted first and is in control of the hardware.
>>  *
>>  * Future firmware versions that swap this around so that the
>>  * bootloader transfers control to the kernel running on the ARM
>>  * immediately. When this happens, different architecture setup
>>  * will be required and the "-vc" will be changed to "-arm" so
>>  * that the kernel knows which mode is being used.
>>  *
>>  * See https://github.com/raspberrypi/firmware/issues/45
>>  */
>> 
>> / {
>> 	model = "BCM2835";
>> 	compatible = "broadcom,bcm2835-vc", "broadcom,bcm2708-vc";
> 
> That doesn't seem right. The compatible value is meant to document the
> hardware the system is running on, not the runtime usage of that
> hardware. As such, I would not expect the compatible value to vary
> depending on which firmware was in use. It may be reasonable to add a
> separate explicit property for this somehow, but this requires some more
> though. A hard compile-time option (or even only supporting one scheme)
> might even be reasonable.

I've changed it back because the architecture differences can probably
be described with different devices for power and clock management.

>> 	system {
>> 		revision = <0>;			/* Set by VideoCore */
>> 		serial = <0 0>;			/* Set by VideoCore */
> 
> Revision of what; board-revision? It may be useful to standardize these
> two properties, but without that, it seems like a vendor prefix is in order.

These are standard Linux properties on the arm architecture... I've
prefixed them with "linux," now.

>> 	cpus {
> 
> Since the child node includes a unit address ("@0"), you need the
> following here:
> 
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> ...
> 
>> 		cpu at 0 {
>> 			compatible = "arm,1176jzf-s";
> 
> ... and also:
> 
> 			reg = <0>;
> 
> Looking at arch/arm/boot/dts/*, very roughly half do this correctly, so
> there's at least some precedent!

Fixed.

>> 	display {
>> 		compatible = "broadcom,bcm2835-fb", "broadcom,bcm2708-fb";
> 
> OK, so there's absolutely no direct HW programming here; everything goes
> through the VC mailbox? I was expecting a reg=<> here, but if it all
> goes through the VC, no reg property is needed.

Correct.

>> 		broadcom,vc-mailbox = <&vc_mbox>;
>> 		broadcom,vc-channel = <1>;
> 
> Having two separate properties for the mailbox/channel is probably fine.
> 
> That said, a single property that defines both the mailbox provider node
> /and/ the channel to use on the node/device would look much more similar
> to many other bindings such a GPIO. That would look like:
> 
> 		broadcom,vc-mailbox = <&vc_mbox 1>;
> 
> However, it's probably OK as-is.

Fixed.

>> 	axi {
>> 		compatible = "broadcom,bcm2835-axi", "broadcom,bcm2708-axi", "simple-bus";
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		reg = <0x20000000 0x01000000>;
>> 		ranges = <0 0x20000000 0x01000000>;
> 
> Is there a driver for the bus itself that uses these registers? If not,
> you would typically omit the reg property here. Also, I believe I recall
> Mitch Bradley (a DT expert) saying that reg and ranges should not overlap.

No, I've fixed this.

> In your response to the previous DT review, I didn't quite follow your
> reasoning behind having this ranges property in place. You had mentioned
> that it was because you thought the reg values the children end up using
> are bus addresses. However, the driver ends up seeing the same

No, it's for ease of use.

> CPU-absolute address irrespective of whether the absolute value is
> written into each node's reg property, or whether each node's reg
> property encodes an offset relative to some parent node's ranges. I
> would personally choose to concentrate more on making the reg values
> everywhere match the documentation rather than encode offsets, for
> ease-of-use. But that would require fixing the documentation to use
> physical addresses everywhere.

The documentation uses bus addresses as that's what it expects when
using the DMA controller.

> But that all said, this DT content is technically acceptable in this
> aspect, yields the correct results, and shouldn't cause any issues, so I
> can't really object to it; I'm really just harping on about my personal
> preferences here.
> 
>> 		dma: dma {
>> 			broadcom,channels = <0>;	/* Set by VideoCore */
> 
> Is that a bitmask or the number of the maximum channel ID the ARM can
> use? I'm mainly curious here; I wouldn't expect any comment in the .dtsi
> file to explain this, but rather something like
> Documentation/devicetree/bindings/dma/broadcom,bcm2708-dma.txt to define
> this.

It's a mask... I know I still need to add documentation but firmware
issue #47 might replace the need to put it in DT.

>> 		intc: interrupt-controller {
>> 			/* Bank 0
>> 			 * 0: ARM_TIMER
>> 			 * 1: ARM_MAILBOX
> 
> I guess it's nice to have these in a comment in the .dtsi file for
> reference, but it's probably unusual. Something like
> Documentation/devicetree/bindings/arm/bcm2708/broadcom,bcm2708-armctrl-ic.txt
> would be a more typical location to define the legal values.

I just needed somewhere to put them so that the list of IRQs isn't lost.

> One day hopefully dtc will have #defines or similar, so we can just put
> them here...
> 
>> 		armtimer {
> 
> I think I'd just name that timer. Since there would then be two timer
> nodes, they'd need to include the unit address in their name; timer at 3000
> and timer at b400.

Fixed.

>> 			/* Not AMBA compatible */
>> 			compatible = "broadcom,bcm2835-sp804", "arm,sp804";
> 
> For consistency, I'd expect broadcom,bcm2708-sp804 in this list too.

Fixed.

>> 		vc_bell: bell {
>> 			compatible = "broadcom,bcm2835-bell", "broadcom,bcm2708-bell";
>> 			reg = <0xB840 0x10>, <0xB8E4 0x4>;
>> 			interrupts = <0 2>, <0 3>;
>> 
>> 			gpio-controller;
>> 			#gpio-cells = <1>;
> 
> It's rare to have #gpio-cells=1; 2 is far more typical since then the
> 2nd cell can encode flags. Still, the flags aren't used consistently (or
> even often) in the Linux kernel, so perhaps it isn't worth it.
> 
>> 			interrupt-controller;
>> 			#interrupt-cells = <1>;
> 
> The bell object is a GPIO and IRQ controller too? IRQ perhaps I could
> understand, but GPIO typicaly implies an external pin on the SoC
> package, which seems slightly unlikely here. Could you explain more
> about this HW; it's not in BCM2835-ARM-Peripherals.pdf right?

I'll have to remove the bell driver as I don't have a user for it.

When you write to the bell register it generates an interrupt. There
are 4 bells, with 2 interrupts on the ARM and 2 on the VC.

>> 		gpio: gpio {
> 
>> 			/* BSC0 */
>> 			pins_bsc0_a: bsc0_a {
>> 				broadcom,pins = <0>, <1>;
>> 				broadcom,function = "ALT0";
>> 			};
> 
> I'm curious why the pins are specified by ID but the function using a
> string; I would have naively expected both to be strings or both to be
> integers. I'd also expect a slight bias towards strings too, since the
> pinctrl subsystem uses strings internally. But of course bindings
> shouldn't be written with some specific SW implementation in mind if
> possible:-)

The names of the functions are from the documentation.

> Still, this binding should work just fine as it is.
> 
>> 			pins_bsc0_b: bsc0_b {
>> 				broadcom,pins = <28>, <29>;
>> 				broadcom,function = "ALT0";
>> 			};
>> 
>> 			pins_bsc0_c: bsc0_c {
>> 				broadcom,pins = <44>, <45>;
>> 				broadcom,function = "ALT1";
>> 			};
> 
> Hmmmm.
> 
> In some ways it is admirable to try and enumerate all the possible
> locations that each HW module could be mux'd out on to. However, it
> means a ton of often unused nodes must exist in the .dtsi file to define
> these. I /personally/ would tend to completely avoid enumerating all the
> (any) options in the .dtsi file, but rather require each board .dts file
> to define /just/ the actual combination that the particular board wants
> to use.

The board .dts selects the one that is actually connected.

> Part of the reason for this - there are 3 combinations listed above, but
> given that the HW muxes pins one-by-one rather than in pairs, you
> actually need to enumerate a ton more combinations of pins; I believe
> there's no reason that BSC0 couldn't use pins 0+1, 0+29, 0+45, 28+1,
> 28+29, 28+45, 44+1, 44+29, 44+45; 9 combinations instead of just 3.

These happen to be the combinations that all use the same function
selections, so it's not unreasonable to define them, although you're
right there are a lot more potential combinations.

>> 		spi {
> 
> For all the peripherals that control an IO bus which the board may chose
> to use, or not to use (choose to mux out on the pinmux or not), I tend
> to add the following property in the .dtsi file:
> 
> 			status = "disabled";
> 
> That way, none of these controllers actually automatically get devices
> created for them in Linux. Each board .dts file can then override this
> by setting:
> 
> 			status = "okay";
> 
> Just for the specific controllers that it actually does use (and also
> define which pinctrl settings to use for it).

This is already done for pinctrl, but I don't see a benefit in adding
"disabled" everywhere until there are boards with different exposed
devices.

>> 		usb {
>> 			compatible = "broadcom,bcm2835-usb", "broadcom,bcm2708-usb", "synopsys,designware-hs-otg2-host";
> 
> Since the HW itself is presumably host/device/otg capable, I might be
> tempted to change that last compatible value to
> "synopsys,designware-hs-otg2" (i.e. remove "-host"), and use a separate
> property to indicate which mode to operate in. There is precedent for a
> "dr_mode" property with value "host", "peripheral", or "otg".

Fixed. This makes sense for the Model A as it can operate in OTG mode.

> bcm2835-rpi-b.dts:
> 
>> / {
>> 	model = "Raspberry Pi Model B (BCM2835)";
> 
>> 	#address-cells = <1>;
>> 	#size-cells = <1>;
> 
> Those two are in skeleton.dtsi, included through bcm2835.dtsi.

Fixed.
 
>> 	memory {
>> 		device_type = "memory";
> 
> That's also in skeleton.dtsi, included through bcm2835.dtsi.

Fixed.

>> 	power: regulator {
>> 		compatible = "broadcom,bcm2835-power-mgr", "broadcom,bcm2708-power-mgr", "simple-bus";
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
> 
> Given that the power manager is a core part of the SoC, I'd expect most
> of this node to be in bcm2835.dtsi, not the board-specific file. The
> board file can always add or override any properties that need to be set
> in a board-specific way, perhaps such as the voltages.

The power manager is not part of the SoC, it's part of the Raspberry Pi
version of the VideoCore, so it should be in the rpi board file. If we
get direct access to the power management then that situation will
change.

>> 		broadcom,vc-mailbox = <&vc_mbox>;
>> 		broadcom,vc-channel = <0>;
>> 
>> 		regulator-name = "VideoCore";
>> 		regulator-min-microvolt = <5000000>;
>> 		regulator-max-microvolt = <5000000>;
>> 		regulator-always-on = <1>;
> 
> It's not clear to me why one regulator is defined at the "top" level
> directly as part of the power manager device, whereas the other two are
> defined as child nodes; I would expect to see all 3 regulators defined
> as child nodes.

I need a parent device as there's currently only a single bit map for
all powered devices, which the child regulators need to share.

>> 		sd_card_power: regulator at 0 {
>> 			compatible = "broadcom,bcm2835-power-dev", "broadcom,bcm2708-power-dev";
>> 			reg = <0>;
> 
> Do you consider each regulator a completely separate device then? And
> hence, a separate Linux device gets created for each? In very recent
> kernels, you /could/ have a single power-manager device that happens to
> register 3 regulators using 3 child nodes. See of_regulator_match(), the
> drivers that use it, and their bindings. But do note that some rework is
> happening to that function right at this moment; best to check the
> regulator mailing list for this discussion and wait for it to settle
> before deciding if you want to use that scheme, and before changing any
> code.

Each regulator is a separate node, this is a DT requirement. With
firmware issue #47 implemented they're better off as separate devices
as they'll then be independent from each other (and I'll change the
compatible name).

>> 		uart1 {
>> 			status = "disable";
>> 		};
> 
> As I alluded to in my comments on bcm2835.dtsi, it seems slightly
> clearer (to me at least) to disable all optional devices in bm2835.dtsi
> and enable only those you use in the board .dts file. That way the board
> .dts file will only mention devices it actually uses, and also will
> explicitly mention all devices it does use, even if there are no
> board-specific properties that the board .dts file needs to set for a
> given device. Either will work though.

This one is more relevant to the board file given that both UARTs share
the same pins, so the second one is redundant.

>> 		usb {
>> 			hcd-supply = <&usb_hcd_power>;
>> 
>> 			hub {
>> 				compatible = "usb,hub", "usb,device";
>> 				port = <1>;
> 
> I think you want to use the reg property to identify USB ports, rather
> than invent some new non-standard property.

Fixed.

> It would be good to discuss this DT style upstream; "this" being
> creating DT nodes to represent devices on probe-able buses. I'm involved
> in a couple other discussions that might benefit from doing that in DT,
> and building some common infra-structure for it would be ideal.

Some sort of include/linux/of_usb.h and drivers/of/usb.c would be
appropriate here. Unfortunately the lack of USB support has stalled any
attempt to work on adding this. I'm also expecting opposition from
netdev :(

-- 
Simon Arlott