Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3:
(3 comments)
Thanks for your thoughts, Julius.
This was mostly a teaser how a clean API could look like. It doesn't seem that there is much interest. Beside some "we can do this later" and rebasing my patches in a broken state, there wasn't any feedback from the stakeholders.
So I'll likely abandon this in the next fex days if nobody takes over.
I suggest that we discuss the whole topic on the mailing list. There are some general questions like should the bootblock console be configurable? and should we make VPD available for such settings or keep it downstream?
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig@146 PS3, Line 146: option to be queried needs to be added manually to vpd_get_option().
I still maintain that using this to control the console is generally not a good idea. […]
I completely agree. But people asked to use VPD for it... I'm not sure if we should deny it.
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h@10 PS3, Line 10: /* Keep in sync with Kconfig VPD_OPTION_REGION. */
Don't you just want to make a little function with […]
Good idea, thank you.
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c@306 PS3, Line 306: char
Why is this `char`? init_log_level() is passing a pointer to `int`. […]
Oh, that's part of the heavily broken traditional get_option() API. There is no length given, hence it is decided based on `name`. And for `debug_level` that always was a single byte.
Any, yes, init_log_level() only works because the more significant bytes were always initialize to zero and we run it on little endian.