Device tree review #2

[Stephen Warren]

As always, hope this all helps. I certainly feel like the DT files are
moving in the right direction quickly. I guess there /is/ a lot of text
below, but it mostly seems more minor than what I recall from my
previous review.

bcm2835.dtsi:

> /*
>  * 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.

> 	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.

> 	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!

> 	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.

> 		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.

> 	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.

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
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.

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.

> 		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.

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.

> 			/* Not AMBA compatible */
> 			compatible = "broadcom,bcm2835-sp804", "arm,sp804";

For consistency, I'd expect broadcom,bcm2708-sp804 in this list too.

> 		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?

> 		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:-)

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.

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.

> 		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).

> 		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".

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.

> 	memory {
> 		device_type = "memory";

That's also in skeleton.dtsi, included through bcm2835.dtsi.

> 	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.

> 		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.

> 		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.

> 		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.

> 		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.

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.