Angel Pons 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: Code-Review+1
(18 comments)
Some comments, but looks good overall.
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:
Add definitions for the GPIO pins on Alder Lake LP, as well as GPIO IRQ routing information and supporting ACPI ASL.
For now, add the following 5 GPIO communities and 13 GPIO groups:
Comm. 0: GPP_B, GPP_T, GPP_A Comm. 1: GPP_S, GPP_H, GPP_D Comm. 2: GPD Comm. 4: GPP_C, GPP_F, GPP_E, GPP_HVCMOS Comm. 5: GPP_R, GPP_SPI
I assume `GPP_HVCMOD` is a typo? 😊
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
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
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 66: static const struct pad_community adl_communities[] = { Looks like these GPIO definitions are specific to ADL PCH-LP. Maybe we want to support PCH-S someday?
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 69: GPP_B0 GPIO_COM0_START
Same for all communities
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/gpi... PS2, Line 70: GPP_ESPI_CLK_LOOPBK GPIO_COM0_END
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)
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 70: GPP_T11IRQ missing a _
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.
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, the other GPIO groups would be off by one.
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?
In any case, Group U consists of 24 pins: - GPP_U0 .. GPP_U19 (20 pins) - GSPI{3,4,5,6}_CLK_LOOPBACK (4 pins)
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?
Also, HVCMOS = 0xd seems to be missing.
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 174: nit: a single space is enough
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
keep them in-line with cnl, where GPD_ prefix was skipped? same for the others. […]
I doubt CNL is a good example on how to do things. We had a problem with a macro named `PECI`, didn't we?
In any case, it would be nice to use a prefix to namespace these GPIO definitions. But this would need to be done on all platforms.
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
these two names are confusing, since there can't be two MOSI. […]
I don't think the proposed renaming is a good idea. When in Dual or Quad I/O mode, these pins are actually IO_0 and IO_1. If anything, the whole SPI group should be renamed, but if this is the only SPI group, it doesn't matter.
In any case, looks like IO_1 should be MISO.
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
SPI_CS0_B […]
`CSx` makes it harder to differentiate the macros. Having the numbers separated eases distinction. IMHO, I'd stick to what the EDS says.
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
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 309:
these are not part of SoC EDS chapter as i have added above, hence don't expect BIOS to configure.
I think we would want to configure some of these GPIOs someday. But since ADL is not released yet, I don't mind if the definitions get added once it is.
Michael: I can't see Azalia in the GPIO group list, but looks like these pins exist in the CPU group. Also, I imagine the kernel doesn't have definitions for ADL GPIOs because ADL is not released yet.