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 2:
(5 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) { Please don't align the indendation with the body.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1557 PS2, Line 1557: (((ICH_BRRA(frap) >> i) & 1) << 0); Overflow for i >= 8 (have a fix in progress somewhere).
https://review.coreboot.org/#/c/20922/2/ichspi.c@1600 PS2, Line 1600: /* From 5 on we have GPR registers and start from 0 again. */ Oh, here's the comment ;)
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 */ Depending on what the regular PCH returns for the register addresses, 12 might not hurt there either. I'm not sure any more why I set it to 10, maybe based on some observation rather than the datasheet... in the end, we only dump the registers anyway.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1704 PS2, Line 1704: 6; /* FIXME: should be 5? */
num_pr = 6 seemed wrong for both Sunrise Point and Lewisburg, but maybe I a
6 including the GPR0 register, sorry should have added a comment...