[coreboot-gerrit] Change in ...coreboot[master]: soc/intel/cannonlake: Add required FSP UPD changes for CML

[Furquan Shaikh (Code Review)]

Furquan Shaikh 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 10:

(3 comments)

https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h@295 
PS10, Line 295: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
              : 	/*
              : 	 * SerialIO device mode selection:
              : 	 * PchSerialIoDisabled,
              : 	 * PchSerialIoPci,
              : 	 * PchSerialIoHidden,
              : 	 * PchSerialIoLegacyUart,
              : 	 * PchSerialIoSkipInit
              : 	 */
              : 	uint8_t SerialIoI2cMode[CONFIG_SOC_INTEL_I2C_DEV_MAX];
              : 	uint8_t SerialIoGSpiMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX];
              : 	uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX];
              : #else
Do we really need this? Can't we just have the same SerialIoDevMode[] and then handle it correctly within chip.c? It will avoid the #if here as well as in serialio.h for PchSerialIo*Index.

https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... 
PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
> we can take 2 steps path. […]

I don't see a very good use-case for why we would need a soctree.cb. If the UPDs are common across all mainboards, why do we even need a config file in that case? Maybe it needs a wider discussion. If you have specific examples of what you think would be good candidates, you can start a CL or email thread to discuss it further.

https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/fsp_params... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/fsp_params... 
PS10, Line 56: continue;
don't need the continue here, you could just use else below. But I think this is being done for consistency with the _cnl_common function, so its okay either ways.