Device tree review

[Simon Arlott]

On 30/05/12 04:25, Stephen Warren wrote:
> 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.

Could you review it again? (branch rpi-split)

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

As discussed, no need for a separate file until it becomes required.

>> /include/ "skeleton.dtsi"
>> 
>> / {
>> 	model = "BCM2835";
> 
>> 	#address-cells = <1>;
>> 	#size-cells = <1>;
> 
> Those two are already in skeleton.dtsi.

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

Moved to system { }.

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

Leaving it in makes it clear what VideoCore actually sets.

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

Fixed.

>> 		};
>> 	};
> 
>> 	memory {
>> 		device_type = "memory";
>> 		reg = <0 0>;			/* Set by VideoCore */
>> 	};
> 
> That whole node and set of properties is in skeleton.dtsi.

Moved to bcm2835-rpi-*.dts and set to 256MB.

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

Fixed.

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

Fixed.

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

Prefix added.

>> 	power: power {

Converted to fixed voltage regulator (as a virtual bus).

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

They also appear to be bus addresses as the DMA engine needs to use them
when accessing peripherals. I'd prefer to leave as is.

>> 		st {
>> 			stc {

Replaced with a single clock/timer device.

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

Fixed.

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

Prefix added. This mask is required as the VideoCore uses some of the
channels and needs to communicate which ones are available.

>> 		};
>> 
>> 		armctrl {
> 

Replaced with a single interrupt controller device. The shortcut map is
now longer in the device tree file which kind of defeats the point of
device tree.

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

I've removed it and made a single bell device. Testing reveals we can
actually ring our own doorbell :)

>> 		pins: pinctrl {
>> 			compatible = "broadcom,bcm2835-pinctrl", "broadcom,bcm2708-pinctrl";
>> 			reg = <0x200000 0x1c 0x200094 0x1c>;
>> 			reg-names = "fsel", "pull";

This has been rewritten by Chris.

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

Removed.

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

Removed.

>> 	leds {
> 
> This node was in bcm2835.dtsi. It's board-specific, so should be removed
> from there.

Removed.

-- 
Simon Arlott