Device tree review

[Stephen Warren]

On 05/30/2012 01:06 PM, Simon Arlott wrote:
> 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.

I'd assume there would have to be some amount of similarity between SoCs
in a family. Still, we can add bcm2835.dtsi now, and then if needed
later, re-factor any common parts into a shared bcm2708.dtsi once we
have more information.

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

Can you point out where you've seen these used? I don't see any mention
of them in arch/arm/boot/dts/* or Documentation/devicetree/bindings/...
in the linux-next kernel as of a few days ago.

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

I'm pretty sure it can. If it can't yet, libfdt can always be 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
> 
> 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.

OK, that's very good new then. It's quite likely that the/a framebuffer
driver can indeed be upstreamed without issue.

I assume that all the OpenGL and video codec stuff is completely
independent from the FB driver then?

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

Hmm. I'd expect any Linux driver to be pretty standalone, and do things
like retrieve the EDID from the display device, and make decisions
mostly based on the EDID rather than whatever random configuration the
bootloader picked. Without all those smarts in the FB driver, it would
be hard for an X server to provide full randr capabilities etc.

Certainly however, the FB driver should be able to find out whatever
configuration the bootloader picked, and continue to use that
configuration by default.

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

Assuming you're talking about the .pdf I mentioned, that's a question
for Broadcom! Certainly, I'd expect to see that document purely describe
the HW physical addresses, and nothing to do with Linux virtual addresses.

If mean the DT content, then just use "ranges;" for each bus, and then
you won't need to subtract anything from the child nodes' reg values.

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

For whatever reason, this is the way device tree has been specified. The
people reviewing DT files upstream are certainly going to pick on
exactly this kind of stuff.

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

Hmmm. It sounds like we should push back on Broadcom to fix the binary
blobs that run on the other CPU not to require this.

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

That makes me even more inclined to recommend having a single driver
that handles all the banks at once then, so this kind of detail can be
hidden entirely within the driver, i.e. not allocating any Linux virtual
IRQ numbers for the inter-bank interrupts, and not doing any cascading.

> There are also several different very very similiar IRQ handlers already
> in arm for 2, 3 or 4 banks.

I'd be interested to look at them too. Can you point out which files?
Thanks.

>>> 			vc_bell0: bell0 {
...
>>> 				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.

Encoding this as an integer might make more sense. Are the bell devices
used in Linux? If not, it might be simplest to just leave this out for now.

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

That's true, but it's still a single HW module, so should be represented
that way in DT. The one DT node can spawn separate GPIO and pinctrl
drivers if you need.

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

DT is mainly meant to allow different boards using the same SoC to be
supported by changing the .dts file. There's a lot less value in
allowing different SoCs to be supported just by changing the .dts file
without writing new drivers. In this case, there is probably even less
value, since apparently there will only ever be the BCM2835 that we need
to care about.

Irrespective of that though, you can structure the driver in such a way
that the information above doesn't need to be parsed from device tree,
yet the driver might still end up being able to support multiple similar
pin controllers in different SoCs.

For example, instead of naming the features on each chip, simply name
the pins "0", "1", "2, and the mux options as "alt0", "alt1", "alt2",
etc. This is in fact exactly how the data sheet names them, so this
would have the advantage of matching the data sheet better too. That
way, if a SoC variant hooked up pin 0's alt0 function to be UART and a
different SoC variant hooked up pin 0's alt0 function to be SPI, the
pinctrl driver would only need to expose "alt0" and the relevant board
files would just select which "alt" function to put in their pinctrl
configuration node based on what the data sheet told them.

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

Sure, but is there really a separate bus in HW just for that device?

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

The .dts file should just be describing the single configuration that
you want to use for that board. There's no need to enumerate all the
possibilities of groups etc. that could be used, but won't be.

The pinctrl driver or SoC.dtsi file should list what features the HW
supports (e.g. there are N pins each supporting M alt functions).

The board SoC .dtsi file should define which of those alt functions to
pick for each pin.

The RPi is a bit unusual here in that many of the pins are exposed to
the user and users will likely want to change between different
configurations. However, the way to make that change is to edit the
board .dts file to represent how they want to hook things up and reboot;
people shouldn't be plugging the HW hookups while the system is running
anyway.

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

Unfortunately, that's just the way pinctrl works right now, so there's
no way around that requirement.

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

That's not a limitation in pinctrl. That would be caused by incorrect
mapping of HW features into the pinctrl driver. There are no groups in
the pinctrl HW, so there shouldn't be any groups exposed by the pinctrl
driver or binding (except for the per-pin groups that the pinctrl
subsystem currently requires for muxing). Once that change is made,
there is only 1 pin in any group, and there's no issue that there's a
single mux function selection for all pins in a group.