Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37427 )
Change subject: soc/intel/tigerlake: Update GPIO config ......................................................................
Patch Set 12:
(7 comments)
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@13 PS12, Line 13:
Yes. But there is a problem.With that kernel the it only list the first 67 pins(group 0). […]
I don't understand what you mean by "list the first 67 pins". Have you seen the comment about assigning numbers to GPD pins? Did you try the suggestion there?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... File src/soc/intel/tigerlake/gpio.c:
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 169: GPP_B_DW0_1
This is for progarmming the MISCCFG register for each community. […]
In that case why is GPP_B named as GPP_B_DW0_1 and GPP_B_DW2?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 26: * DW0,1 and DW2. Based on this the devicetree should assign them. : * Example of devicetree: : * register "gpe0_dw0" = "GPP_C_DW0_1" # GPP_C : * register "gpe0_dw1" = "GPP_D_DW0_1" # GPP_D : * register "gpe0_dw2" = "GPP_E_DW2" # GPP_E
OK. I will try to implement this in soc specific driver. I don't want to change the common code.
I would recommend thinking if this can be solved in a generic way across Intel platforms. Reason is because we tend to copy-paste mainboard code from older to newer boards and in general gpe0_dw0, gpe0_dw1 and gpe0_dw2 naming remains consistent. It would be good if we can establish what those really mean i.e. GPE0_DWx configuration in PMC, GPIO communities or a separate coreboot-specific naming.
For TGL and effectively for JSL, you would be going with the third option i.e. coreboot specific naming.
--> GPP_A ... GPP_* : Coreboot-specific numbering scheme --> Common code when configuring GPIO_CFG : Queries for gpe0_dwx which SoC can adjust based on the DWx where the mapping is being done. This is basically "coreboot-specific numbering" to "PMC identifiable numbering". --> Common/SoC code when configuring GPIO community DWx: Queries for gpe0_dwx which SoC can adjust based on DWx where the mapping is being done. This is basically "coreboot-specific numbering" to "GPIO community identifiable numbering". In the future, if the numbering for each community for each DWx is different, that can be handled here too.
Depending upon SoC implementation, all, few or none of the mappings above can be identity mapping. But, the common code would remain the same.
BTW, before jumping into this implementation, have you verified with the EDS author that the GPE0_DWx numbering for the PMC in GPIO_CFG register is actually different for different DWx and not a typo in the document? Also, have you verified by doing various configurations in mainboard whether the GPE_STS registers in PMC get updated as per your expectation for DW0, DW1 and DW2?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 225: GPP_VGPIO0
Kernel is using native functions as the name here. […]
Yeah, that might make it easier to follow what the GPIOs are. Or maybe add a comment here indicating what the vGPIO is?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 374: #define GPP_MLK 277
I got this from the excel sheet shared by the hardware team. […]
If this is something that is present in hardware, I would recommend working with the kernel team to see if this was intentionally dropped there or just a miss. Ideally it would be better to update the kernel driver to handle it.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 401: 5
Yes. Because of the missing community i kept the total number as 5.
But, then your assignment for the properties of the communities are all messed up? BTW, why is GPIOCOM3 missing?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 118: #define PMC_GPP_B 0x0 : #define PMC_GPP_T 0x1 : #define PMC_GPP_A 0x2 : #define PMC_GPP_R 0x3 : #define PMC_GPD 0x4 : #define PMC_GPP_S 0x5 : #define PMC_GPP_H 0x6 : #define PMC_GPP_D 0x7 : #define PMC_GPP_U 0x8 : #define PMC_GPP_F 0xA : #define PMC_GPP_C 0xB : #define PMC_GPP_E 0xC
My understanding was this is used for programming MISCCFG. […]
These are configured in GPIO_CFG register in PMC: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...