[U-Boot] [PATCH v2 3/6] tegra: dts: Sync tegra20 device tree files with Linux

[Stephen Warren]

On 05/09/2016 01:26 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 9 May 2016 at 11:09, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 05/08/2016 04:55 PM, Simon Glass wrote:
>>>
>>> Sync everything except the display panel, which will come in a future
>>> patch.
>>> One USB port is left disabled since we don't want to support it in U-Boot.
>>
>>
>> I'd rather be a bit more careful here, and only import the DT nodes directly
>> related to display output.
>>
>> This change brings in a slew of other nodes that aren't used by U-Boot
>> (something we've historically explicitly avoided) such as pinctrl, audio,
>> Tegra KBC, I2C mux, & regulators.
>
> I believe that audio, KBC and regulators are used.

Audio and regulators certainly aren't used by mainline U-Boot; we don't 
have any audio drivers and IIRC we don't have any regulators 
instantiated in the DTs currently in U-Boot for Tegra devices.

>> It also doesn't sync the /aliases node with the kernel (e.g. Seaboard I2C,
>> and I think USB for all boards), and at least Harmony's USB nodes don't seem
>> to match what's in the kernel so I'm not sure where the DT content came
>> from, e.g. consider usb at c5004000's nvidia,phy-reset-gpio used an integer
>> rather than GPIO_ACTIVE_LOW, which is present in at least the most recent
>> kernel release (v4.5).
>
> This was against v4.4, but I may have messed up the merge in some
> cases. since I had to change the addresses from 64 bit.

It looks like that particular change was made in v3.11 in the kernel.

>> Splitting this up a bit or limiting it to just display-related nodes would
>> make it easier to debug any issues that crop up with the sync. Also, have we
>> made an explicit decision to change the policy of only including DT nodes
>> that U-Boot actually uses, rather than simply copying the entire kernel DT
>> into U-Boot? I'm pretty sure that some board(s) have deliberate differences
>> in areas other than display, e.g. since U-Boot doesn't (or at least didn't)
>> support pinctrl-based I2C muxed which are used on some Tegra20 boards in the
>> kernel at least, and hence U-Boot likely either disabled those I2C ports or
>> picked an explicit pinmux configuration to hard-code to.
>
> I think I know what you mean, and I don't believe that actually
> affects any I2C ports that are used in U-Boot. Do you have any
> example?

In tegra20-seaboard.dts, the kernel DT has a pinctrl-based I2C mux on 
controller /i2c at 7000c400, whereas U-Boot has that I2C controller routed 
to some fixed pinctrl setting, and is currently enabled. Thinking more 
about this, since the i2c-mux-pinctrl node won't currently be processed 
by U-Boot in any way, since there is no driver for it, that mux node 
shouldn't affect the I2C controller's operation any way in U-Boot, so 
everything should work out fine.

tegra20-ventana.dts has an i2c-mux-pinctrl node in the kernel DT, but 
the I2C controller isn't enabled in U-Boot, so there should be no 
possibility of regression.

tegra20-tamonten.dtsi already has the i2c-mux-pinctrl node in the U-Boot 
DT without issue.

> I'd rather have the DT completely in sync, so far as can be done. We
> have this merge window to find problems. I don't see a big benefit to
> leaving stuff out...at least with other boards we've defaulted to just
> bringing everything in.
>
> Are we mostly talking about the pinmux stuff?

I guess keeping the DT completely in sync, or as close as possible, will 
simply future comparisons. However, I have the following concerns, 
especially just doing it all at once:

1) This likely enables a lot more devices. In particular, there are 
certainly I2C controllers enabled in some kernel DTs that aren't enabled 
in U-Boot DTs yet, since we simply haven't needed them. Likewise, we 
don't instantiate complete regulator drivers (at least for Tegra) in 
(m)any cases in U-Boot. In theory, this will not cause any issue. 
However, there might be bugs such as:

* The clock driver doesn't correctly support/implement certain 
clocks/resets we haven't yet happened to use. This is quite likely.

* I2C controller driver is broken and doesn't call the clock driver with 
the correct parameters to enable clock and release reset for some 
controllers we haven't used yet. This is less likely.

* pinmux may not be set up correctly for controllers we haven't used 
yet. This is especially likely on older boards where U-Boot only 
partially programs the pinmux for controllers known to be used, rather 
than programming the entire thing at once to the final configuration. 
Hopefully this won't cause any issues, but in the worst case it could 
cause some individual controller to malfunction, which just perhaps 
could impact some other part of the system.

* Hopefully any regulator driver doesn't touch the HW unless explicitly 
requested to by some client, so hopefully we don't end up with rails 
being explicitly disabled at boot, this preventing HW from operating 
when it worked fine without the driver.

2) There are some bindings that are different between U-Boot and the 
kernel. One obvious example is the Tegra USB controller, which has a 
single "usb" node in U-Boot but separate "usb" and "usb-phy" nodes in 
the kernel. This change doesn't look backwards-compatible, since e.g. 
the vbus-supply node moved from the "usb" node (which U-Boot currently 
processes) to the "usb-phy" node (which U-Boot does not know about).

There are probably other more minor examples of similar issues.

3) Having all HW represented in the DTs in U-Boot is certainly a change 
in policy at least for Tegra. I hope it won't give the impression 
incorrect that U-Boot actually processes all those DT nodes and uses 
them as an information source. This is mainly just a mind-set change 
though, so shouldn't be too much of an issue.

To address your last question above: pinctrl is one obvious example 
where this wouldn't be true. Regulators are another. Neither of those 
currently have any DT integration in U-Boot, for Tegra at least. Clocks 
and resets are only partially integrated with DT; the clock/reset IDs 
are parsed from properties in client nodes, but the provider side nodes 
are entirely ignored.


Due to all that, I'd still strongly prefer this patch only sync the 
display-related nodes in order to limit and possible fall-out to display 
functionality; the topic of the series.

I'm fine with syncing the other nodes too, where possible. However, I'd 
like to see the sync split up into a variety of separate patches, likely 
based on node/HW type, to make "git bisect" easier in the face of any 
problems that crop up. Nothing would be more annoying than trying to 
debug an issue with display not working but being unable to do so 
because a new I2C controller's driver was causing the system to 
hard-hang by touching an unclocked register. Equally, just blindly 
syncing the entire DT content isn't going to work e.g. due to the USB 
binding differences I mentioned.

In summary, perhaps we can have a patch series like:

* Add/update all I2C nodes
* Add/update all MMC nodes
* Add/update all USB nodes
* Add/update all PCIe nodes
* Add/update all host1x/display/LCD/... nodes
* Add/update any misc nodes
* Add all nodes for which we know there's no kernel driver

That will make it much easier to debug any problems that do arise.