Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Edward O'Callaghan, Angel Pons. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57635 )
Change subject: Add support for Intel Emmitsburg PCH ......................................................................
Patch Set 3:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/57635/comment/64f1e986_0ffaa13d PS1, Line 16: TESTED=tried on a server with Intel Emmitsburg PCH, flash update : was successful. This server, however, does not have flash chip : installed, it instead has em100 emulator connected.
Recently the patch(es) were tested on 2 different servers with Intel Emmitsburg PCH, with flash chip […]
Thanks, Jonathan. I'll also do a final test on the CRB before submitting in case any of the intermediate changes we've made caused problems.
Commit Message:
https://review.coreboot.org/c/flashrom/+/57635/comment/c9367ec0_c27ed768 PS2, Line 11: - Based on ICH descriptor content, choose This line break was bothering me, so I updated the comment to fit in the margins more cleanly.
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/57635/comment/b820830e_aceec00c PS1, Line 945: if (content->ISL <= 139)
Can there be Emmitsburg IFDs with less than 80 straps? I'd check for `content->ISL < 80` somewhere, […]
AFAIK Eagle Stream is the only existing platform with the Emmitsburg PCH, and the BKC images I have for it show 80 straps. That's why I thought it was weird that we're using `<=` everywhere instead of `==`, although perhaps the person who originally wrote this code noticed some earlier chipsets did have overlapping ISL values, or they just didn't have access to enough docs/resources to know for sure.
In any case, it's a pretty big range we're allowing to fall into this case so I think your suggestion is good and have added the peculiar descriptor warning. Thanks!
-
David Hendricks (Code Review)