<vagrantc>
marex: for various imx8 boards i have seen memory training blobs ... are those no longer needed either?
<marex>
broonie: ...so that when there is a bug in all that code, you can't just fix it by safely easily updating the kernel, you have to live dangerously and upgrade the entire firmware ...
<broonie>
I didn't comment either way on the desirability of this approach, just observed what it's trying to accomplish.
<marex>
:)
<broonie>
It does help a lot for applications where you need to run a generic OS on the board, but of course that's not every application.
<marex>
sjg1: except I want it to be obtainium and cheap-ish
<marex>
with DPI/LVDS , it is viable with simple FPGA
<mwalle>
sjg1: why is IS_ENABLED() preferred over CONFIG_IS_ENABLED() when there is no SPL config option?
ikarso has joined #u-boot
goliath has quit [Quit: SIGSEGV]
naoki has quit [Quit: naoki]
naoki has joined #u-boot
Leopold has quit []
naoki has quit [Remote host closed the connection]
naoki has joined #u-boot
Leopold has joined #u-boot
<sjg1>
mwalle: Basically I am trying to merge those two options as it is too confusing to have two. A step towards that is to clearly know which options have an SPL variant and which don't
<sjg1>
mwalle: An option CONFIG_FOO without an SPL variant needs to be enabled in SPL too, i.e. it applies to all builds. But if there is a CONFIG_SPL_FOO then that signals that it should only be enabled in an SPL build if there is a corresponding SPL option
<sjg1>
Tartarus: ^ also
naoki has quit [Remote host closed the connection]
naoki has joined #u-boot
<Tartarus>
Yeah, and I'm feeling like making checkpatch.pl complain was a step backwards in terms of getting things used correctly, unfortunately
<Tartarus>
And, we aren't going to add a bunch of unused config SPL_FOO options, in order to use CONFIG_IS_ENABLED(FOO) and not run in to the case where we don't have SPL_FOO as an option
<Tartarus>
That to me shows that we took a wrong turn
naoki has quit [Quit: naoki]
naoki has joined #u-boot
naoki has quit [Client Quit]
naoki1 has joined #u-boot
Leopold has quit [Remote host closed the connection]
Leopold has joined #u-boot
naoki1 is now known as naoki
naoki has quit [Quit: naoki]
naoki has joined #u-boot
prabhakarlad has joined #u-boot
naoki has quit [Quit: naoki]
naoki has joined #u-boot
nikhilmj has quit [Quit: Client closed]
<mwalle>
sjg1: that clear, but what is the drawback of using CONFIG_IS_ENABLED(FOO) when there is no CONFIG_SPL_FOO?
<mwalle>
sjg1: also, these changes might increase the spl binary size, if the code is also used in SPL, right?
<mwalle>
sjg1: I don't argue against these changes, I just try to understand em
<Tartarus>
The series should be size neutral
<sjg1>
mwalle: So long as the intent is to disable the code in SPL, that is fine, but in that case we need an SPL option. It allows the split config stuff to happen
<Tartarus>
And the drawback is potential bugs, or actual bugs when someone assumes that they've enabled FOO and it'll be set in SPL
<sjg1>
Tartarus: Right, there should be no SPL_ options that are not needed in the source
<Tartarus>
We just had one of those reported around MMC_QUIRKS
<mwalle>
but isn't the default off if there is no CONFIG_SPL_FOO for IS_ENABLED(FOO) ?
<mwalle>
err CONFIG_IS_ENABLED()
naoki has quit [Quit: naoki]
naoki1 has joined #u-boot
naoki1 is now known as naoki
<Tartarus>
Yes, it defaults to evaluating false
<Tartarus>
sjg1: Yes, but your 171 part series adds numerous not needed SPL_FOO, def_bool n options.
naoki has quit [Quit: naoki]
goliath has joined #u-boot
guillaume_g has quit [Quit: Konversation terminated!]
* cambrian_invader
also doesn't see why we need to convert CONFIG_IS_ENABLED to IS_ENABLED
<cambrian_invader>
can we really say no one will ever want to use these for SPL?
<Tartarus>
Depends on the context
<Tartarus>
The CMD_FOO ones are correct, SPL_CMD_FOO will never be true
<Tartarus>
Everything else I'd like to evaluate in batches, and understand the context better
<Tartarus>
As I feel like we have non-trivial number of "do this to make checkpatch.pl not complain" without a deeper understanding, and so issues being papered over or introduced
<Tartarus>
And as a new tangent, I see now Linux has IS_REACHABLE and IS_ENABLED
<Tartarus>
But, ah, nevermind, that's for module related things, not useful here
<Tartarus>
Skimmed too quick when trying to see if SYMBOL has to be all upper case or just is implied as much
<cambrian_invader>
but if we know CONFIG_SPL_CMD_FOO will never be enabled, why does it matter if we use IS_ENABLED or CONFIG_IS_ENABLED?
<cambrian_invader>
IMO it would be better to use CONFIG_IS_ENABLED everywhere and IS_ENABLED only where we really mean it
<cambrian_invader>
since then you don't have to think about whether you may want to add CONFIG_SPL at some point
frieder has quit [Remote host closed the connection]
<Tartarus>
To my mind, that we need to check at compile time for CMD_FOO being set somewhere else is a reason to examine more closely what's going on
<Tartarus>
And if we shouldn't restructure the code more so that it's a Makefile located decision (ie we need foo_support.o for FOO but for CMD_FOO but need foo_cmd.o), or perhaps some of our bigger macros need some further tweaks, if it's a common thing to only set a member of something for a cmd
mmu_man has quit [Ping timeout: 252 seconds]
monstr has quit [Remote host closed the connection]
mmu_man has joined #u-boot
ikarso has quit [Quit: Connection closed for inactivity]
<Tartarus>
Welp, lets see if I find a good reason to stop using irccloud and instead try yet another thing
pgreco has quit [Ping timeout: 268 seconds]
<sjg1>
Tartarus: Yes I add 21, which are the ones that I think do need an SPL symbol, or at least rely on it not existing today. Do you think I am wrong on any of them?
mmu_man has quit [Ping timeout: 252 seconds]
<Tartarus>
sjg1: All of the EFI ones are wrong and the a38x pinctrl one is also wrong
<Tartarus>
I didn't look further
<sjg1>
Fundamentally, we need to support two types of CONFIG options: those that apply to all builds and those that only apply to one
<sjg1>
Tartarus: So for example this one is wrong? 'Correct SPL use of EFI_LOADER_HII' ?
<Tartarus>
sjg1: SPL_EFI_* symbols are wrong, with the afaik sole exception of SPL_EFI_PARTITIONS, since that has nothing to do with EFI_LOADER / similar
<sjg1>
Tartarus: So SPL_EFI_LOADER is wrong and SPL_EFI_DEVICE_PATH_TO_TEXT ?
<sjg1>
Tartarus: (they are the only two EFI ones I see)
pgreco has joined #u-boot
<Tartarus>
There is no SPL_EFI_LOADER user, and SPL_EFI_DEVICE_PATH_TO_TEXT might be some very oddball case, moment
<Tartarus>
No, I was thinking of some other funny place where we had been able to abuse a symbol prior to Kconfig migration
<Tartarus>
So yes, neither of those should exist
<Tartarus>
There is never the EFI loader or related in SPL at this time
<Tartarus>
(and if someone wants to do falcon mode, but with EFI payload, I think we've gone a long way in odd direction)
mmu_man has joined #u-boot
<Tartarus>
sjg1: I think we need to stop, drop the checkpatch.pl message, and then carefully and in small'ish batches, check all of the current users of both of these macros
<Tartarus>
That tells me we have far too many places where someone just went to shut the warning up, as all of those should just be ifdef or if defined(A) || defined(B)
Leopold has quit [Remote host closed the connection]
<sjg1>
Tartarus: I think you are missing the point?
<Tartarus>
Yes, all of those are bugs and should be IS_ENABLED(CONFIG_EFI_FOO)
<Tartarus>
Or possibly just #ifdef CONFIG_EFI_FOO
<Tartarus>
Those are all cases where someone wanted to make checkpatch.pl be quiet and didn't dig further
<Tartarus>
Since it has never ever been possible to have CONFIG_SPL_EFI_LOADER set
<sjg1>
Tartarus: I don't think so. Some of those files are used in SPL and need to work there, without bringing in EFI stuff
<Tartarus>
But they've never been able to be used
<sjg1>
Tartarus: ??? I an missing something. See lib/vsprintf.c - this can be used in SPL
<Tartarus>
sjg1: You're missing that it's never been able to be used
<Tartarus>
Never has CONFIG_SPL_EFI_LOADER been set
<Tartarus>
That vsprintf.c case for example
<sjg1>
Tartarus: Of course, but that's the point, right?
<Tartarus>
sjg1: Yes? It's the point I've been trying to make which is that checkpatch.pl telling people to not #ifdef things got a bunch of subtle bugs introduced, or simply bogus code introduced
<sjg1>
Tartarus: In most cases CONFIG_IS_ENABLED() is wrong and I changed it to IS_ENABLED()
<Tartarus>
And the fix is not to add the bogus symbol
<sjg1>
In some cases, it has meaning and I cannot
<sjg1>
What is the fix?
<Tartarus>
To use IS_ENABLED or #ifdef/#if defined
<Tartarus>
And CC the relevant custodian to let them know in case they thought some path was ever being used, when it was not.
<Tartarus>
And in turn, figure out what they did intend and so need to do
<sjg1>
But if I change it to IS_ENABLED() in vsprint.c, then the code is compiled into SPL
<Tartarus>
Ah, ok
<Tartarus>
So then it's not a bug in the first place
<Tartarus>
And we either need a more clever solution
<Tartarus>
Or just accept that we don't have every symbol with a config entry
<Tartarus>
drivers/mmc/mmc.c: if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
<sjg1>
Yes but that is exactly what I have done. I have spent ages analysing this. You can see that some CONFIG_IS_ENABLED() uses are needed, right?
<Tartarus>
should have been IS_ENABLED(CONFIG_MMC_QUIRKS)
<Tartarus>
CONFIG_IS_ENABLED is needed, but def_bool n symbols are not.
<Tartarus>
And come back and change that, with every other instance of #ifdef / #if defined in that vein later, when we've got something that does make the code better overall
<Tartarus>
Looking at board/keymile/pg-wcom-ls102xa/pg-wcom-ls102xa.c the better still solution would be to ask Holger about cleaning up the board detection code, and either yes, if (IS_ENABLED(CONFIG_BOARD_A)) { ... } else if (IS_ENABLED(CONFIG_BOARD_B)) { ... } reads better than #ifdef
<sjg1>
OK but that's a different issue. You are now talking about the syntax, not whether it is correct or not
<sjg1>
...and I am hoping to drop #ifdef from U-Boot, mostly
<sjg1>
I think I will send splc so you can see the end state
<Tartarus>
And, well, pah
<Tartarus>
Sec..
<Tartarus>
I guess as much as _I_ don't like #if IS_ENABLED(...) it doesn't seem to have set off Linus, so I guess I'll give up on that
<Tartarus>
Even if it's ~4k of those vs 34k #ifdef
jybz has left #u-boot [Konversation terminated!]
<Tartarus>
So yes, OK, it's likely those 148 patches are fine, and a world build before/after would go a good ways to convincing me there's nothing tricky missed under the hood
naoki has joined #u-boot
<sjg1>
I did a world build and it worked fine. But I think I should do it again on a per-commit basis, since there are lot of changes in there
<sjg1>
So we are left with the issue of what to do with options which must do something different in SPL and Proper
<Tartarus>
Well, per commit is gonna take a literal week I bet
<Tartarus>
but --step 0 is great
<Tartarus>
I use it nearly every PR I get :)
<Tartarus>
And, hold on, I still don't want to add any def_bool n options, I think it's fine to say that CONFIG_IS_ENABLED(FOO) evaluates to false in the SPL case due to the symbol being not set
<Tartarus>
We just need to be more careful as I think that checkpatch.pl message is doing more harm than good.
<sjg1>
Tartarus: Yes the checkpatch.pl thing is pointing at something with no guidance...but as I say, I'd like to get rid of the distinction anyway (which is why I added the def_bool for now)
<sjg1>
There is another means which is to have a file containing a list of options which apply only in Proper. I've implemented that as well...
<Tartarus>
We'll always have both, since Linux uses IS_ENABLED
<Tartarus>
I don't think we need a list nor def_bool's
<Tartarus>
We just need to audit the current users, which I gather you've done
<Tartarus>
And drop the encouragement to use it, until we can point at an rST that has enough details
<Tartarus>
And then review new usage more carefully
<sjg1>
It says about 6 hours for the build at present. Will see the true answer when it gets on a bit further. But hopefully overnight. There are definitely differences as I can see very small size variations
<Tartarus>
Uh, you should probably cancel it now
<Tartarus>
You'll want to adopt something like this, to avoid some minor issues
<Tartarus>
You can drop the LLVM stuff, and checkpatch stuff
<Tartarus>
but SOURCE_DATE_EPOCH is important
<Tartarus>
and you may need to fiddle with -b a little, which is why --dry-run is important, to make sure first/last are correct
<Tartarus>
It should result in zero size changes, unless there's something being changed by a given patch tho
vagrantc has quit [Quit: leaving]
<sjg1>
Tartarus: OK ta...
<sjg1>
To put it another way, my goal is that IS_ENABLED() does the right thing for all options, and we don't need CONFIG_IS_ENABLED(). I've always hated that
<sjg1>
For that to work we need to know which options apply to all build phases and which only apply to one
<sjg1>
I would prefer to define that by looking for SPL_FOO options. It means it is mostly automatically
<Tartarus>
sjg1: OK, but I don't see immediately why we need what I keep calling dummy options, the def_bool n ones.
<Tartarus>
You want to move the CONFIG_IS_ENABLED macro over to IS_ENABLED, more or less, but it already evaluates correctly in the case of the symbol not existing
<sjg1>
Tartarus: right, but IS_ENABLED wouldn't
<sjg1>
Ah but it would :-)
<sjg1>
(with the dummy options)
<sjg1>
Also we can drop the SPL_TPL_ stuff in Makefiles
sakman has quit [Remote host closed the connection]
sakman has joined #u-boot
goliath has quit [Quit: SIGSEGV]
<Tartarus>
Well... leave the dummy options for the series where you convince me everything turns out cleaner, when we go that way then please.
<Tartarus>
And you can do all of them in one commit where it's clear why we're adding them
<sjg1>
OK. Well we have the first series (spla) and the 148 patches of the second (spla) that I think are applicable
stefanro has quit [Ping timeout: 252 seconds]
<sjg1>
I'll do some more work on it, hopefully over the next week. I really want to be done with it but it needs one more push
<Tartarus>
Yeah, size check that first 88'ish part series, and lets see if Yamada-san or someone else speaks up on if Kconfig symbols have to be all upper case or not
<sjg1>
Hmm, still 85k commits to build, will see how it looks in the morning
<Tartarus>
Then we can look at v2 of fixing incorrect CONFIG_IS_ENABLEDs
<sjg1>
Yes
<sjg1>
Also once I finish and send splc (u-boot-dm/splc-working), it may be helpful to have his thoughts on the kconf changes (or may not :-)