Attention is currently required from: Anil Kumar K, Bora Guvendik, Elyes Haouas, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Subrata Banik, Wonkyu Kim.
Cliff Huang 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 61:
(8 comments)
File src/soc/intel/pantherlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/83789/comment/33fcbc94_2bd5c2ff?usp... : PS55, Line 60: /* Device Properties for _DSD */,
don't need this. […]
These comments are generated from .asl to .dsl file by iasl. The template that we started with gpio.asl has these comments. Same thing to the comment on line 58.
https://review.coreboot.org/c/coreboot/+/83789/comment/9ffa074b_9cc83e8a?usp... : PS55, Line 84: 0x10
can you please help me to understand what this field actually implies to ?
I checked with pinctrl owner, we will remove this package. The kernel pinctrl driver does not use it.
https://review.coreboot.org/c/coreboot/+/83789/comment/feb8619c_a8c3832c?usp... : PS55, Line 135: GPPV
may be worth adding a comment saying, this section explains each GPIO bank, for example: GPIO_0 COM […]
Acknowledged
File src/soc/intel/pantherlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/d95621f0_189b4d3f?usp... : PS49, Line 260: GPP_E_OFFSET
I don't see GPP_E0 in the EDS at offset 0x9a0. […]
Subrata,
I've asked to fix this a while back. and it got fixed in July. Can you check newer release of EDS?
https://review.coreboot.org/c/coreboot/+/83789/comment/3064067e_5e7f15e3?usp... : PS49, Line 261: GPP_CPUJTAG_OFFSET
do we need this macro?
This is used in gpio.asl.
File src/soc/intel/pantherlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/e84ec45a_1bc44880?usp... : PS49, Line 31: 1
INC(GPP_COMM0_ID)
We don't have COMM2, so GPP_COMM3 cannot use this macro, though.
https://review.coreboot.org/c/coreboot/+/83789/comment/76c89335_9a39113e?usp... : PS49, Line 36: #define GPP_COMM0_NAME "Community0" : #define GPP_COMM1_NAME "Community1" : #define GPP_COMM3_NAME "Community3" : #define GPP_COMM4_NAME "Community4" : #define GPP_COMM5_NAME "Community5" : : #define GPP_V_NAME "GPP_V" : #define GPP_C_NAME "GPP_C" : #define GPP_F_NAME "GPP_F" : #define GPP_E_NAME "GPP_E" : #define GPP_CPUJTAG_NAME "GPUJTAG" : #define GPP_H_NAME "GPP_H" : #define GPP_A_NAME "GPP_A" : #define GPP_VGPIO3_NAME "vGPIO_3" : #define GPP_S_NAME "GPP_S" : #define GPP_B_NAME "GPP_B" : #define GPP_D_NAME "GPP_D" : #define GPP_VGPIO_NAME "vGPIO" :
move macro definition into GPIO. […]
This are used in gpio.asl. gpio.h is not included directly in gpio.asl. Let me try.
https://review.coreboot.org/c/coreboot/+/83789/comment/0fa6149f_b1df215f?usp... : PS49, Line 250: #define NUM_COM1_GPP_GROUPS 2
I don't see any consumer for NUM_COMx_GPP_GROUPS macro
This marco is does not count the non GPP_[grp]_[num] group in a community. Initially, these are used for a KCONFIG flag not to add non GPP_[grp]_[pad] in GPIO asl. I can remove these defines.