Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44037 )
Change subject: sconfig: Allow chipset to provide a base devicetree ......................................................................
Patch Set 9:
(8 comments)
https://review.coreboot.org/c/coreboot/+/44037/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44037/9//COMMIT_MSG@15 PS9, Line 15: device Because of the way things are organized and compared in sconfig (matching devices using chip to ensure they are the same devices in two different trees), I guess the assumption would be that the devices in chipset.cb and mainboard devicetree.cb use the exact same chip driver i.e. either the SoC chip driver or any other specialized driver used by that controller device.
I think we currently have just one such example - CNVi devices defined very differently in mainboard devicetree compared to other controller devices i.e. a chip driver directly associated with the SoC controller (Example: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt...).
We should document this assumption somewhere because it might be easy to miss this. (As a follow-up, I think we should clean up the CNVi handling in coreboot to make it similar to how other controller devices are organized. Do we want to add a restriction that the SoC controller devices must never have a chip driver associated with it other than the SoC chip?)
https://review.coreboot.org/c/coreboot/+/44037/9/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44037/9/Makefile.inc@16 PS9, Line 16: CONFIG_CHIPSET_DEVICETREE Shouldn't this config option be added to src/Kconfig in this CL?
https://review.coreboot.org/c/coreboot/+/44037/9/Makefile.inc@607 PS9, Line 607: $(src)/ The expectation is that the chipset devicetree path should be relative to src/ directory. It would be good to capture that in the Kconfig help text.
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c@713 PS9, Line 713: base_root_dev This should be chipset_root_dev in case aliases are provided by that tree. I think we should actually use root_dev and root_bus independent of chipset/base/override. See my comment below about root_dev and root_bus.
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c@755 PS9, Line 755: new_d->devnum = devnum; nit: I think we can also split this function into two parts. One `new_device()` which checks alias, does strtol for devnum and calls `new_device_with_path()`. Second `new_device_with_path()` which does the rest of the work in this function and can also be called directly from `new_device_reference()`. Just saves having to store another parameter in `struct dev` since it is already converted into path_a and path_b. But not a big deal either ways.
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c@1762 PS9, Line 1762: base_root_bus Shouldn't this be chipset_root_bus?
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c@1763 PS9, Line 1763: parse_override_devicetree I think we need to update this function as well to pass in the base root bus that is actually being used. We now have three trees being maintained here:
chipset_devtree with its root bus as chipset_root_bus base_devtree with its root bus as base_root_bus override_devtree with its root bus as override_root_bus
Probably we should organize this differently:
process_devicetree(chipset_devtree, &chipset_root_dev); process_devicetree(base_devtree, &base_root_dev); process_devicetree(override_devtree, &override_root_dev);
void process_devicetree(const char *file, struct dev *parent_dev) { struct bus *parent_bus = &parent_dev->bus; if (!file) return;
parse_devicetree(file, parent_bus);
if (!root_dev) { root_dev = parent_dev; } else { override_devicetree(&root_dev->bus, parent_bus); } }
And then we need to ensure that rest of the code uses root_dev and root_bus instead of base_root_dev/base_root_bus.
I also like Stefan's comment that maybe it's time to just allow n trees which can act as overrides over previous trees in the list.
for (i = 0; i < n; i++) { parse_devicetree(file[i], bus[i]); if (i == 0) continue; override_devicetree(bus[0], bus[i]); }
root_bus = bus[0]; root_dev = dev[0];
where bus and dev arrays are allocated at runtime instead of hard-coding the structures for root dev and bus at the top of the file. But it might need some surgery to get all of these changes in. If you think it is too big of a change, we can also do the generic n trees as a follow-up.
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/sconfig.h File util/sconfig/sconfig.h:
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/sconfig.h@136 PS9, Line 136: devnum It would be helpful to have a comment indicating what this really is.