Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Wonkyu Kim.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83789?usp=email )
Change subject: soc/intel/ptl: Add GPIOs for Panther Lake SOC ......................................................................
Patch Set 28:
(6 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83789/comment/be5f23fd_0b8b6557?usp... : PS24, Line 261: config SOC_INTEL_ACPI_GPIO_PINCTRL_COMPACT
when true, only GPP_[group]_[n] pads will be included in pincrtl. Currently, we follow what BIOS is doing, all PADs.
I'd like to know more about the rationale behind making this config true. If I understand your response correctly, this is a new requirement for LNL onwards SoC, so all PTL SOCs should default to enabling this Kconfig. I can't think of any case where we wouldn't want to select this Kconfig, so I'm wondering if we still need to use a Kconfig if the default behavior is true for PTL SoCs.
File src/soc/intel/pantherlake/gpio.c:
https://review.coreboot.org/c/coreboot/+/83789/comment/42955c9f_97b0d6c4?usp... : PS17, Line 38: GPP_ASPIO
The GPIO group names come from Intel's GPIO HAS document. GPP_ASPI0 contains GPP_A and SPI0 pads. There are 10 SPI0 pads after GPP_A pads in the same group. Please also see the gpio_soc_defs.h for the complete pad list, although most of these non GPP_[group]_[number] pads are not listed in EDS.
Allow me to elaborate.
If you are stating that you have added a comment that says `GPP_ASPIO` because the GPIO bank has both GPP_A and GPP_SPI0, then I am in agreement. However, you must also change line #37 to reflect both GPP_H and GPP_ISH. In that case, the name should be `GPP_HISH`. The same principle applies to lines #31-32 and many other GPIO banks as well.
Therefore, for the sake of simplicity, please only use meaningful GPIO PAD names (and not the native functions ones).
https://review.coreboot.org/c/coreboot/+/83789/comment/ab5af0ea_552e19c1?usp... : PS17, Line 112: GPP_CPUJTAG_H_ASPIO_VGPIO3
ASPI0 group is the GPP_A plus the SPI0 pads
In the same logic, why did you skip ISH, which is part of GPIO community 3 along with GPP_H? It appears that you are not using a consistent approach that applies to everyone.
https://review.coreboot.org/c/coreboot/+/83789/comment/4506475a_52ef645f?usp... : PS17, Line 177: { PMC_GPP_E, GPP_E },
will modify the this order accordingly. GPP_A (GPP_ASPI0 internally) was missing and added in EDS recently.
Please refrain from repeating yourself. We can only program GPP_A as GPE, and we will not be able to program SPI0 as GPE. As a result, adding GPP_ASPI0 does not make sense. Please look at EDS vol 2, GPIO section offset 10 (MISC register). I do not see any PAD name as GPP_ASPI0 there.
File src/soc/intel/pantherlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/9e46d0a9_d39c1066?usp... : PS28, Line 275: drop one empty line
File src/soc/intel/pantherlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/4f1799d4_e62dd38c?usp... : PS28, Line 174: /* 48 */ Do you genuinely require these comments? I believe that lines #164 and #171 are sufficient for anyone to comprehend the beginning and end PAS counts. One can simply count from the beginning of the PAD number to determine the index of GPP_F05. I found that the comments made it too unwieldy.
-
Subrata Banik (Code Review)