Nico Huber has posted comments on this change. ( https://review.coreboot.org/20922 )
Change subject: chispet_enable: Add support for C620-series Lewisburg PCH ......................................................................
Patch Set 3: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/20922/2/ichspi.c File ichspi.c:
https://review.coreboot.org/#/c/20922/2/ichspi.c@398 PS2, Line 398: ich_generation == CHIPSET_C620_SERIES_LEWISBURG) {
Done.
Agreed.
Another option would be to enforce an order in the enum and check for
= CHIPSET_100_SERIES_SUNRISE_POINT. That might work for some time but
it depends on future incompatibilities, of course, when we'll finally have to use a `switch` anyway.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1703 PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */
So should we leave it at 12 to match the datasheet, even though FREG10 and 11 read 0 and are ignored? Should we explain this behavior in the comment?
Sorry, that was confusing. I tested this on Sunrise Point not Lewisburg (don't have the latter). The Sunrise Point datasheet says 6 while 10 registers are implement. I expect Lewisburg to implement 12 like the datasheet suggests.
LMK your thoughts on how to proceed from here.
Current verison looks good. Do you want to trigger Sandy to test it?
-
Nico Huber (Code Review)