Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45571 )
Change subject: soc/intel/alderlake: Add GPIOs for Alder Lake SOC ......................................................................
Patch Set 2:
(20 comments)
https://review.coreboot.org/c/coreboot/+/45571/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45571/2//COMMIT_MSG@9 PS2, Line 9: List of changes: : : 1. This implementation add the GPIO pins, communities and : group mapping. : 2. Add 5 GPIO community includes 13 GPIO groups : GPIO COM 0 : GPP_B, GPP_T, GPP_A : GPIO COM 1 : GPP_S, GPP_H, GPP_D : GPIO COM 2 : GPD : GPIO COM 4 : GPP_C, GPP_F, GPP_E, GPP_HVCMOD : GPIO COM 5 : GPP_R, GPP_SPI : 3. Add GPIO IRQ routing. : 4. Add gpio.asl for ADL GPIO community.
Instead of having a list of changes, I'd say the following: […]
Thanks for suggestion.
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/acp... PS2, Line 89: 05
5
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... File src/soc/intel/alderlake/gpio.c:
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 35: *
nit: trailing blank line in comment
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 69: GPP_B0
GPIO_COM0_START […]
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 70: GPP_ESPI_CLK_LOOPBK
GPIO_COM0_END
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 177: GPD
nit: line up with the others
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 14: (ALIGN_UP((n), GPIO_MAX_NUM_PER_GROUP) / GPIO_MAX_NUM_PER_GROUP)
DIV_ROUND_UP((n), GPIO_MAX_NUM_PER_GROUP)
src/commonlib/bsd/include/commonlib/bsd/helpers.h:57:28: error: braced-group within expression allowed only inside a function 57 | #define DIV_ROUND_UP(x, y) ({ \ | ^ src/soc/intel/alderlake/include/soc/gpio_defs_lp.h:14:35: note: in expansion of macro 'DIV_ROUND_UP' 14 | #define NUM_GPIO_COMx_GPI_REGS(n) DIV_ROUND_UP((n), GPIO_MAX_NUM_PER_GROUP)
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 70: GPP_T11IRQ
missing a _
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 235: : /* Group E */ : #define GPP_E0_IRQ 0x26 : #define GPP_E1_IRQ 0x27 : #define GPP_E2_IRQ 0x28 : #define GPP_E3_IRQ 0x29 : #define GPP_E4_IRQ 0x30 : #define GPP_E5_IRQ 0x31 : #define GPP_E6_IRQ 0x32 : #define GPP_E7_IRQ 0x33 : #define GPP_E8_IRQ 0x34 : #define GPP_E9_IRQ 0x35 : #define GPP_E10_IRQ 0x36 : #define GPP_E11_IRQ 0x37 : #define GPP_E12_IRQ 0x38 : #define GPP_E13_IRQ 0x39 : #define GPP_E14_IRQ 0x3A : #define GPP_E15_IRQ 0x3B : #define GPP_E16_IRQ 0x3C : #define GPP_E17_IRQ 0x3D : #define GPP_E18_IRQ 0x3E : #define GPP_E19_IRQ 0x3F : #define GPP_E20_IRQ 0x40 : #define GPP_E21_IRQ 0x41 : #define GPP_E22_IRQ 0x42 : #define GPP_E23_IRQ 0x43
can you move this between D and F?
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/gpio_soc_defs.h:
PS2:
If we don't need to use these definitions in ACPI, we could use enums instead.
we are using in ACPI 😞
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 13: #define GPP_R 0x3
The SPI group is mentioned in the commit message, but it's not here? Also, isn't SPI = 0x4? If so, t […]
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 18: GPP_U
hmm, I don't see any other definitions for Group U? […]
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 19: #define GPP_F 0xA : #define GPP_C 0xB
I think these two are backwards. GPP_C = 0xb, but isn't GPP_F = 0xc? […]
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 23: 27
I don't see more than 24
Ack, its 26 at max due to GPP_B (0 to 25)
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 174:
nit: a single space is enough
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 189: __
one _
Ack
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 187: GPD_INPUT3VSEL 132 : #define GPD_SLP_LANB 133 : #define GPD__SLP_SUSB 134
right, we *had* a problem; it's merged... […]
Right now TGL and JSL has GPD_ prefix. marking resolve, let me know if i need to remove those ?
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 302: #define GPP_SPI_MOSI_IO_0 227 : #define GPP_SPI_MOSI_IO_1 22
The names _MOSI_ are wrong in EDS and FSP already, where those names come from. […]
Added the exact name matching schematics
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 305: PI_FLASH_0_CSB 230 : #define GPP_SPI_FLASH_1_CSB
Same as above; yes, it makes it harder to differentiate, however, I'd keep the "real" names of the p […]
Added the exact name matching schematics
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 308: GPP_CLK_LOOPBK
nit: missing the `SPI_` part: GPP_SPI_CLK_LOOPBK
This PIN in not there, removed it now