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 7:
(3 comments)
Please don't draw every cosmetic change in FSP UPDs into our devicetrees. It makes things messy and in- compatible.
I don't object to such changes in general. But please ask your FSP team to do such cleanups to all FSP plat- forms in parallel so FSP users don't have to suffer.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h@304 PS7, Line 304: uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX];
Yes, It an make device tree struct constant but it'll be add more complexity in fsp_param.c.
Try to see it as a courtesy to Intel's customers. If you abstract here, they don't have to relearn everything with every FSP release. There might be even cases where somebody reuses a board design and only changes the SoC. In that case it would be very nice to keep the devicetree compatible.
Also I feel that it would make the code difficult to understand. This change will provide clear picture of what changes are coming into new fsp.
I see it much different. If you implement it here, you have a common place where you can see what changed in the FSP interface without technical reason. That would provide a clear picture of the extra burden brought in by the unstable FSP interface. I mean, it's just the names that changed, not the semantics. Sometimes it seems that some Intel employees do such changes just to show that they really worked on something (without thinking on the implications for the teams that have to incorporate FSP). Or maybe it's just Intel's way to slow their customer's projects down.
So please ask your FSP team fellows to align the UPDs for all FSPs that are supported here by soc/intel/cannonlake/. And remind them that such cosmetic changes create a lot of work for naught. Maybe there is still a chance that we don't have to drag around the #if mess here forever?
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 37: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
If we use C if statement, compilation would fail since fsp paramaters wouldn't be included in header […]
Ack, forgot about that.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... PS7, Line 26: PchSerialIoSkipInit
It'll be difficult to add FSP version details since these changes are coming up with new CML FSP cha […]
Yes, that's why I said "for the future", I know we can't fix it right now. Intel's FSP development process just slows things down, I've long accepted that.