Attention is currently required from: Maciej Pijanowski, Michał Żygowski, Jakub Czapiga, Stefan Reinauer, Michal Zygowski.
Karol Zmyslowski 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:
(16 comments)
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/b98321cd_0f3e5f7f PS12, Line 8: const char *const jasperlake_pch_group_a_names[] = {
This group is not used in any of the communities.
Done. Corrected, it is already in community_5
https://review.coreboot.org/c/coreboot/+/73934/comment/76f4a9b4_07e49004 PS12, Line 28: "GPP_A19", "PCHHOT_N", "n/a",
After A19, we have ESPI_CLK_LOOPBK in datasheet
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/8ace1eb9_3b830d4f PS12, Line 63:
Please remove empty line
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/2490bd97_56958421 PS12, Line 64: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK", "n/a", "n/a",
Inconsistent indents in this group after adding LOOBPKG entries
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/807e3680_32180de7 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.
Done. I could not find NF1 for this pad, so currently, I left "n/a".
https://review.coreboot.org/c/coreboot/+/73934/comment/cacb30c5_dfd1c935 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.
Done, but I could not find NF1 for this pad.
https://review.coreboot.org/c/coreboot/+/73934/comment/11b40297_7bf31f1f PS12, Line 274: "GPP_H19", "n/a", "n/a", "n/a",
Don't we have up to H23 in datasheet?
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/f6f9be5a_e7624200 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"... […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/048e19f8_fca5d52c PS12, Line 458: const struct gpio_group jasperlake_pch_group_sys_pcie = {
Again, lack of consistency... […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/824deb50_0b56a641 PS12, Line 480: const struct gpio_group jasperlake_pch_group_sys_vusb = {
Change to: "jasperlake_pch_group_vgpio_usb"
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/cabbc635_2b702b81 PS12, Line 539: // Deep Sleep Well group
Not sure how relevant this comment is. […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/8b14c4f2_f44db641 PS12, Line 552:
Why empty line?
Just for visibility. So, I will remove it.
https://review.coreboot.org/c/coreboot/+/73934/comment/00a888e2_aa0e47de PS12, Line 584:
Sometimes 0, sometimes 1, sometimes 2 empty lines after this comment... This is mess. […]
Only for reference with original Intel documentation. Currently, it's a helper comment.
https://review.coreboot.org/c/coreboot/+/73934/comment/11a558c3_d2281f7f PS12, Line 657: &jasperlake_pch_group_b,
group_a is missing after group_b in this community
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/8da821a7_e8cfbf61 PS12, Line 660:
empty line?
Done
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/da7c9fbe_eff6e9e9 PS9, Line 281: "GSPI2_CLK_LOOPBK", "GSPI2_CLK_LOOPBK",
Again, it is still not changed. It should go to group D, if I see correctly.
Done