[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 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)
> A value of 0 (i.e. unset) is also <= PchSerialIoSkipInit and would […]

Is the concern here that uninitialized values in DT could result in unwanted side-effects by setting SerialIoXXXXMode incorrectly? It is possible to handle that case by simply adding a dummy index 0 value:

enum {
    PchSerialIoNotInitialized,
    PchSerialIoDisabled,
    ...,
    PchSerialIoMax,
};

and then just subtract 1 when setting the UPD.

if (!dev || !dev->enabled) {
     ... = PchSerialIoDisabled;
     continue;
}

if (config->SerialIoDevMode[dev_offset] == PchSerialIoNotInitialized ||
    config->SerialIoDevMode[dev_offset] >= PchSerialIoMax) {
     ... = PchSerialIoPci;
     continue;
}

... = config->SerialIoDevMode[dev_offset] - 1;

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

something to decide in the dt.

There can be cases where this will have to be done in DT. Mainboards might require certain blocks to be left as is and not initialized by FSP e.g. UART that is initialized by coreboot and doesn't need to be reinitialized by FSP. So, it will have to be done in DT.

Or consider the case where function 0 needs to be kept enabled even though it is unused just because you don't want function 1..n to be disabled. However, the GPIOs for function 0 are not used for native functions, but some other purpose and you don't want FSP to re-initialize or disable that block.