From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Thu, 12 May 2016 21:11:17 -0600 Subject: [U-Boot] [PATCH v2 3/6] tegra: dts: Sync tegra20 device tree files with Linux In-Reply-To: <57321E22.9030703@wwwdotorg.org> References: <1462748122-30918-1-git-send-email-sjg@chromium.org> <1462748122-30918-4-git-send-email-sjg@chromium.org> <5730C431.5010300@wwwdotorg.org> <57321E22.9030703@wwwdotorg.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Stephen, On 10 May 2016 at 11:45, Stephen Warren wrote: > On 05/09/2016 01:26 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 9 May 2016 at 11:09, Stephen Warren 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