Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )
Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Thanks for the fix. I wouldn't mind merging it in this state, though maybe see if we can settle on the Kconfig vs `chip.h` thing, first.
NB. There may be more unset UPDs related to serial irqs. Maybe we should set them all at once.
https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... PS2, Line 250: /* Set correct Sirq mode based on config */
That's already done in lpc_soc_init(). […]
For once, I wouldn't say that. FSP is full of these options (fixing that would imply redesigning FSP entire- ly; I wouldn't mind, though). coreboot should never rely on UPD defaults, every UPD must be set according to coreboot's expectations, and that is what is done here (at least for one more option).
That we have so much redundant code in coreboot, IMHO, is a documentation problem. Nobody knows what FSP al- ready does exactly, so people add more and more con- flicting code... that is something that can be fixed on the FSP side. Please, somebody document it!
The coreboot code around this is rather messy and needs a clean-up, IMHO. For instance, Skylake provides both a Kconfig and a `chip.h` setting for this :-/
It was a Kconfig historically, but I wouldn't mind a `chip.h` setting in this case. Simply because we decide it once per board and don't have any complex require- ments, neither do we want it user configurable with a Kconfig prompt.