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: I'm a bit worried about the overall binary size added by these tables to (potentially) every stage. We already have a pretty unfortunate situation with devicetree.cb sometimes taking up near half of the available bootblock space on some boards, and this looks like it may end up causing similar pain. If this is supposed to be our new, be-all/end-all query interface for any kind of firmware-relevant SKU differences, it may very well be possible that you need to check one of those differences in the bootblock, another one in romstage, etc... you end up linking the whole table *and* all the option callbacks into each of those stages (which might not even compile for some of them). That's particularly unfortunate because the bootblock may not even care about all those fields -- it might only want to query one particular one, and it would be great if we could come up with some clever macros that reduce that check down to a single mask-and-compare without having to build in information about all the other fields.
I think you should be able to condense almost all of this down to compile-time constants so each stage can avoid linking in anything other than what it actually really uses. Consider if you made every supporting board provide a <board/fw_config_fields.h> header (similar to how including the <soc/...> headers works) which is then included here. In that header you would write stuff like:
FW_CONFIG_DEFINE_FIELD(DB, 3, 0) FW_CONFIG_DEFINE_FIELD(THERMAL, 7, 4) FW_CONFIG_DEFINE_FIELD(AUDIO, 10, 8, NONE, MAX98357_ALC5682I_I2S, MAX98373_ALC5682I_I2S, MAX98373_ALC5682_SNDW) ...etc...
FW_CONFIG_DEFINE_FIELD() would be a variadic macro (like the stuff in <base3.h> or <device/mmio.h>) that would auto-generate the following definitions for each field:
#define FW_CONFIG_FIELD_AUDIO_LOW 8 #define FW_CONFIG_FIELD_AUDIO_HIGH 10 #define FW_CONFIG_FIELD_AUDIO_MASK ((1 << (1 + 10 - 8)) - 1) #define FW_CONFIG_FIELD_AUDIO_OPTION_NONE 0 #define FW_CONFIG_FIELD_AUDIO_OPTION_MAX98357_ALC5682I_I2S 1 #define FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682I_I2S 2 #define FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682I_SNDW 3
FW_CONFIG_MATCH() could use token pasting so that FW_CONFIG_MATCH(AUDIO, MAX98357_ALC5682I_I2S) results in FW_CONFIG_FIELD_AUDIO_LOW, FW_CONFIG_FIELD_AUDIO_MASK and FW_CONFIG_FIELD_AUDIO_OPTION_MAX98357_ALC5682I_I2S being stored in the devicetree structure, so you can have basically the same nice, readable syntax in devicetree.cb.
If you want to check an option at runtime, fw_config_match() would have to turn into a macro (which would then clash with the devicetree one, so maybe call it FW_CONFIG_CHECK(field, option) or something). The space-constrained bootblock code would say
if (FW_CONFIG_CHECK(AUDIO, MAX98357_ALC5682I_I2S))
and that would be replaced to
if ((fw_config_get() >> FW_CONFIG_FIELD_AUDIO_LOW) & FW_CONFIG_FIELD_AUDIO_MASK == FW_CONFIG_FIELD_AUDIO_OPTION_MAX98357_ALC5682I_I2S)
so you still have pretty syntax, but it compiles down to ~3 instructions. (For checking values from the devicetree there could be a similar macro to make that easier. For convenient handling it might be nice to package the offset/mask pair or the offset/mask/option tuple into typedeffed structures so it is easier to pass them around.)
For the run callbacks thing, well... wouldn't it be simpler (and probably smaller in binary) to just have a bunch of switch statements? Your volteer/fw_config.c could just have a
void mainboard_handle_fw_config(void) { switch (FW_CONFIG_FIELD_VALUE(AUDIO)) { case FW_CONFIG_FIELD_AUDIO_OPTION_NONE: gpio_pads_stuff(); case FW_CONFIG_FIELD_AUDIO_OPTION_MAX98357_ALC5682I_I2S: other_gpio_pads_stuff(); ...etc... }
switch (...next field that needs special handling...) { ... }
Then either make a boot state hook right there or make a central one that calls mainboard_handle_fw_config() as a well-known function name the mainboard must provide.
If you really want a table approach, you could still do it with that approach as something like
struct fw_config_callbacks[] = { FW_CONFIG_CALLBACK(AUDIO, NONE, audio_db_none_cb), FW_CONFIG_CALLBACK(AUDIO, MAX98357_ALC5682I_I2S, audio_db_i2s_cb), ...etc... }
with FW_CONFIG_CALLBACK unpacking it into low offset, mask, match value and callback pointer. That should still be quite a bit smaller than reserving space for 32 options per field, and it would only have to be linked into ramstage where it is used.