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 10:
(15 comments)
Patchset:
PS10: Still not all of my comments were worked on in the patchset #10
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/c52aac35_d4e73834 PS9, Line 190: const char *const jasperlake_pch_community1_jtag_names[] = {
If you see at others, the naming convention is typically: "emmitsburg_group_jtag ", We can drop the […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/5f382d0e_dc15895e PS9, Line 203: .display = "------- GPIO Group GPP_SYS_JTAG -------",
As far as I can see, others uarchs typically use: "------- GPIO Group JTAG -------"
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/a59e8d12_5e29a4d9 PS9, Line 281: "GSPI2_CLK_LOOPBK", "GSPI2_CLK_LOOPBK",
This can probably be included in a group. See alderlake_p.h for example, as a reference.
I still don't see this being changed
https://review.coreboot.org/c/coreboot/+/73934/comment/f313f524_6527dd65 PS9, Line 292: const char *const jasperlake_pch_group_spi0_names[] = {
"jasperlake_pch_group_spi_names" should be enough
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/13024391_2f699c41 PS9, Line 305: .display = "------- GPIO Group GPP_H -------",
This should be: "------- GPIO Group SPI -------"
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/66398fac_cb7d7b8e PS9, Line 312: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK",
Another instance that can probably be included in already existing group, and this group can be then […]
Still not done
https://review.coreboot.org/c/coreboot/+/73934/comment/43650ec9_02de7bda PS9, Line 359: const char *const jasperlake_pch_group_sys_comm2_names[] = {
This should be "jasperlake_pch_group_cpu_names". Consult alderlake_h.h as an example.
Still not done
https://review.coreboot.org/c/coreboot/+/73934/comment/22143970_ce9dcfb9 PS9, Line 378: .display = "------- GPIO Group GPP_SYS_COMM2 -------",
"------- GPIO Group CPU -------"
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/cef8887e_6a475d62 PS9, Line 384: const char *const jasperlake_pch_group_sys_pcie_names[] = {
"jasperlake_pch_group_pcie_vgpio_names" fits probably better
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/b8a6fe99_2f8892c1 PS9, Line 467: .display = "------- GPIO Group GPP_SYS_PCIE -------",
"------- GPIO Group PCIe vGPIO -------"
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/f4bd0b18_104d9f0e PS9, Line 473: const char *const jasperlake_pch_group_sys_vusb_names[] = {
maybe "jasperlake_pch_group_vgpio_usb_names", or similar
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/498fe06e_6f7239f0 PS9, Line 496: const char *const jasperlake_pch_group_dsw_names[] = {
This should be: "jasperlake_pch_group_gpd_names"
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/81dd3996_e4269799 PS9, Line 510: const struct gpio_group jasperlake_pch_group_dsw = {
This should be: jasperlake_pch_group_gpd
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/3d9f3fa4_0892a647 PS9, Line 511: .display = "------- GPIO Group GPP_S -------",
This should be: "------- GPIO Group GPD -------:
Done
-
Maciej Pijanowski (Code Review)