Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36211 )
Change subject: cpu/intel/car: Add EC software sync to Intel romstage ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
Patch Set 1:
> Patch Set 1: Code-Review-1 > > (3 comments) > > Romstage starts here for all Intel platforms. I don't think it makes sense to put platforms specific things here.
That's fair. I'll drop this patch and look into that.
The alternative is to add call points to all mainboard_romstage_entry() code which is also decoupled from the Kconfig itself. i.e. selecting Kconfig won't necessarily make things work generically.
Don't those typically exist already to have mainboard specific init before the raminit?
Somewhat. for our mainboards I try to push everyone to have a common flow instead of open coding the flow. Basically have less duplicated code. If you look back to the mainboards that had traditionally been around in coreboot they have a large amount of duplicated code. That is a maintenance burden. If Kconfig is set up correctly with the proper dependencies then I don't see an issue in placing calls at a strategic point that minimizes code duplication.
How about having a linker script 'section(.rodata.romstage_entry)' for such fucntion pointers similar to the cbmem initialization? Is that too much complexity?
I'm not sure I'm following. You mean having callbacks that are traversed at certain points in the bootflow but for romstage? That's certainly possible, but I'm not sure it's necessary. For one, we don't have a common romstage stage machine. If we had that then it wouldn't be too bad to do what you suggested. Although, I could just as easily make an argument to put the call guarded by a Kconfig at the correct point there as well. The early stage boot flows are less demanding and more straight forward. It seems to me you are designing a solution to prevent a 'if (CONFIG(BLAH)) call_function()' sequence within this file, and it's not clear to me why.
I think Kyösti was working making this file an architectural romstage entry point for x86 (it is already not Intel specific and used on AMD). I don't mind 'if (CONFIG(...))' but given that this entry point is common to most/all Intel x86 boards, I think it's a bit weird to put board/platform/vendor specific things in there. We don't want a list of 'if (CONFIG(VENDOX_X_FEATURE_Y))do_z()' in here.
Would a common romstage state machine make sense? You'd have pre_raminit (console init if done in bootblock, stack guards, ), early/raminit, post_raminit (prepare postcar, ...) callbacks?