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

[Sébastien Szymanski]

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:
>>>>>> + Shawn, Krzysztof, Conor
>>>>>>
>>>>>> Hi Tom,
>>>>>>
>>>>>> 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.
>>>>>
>>>>>>> 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.
>>
> 
> Okay I can see your point. I suppose this leads us to option (1) to
> partially revert the Linux kernel commit [1] to add back the display
> timings in DT. Ironically all the folks (developer, U-Boot and Linux
> kernel iMX maintainers) were involved in the upstream process for the
> Linux kernel commit [1] under question. So I will let them chime in
> too.

It is also now possible to have the display timings under the panel node:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/panel/panel-simple.c?h=v6.8-rc6&id=4a1d0dbc8332231d1d500d7a1d13c45457262a97

Not sure if that could help here.

Regards,

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d9aa4d4fca67823838fe9861456201430c545e69
> 
> -Sumit
> 
>> --
>> Tom

-- 
Sébastien Szymanski, Armadeus Systems
Software engineer