Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, Christian Walter, Stefan Reinauer, Edward O'Callaghan, David Hendricks, Deomid "rojer" Ryabkov, Tim Chu. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 )
Change subject: Revert "Add support for Intel Emmitsburg PCH" ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
The patch was reviewed and tested by multiple parties - you were one of them! Your comments didn't contain any useful guidance for how you'd like to see this implemented, nor did you raise any specific, actionable concerns.
I asked for something. I didn't get an answer. I'm too busy to take on all details of a patch before the basic questions are answered. Usually that turns into wasted time.
You asked about freq_read and ICCRIBA, neither of which that patch touched or changed as a consequence of that patch. You didn't explain why those might be relevant, either. We're all too busy to go on wild goose chases.
Except that somebody forgot to write consistent code.
if (x <= 80) return;
That's not quite stating it accurately. Consider the flow of the outer if-statment: https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/heads/master/ich...
Just like in the other cases where we check a range of ISL values at successively larger values, if the ISL is greater-than the highest value we compare with then the outer if-statement will end by returning the default. It happens that in this case the last value we compare with was quite high, perhaps due to it being a server chip.
...x != 16...
The `warn_peculiar_desc(content->ISL != 16, "Ibex Peak")` prints a warning that we're assuming Ibex Peak for compatibility, and then outer if-statement finishes by returning CHIPSET_5_SERIES_IBEX_PEAK as a default. This is not deliberately selecting Ibex Peak, it's choosing an arbitrary default based on failure to detect other chipsets.
We can also argue, based on the broken code, that the consistent code wasn't good anyway, ofc.
Adding EBG in a consistent manner is what got us here :-P What we really need to do is remove brittle assumptions that cause problems when new hardware is added. ifdtool already does that for the Ibex Peak case, though it still allows a range of ISL values to match: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/uti...
Perhaps we can stick to allowing a range of ISL values for older hardware where we don't have documentation or IFDs handy to look for an exact value to compare with, and be more careful about adding values to match newer hardware.
Let's be serious again.
Yes, please. I've sent two patches to fix the problems mentioned above: CB:57580 and CB:57581. If you'd like you can add this revert patch into that patch chain and rebase the others on top of it, and we'll have three patches to merge instead of only two. LMK how you'd like to proceed.
-
David Hendricks (Code Review)