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 15:
(4 comments)
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 78: config->SerialIoDevMode[dev_offset]; Here, you can both disable the device via `register "SerialIoDevMode"` in dt, and via `device pci ... off`. That differs from the existing logic, see below.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 95: params->SerialIoDevMode[i] = config->SerialIoDevMode[i]; Here, mode is "Pci" when device is set to `on` and config says `Disabled`. It seems intentional (allows a default "Pci" mode w/o mentioning the config in dt). Although, it doesn't match the `enum` name.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 110: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) Nit, if both implementations would share their name, this #if wouldn't be necessary.
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 We have still `PchSerialIoHidden` two times with different meaning.
This and the other commented problem could be solved easily by a coreboot specific enum instead of an FSP specific one.
For instance,
enum { serial_io_default = 0, /* default if not mentioned in dt, same as pci */ serial_io_pci, serial_io_hidden_acpi, serial_io_hidden_legacy_uart, };
deliberately ignores `SkipInit` as that doesn't sound like something to decide in the dt. Code could work like this:
if (config->serial_io_mode == serial_io_default) params->SerialIoDevMode = serial_io_pci; else params->SerialIoDevMode = config->serial_io_mode;