Device tree review

[Simon Arlott]

On 31/05/12 02:57, Stephen Warren wrote:
> 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.

The problem is claiming that a certain set of devices will exists in
bcm2708 is almost certainly going to be wrong, so bcm2835 makes more
sense.

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

$ git grep "EXPORT_SYMBOL(system_" arch/arm/kernel/
arch/arm/kernel/setup.c:EXPORT_SYMBOL(system_rev);
arch/arm/kernel/setup.c:EXPORT_SYMBOL(system_serial_low);
arch/arm/kernel/setup.c:EXPORT_SYMBOL(system_serial_high);

These lack standard DT bindings but they are global identifiers in the
ARM arch.

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

Well, there is the poor interface which requires it to BUG() from
bcm2708_fb_set_par because the caller can't handle an error return code.

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

I've not tried using anything except the frame buffer code and it works
without anything external.

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

You can't do EDID on the composite interface. It's a bit odd but that's
how the initial resolution is specified.

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

No, I meant getting Linux to not use fixed virtual addresses. The macro
for translation doesn't even match the 0x7e values from the PDF.

arch/arm/mach-bcm2708/include/mach/hardware.h:
#define IO_ADDRESS(x)           (((x) & 0x0fffffff) + (((x) >> 4) & 0x0f000000) + 0xf0000000)
is non-linear when mapping 0x20 to 0x7e.

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

@nnn is redundant as DT always appends the first reg address to the name
and @* is ignored.

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

They currently only appear to be used by vchiq.

I could encode it like a umask instead and use 0400?

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

Really? Can two drivers can claim the same device?

However, the GPIO driver becomes a bit useless without pinctrl as it
tries to request the GPIO which will fail if no pinctrl device has
registered the GPIO numbers. I'll merge the GPIO code into the pinctrl
driver.

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

The pinctrl data in bcm2835.dtsi could be put into the driver.

The pinctrl data in bcm2835-rpi-b.dts is board-specific.

I'll rework the driver. The manual locking of pins via sysfs isn't as
useful as I thought it would be because GPIO export already locks them.

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

I don't name the GPIOs, only the physical pins on the headers which
aren't exported to pinctrl. The GPIOs don't even have names.

Not all functions use the same ALT settings for all GPIOs (ARM_JTAG and
the CSI). Pinctrl is incapable of supporting the ALT settings because it
makes this assumption.

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

I don't know.

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

Why? I think it's entirely reasonable to unload the modules for SPI or
I2C and be able to use those GPIOs for something else.

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

It doesn't need a group for every pin because it keeps all the
information about the GPIOs, not just what is given to pinctrl.

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

Ok, so "group" is a mis-named feature and PIN_MAP_CONFIGS_PIN isn't
quite adequate to describe what a device needs.

-- 
Simon Arlott