Attention is currently required from: Werner Zeh. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 )
Change subject: Add Elkhart Lake support ......................................................................
Patch Set 1: Code-Review+1
(10 comments)
Patchset:
PS1: Nice! looks almost ready :)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/d7633cdc_5e8ad909 PS1, Line 709: boot_straps = boot_straps_pch500; The EDS says `bbs == 1` would be "UFS". Not sure how that's connected. Is it related to eSPI?
https://review.coreboot.org/c/flashrom/+/60711/comment/8aa841ba_97c9aebe PS1, Line 995: dev So AFAICS, this is already 1f.5. We could save us the dance to find it (the PCH100 code assumes it is hidden), e.g. extract the code in enable_flash_pch100_or_c620() that works on `spi_dev` into a function that we call here directly.
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/acd5afa7_31fcb7bc PS1, Line 538: IFWI Intel likes to haunt us with this it seems. Now all the non-BIOS firmware is back in the TXE/ME region, but they still call it IFWI. Maybe we should just change this back to BIOS. After all, everybody is calling it the BIOS region.
https://review.coreboot.org/c/flashrom/+/60711/comment/07f104ed_507f4713 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. We can identify EHL by CSSO == 0x6c (please check that in your descriptor), i.e.
if (content->CSSL == 0x11) { if (content->CSSO == 0x68) return CHIPSET_500_SERIES_TIGER_POINT; if (content->CSSO == 0x6c) return CHIPSET_ELKHART_LAKE; }
https://review.coreboot.org/c/flashrom/+/60711/comment/d8037f2d_edc231ea PS1, Line 1055: case CHIPSET_GEMINI_LAKE: EHL here?
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/1c70e168_d2c3a495 PS1, Line 452: pprint_reg(HSFS, FLOCKDN, reg_val, "\n"); Looks like there are several new SAF (slave-attached flash?) bits. We must have missed that when adding APL already. Or actually, missed the whole function :-(
I don't mind leaving them out as long as the printed bits are correct.
https://review.coreboot.org/c/flashrom/+/60711/comment/bb51cd48_b6871c2e PS1, Line 464: case CHIPSET_500_SERIES_TIGER_POINT: Looks like we missed APL/GLK here? EHL should go here too it seems.
https://review.coreboot.org/c/flashrom/+/60711/comment/1b8bf0b8_05ee0633 PS1, Line 1774: case CHIPSET_ELKHART_LAKE: This is tricky, the additional regs are at 0xe0. So it actually has 16.
https://review.coreboot.org/c/flashrom/+/60711/comment/ccdc3fba_8b023875 PS1, Line 2027: ich_gen == CHIPSET_ELKHART_LAKE)) { Hmmm, would fit better below. Or maybe just merge the two if's. The idea was to have an accurate message, but becomes harder with every new chip.