Kyösti Mälkki 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 9:
Patch Set 9:
I'm a little nonplussed by the comments. Multiple vendors have shown a need for what this CL does. It's simple. It's off by default. It fixes a problem.
What's the issue here?
Ron, I enjoyed Jeremy's thorough answer. Yours, not so much.
If we are seeking the minimal change approach to resolve resource allocation problem (which appears to be *the* vendor request based on the feedback we received so far) changes to ACPI namespace and/or AML seem largely out-of-scope and undesirable.
The keyword 'mandatory' sounds very much like a platform property that will end up being copied for each individual mainboard. Well... PCI drivers would be the perfect place to have that instead, that approach leaves all devicetree.cb files unaffected and no util/sconfig changes necessary either. One more reason why you would want to probe the device ID and bind it to a driver.
If my hunch about ACPI is correct here, and we eventually want to keep PCI enumeration without resource allocation, we are looking at 99% revert of this commit to fix it. Cases where vendors (and/or original authors) return to their code and willingly accept reverts are unfortunately rare. I did not fix the previous regressions I caused related to hidden Intel PCI devices either, I believe ultimately Nico committed the fixes(?).
The other issue that triggers -2 from me 99% of the time; Authors not respecting the time others put into reviews with good intentions. The attitude is there somewhere in the previous replies, 'Lets just merge our code now so others can fix it afterwards'. For a moment I thought I had mistakenly written my question in Finnish, but no, seemed to be fairly plain English.