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

[Simon Glass]

Hi Stephen,

On 10 May 2016 at 11:45, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

Regulators are used by the new LCD support. Yes you are right about audio.

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

The regulators are not used unless enabled, but they will be enabled
if a display is used.

I worry that what you are saying above is that we *need* a different
device tree from Linux. Would it not be better to make it the same and
then fix any problems we discover?

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

Hmm well that's the sort of thing I'd like to tidy up.

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

I'm hoping that you will create a pinctrl driver. Regulators are well
supported in U-Boot now - please see drivers/power/regulator.

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

OK I'll take a look at this. Really I wish the Tegra maintainer would
do this :-)

Regards,
Simon