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:
(3 comments)
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
base_root_dev is the chipset tree when it exists, but only if it exists.. having the first applied tree be optional makes this more difficult to deal with the naming.
Yeah, I tripped over the naming usage. What do you think about using generic root_dev and root_bus pointers rather than using chipset_, base_ or override_. Then we don't really need to worry about which tree is actually present. The parsing logic would take care of setting the primary root pointers correctly and rest of the code just needs to rely on using that.
the issue that did find in one of the later patches in this stack is that this doesn't account for aliases added in the override tree.
Do you mean a device that gets added in mainboard device tree or override tree and an alias for it being added in override tree? Or was the alias added in override tree for a device that is defined in chipset tree?
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c@1762 PS9, Line 1762: base_root_bus
it has to be base_root_bus as the initial because that is where the devices first get added, this ti […]
Ah yes, the naming is definitely confusing. Since we are using base_root_bus to parse chipset tree and chipset_root_bus to parse mainboard base devtree.
Responded to your comment below.
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/main.c@1763 PS9, Line 1763: parse_override_devicetree
the chipset device tree doesn't exist for most soc (or even non-soc for the older stuff using north/south layout) so it has to be treated special. when it does exist those devices actually get added to the 'base' root bus first and then overridden. so the naming is actually off in what it does because of the way the parser adds devices.
I agree that the naming is really confusing. As I was reading the code, I assumed that the hierarchy would be that the chipset tree would be the true "base" tree now and all other trees would override the entries in chipset tree. I see now that the base_devtree gets used to parse the initial tree -- chipset or mainboard base depending upon whether chipset provides its own tree.
What do you think about the first part of the comment i.e. using a pointer root_dev and root_bus which is set based on whatever tree is actually parsed first i.e. the order of parsing matters. All the secondary trees would just end up overriding the first tree that is parsed.
a more generic solution would be useful but I think it is going to take some effort to clean up how devices get added based on the parser calls.
Agreed. I think if we organize the code around use of root_dev and root_bus, it would be simpler to later move to the generic solution.