David Hendricks has posted comments on this change. ( https://review.coreboot.org/20922 )
Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ......................................................................
Patch Set 4:
(9 comments)
New results: https://paste.flashrom.org/view.php?id=3047
https://review.coreboot.org/#/c/20922/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/20922/4//COMMIT_MSG@7 PS4, Line 7: sp
chi*ps*et
oops - done.
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c:
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@292 PS4, Line 292: desc->component.FLILL1 == 0) {
Just realized that you changed parentheses here. Which is wrong.
Darn! It felt a bit awkward since I also had to invert logic and a add set of parens.
I tried to make this a bit more idiot-proof by breaking apart the if-statements for FLILL/FLILL1 and the chipset. This way we only need to check the chipset once, and it will make it easier to use a switch statement later if we want. At the very least it should require fewer functioning brain cells when going thru the tedious effort of adding a new chipset.
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@320 PS4, Line 320: unknown
Region 6 (counting from 0) is the Secondary BIOS region, according to
I went ahead and named it BIOS2 as suggested (it was one of a few suggestions actually) on IRC.
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@321 PS4, Line 321: EC
IIRC, this is "BMC" for Lewisburg. Maybe name it "EC/BMC"?
Done
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@371 PS4, Line 371: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) {
missing C620 here
Done. I gave it its own special case since since we have a longer region name length (5 chars instead of 4). Here's how it looks (I'll post another example in case gerrit mangles this):
--- Details --- FD BIOS ME GbE Pltf DE BIOS2 Reg7 BMC DE2 IE 10GbE OpROM Reg13 Reg14 Reg15 BIOS rw rw rw rw rw r ME r rw rw r GbE rw DE rw rw rw BMC rw IE rw
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: ,
We have to add a sixth region, "unknown", or we'd run into the
Yes, 4th region is DE. Since I'm not sharing the Sunrise Point code, I guess this can just be "BMC"? Though in the future some PCH that calls it "EC" will re-use the Lewisburg codepath, in which case we can change it.
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@381 PS4, Line 381: msg_pdbg2(" FD BIOS ME GbE Pltf Reg5 Reg6 Reg7 EC Reg9\n");
More regions, sigh.
Done
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@1133 PS4, Line 1133: };
I just remembered, here's a third list. Though, this one uses identi-
I went ahead and updated this list for what I would expect ifdtool to use, though I haven't gone thru all of ifdtool yet.
I thought access to unallocated space was generally forbidden. I'm not certain of that, though.
https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descri... File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descri... PS4, Line 48: };
That's a lot of maintenance, maybe we should move the list from
Yes, in another patch :-)
-
David Hendricks (Code Review)