Attention is currently required from: Michał Żygowski, Paul Menzel. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 )
Change subject: Add Tiger Lake U Premium support ......................................................................
Patch Set 5: Code-Review+1
(4 comments)
Patchset:
PS5: Thanks for also looking at the registers. I guess we could add a TODO for earlier chipsets that support eSPI. Currently we might add LPC/FWH buses by accident for them. For these transition chipsets, we should probably check a strap if it's using eSPI or LPC.
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/55578/comment/bd00365c_e3ce128c PS3, Line 659: { "eSPI", BUS_LPC | BUS_FWH } };
Not sure about this one, whether eSPI should be mapped to LPC and FWH
No, it shouldn't. We use these flags to decide which chips to probe for, and I guess even if an eSPI chip might work similarly (IIRC, LPC/FWH just had memory mapped registers) it would be a different set of chips (does anybody make/use eSPI NOR flash?).
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55578/comment/72719007_5b975178 PS5, Line 278: return "reserved"; I'd suggest "unknown" if we don't want to add a table above. Note, that the call to this function for `freq_read` is spurious since, I guess, Cannon Point and should probably be guarded.
https://review.coreboot.org/c/flashrom/+/55578/comment/836940d1_92c2adbf PS5, Line 967: return CHIPSET_500_SERIES_TIGER_POINT; As suggested somewhere else, I would make this the new fall-back, i.e.
if (ICCRIBA == 0x34) return CANNON_POINT; if (ICCRIBA != 0x11) msg_pwarn(...); return TIGER_POINT;
That should make things more future-proof.
Calling it ICCRIBA for Tiger Point is wrong ofc. but that's just cosmetics. We could add another union in `ich_descriptors.h`.
-
Nico Huber (Code Review)