On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote: > Hi Ilias, > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas > <ilias.apalodi...@linaro.org> wrote: > > > > Hi Tom, > > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > > > Hi Simon, > > > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > Hi Ilias, > > > > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > [...] > > > > > > > > > > > >> > Changes since v1: > > > > > >> > - Tokenize the DT node entry and use the appropriate value > > > > > >> > instead of > > > > > >> > the entire string > > > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > > > >> > lib/smbios.c | 94 > > > > > >> > +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > > >> > > > > > > >> > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I > > > > > > don't mind I can add a Kconfig > > > > > > > > > > > >> > > > > > >> > + > > > > > >> > + dt_str = ofnode_read_string(ofnode_root(), > > > > > >> > nprop->dt_str); > > > > > >> > > > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > > > > > Unless I am missing something this doesn't work. > > > > This is designed to return a string index from a DT property that's > > > > defined as > > > > foo_property = "value1", "value2" isn't it? > > > > > > > > The code above is trying to read an existing property (e.g compatible) > > > > and get the string after the comma delimiter. > > > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > >> > > > > > >> Any chance of a test for this code? > > > > > > > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > > > SMBIOS tables are populated on OS booting, do we have a test > > > > > > somewhere that boots an OS? > > > > > > > > > > They are written on startup, right? They should certainly be in place > > > > > before U-Boot enters the command line. > > > > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > > architectures, they are only initialized when the efi smbios code > > > > runs. Wasn't this something you were trying to change? > > > > > > One of those things I keep repeating is that we don't know for sure what > > > the right values here are until we load the DT the OS _will_ use. It's > > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > > what the real DT to load before passing to the OS is. Since U-Boot > > > doesn't need SMBIOS tables itself, we should make these "late" not > > > "early". > > > > Fair enough, we can defer the init and testing of those late, just > > before we are about to boot. But this is irrelevant to what this patch > > does, can we get the fallback mechanism in first, assuming everyone is > > ok with the patch now? > > > > I would like this behind a Kconfig. Making it the default means people > are going to start relying on (in user space) and then discover later > that it is wrong.
What do you mean wrong, exactly? -- Tom
signature.asc
Description: PGP signature
