Aaron Durbin 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:
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.
src/cpu/intel/car/romstage.c is not used by AMD. It's intel only. That said, if it becomes that my argument still holds. I'm not sure I buy your concern about blowing out the potential features that start littering such a file. If it does become like that I think then it's a situation where we would want to refactor things. But I don't think we need to be concerned until that time comes.
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?
It's a little more complicated than that given the new AMD boot architecture. But I currently don't see a need that necessitates more infrastructure aside placing calls at the correct point. One could put in a generic callback at various points in that file and hook in like that. But is that necessary when we already know Kconfig is honoring dependencies. The compiler will omit the call sequences through dead code elimination.
-
Aaron Durbin (Code Review)