Maulik V Vaghela 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:
(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 […]
True. Existing logic assigns default value as SerialIoPci in case no value assigned in dt. Also logic checks that value assigned are valid ones. I have updated new code to check validity as well as assign default value in case no values assigned in dt.
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 […]
True. Existing logic assigns default value as SerialIoPci in case no value assigned in dt. Also logic checks that value assigned are valid ones. I have updated new code to check validity as well as assign default value in case no values assigned in dt.
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.
Done
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 […]
PchSerialIoHidden has 2 different value but does the same job. It'll communicate to fsp that SerialIo is hidden from pci bus. I feel that making enum changes in coreboot will create one more layer between device tree and fsp_param.c. Wouldn't it be simpler if we can pass values directly from dt to FSP? This way developer are also aware of what value they are passing to FSP.