[PATCH 1/2] opos6uldev: make the LCD work again

Fri Mar 1 19:54:13 CET 2024

On Fri, Mar 01, 2024 at 01:32:53PM +0000, Conor Dooley wrote:
> Hey,
> 
> Replying here because this is only version of this in my inbox atm.

Please note that for additional context, in 2019 when d9aa4d4fca67 ("ARM:
dts: opos6uldev: use OF graph to describe the display") was merged
re-sync of DTS files to U-Boot was a best-effort per platform and
infrequent. As of yesterday we have devicetree-rebasing included as a
git subtree and platforms can and should switch to using that, and that
tree will be updated every U-Boot merge window. So I wanted to ask a
broader question in this thread about how to handle dts changes break
U-Boot functionality, and have an example that wasn't ancient (the
serial problem from 2013 or so) nor incorrect PHY mode was specified in
the dts file to start with.  This thread is that example.

> On Fri, Mar 01, 2024 at 10:17:35AM +0100, Sébastien Szymanski wrote:
> > On 3/1/24 07:02, Sumit Garg wrote:
> > > On Thu, 29 Feb 2024 at 19:31, Tom Rini <trini at konsulko.com> wrote:
> > > > On Thu, Feb 29, 2024 at 08:42:42AM -0500, Tom Rini wrote:
> > > > > On Thu, Feb 29, 2024 at 11:17:28AM +0530, Sumit Garg wrote:
> > > > > > On Wed, 28 Feb 2024 at 20:50, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > On Wed, Feb 28, 2024 at 07:44:42PM +0530, Sumit Garg wrote:
> > > > > > > > On Wed, 28 Feb 2024 at 18:40, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > 
> > > > > > > > > On Wed, Feb 28, 2024 at 10:09:13AM +0300, Dan Carpenter wrote:
> > > > > > > > > > On Tue, Feb 27, 2024 at 04:40:01PM +0100, Sébastien Szymanski wrote:
> > > > > > > > > > > Commit 5d7a95f49999 ("imx6ul/imx6ull: synchronise device trees with
> > > > > > > > > > > linux") removed the display timings from the board device tree whereas
> > > > > > > > > > > they are still needed by the mxsfb driver.
> > > > > > > > > > > Add the timings back (the correct ones) in the
> > > > > > > > > > > imx6ul-opos6uldev-u-boot.dtsi file and remove them from the
> > > > > > > > > > > opos6uldev.env file.
> > > > > > > > > > > 
> > > > > > > > > > > Update the opos6uldev_defconfig file so that the LCD turns on at boot.
> > > > > > > > > > > 
> > > > > > > > > > > Fixes: 5d7a95f49999 ("imx6ul/imx6ull: synchronise device trees with linux")
> > > > > > > > > > > Signed-off-by: Sébastien Szymanski <sebastien.szymanski at armadeus.com>
> > > > > > > > > > 
> > > > > > > > > > Huh.  This is the commit that did that upstream.
> > > > > > > > > > 
> > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d9aa4d4fca67823838fe9861456201430c545e69
> > > > > > > > > > 
> > > > > > > > > > It's interesting how the timings in linux were always slightly different
> > > > > > > > > > from in u-boot.
> > > > > > > > > 
> > > > > > > > > Thanks for tracking that down, Dan. I'm adding in Sumit and Rob here
> > > > > > > > > because this is a recent (rather than ancient) example of one of the
> > > > > > > > > concerns about OF_UPSTREAM.
> > > > > > > > 
> > > > > > > > I rather think about this as an opportunity to improve with
> > > > > > > > OF_UPSTREAM. We can feed these kinds of DT ABI breakages to
> > > > > > > > corresponding Linux kernel sub-arch maintainers. Especially once we
> > > > > > > > move to OF_UPSTREAM and a sub-arch maintainer profile in Linux kernel
> > > > > > > > to keep them aware that U-Boot should be considered too.
> > > > > > > 
> > > > > > > Yes, a more extensive check around when removing information from dts
> > > > > > > files would be good.
> 
> Whenever people remove things from bindings (or from being required) we
> do ask them "have you checked that there's no users for this outside of
> linux" - but for me at least I don't apply that scrutiny for most (read
> arm{,64}) devicetrees. There's just too much volume if I went asking
> those questions on every removal, it's up to the platform maintainers to
> keep an eye on that.
> 
> That said, modifications to a devicetree are fixable with a revert,
> while modifications to a binding may not really be fixable in a way that
> isn't disruptive for some user, so I think that I am spending my time
> wisely.

I agree that so long as reverting dts changes, even after a release
includes them, is possible, it's not the end of the world and we can all
manage.

> > > > > > > > > I think the commit in question can be
> > > > > > > > > summarized as "remove a bunch of explicit HW information because there's
> > > > > > > > > now a Linux Kernel driver that determines that dynamically". What do we
> > > > > > > > > do next? The old information is in a presumably valid binding still, can
> > > > > > > > > we just put it back and comment that consumers outside of Linux use this
> > > > > > > > > still so it's not removed again later? Or am I just missing where we can
> > > > > > > > > instead get this information from the DT still and not need to come up
> > > > > > > > > with a new driver and subsystems?
> 
> I don't think this is an accurate summary. The "explict hw information"
> was never allowed for an imx6ul, only for some older devices, so "the
> old information is in a presumably valid binding" is not accurate.
> See 7b920aae917d ("dt-bindings: mxsfb: Add new bindings for the MXSFB
> driver") for when doing things that way was deprecated. The imx6ul was
> only documented several years after it was introduced (so likely several
> years after it started incorrectly using that binding).
> 
> Seeing that, I am not sure that I would even question the kernel patch
> cited above, the change was done for binding compliance and I would not
> be inclined to think twice, given the bindings are the ABI.

So part of the problem I see here is that legacy platforms (which to me
is a large chunk of 32bit ARM) are trickier. I'm not asking for anything
more to be done in this example to be clear (I think the new panel-dpi
binding is what U-Boot needs to implement and solve the issue here).

But I don't think the fact that the old binding was handled
suboptimally means that its usage should be ignored. They were added in
2017 which is after they were deprecated and not caught then. I consider
that to mean it was still valid. And I also assume that today we have
the tooling to catch that and not let it in, in the first place.

Time will tell now how bumpy a ride things end up being as I do agree
that getting all the DTS files following the ABI correctly is an
important goal.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240301/f15de6a7/attachment.sig>