Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28791 )
Change subject: soc/intel/skylake: Ensure FSP don't override ITSS IPCx registers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/28791/3/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/28791/3/src/soc/intel/skylake/chip_... PS3, Line 177: fsp_silicon_init(romstage_handoff_is_resume());
This includes calling back to the soc and then the mainboard code (mainboard_silicon_init_params()) where many boards configure the pads which includes configuring IRQ polarities. So it may supersede the config that was backed up above.
You mean when pads are configured? Why would pads be configured during mainboard_silicon_init_params() ? I see it's happening, but it doesn't seem correct.
We should be doing pad configuration either prior to this fsp call through mainboard chip->init(). I think big core's flow is messed up as the pad configuration is in the flow of the callback you mentioned as opposed to prior to this call. i.e. there is not an init() in mainboard_ops for these mainboards.
This shows that we have way too many hooks around. It seems, nobody can keep track when what is called. I'm thinking about dropping mainboard_silicon_init_params(), it seems only useful for downstream solutions (i.e. if the soc/ code doesn't fill the params properly, that should be fixed instead).
That callback's intention is for configuring settings to FSP that are specific to the mainboard. Now, if we've plumbed all those settings from devicetree.cb to FSP UPD using the soc code, then I agree we can remove this callback.
This would leave us with the question when to do the pad configu- ration? Any suggestions? Some boards do it in the mainboard's chip->init(), which should work for this case, but I wonder if we shouldn't just make it a rule to always do it early, e.g. part of the romstage soc control flow?
We can do it in romstage or chip->init() as you suggested. I think you'll find the latter is easier because romstage is a more constrained environment (CAR, etc). There's a lot of state maintained because of the gpio/pad complexity where it would be harder to put into romstage. As such, I prefer mainboard_ops->init().