Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31500 )
Change subject: inteltool: add 300 and C240 Series Northbridge (Coffeelake) ......................................................................
Patch Set 3:
(23 comments)
I'm not sure if it's worth to add the extra groups only known through coreboot. If the datasheet doesn't mention them, it's likely that they are not useful in regular operation (i.e. current hw revision and not debugging things).
The GPIO_RSVD_* entries that fill gaps between the existing groups (e.g. a/b, i/j), however, have to be added.
https://review.coreboot.org/#/c/31500/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31500/3//COMMIT_MSG@7 PS3, Line 7: Northbridge s/Northbridge/PCH/ ? if any bridge, it would be a southbridge.
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c File util/inteltool/gpio_groups.c:
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1197 PS3, Line 1197: VDD VDD2
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1209 PS3, Line 1209: "GPP_A23", "ISH_GP5", "n/a", "n/a", "n/a", missing GPIO_RSVD_0 (coreboot notion, see soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h:69)
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1210 PS3, Line 1210: }; When all function 4 values are "n/a" you can leave it out and reduce the `func_count` below.
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1222 PS3, Line 1222: VRALRERT VRALERT#
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1243 PS3, Line 1243: "GPP_B23", "SML1ALERT#", "PCHHOT#", "n/a", "n/a", missing GPIO_RSVD_1, GPIO_RSVD_2 only needs three columns, i.e. `func_count = 3`
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1278 PS3, Line 1278: }; `func_count = 3` ?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1291 PS3, Line 1291: SPI2 SPI1
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1292 PS3, Line 1292: I2C2SDA I2C2_SDA
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1294 PS3, Line 1294: modem MODEM
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1303 PS3, Line 1303: GSPI GSPI2
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1334 PS3, Line 1334: "GPP_E12", "USB2_OC3#", "n/a", "n/a", "n/a", `func_count = 3` ?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1368 PS3, Line 1368: "GPP_F23", "DDPF_CTRLDATA", "n/a", "n/a", "n/a", `func_count = 3` ?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1369 PS3, Line 1369: drop empty line here
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1370 PS3, Line 1370: }; coreboot has a "SPI" group after F: GPIO_RSVD_11..19
and not accounted groups "CPU" and "JTAG"
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1383 PS3, Line 1383: DATRA2 DATA2
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1387 PS3, Line 1387: "GPP_G7", "SD_WP", "n/a", "n/a", "n/a", coreboot treats these as a separate group (AZA) after G: GPIO_RSVD_3..10
... and also groups "VGPIO_0" and "VGPIO_1", hmmm
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1388 PS3, Line 1388: }; `func_count = 2`?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1421 PS3, Line 1421: "GPP_H23", "TIME_SYNC0", "n/a", "n/a", "n/a", `func_count = 2` ?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1446 PS3, Line 1446: "GPP_I14", "M2_SKT2_CFG3", "n/a", "n/a", "n/a", `func_count = 3` ?
+GPIO_RSVD_40..42
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1468 PS3, Line 1468: "GPP_J11", "A4WP_PRESENT", "n/a", "n/a", "n/a", `func_count = 3` ?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1502 PS3, Line 1502: "GPP_K23", "IMGCLKOUT1", "n/a", "n/a", "n/a", `func_count = 2` ?
https://review.coreboot.org/#/c/31500/3/util/inteltool/gpio_groups.c@1524 PS3, Line 1524: "GPD11", "LANPHYPC", "n/a", "n/a", "n/a", `func_count = 2` ?