David Hendricks 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:
(5 comments)
https://review.coreboot.org/#/c/20922/2/chipset_enable.c File chipset_enable.c:
https://review.coreboot.org/#/c/20922/2/chipset_enable.c@836 PS2, Line 836: or_
s/and/or/? (ich you read the identifier as a statement...)
Done
https://review.coreboot.org/#/c/20922/2/ich_descriptors.c File ich_descriptors.c:
https://review.coreboot.org/#/c/20922/2/ich_descriptors.c@76 PS2, Line 76: default: : if (cont->N
You could remove both else paths and just return -1 at the end.
Done.
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.
Done.
BTW - I wonder if we should change most of these `if (ich_generation == ...)` statements to `switch (ich_generation)`. That would solve any indentation issue now and moving forward, and would also be more uniform with other parts of the ichspi and ich_desctriptors code that have to deal with multiple generations.
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 */
10 is correct, the registers read with 0x00007fff just what is set
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?
LMK your thoughts on how to proceed from here.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1704 PS2, Line 1704: 6; /* Includes GPR0 */
6 including the GPR0 register, sorry should have added a comment...
Ah, that makes sense. I went ahead and added a comment for Sunrise Point and Lewisburg. ICH8/9 don't have a GPR register, so 5 is already correct to account for PR0-PR4.
-
David Hendricks (Code Review)