Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Fri, Mar 08, 2024 at 01:21:15PM +, Conor Dooley wrote:
> On Fri, Mar 01, 2024 at 01:54:13PM -0500, Tom Rini wrote:
> > On Fri, Mar 01, 2024 at 01:32:53PM +, Conor Dooley wrote:
> > > > > > > > > > On Wed, 28 Feb 2024 at 18:40, Tom Rini
> > > > > > > > > > wrote:
> > > > > > > > > > > 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.
>
> Reading this back today, it took me second to recall what I meant by
> "question". What I meant was that during a review I would would see a
> patch changing this to comply with the bindings and think nothing of it,
> not that removing it was a unquestionably correct thing to do.
>
> > 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.
>
> What often happens is that Linux retains support for the old way of
> doing things but the dts is updated to the documented way. I'm not
> really sure how to deal with this kind of thing, other than being extra
> diligent about reviewing cleanup work that may impact other users of the
> dts sources and letting the linux platform maintainers know if something
> gets broken in U-Boot.
I think that's perhaps the crux of the issue. Since the kernel is the
definitive source of the dtb files too, cleanups of the sources can
impact other projects (U-Boot, OpenBSD, etc) who might not have been
aware the binding was deprecated/etc.
> > They were added in
> > 2017 which is after they were deprecated and not caught then. I consider
> > that to mean it was still valid.
>
> I don't agree that that makes it valid, but I can see how it would be
> assumed to be valid by other users of the dts file.
Yes, fair.
> > And I also assume that today we have
> > the tooling to catch that and not let it in, in the first place.
>
> Unfortunately, not really. The tooling is capable of spotting these
> things, but it totally depends on the effort people have put in to
> that platform as to whether or not the single to noise ratio actually
> allows a maintainer to spot when these things are introduced.
> The RISC-V platforms, Samsung and some others have really good
> compliance, but other platforms like aspeed or at91 have so many
> warnings etc that it is very very difficult for the platform maintainers
> to actually spot things like these being added.
OK.
> > 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.
>
> Fixing up a platform is utterly mind-numbing work that requires
> understanding datasheets and bindings for really varied hardware, so
> it's best done by either the platform maintainers or piecemeal by people
> that want to improve one particular board. It's gonna take a long time
> to get "all" files following the ABI, particularly away from the ones
> the linux dt-maintainers take more of an interest in.
> Next time we get a new grad starting I might subject them to some
> cleanup activities, they'll have to learn about dt at some point anyway
> ;)
It's all tricky and complex, yup :(
> Sorta unrelated to the above, but related to the OF_UPSTREAM stuff is
> that I do notice a bit
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Fri, Mar 01, 2024 at 01:54:13PM -0500, Tom Rini wrote:
> On Fri, Mar 01, 2024 at 01:32:53PM +, Conor Dooley wrote:
> > > > > > > > > On Wed, 28 Feb 2024 at 18:40, Tom Rini
> > > > > > > > > wrote:
> > > > > > > > > > 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.
Reading this back today, it took me second to recall what I meant by
"question". What I meant was that during a review I would would see a
patch changing this to comply with the bindings and think nothing of it,
not that removing it was a unquestionably correct thing to do.
> 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.
What often happens is that Linux retains support for the old way of
doing things but the dts is updated to the documented way. I'm not
really sure how to deal with this kind of thing, other than being extra
diligent about reviewing cleanup work that may impact other users of the
dts sources and letting the linux platform maintainers know if something
gets broken in U-Boot.
> They were added in
> 2017 which is after they were deprecated and not caught then. I consider
> that to mean it was still valid.
I don't agree that that makes it valid, but I can see how it would be
assumed to be valid by other users of the dts file.
> And I also assume that today we have
> the tooling to catch that and not let it in, in the first place.
Unfortunately, not really. The tooling is capable of spotting these
things, but it totally depends on the effort people have put in to
that platform as to whether or not the single to noise ratio actually
allows a maintainer to spot when these things are introduced.
The RISC-V platforms, Samsung and some others have really good
compliance, but other platforms like aspeed or at91 have so many
warnings etc that it is very very difficult for the platform maintainers
to actually spot things like these being added.
> 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.
Fixing up a platform is utterly mind-numbing work that requires
understanding datasheets and bindings for really varied hardware, so
it's best done by either the platform maintainers or piecemeal by people
that want to improve one particular board. It's gonna take a long time
to get "all" files following the ABI, particularly away from the ones
the linux dt-maintainers take more of an interest in.
Next time we get a new grad starting I might subject them to some
cleanup activities, they'll have to learn about dt at some point anyway
;)
Sorta unrelated to the above, but related to the OF_UPSTREAM stuff is
that I do notice a bit of an attitude of "U-Boot bundles the dtb with
the binary, so it's okay to break U-Boot because they have a copy of
the old dts and we can update both code and dts at the same time in
the 'future'". I can't cite anything off the top of my head, but I've
not been reviewing binding patches for all that long and it has been
said multiple times.
I'm not entirely sure how to handle that sort of situation, "force" the
submitter to send patches to U-Boot before I ack the binding?
I do skim all the patch subjects that get sent
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Tue, Feb 27, 2024 at 12:40 PM Sébastien Szymanski
wrote:
>
> Commit 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
> Signed-off-by: Sébastien Szymanski
Applied all, thanks.
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Mon, Mar 4, 2024 at 10:35 AM Tom Rini wrote: > For v2024.04 yes, this is fine. For long term, we should parse the new > timing properties that the simple panel driver takes in our simple panel > driver instead. Sounds good. Sébastien, please follow Tom's suggestion when you have a chance. Thanks.
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Mon, Mar 04, 2024 at 10:04:28AM -0300, Fabio Estevam wrote: > Hi Tom, > > On Tue, Feb 27, 2024 at 12:42 PM Tom Rini wrote: > > > What's the long term fix here? Why aren't these needed in Linux anymore, > > and perhaps why was it OK to remove them? This is perhaps another case > > where we as the U-Boot community need to go and talk with some Linux > > Kernel community people. Thanks. > > To fix the issue for 2024.04 I suggest that this series gets applied. > > In the long term, I suggest Sébastien send a patch to the Linux DT to > add back the panel timings. > > Do you agree? If so, I can apply it to u-boot-imx master. For v2024.04 yes, this is fine. For long term, we should parse the new timing properties that the simple panel driver takes in our simple panel driver instead. -- Tom signature.asc Description: PGP signature
Re: [PATCH 1/2] opos6uldev: make the LCD work again
Hi Tom, On Tue, Feb 27, 2024 at 12:42 PM Tom Rini wrote: > What's the long term fix here? Why aren't these needed in Linux anymore, > and perhaps why was it OK to remove them? This is perhaps another case > where we as the U-Boot community need to go and talk with some Linux > Kernel community people. Thanks. To fix the issue for 2024.04 I suggest that this series gets applied. In the long term, I suggest Sébastien send a patch to the Linux DT to add back the panel timings. Do you agree? If so, I can apply it to u-boot-imx master. Thanks
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Fri, Mar 01, 2024 at 01:32:53PM +, 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 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 wrote:
> > > > > > > On Wed, Feb 28, 2024 at 07:44:42PM +0530, Sumit Garg wrote:
> > > > > > > > On Wed, 28 Feb 2024 at 18:40, Tom Rini
> > > > > > > > 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device
> > > > > > > > > > > trees with linux")
> > > > > > > > > > > Signed-off-by: Sébastien Szymanski
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 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
> > > > >
Re: [PATCH 1/2] opos6uldev: make the LCD work again
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 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 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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device
> > > > > > > > > > trees with linux")
> > > > > > > > > > Signed-off-by: Sébastien Szymanski
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > 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
Re: [PATCH 1/2] opos6uldev: make the LCD work again
Hey,
Replying here because this is only version of this in my inbox atm.
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 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 wrote:
> > > > > > On Wed, Feb 28, 2024 at 07:44:42PM +0530, Sumit Garg wrote:
> > > > > > > On Wed, 28 Feb 2024 at 18:40, Tom Rini 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device
> > > > > > > > > > trees with linux")
> > > > > > > > > > Signed-off-by: Sébastien Szymanski
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > 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 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.
Cheers,
Conor.
signature.asc
Description: PGP signature
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On 3/1/24 07:02, Sumit Garg wrote:
On Thu, 29 Feb 2024 at 19:31, Tom Rini 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 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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
Signed-off-by: Sébastien Szymanski
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=4a1d0dbc8332231d1d500d7a1d13c45457262a97
Not sure if that could help here.
Regards,
[1]
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Thu, 29 Feb 2024 at 19:31, Tom Rini 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 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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees
> > > > > > > > with linux")
> > > > > > > > Signed-off-by: Sébastien Szymanski
> > > > > > > >
> > > > > > >
> > > > > > > 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
Re: [PATCH 1/2] opos6uldev: make the LCD work again
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 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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees
> > > > > > > with linux")
> > > > > > > Signed-off-by: Sébastien Szymanski
> > > > > > >
> > > > > >
> > > > > > 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
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Thu, Feb 29, 2024 at 11:17:28AM +0530, Sumit Garg wrote:
> On Wed, 28 Feb 2024 at 20:50, Tom Rini 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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with
> > > > > > linux")
> > > > > > Signed-off-by: Sébastien Szymanski
> > > > > >
> > > > >
> > > > > 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.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Wed, 28 Feb 2024 at 20:50, Tom Rini 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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with
> > > > > linux")
> > > > > Signed-off-by: Sébastien Szymanski
> > > >
> > > > 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
> 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.
-Sumit
>
> --
> Tom
Re: [PATCH 1/2] opos6uldev: make the LCD work again
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 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with
> > > > linux")
> > > > Signed-off-by: Sébastien Szymanski
> > >
> > > 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? 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.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 1/2] opos6uldev: make the LCD work again
+ Shawn, Krzysztof, Conor
Hi Tom,
On Wed, 28 Feb 2024 at 18:40, Tom Rini 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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with
> > > linux")
> > > Signed-off-by: Sébastien Szymanski
> >
> > 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.
> 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.
-Sumit
>
> --
> Tom
Re: [PATCH 1/2] opos6uldev: make the LCD work again
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 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
> > Signed-off-by: Sébastien Szymanski
>
> 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 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?
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Tue, Feb 27, 2024 at 04:40:01PM +0100, Sébastien Szymanski wrote:
> Commit 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
> Signed-off-by: Sébastien Szymanski
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.
regards,
dan carpenter
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Tue, Feb 27, 2024 at 04:40:01PM +0100, Sébastien Szymanski wrote:
> Commit 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
> Signed-off-by: Sébastien Szymanski
What's the long term fix here? Why aren't these needed in Linux anymore,
and perhaps why was it OK to remove them? This is perhaps another case
where we as the U-Boot community need to go and talk with some Linux
Kernel community people. Thanks.
--
Tom
signature.asc
Description: PGP signature
[PATCH 1/2] opos6uldev: make the LCD work again
Commit 5d7a95f4 ("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: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
Signed-off-by: Sébastien Szymanski
---
arch/arm/dts/imx6ul-opos6uldev-u-boot.dtsi | 28 +-
board/armadeus/opos6uldev/opos6uldev.env | 1 -
configs/opos6uldev_defconfig | 3 ---
3 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/arch/arm/dts/imx6ul-opos6uldev-u-boot.dtsi
b/arch/arm/dts/imx6ul-opos6uldev-u-boot.dtsi
index aa88964f210d..3b52d6bbd9b9 100644
--- a/arch/arm/dts/imx6ul-opos6uldev-u-boot.dtsi
+++ b/arch/arm/dts/imx6ul-opos6uldev-u-boot.dtsi
@@ -7,12 +7,6 @@
#include "imx6ul-opos6ul-u-boot.dtsi"
-/ {
- aliases {
- display0 =
- };
-};
-
{
bootph-pre-ram;
@@ -22,7 +16,29 @@
};
{
+ display = <>;
bootph-some-ram;
+
+ display0: display0 {
+ bits-per-pixel = <18>;
+ bus-width = <18>;
+
+ display-timings {
+ timing0 {
+ clock-frequency = <3330>;
+ hactive = <800>;
+ vactive = <480>;
+ hback-porch = <36>;
+ hfront-porch = <210>;
+ vback-porch = <13>;
+ vfront-porch = <22>;
+ hsync-len = <10>;
+ vsync-len = <10>;
+ de-active = <1>;
+ pixelclk-active = <0>;
+ };
+ };
+ };
};
_uart1 {
diff --git a/board/armadeus/opos6uldev/opos6uldev.env
b/board/armadeus/opos6uldev/opos6uldev.env
index f90029787104..2e7b65968d1d 100644
--- a/board/armadeus/opos6uldev/opos6uldev.env
+++ b/board/armadeus/opos6uldev/opos6uldev.env
@@ -24,7 +24,6 @@ mmcrootfstype=ext4 rootwait
kernelimg=opos6ul-linux.bin
splashpos=0,0
splashimage=CONFIG_SYS_LOAD_ADDR
-videomode=video=ctfb:x:800,y:480,depth:18,pclk:33033,le:96,ri:96,up:20,lo:21,hs:64,vs:4,sync:0,vmode:0
check_env=if test -n ${flash_env_version};
then env default env_version;
else env set flash_env_version ${env_version}; env save;
diff --git a/configs/opos6uldev_defconfig b/configs/opos6uldev_defconfig
index e1884df9dd23..7d21a6fe93c5 100644
--- a/configs/opos6uldev_defconfig
+++ b/configs/opos6uldev_defconfig
@@ -115,13 +115,10 @@ CONFIG_CI_UDC=y
CONFIG_USB_GADGET_DOWNLOAD=y
CONFIG_VIDEO=y
CONFIG_VIDEO_LOGO=y
-# CONFIG_VIDEO_BPP8 is not set
-# CONFIG_VIDEO_BPP32 is not set
CONFIG_SYS_WHITE_ON_BLACK=y
CONFIG_VIDEO_MXS=y
CONFIG_SPLASH_SCREEN=y
CONFIG_SPLASH_SCREEN_ALIGN=y
-CONFIG_SPLASH_SOURCE=y
CONFIG_BMP_16BPP=y
CONFIG_BMP_24BPP=y
CONFIG_BMP_32BPP=y
--
2.43.0
