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 10:
(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:
- Why do mainboard/devicetree.cb will have SOC related configuration.
I agree, we shouldn't have settings in mainboard/ that are not board specific. However, it may be hard to decide which settings that are. Sometimes, all boards already in the coreboot tree may just use the same setting by coincidence. It is hard to predict if the next board that will be added might need a different set- ting.
- 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.
Yes, wold be really nice if we could reduce that.
- 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.
I dare to object here. Whether the code in some `fsp_param.c` is better readable than some configuration file, depends much on your choice of local variable names and style in either file. For instance:
fsps->SomeSettingWithLongName = 0; fsps->AnotherSetting = { 2, 3 };
vs
register "SomeSettingWithLongName" = "0" register "AnotherSetting" = "{ 2, 3 }"
It doesn't make much of a difference, does it? Ofc. we could also make up a better configuration syntax, but I'm not sure if it's worth it.
If you keep the SoC specific UPD settings in C, you also avoid all the added declaration clutter (e.g. entry in chip.h) and maintenance burden added by that. So my proposal: Just reduce the entries in `chip.h` to the minimum existing boards need. If we add another board that needs a new entry, add it on demand. If we care, we could even implement the new entry in a way that it doesn't need to be added to existing boards (i.e. on the default 0 for that entry, set the old static value).
Ugh, so much written already. Furquan is right, we should continue on ML in case. It doesn't affect this change here, anyway.