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

[Nico Huber (Code Review)]

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 16:

(2 comments)

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

https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... 
PS16, Line 58: 		if (config->SerialIoDevMode[dev_offset] <= PchSerialIoSkipInit)
> Is the concern here that uninitialized values in DT could result in unwanted side-effects by setting […]

I'm fine with any solution that either maintains the current behavior
or makes dt writing more convenient.

A change in behavior should be mentioned in the commit message.

https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... 
File src/soc/intel/cannonlake/include/soc/serialio.h:

https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... 
PS15, Line 35: #endif
> > deliberately ignores `SkipInit` as that doesn't sound like […]

Wrt `SkipInit` I meant, I'd rather handle such things in soc
code. i.e. if coreboot implements alternative code and that
code is used, having to set `SkipInit` in the dt would be
redundant. A function 0 shouldn't be hidden case, can be
handled in soc code, too.