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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5097 bytes --]

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@konsulko.com> wrote:
> > >
> > > On Wed, Feb 28, 2024 at 07:44:42PM +0530, Sumit Garg wrote:
> > > > + Shawn, Krzysztof, Conor
> > > >
> > > > Hi Tom,
> > > >
> > > > On Wed, 28 Feb 2024 at 18:40, Tom Rini <trini@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@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.
> > >
> > > > > 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 can see following two paths forward:
> > > >
> > > > 1) Partially revert the Linux kernel commit to add back the display
> > > > timings in DT.
> > > > 2) Extend drivers/video/simple_panel.c in U-Boot to add support for
> > > > compatible: "armadeus,st0700-adapt".
> > > >
> > > > If possible then I would be in favour of (2) rather than the current
> > > > patch to do this properly.
> > >
> > > Well, looking at the kernel drivers/gpu/drm/panel/panel-simple.c driver
> > > and then our drivers/video/simple_panel.c it sure would be nice if it's
> > > just a matter of adding a compatible but I wouldn't be surprised if it
> > > ends up needing more information being passed along too?
> > 
> > Although I am not a LCD panel expert but looking at the kernel driver
> > code [1], the display timings are rather taken from a static data
> > structure matching the compatible "armadeus,st0700-adapt".
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-simple.c#n901
> 
> Yes. My point is that it seems like the situation changed from "device
> tree provides timings for the platform" to "driver has timing
> information for N displays" and so we'll need to do something clever to
> avoid including the structs for 5 panels when we'll only ever
> (likely...) see one. And that also yes, we'll probably need to add data
> for this panel rather than re-use the PANASONIC_VVX10F004B00 data.
> 
> > > And I'm going
> > > assume there's good reasons for the design change in how the drivers
> > > work in Linux now and note that it might make things more challenging
> > > for us when we do care about space.
> > 
> > I agree it is always going to be challenging to use DT during SPL
> > stage which is mostly constrained by limited on-chip RAM.
> 
> Well, no. The DT way handled this more efficiently, I think I wasn't
> clear enough in my reply.

And it's not just SPL, full U-Boot needs to stay small and within flash
partition considerations and I become cranky and question people when
non-generic changes impact platforms that don't need the change.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]