Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41209 )
Change subject: fw_config: Add firmware configuration interface ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41209/2/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/41209/2/src/include/fw_config.h@9 PS2, Line 9:
mainboards themselves can't include code to static.c currently
I'd be curious why that is, what was the exact problem? I thought static.c is treated like any other stage source file once it is generated.
I want a string for each field and option for console output anyway
It's possible to have the generation macro spit out a #define FW_CONFIG_FIELD_AUDIO_NAME "AUDIO" as well, of course (and same for options).
lots of macro expansion which usually generates complaints in reviews
Well... it's a matter of opinion I guess, but in my mind macros are fine as long as it's an isolated API that only really needs to be written and carefully reviewed once and then rarely touched in the internals again, the external interface is clearly documented and easy to understand (even if not everyone understands the internals), and there's a sufficient benefit coming from it that you can't really get with other methods. We have other big complicated macro APIs like that in coreboot already... for example memlayout, or the SET32_BITFIELDS() we added to <device/mmio.h> recently (which I hope takes off because while the internals of how it works are confusing, it is very simple to understand how to use it and it makes code working on hardware registers a lot safer, more readable and easier to follow).
I for one would definitely complain a lot more about leaving binary size on the table ;) (for something that is supposed to be so universal and should work and be used across all platforms with all their different needs and constraints). Even if you apply very careful struct packing to your solution to make it as tight as possible, I think the fundamental issue remains that if one stage only needs to check a single field it shouldn't have to link in info about everything else.
Auto-generating the table from somewhere else also sounds fine but I think that's a pretty orthogonal issue to how it is represented in C. You could auto-generate macros or you could auto-generate structs. But at least if you don't want to auto-generate it I think we can definitely find ways to make a header file with macro definitions just as compact and readable as a struct initializer.