Attention is currently required from: Hung-Te Lin, Jason Glenesk, Raul Rangel, Marshall Dawson, Tim Wawrzynczak, Christian Walter, Arthur Heymans, Kyösti Mälkki, Andrey Petrov, Fred Reitberger, Yu-Ping Wu, Felix Held. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63375 )
Change subject: [RFC] CBMEM: Have INIT_HOOKS in every stage ......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1: Not a fan of the extra binary size impact that comes from this. Registering and running through a no-op function hook is not quite as efficient as not registering it at all.
If you want more flexibility to do things like !ENV_ROMSTAGE, how about changing this to CBMEM_INIT_HOOK(function, condition)? `condition` could only be something the preprocessor can evaluate, but that's good enough for ENV_xxx and Kconfigs. (Universal hooks can just use `true` as the condition.)
Also, we don't want to link hook functions into stages that don't even have CBMEM, so it would be good to still do something with __PRE_RAM__ and ENV_STAGE_CREATES_CBMEM to exclude that.
File src/soc/amd/stoneyridge/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/1e5a966f_103fe282 PS1, Line 218: CBMEM_INIT_HOOK(migrate_power_state) This looks like a functional change? Intentional? Maybe still put it in a separate commit in that case, for clarity.
File src/soc/intel/baytrail/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/4f963b9c_6454f9ec PS1, Line 35: CBMEM_INIT_HOOK(migrate_power_state) Same? This doesn't look like something you'd want to run more than once?
File src/soc/intel/braswell/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/b58603c3_63f59935 PS1, Line 29: CBMEM_INIT_HOOK(migrate_power_state); Same.
-
Julius Werner (Code Review)