Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h@304 PS7, Line 304: uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX]; I think this file is a perfect place to abstract over the unnecessary differences in the CFL/CML UPD headers. You can keep this file as is and limit the changes to `fsp_params.c`. This way, our devicetree format would stay stable and we'd avoid the #if here (it's generally discouraged).
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 37: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) Please use a C `if` statement instead.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 38: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode, CONFIG_SOC_INTEL_I2C_DEV_MAX); The original code below was explicitly written to allow the use of a devicetree-native `off` for the disabled state. Please maintain that functionality.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 184: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) C `if` please.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
PS7: I have a feeling that I commented on this on another change before. If this has to be in sync with FSP, please mention it in a comment. But I'd prefer not to draw the interface changes in FSP into the coreboot code. That one version of FSP names the same thing dif- ferently than another doesn't have to mean that we have to jump through hoops in coreboot.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... PS7, Line 26: PchSerialIoSkipInit AIUI, the last two are only available for UARTs (at least that's what the comments in FSP headers suggest). Maybe a comment on each value, when it is valid (serial i/o type, fsp version), would be better here than the #if.
And for the future, it would be nice if the FSP headers would ship such enums. Than we wouldn't have to keep things in sync manually, and without all the maintenance burden, would get everything ready much faster ;)