Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37427 )
Change subject: soc/intel/tigerlake: Update GPIO config ......................................................................
Patch Set 13:
(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:
I don't understand what you mean by "list the first 67 pins". […]
Yes. I did that. Even removing the GPD from the asl code i m seeing only 67 pins listed. But with the older version of kernel pinctrl driver that problem was not there. I checked with the kernel guys. Even they are saying that they will go back to older kernel pinctrl driver code.
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
In that case why is GPP_B named as GPP_B_DW0_1 and GPP_B_DW2?
I m not able to find where m using DW2 for PMC mapping here. Can you please let me know the line number where DW2 is used.
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
I would recommend thinking if this can be solved in a generic way across Intel platforms. […]
Working on an implementation you suggested. I am trying to find who is the proper owner for that specific section of GPIO in the EDS to confirm if EDS is right. I had verified
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 225: GPP_VGPIO0
Yeah, that might make it easier to follow what the GPIOs are. […]
Will use the native function names.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 374: #define GPP_MLK 277
If this is something that is present in hardware, I would recommend working with the kernel team to […]
I had asked kernel team. Waiting for their reply.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 401: 5
But, then your assignment for the properties of the communities are all messed up? BTW, why is GPIOC […]
As per my understanding : I m not seeing any Community 3 in the EDS. TOTAL_GPIO_COMM is only used here :soc_fill_gpio_pm_configuration. From that function gpio_pm_configure is called.gpio_pm_configure calls soc_gpio_get_community which should return the array size as 5 from gpio.c file.So i kept it here as 5 to keep the number same as the array size in gpio.c or else there would be error in "src/soc/intel/common/block/gpio/gpio.c" line#622. Please correct me if i am wrong.
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
These are configured in GPIO_CFG register in PMC: https://review.coreboot.org/cgit/coreboot. […]
As per my understanding in pmclib.c line#554 the gpio_cfg register is written with the values that comes directly from the devicetree(In devicetree with the current implementation we are using different config for DW2 and DW0/DW1). After that in in line#557 gpio_route_gpe(dw0, dw1, dw2); is called which in turn calls gpio_route_pmc_gpio_gpe->soc_pmc_gpio_routes->soc_pmc_gpio_routes where the pmc_to_gpio_route is called. This is used to write to the MISCCFG register. So i kept the pmc_to_gpio_route array as per the DW0,DW1 config of GPIO_CFG.
-
Shaunak Saha (Code Review)