xPL Proposal

Thu Feb 27 17:24:45 CET 2025

Hi Tom,

On Wed, 26 Feb 2025 at 07:53, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Feb 25, 2025 at 07:51:07PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 25 Feb 2025 at 14:51, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Feb 25, 2025 at 02:33:17PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 25 Feb 2025 at 07:02, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Mon, Feb 24, 2025 at 04:38:50PM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 24 Feb 2025 at 09:00, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 21, 2025 at 07:07:06PM -0700, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Fri, 21 Feb 2025 at 18:06, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Feb 21, 2025 at 05:24:35PM -0700, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Fri, 21 Feb 2025 at 17:03, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Feb 21, 2025 at 04:35:16PM -0700, Simon Glass wrote:
> > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, 21 Feb 2025 at 16:20, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, Feb 21, 2025 at 03:54:52PM -0700, Simon Glass wrote:
> > > > > > > > > > > > > > Hi  Tom,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, 21 Feb 2025 at 12:26, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Fri, Feb 21, 2025 at 08:19:40AM -0600, Tom Rini wrote:
> > > > > > > > > > > > > > > > On Thu, Feb 20, 2025 at 06:30:18PM -0700, Simon Glass wrote:
> > > > > > > > > > > > > > > > > Hi Tom,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On Thu, 20 Feb 2025 at 13:40, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Thu, Feb 20, 2025 at 11:13:34AM -0700, Simon Glass wrote:
> > > > > > > > > > > > > > > [snip]
> > > > > > > > > > > > > > > > > > I will look at "splg4" once it's somewhere on source.denx.de and I can
> > > > > > > > > > > > > > > > > > look at it, and refrain from otherwise assuming how it solves the
> > > > > > > > > > > > > > > > > > problems I had seen previously.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > I pushed an updated version to dm/splg-working but it is not very
> > > > > > > > > > > > > > > > > updated. Still needs more work.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Thanks.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > So, after doing the remaining CONFIG_TEXT_BASE -> CONFIG_PPL_TEXT_BASE
> > > > > > > > > > > > > > > changes, here's another example of the problem with your approach. What
> > > > > > > > > > > > > > > stops xilinx_zynqmp_kria from building in splg-working is that
> > > > > > > > > > > > > > > BUTTON was missing from scripts/conf_nospl. Annoyingly, a mrproper (or
> > > > > > > > > > > > > > > since I always use O=, rm -rf) is needed for changes there to be picked
> > > > > > > > > > > > > > > up, but that's maybe just a missing Makefile dependency line. But that
> > > > > > > > > > > > > > > just makes it easier to see the next problem, which I don't see the
> > > > > > > > > > > > > > > answer to. For PPL, we can build drivers/spi/zynqmp_gqspi.o just fine.
> > > > > > > > > > > > > > > For SPL however:
> > > > > > > > > > > > > > >   CC      spl/drivers/spi/zynqmp_gqspi.o
> > > > > > > > > > > > > > > /home/trini/work/u-boot/u-boot/drivers/spi/zynqmp_gqspi.c: In function 'zynqmp_qspi_of_to_plat':
> > > > > > > > > > > > > > > /home/trini/work/u-boot/u-boot/drivers/spi/zynqmp_gqspi.c:203:22: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> > > > > > > > > > > > > > >   203 |         plat->regs = (struct zynqmp_qspi_regs *)(dev_read_addr(bus) +
> > > > > > > > > > > > > > >       |                      ^
> > > > > > > > > > > > > > > /home/trini/work/u-boot/u-boot/drivers/spi/zynqmp_gqspi.c:205:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> > > > > > > > > > > > > > >   205 |         plat->dma_regs = (struct zynqmp_qspi_dma_regs *)
> > > > > > > > > > > > > > >       |                          ^
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > And I don't see, really, what's even getting us down this error path.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > It's the FDT_64BIT in conf_nospl - that symbol needs to be the same
> > > > > > > > > > > > > > across all phases.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I pushed a new tree which builds without the warning. Note that
> > > > > > > > > > > > > > SPL_SPI is enabled.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So, the "what" is FDT_64BIT wasn't correct. I think this is showing that
> > > > > > > > > > > > > scripts/conf_nospl is going to be a problem in and of itself, and likely
> > > > > > > > > > > > > as confusing if not more-so than any of the in-the-end visible changes.
> > > > > > > > > > > >
> > > > > > > > > > > > Yes, perhaps the key point I've been trying to get across is this confusion.
> > > > > > > > > > > >
> > > > > > > > > > > > As you know, at present we have two types of options:
> > > > > > > > > > > > a) those for which each phase has its own value
> > > > > > > > > > > > b) those for which there is a single value shared across all phases
> > > > > > > > > > >
> > > > > > > > > > > Agreed.
> > > > > > > > > > >
> > > > > > > > > > > > The only way today that you can tell them apart is by looking for uses
> > > > > > > > > > > > of CONFIG_IS_ENABLED() and $(PHASE_) with the option. If you see them,
> > > > > > > > > > >
> > > > > > > > > > > Partially agreed. Those are strong indicators that both CONFIG_FOO and
> > > > > > > > > > > CONFIG_SPL_FOO exist, but not always. We have, intentionally, both the
> > > > > > > > > > > inverse case (CONFIG_SPL_BAR and CONFIG_TPL_BAR exist, CONFIG_BAR does
> > > > > > > > > > > not) and some future-proofing (CONFIG_SPL_BAZ may exist in the future,
> > > > > > > > > > > but not yet).
> > > > > > > > > >
> > > > > > > > > > OK
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > then the option is a) otherwise it is b). There is no way to tell from
> > > > > > > > > > > > Kconfig.
> > > > > > > > > > >
> > > > > > > > > > > Kconfig will happily allow "depends on BOGUS_SYMBOL" yes, and a linter
> > > > > > > > > > > would be a handy thing to have. But you're mentioning this in another
> > > > > > > > > > > context, why we need some additional knowledge somewhere.
> > > > > > > > > >
> > > > > > > > > > What I meant was that we don't have anything in the Kconfig for FOO
> > > > > > > > > > that says this is a global option or an xPL-specific one. We have to
> > > > > > > > > > hunt for SPL_FOO, etc.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Also, some parts of the code may use CONFIG_IS_ENABLED() for
> > > > > > > > > > > > an option, some may use IS_ENABLED() for that same option. Some may
> > > > > > > > > > > > use $(PHASE_) and some may not. It's a bit of a mess.
> > > > > > > > > > >
> > > > > > > > > > > I'm sure you can find some examples where we have CONFIG_IS_ENABLED(FOO)
> > > > > > > > > > > and IS_ENABLED(CONFIG_FOO) and it's not intentional, but that's not a
> > > > > > > > > > > big deal, and should be fixed.
> > > > > > > > > >
> > > > > > > > > > But this is largely the point of my series. It's the reason why
> > > > > > > > > > qconfig is able to locate these cases and warn about them. It is a big
> > > > > > > > > > deal, IMO, or at least big enough for me to attempt this.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I'm only going to rant slightly that checkpatch.pl telling people to use
> > > > > > > > > > > these macros has made the situation worse, not better, out of an
> > > > > > > > > > > ingrained need to silence checkpatch.pl.
> > > > > > > > > > >
> > > > > > > > > > > And what you're missing is that sometimes we intentionally don't want
> > > > > > > > > > > $(PHASE_), or would need to rewrite the Makefile to make use of it.
> > > > > > > > > > > fs/Makefile is an example of this.
> > > > > > > > > >
> > > > > > > > > > The next step from my side would be to get rid of the 'ifdef
> > > > > > > > > > CONFIG_XPL_BUILD' in the Makefiles. It's confusing and annoying.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Stepping back a bit, perhaps the goal of my series is to identify
> > > > > > > > > > > > options in b) so we can deal with them in a better way. They are all
> > > > > > > > > > > > listed in conf_nospl, in preparation for some future action.
> > > > > > > > > > >
> > > > > > > > > > > There are two big problems here. The first of which is that conf_nospl,
> > > > > > > > > > > as a concept, is going to be incomplete. Do you list every CMD in there?
> > > > > > > > > > > Why? They'll never be in a non-PPL phase. It will be its own nightmare
> > > > > > > > > > > to keep correct, once it is bug-compatible with what we have today.
> > > > > > > > > >
> > > > > > > > > > This is actually the *nice* thing about conf_nospl. We should reduce
> > > > > > > > > > it to empty, just like we did with the Kconfig whitelist.
> > > > > > > > > >
> > > > > > > > > > We have this rule:
> > > > > > > > > >
> > > > > > > > > > libs-$(CONFIG_CMDLINE) += cmd/
> > > > > > > > > >
> > > > > > > > > > which is enough for most things. The only issue is that sometimes,
> > > > > > > > > > e.g. with CONFIG_CMD_DHCP it doesn't mean the command at all.
> > > > > > > > > >
> > > > > > > > > > So I don't agree at all that my series is a 'big problem'. It is a
> > > > > > > > > > solution to the current confusion and it shows up what is broken and
> > > > > > > > > > needs to be fixed.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > The second big problem is that it doesn't make it any easier to solve
> > > > > > > > > > > what I keep calling the DWC3 problem. It's a valid use case that two
> > > > > > > > > > > developers have hit independently of wanting to enable USB gadget
> > > > > > > > > > > support (and the HW uses DWC3) in SPL and not PPL. Not only are you not
> > > > > > > > > > > solving this problem, it gets worse to solve. Today it's "OK, I need to
> > > > > > > > > > > find where to move obj-$(CONFIG_FOO) to be more visible and
> > > > > > > > > > > obj-$(CONFIG_$(PHASE_)FOO". Tomorrow it's "Why isn't obj-$(CONFIG_FOO)
> > > > > > > > > > > working here but not there?".
> > > > > > > > > >
> > > > > > > > > > Is that because some Makefile higher in the hierarchy is not building
> > > > > > > > > > that subdir? I don't know what this is about.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > To me, at absolute best case here, we're making a lot of changes and
> > > > > > > > > > > spending a lot of time to not really address the underlying problems,
> > > > > > > > > > > just making some questionable value visibility changes. We could reduce
> > > > > > > > > > > ourselves to one macro by saying only ever use CONFIG_IS_ENABLED(FOO)
> > > > > > > > > > > and IS_ENABLED(CONFIG_FOO) goes back to an ifdef for the case where it
> > > > > > > > > > > must only be tested on CONFIG_FOO.
> > > > > > > > > >
> > > > > > > > > > Or we could finish and apply my series, which does this.
> > > > > > > > > >
> > > > > > > > > > > I'm 80% sure we could simplify all of
> > > > > > > > > > > $(PHASE_)/$(XPL_)/$(SPL_) down to just $(PHASE_) and that eliminates the
> > > > > > > > > > > which to use of those question.
> > > > > > > > > >
> > > > > > > > > > Again, let's apply my series, which actually gets rid of PHASE_, SPL_
> > > > > > > > > > and XPL_ altogether.
> > > > > > > > > >
> > > > > > > > > > > And update / expand upon the existing
> > > > > > > > > > > documentation we have as it's not clear enough for everyone. Then we can
> > > > > > > > > > > think, again, about how to solve the problems that are introduced by
> > > > > > > > > > > building our entire source tree N times from a single configuration
> > > > > > > > > > > file. Or if we need to do something radical there.
> > > > > > > > > >
> > > > > > > > > > At this point I'm getting the feeling that you imagine my series is
> > > > > > > > > > some grand unified plan for Kconfig. It really isn't and this thread
> > > > > > > > > > is reminding me of why I originally wrote it. Bear in mind it was over
> > > > > > > > > > two years ago and I have mostly forgotten all the issues. It is a
> > > > > > > > > > clean-up series. It isn't the second coming but it isn't the
> > > > > > > > > > antichrist either.
> > > > > > > > >
> > > > > > > > > I worry you're going to spend another month of effort to get this to 1:1
> > > > > > > > > compatibility (modulo fixing bugs) with what we have today and get
> > > > > > > > > disappointed once it rolls out to -next. But I guess I've already spent
> > > > > > > > > too much time trying to convince you this is a bad idea and that your
> > > > > > > > > cure is worse than the disease.
> > > > > > > >
> > > > > > > > To me the core issue is whether to completely split the defconfig
> > > > > > > > files. I am quite worried about that. You are quite worried about the
> > > > > > > > confusion my series will cause.
> > > > > > > >
> > > > > > > > I think it is reasonable, if we go with my series, that I drive
> > > > > > > > conf_nospl down to zero lines (and remove the feature) before going
> > > > > > > > any further. Would that make you more comfortable? It would be a fair
> > > > > > > > bit of work, but could be done over a few releases.
> > > > > > >
> > > > > > > Here is my core concern. Can macros be tricky? Yes. Do we need a
> > > > > > > checkpatch.pl test for 'IS_ENABLED(FOO)' ? Yes. But the real problem is
> > > > > > > bugs like:
> > > > > > > - Take pinebook-pro-rk3399_defconfig
> > > > > > > - Enable all 3 of: CONFIG_SPL_USB_DWC3_GENERIC CONFIG_SPL_USB_GADGET
> > > > > > >   CONFIG_SPL_USB_SDP_SUPPORT
> > > > > > >
> > > > > > > Your proposal neither fixes that bug nor makes it easier to understand
> > > > > > > why that bug happens. And this is the category of build problems that we
> > > > > > > get that aren't "you missed using the right macro".
> > > > > >
> > > > > > Honestly, what on earth does this have to do with my series?
> > > > >
> > > > > It's that your series doesn't address the real problems we keep having.
> > > > >
> > > > > > The problem happens before and after my series, from what I can tell.
> > > > >
> > > > > Yes, I've said that numerous times.
> > > > >
> > > > > > If you want these sorts of combinations to be tested, perhaps add a
> > > > > > board that enables them, or even rethink your opposition to my
> > > > > > buildman proposal?[4]
> > > > >
> > > > > This isn't relevant to the point I've raised several times now. The
> > > > > failure mode above was reported by two different developers, neither of
> > > > > whom saw how to correctly solve the problem.
> > > >
> > > > That surprises me a little, as the problem seems pretty fundamental.
> > > > If you don't enable USB_GADGET, then symbols which depend on it don't
> > > > exist.
> > >
> > > But they don't want USB_GADGET in PPL. They only want it in SPL.
> >
> > That seems to be splitting hairs, but OK. So why not make
> > USB_GADGET_MANUFACTURER depend on USB_GADGET || SPL_USB_GADGET ?
>
> Yes, the solution today involves reworking drivers/usb/gadget/Kconfig so
> that USB_GADGET_MANUFACTURER, USB_GADGET_VENDOR_NUM,
> USB_GADGET_PRODUCT_NUM, USB_GADGET_VBUS_DRAW and that might be it, are
> exposed to USB_GADGET || SPL_USB_GADGET and possibly down the line
> VPL_USB_GADGET.

OK

>
> > It wouldn't make sense to add SPL_USB_GADGET_MANUFACTURER as surely it
> > would be the same value? This is once good thing about what we have:
> > we can share values between phases without typing them in separately.
>
> Most of these should be, there may or may not be the questionable case
> where one of the ID changes so the host knows what stage things are at.
> But that's just an aside.
>
> My point is that drivers/usb/gadget/Kconfig is yet another case where we
> need to make it much more complicated so that it works for all the use
> cases. And that it's a more common and harder for developers to fix
> problem than "Do I use $(SPL_TPL_) I mean $(PHASE_) or $(XPL_) in the
> Makefile?"

Yes, I understand that, but this is a tradeoff between that complexity
and the one we would introduce by splitting the defconfigs. Given all
the Kconfig churn it would require just to get things to work, it
isn't a clear win, to say the least.

>
> > > > > And again, if you tried to solve this problem on your series you might
> > > > > see how what you're proposing will make things worse, not better.
> > > >
> > > > At least with my scheme you can do something like this:
> > > >
> > > > config SPL_USB_GADGET
> > > >    bool "USB Gadget Support in SPL"
> > > >    depends on USB_GADGET
> > >
> > > That symbol already exists. The problems are around all of the gadget
> > > symbols that don't exist.
> >
> > OK. But we have to move in steps. We can't do everything at once.
>
> Yes, which is why we have so many of these duplicative symbols
> (USB_GADGET, SPL_USB_GADGET) and keep needing to add more.

Yes, I don't like it either. I believe that if I had been able to land
my solution last time, we would be having different discussions by
now, e.g. how to tidy up the Kconfig without changing the build
system.

>
> > > > I normally make the SPL symbols depend on PPL, since it normally
> > > > doesn't make a lot of sense to have the feature in SPL unless it is in
> > > > PPL. Is this an exception to that rule?
> > >
> > > This half of the problem (you're still missing the other half of the
> > > problem, the DWC3 code being built in TPL now and throwing
> > > warnings-turned-error with -Werror and then discarded at link time) is
> > > one of many examples where we keep having to duplicate symbols in
> > > Kconfig.
> >
> > >
> > > > If we do go ahead and enhance Kconfig, then we can combine the two
> > > > symbols, which is something.
> > >
> > > Or, we go the direction I suggested instead. Where we never duplicate
> > > symbols, because we never need to anymore.
> > >
> > > Or, we step back because believe you're missing the bigger problems.
> >
> > At this point I'm not sure where to go. You are determined to split
> > the defconfig files and have grace concerns about my schema. Vice
> > versa for me.
> >
> > But my scheme takes us forward without needing to split the
> > defconfigs. It does offer some benefits IMO. Once we split the
> > defconfigs we can never put them back together.
>
> My continued strongest preference is to do the minimal effort to better
> document what we are doing today and add the missing tooling so we don't
> keep getting wrong macros in the code.

I did actually do the tooling in qconfig - give it a try and see what
you think. For documentation, we can discuss that as part of myt
series.

> If you're hellbent on doing this
> and do it against master and not your personal tree, I'm expecting you
> to be available to help clarify problems for developers if they report
> them.

That's fine. I do my development on my own tree, but once I actually
do the series and it is reviewed, I can do a version against -next. As
you know, there are a lot of moving parts, so I would want it to go in
quickly to avoid a lot of rework.

Regards,
Simon