Device tree review

[Stephen Warren]

On 06/02/2012 01:55 PM, Simon Arlott wrote:
> 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:

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

Ah OK. My comments were purely directed at the device tree
representation, so global variables in the Linux kernel have no
relevance there.

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

It is definitely possible to both change that macro to something else
(the patch I posted already did that), or in fact to eliminate most, if
not all, the static mappings completely. All you need to do is call
ioremap() on the physical address to get a dynamically allocated/mapped
virtual address in all cases. Once all users of IO_ADDRESS are removed,
you can get rid of the static mappings required for them.

A few small static mappings might still be needed. For example, if you
enable DEBUG_LL, that will typically rely on a static mapping having
been set up for the UART. There /might/ be other cases to watch out for
too; experimentation would tell!

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

What the kernel does isn't the point, and I don't believe DT "does" any
such thing; it's a data format not code.

The way device tree is designed/specified requires the node names to
represent the generic type of object. If this then results in multiple
nodes with the same name, you must include the "@nnn" in the node name
in the .dts file. Now, this is all arguably somewhat of an arbitrary
convention and there are many other ways it culd have been defined, but
it is defined this way. This kind of thing is something that upstream
reviews will catch and if the conventions aren't followed, the patches
will need to be reworked until they are.

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

What exactly is this representing? A umask would only make sense for
filesystem permissions, and I'm not sure how that would correlate to a
description of HW.

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

Depending on how the drivers are written, that's certainly possible,
both when desired and accidentally.

What I'm advocating is a single driver for the single HW module (so a
single platform device and single probe() routine), and that driver will
then register both as a GPIO driver with the GPIO subsystem and as a
pinctrl driver with the pinctrl subsystem, all from the single probe()
routine. IIRC, there's already at least one example of this in
drivers/pinctrl in the latest Linus tree.

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

OK, that sounds like what I'm talking about.

[discussion of pincrtl]
> 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.

That's not true.

If you represent the HW as having the functions ALT0, ALT1, ... and
don't create virtual groups, pinctrl can represent the HW perfectly.

The only issue comes into play when you start creating "virtual" groups
in the pinctrl driver that don't really exist in HW. That's the only
case where a group even has multiple pins, and hence where you might
care that a given HW module may be muxed in using different ALT
functions on different pins. And even then, that's quite possible to
represent in pinctrl; just store the set of ALT functions that are
required as data in the pinctrl driver associated with the group or
function.

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

SW-wise yes. However, that's only practically useful if the HW connected
to those pins has changed. The only way to do that is to physically
unplug something and plug in something else. Since the pins probably
aren't intended to be part of a hot-pluggable bus, the unplug/replug is
something that should be done with the power off anyway.

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

No, group is not mis-named. What makes you think it is? A "group" in
pinctrl terminology is a group of pins in physical actuality.