Subrata Banik 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 9:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
I don't see a very good use-case for why we would need a soctree.cb. If the UPDs are common across all mainboards, why do we even need a config file in that case?
[Subrata] Few reasons: 1. Why do mainboard/devicetree.cb will have SOC related configuration. 2. Why do we want to make duplicate SOC configuration copy into all mb/devicetree.cb. I have made some study in existing coreboot code and looks like 80% FSP UPD exist in devicetree.cb are duplicated across all MB. 3. Fix it once and use it across, something like common code model, Hope you remember recent hatch PO issue related to SAGV UPD value. Looks like someone didn;t copy it correctly in mainboard devicetree.cb hence we need to suffer. But assume if this would be a common soc config then once its working for 1 mb, the same should work for all. 4. Code readability: Hope you have seen lots of hard coding in fsp_param.c file while assigning UPDs, we could refer to single config file like soc_devicetree.cb to make better readability.
Maybe it needs a wider discussion. If you have specific examples of what you think would be good candidates, you can start a CL or email thread to discuss it further.
[Subrata] We are working on proposal side and initial POC patch sets to see how it works. so far we are seeing some sconfig limitations with multiple .cb file between mainboards and soc. but i believe we can fix those.