Attention is currently required from: Michał Żygowski, Jakub Czapiga, Karol Zmyslowski, Stefan Reinauer, Michal Zygowski.
Maciej Pijanowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73934 )
Change subject: util/inteltool: Add support for Jasper Lake ......................................................................
Patch Set 12: Code-Review-1
(23 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/73934/comment/4f1e42f2_543e885d PS9, Line 10: Documents: Intel Datasheet: 633935, rev. 006
Done
Done
Patchset:
PS12: Still, not all old comments were resolved. New fixes and improvements came with new bugs.
I confirmed with datashseet, and found quite a lot of errors, or at least something to explain. Please do.
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/6dc47560_35c824d4 PS12, Line 8: const char *const jasperlake_pch_group_a_names[] = { This group is not used in any of the communities.
https://review.coreboot.org/c/coreboot/+/73934/comment/bbfee5cc_d775712b PS12, Line 28: "GPP_A19", "PCHHOT_N", "n/a", After A19, we have ESPI_CLK_LOOPBK in datasheet
https://review.coreboot.org/c/coreboot/+/73934/comment/0377aeaf_ddd9114b PS12, Line 63: Please remove empty line
https://review.coreboot.org/c/coreboot/+/73934/comment/abba133d_02526e92 PS12, Line 64: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK", "n/a", "n/a", Inconsistent indents in this group after adding LOOBPKG entries
https://review.coreboot.org/c/coreboot/+/73934/comment/3f7cc3b8_7e2a48db PS12, Line 214: "GPP_F04", "CNV_RF_RESET_N", Can you explain, why do we start from F04? In datasheet I can see it starts from F00.
https://review.coreboot.org/c/coreboot/+/73934/comment/87c901a3_3b96dfc0 PS12, Line 226: "GPP_F18", "EMMC_RESET_N", Can you explain, why do we stop at F18? In datasheet I can see F19 as well.
https://review.coreboot.org/c/coreboot/+/73934/comment/ee7ac0fc_65b4bdac PS12, Line 274: "GPP_H19", "n/a", "n/a", "n/a", Don't we have up to H23 in datasheet?
https://review.coreboot.org/c/coreboot/+/73934/comment/3d2c502d_f444aa14 PS12, Line 308: const struct gpio_group jasperlake_pch_group_spi0 = { We have "jasperlake_pch_group_spi_names` but group is called: "jasperlake_pch_group_spi0"... Why? Please be consistent and use "jasperlake_pch_group_spi"
https://review.coreboot.org/c/coreboot/+/73934/comment/9b19c998_b026e0fe PS12, Line 458: const struct gpio_group jasperlake_pch_group_sys_pcie = { Again, lack of consistency... if we have "jasperlake_pch_group_pcie_vgpio_names", we should have: "jasperlake_pch_group_pcie_vgpio"
https://review.coreboot.org/c/coreboot/+/73934/comment/445fed79_b3726de6 PS12, Line 480: const struct gpio_group jasperlake_pch_group_sys_vusb = { Change to: "jasperlake_pch_group_vgpio_usb"
https://review.coreboot.org/c/coreboot/+/73934/comment/61bd3222_8d3e0deb PS12, Line 539: // Deep Sleep Well group Not sure how relevant this comment is. Definitely empty line is missing here.
https://review.coreboot.org/c/coreboot/+/73934/comment/17e44080_e320263c PS12, Line 552: Why empty line?
https://review.coreboot.org/c/coreboot/+/73934/comment/7c1220e8_9d124d1d PS12, Line 553: "GPD_INPUT3VSEL", "GPD_INPUT3VSEL", You broke indents in this group.
https://review.coreboot.org/c/coreboot/+/73934/comment/e8bef4ba_1304e5d7 PS12, Line 584: Sometimes 0, sometimes 1, sometimes 2 empty lines after this comment... This is mess. Please consistent.
https://review.coreboot.org/c/coreboot/+/73934/comment/230407fe_afeed922 PS12, Line 657: &jasperlake_pch_group_b, group_a is missing after group_b in this community
https://review.coreboot.org/c/coreboot/+/73934/comment/4c33b4b5_e676fc49 PS12, Line 660: empty line?
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/d2e21bab_12d697e6 PS9, Line 140: const char *const jasperlake_pch_community1_sys_names[] = {
For example, ADL-P defines almost the same group as: "alderlake_pch_p_group_hvcmos_names". […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/23f4acec_2037af63 PS9, Line 281: "GSPI2_CLK_LOOPBK", "GSPI2_CLK_LOOPBK",
I still don't see this being changed
Again, it is still not changed. It should go to group D, if I see correctly.
https://review.coreboot.org/c/coreboot/+/73934/comment/7c6232a7_438940d5 PS9, Line 312: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK",
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/6fbb3ad1_102aefba PS9, Line 317: .display = "------- GPIO Group GPP_B -------",
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/cfa332ef_b36ebb8e PS9, Line 359: const char *const jasperlake_pch_group_sys_comm2_names[] = {
Done
Done