ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36221 )
Change subject: Add configurable ramstage support for minimal PCI scanning ......................................................................
Patch Set 8:
(10 comments)
no sooner did I abandon it than two different companies asked me not to.
https://review.coreboot.org/c/coreboot/+/36221/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36221/8//COMMIT_MSG@25 PS8, Line 25: keyword to sconfig
Strange line break. […]
it's intentional
https://review.coreboot.org/c/coreboot/+/36221/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36221/8/src/Kconfig@339 PS8, Line 339: bool "Enable a configurable ramstage."
Please remove the dot at the end.
Done
https://review.coreboot.org/c/coreboot/+/36221/8/src/Kconfig@344 PS8, Line 344: The minimal PCI scanning will only check those parts that are enabled : in the devicetree.cb. By convention none of those devices should be bridges.
Move this to the description of the option below?
I just blew it away, I did not see a need for this text.
https://review.coreboot.org/c/coreboot/+/36221/4/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/36221/4/src/device/pci_device.c@119... PS4, Line 1195: dev = dev_find_slot(0, devfn);
Sigh.. dev_find_slot() should be banned. Would following work for you: […]
Done
https://review.coreboot.org/c/coreboot/+/36221/4/src/device/pci_device.c@119... PS4, Line 1197: printk(BIOS_WARNING, "%#x is NOT static, skipping it\n", devfn);
We prefer B:D.f notation for PCI devices on console output.
I blew away the print, it's a time waster.
https://review.coreboot.org/c/coreboot/+/36221/8/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/36221/8/src/device/pci_device.c@119... PS8, Line 1191: dev = pcidev_path_behind(bus, devfn);
I did not see this on first review, but this probably evaluates to same 'dev' as pci_scan_get_dev() […]
yes it should. And it will. What we're showing here is the minimal set of changes to make this work at all, b/c every pass we've taken at this has been bikeshedded into oblivion.
https://review.coreboot.org/c/coreboot/+/36221/8/src/device/pci_device.c@122... PS8, Line 1229: *prev = dev->sibling;
It is pci_probe_dev() that filled vendor field. […]
yes we are.
https://review.coreboot.org/c/coreboot/+/36221/8/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/36221/8/src/include/device/device.h... PS8, Line 133: *
Remove for concise multi-line comments.
Done
https://review.coreboot.org/c/coreboot/+/36221/8/util/sconfig/sconfig.tab.c_... File util/sconfig/sconfig.tab.c_shipped:
https://review.coreboot.org/c/coreboot/+/36221/8/util/sconfig/sconfig.tab.c_... PS8, Line 112: #ifndef YY_YY_HOME_RMINNICH_PROJECTS_LINUXBOOT_COREBOOTNERF_GITHUBCOREBOOT_UTIL_SCONFIG_SCONFIG_TAB_H_SHIPPED_INCLUDED
Is this wanted?
this is a generated file. Not much to be done I think.
https://review.coreboot.org/c/coreboot/+/36221/8/util/sconfig/sconfig.tab.h_... File util/sconfig/sconfig.tab.h_shipped:
https://review.coreboot.org/c/coreboot/+/36221/8/util/sconfig/sconfig.tab.h_... PS8, Line 33: #ifndef YY_YY_HOME_RMINNICH_PROJECTS_LINUXBOOT_COREBOOTNERF_GITHUBCOREBOOT_UTIL_SCONFIG_SCONFIG_TAB_H_SHIPPED_INCLUDED
Ditto.
ditto :-)