Hi Tom, On Wed, 26 Feb 2025 at 07:53, Tom Rini <tr...@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 <tr...@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 <tr...@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 <tr...@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 <tr...@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 <tr...@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 > > > > > > > > > > > > <tr...@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 > > > > > > > > > > > > > > <tr...@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 > > > > > > > > > > > > > > > > > <tr...@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
