Attention is currently required from: Nico Huber. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 )
Change subject: Add Elkhart Lake support ......................................................................
Patch Set 2:
(9 comments)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/f0e343e6_af6b006f PS1, Line 709: boot_straps = boot_straps_pch500;
The EDS says `bbs == 1` would be "UFS". Not sure how that's connected. […]
In EDS there is just SPI defined (bbs=0), whereas bbs=1 is marked as reserved. So I guess it is more like it was on APL/GLK. I will move the case CHIPSET_ELKHART_LAKE one down.
https://review.coreboot.org/c/flashrom/+/60711/comment/59bb318f_6cb10714 PS1, Line 995: dev
So AFAICS, this is already 1f.5. We could save us the dance to find it […]
I guess this would be something for a follow-up patch, right?
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/cdc888e7_1ed666be PS1, Line 538: IFWI
Intel likes to haunt us with this it seems. Now all the non-BIOS firmware is […]
Yeah, it is this back and forth with every new platform...
https://review.coreboot.org/c/flashrom/+/60711/comment/ab33e822_e7732301 PS1, Line 1033: return CHIPSET_500_SERIES_TIGER_POINT;
If the SPI guide is correct and I followed it, we would end up on this path. […]
Yes, I was starring at this code back than for a while. Your approach seems to be OK, at least I can see the value 0x6c for EHL in the docs. Will change accordingly.
https://review.coreboot.org/c/flashrom/+/60711/comment/ebde677d_075a53a5 PS1, Line 1055: case CHIPSET_GEMINI_LAKE:
EHL here?
With the change above, yes. Will add.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/7dc2342f_734c966d PS1, Line 452: pprint_reg(HSFS, FLOCKDN, reg_val, "\n");
Looks like there are several new SAF (slave-attached flash?) bits. We […]
I have checked the printed bits and they do match the docs.
https://review.coreboot.org/c/flashrom/+/60711/comment/b5b70256_9367a620 PS1, Line 464: case CHIPSET_500_SERIES_TIGER_POINT:
Looks like we missed APL/GLK here? EHL should go here too it seems.
Cheked the bits in the register and they do macth with EHL. Will add.
https://review.coreboot.org/c/flashrom/+/60711/comment/74b2b55c_ad1d4abd PS1, Line 1774: case CHIPSET_ELKHART_LAKE:
This is tricky, the additional regs are at 0xe0. So it actually has 16.
Oh yes, I wasn't aware. Will shift back to 16 (had it in the beginning).
https://review.coreboot.org/c/flashrom/+/60711/comment/21fe4c4d_4430a4c0 PS1, Line 2027: ich_gen == CHIPSET_ELKHART_LAKE)) {
Hmmm, would fit better below. Or maybe just merge the two if's. The idea […]
If moved below, something like this? msg_pdbg("Enabling hardware sequencing by default for Apollo/Gemini/Elkhart Lake.\n");
-
Werner Zeh (Code Review)