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 12:
(28 comments)
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@10 PS12, Line 10: GPIO's
GPIOs
Ack
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@13 PS12, Line 13:
I am looking at this as reference: https://github. […]
Yes. But there is a problem.With that kernel the it only list the first 67 pins(group 0). But as far as the group ordering is concerned this should be fine.
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@14 PS12, Line 14: none
b:144680462
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 78: LAnd (LGreaterEqual (Arg0, GPP_B0), LLessEqual (Arg0, ESPI_CLK_LOOPBK))
It would be better to stick to ASL2.0 syntax as it was before. […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 101: *
space before *
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 101: 05
"5" to keep the naming consistent.
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 140: And (GPIOTXSTATE_MASK, VAL0, Local0)
Can you please use ASL2. […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 159:
extra blank lines
Ack
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 20: * Chapter number: 27
Was the chapter number dropped intentionally?
It was a mistake.Will add.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 87: [COMM_0]
Why were these dropped? Because of this, now you would be setting the properties of community 4 to [ […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 94: gpi_int_sts_reg_0 = GPI_INT_STS_0, : .gpi_int_en_reg_0 = GPI_INT_EN_0,
Why were these fields dropped?
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 44: INTEL_GPP_BASE
Why do we need INTEL_GPP_BASE for TGL? I think it should be perfectly fine to use INTEL_GPP instead […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 45: INTEL_GPP(GPP_B0, GSPI0_CLK_LOOPBK, GSPI1_CLK_LOOPBK)
Can't these be clubbed with the above entry? […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 51: INTEL_GPP_BASE
Same question about using INTEL_GPP instead of INTEL_GPP_BASE for all entries here.
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 93: S,D,H,U,VGPIO
nit: space after comma
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/gp... PS12, Line 169: GPP_B_DW0_1
Why are all the entries here mapping to DW0_1 only? What happens if a mapping for DW2 is required?
This is for progarmming the MISCCFG register for each community. As per EDS the mapping in MISCCFG is same as the Dw0,Dw1 in GPIO_CFG. So i kept it like this here.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 268: #define GPI_INT_STS_0 0x100 : #define GPI_INT_EN_0 0x110
Why were these dropped?
Will check. I just rebased on top of patchset 11.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 130: Group D
This is not GPP Group D.
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 255: iIRQ
extra i
Ack
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
Do we really need to do that? Is it possible for us to have a mapping scheme such that the devicetre […]
OK. I will try to implement this in soc specific driver. I don't want to change the common code.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 89: GSPI0_CLK_LOOPBK
So, there is no GPP# for this pad? e.g. […]
Will rename this to B24. The name B24 is not there but its ok to name for simplicity.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 135: ESPI_CLK_LOOPBK
No GPP#?
Will rename this to A24. The name B24 is not there but its ok to name for simplicity.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 225: GPP_VGPIO0
Can we use the same names as used by the kernel driver?
Kernel is using native functions as the name here. You are suggesting to use those names here?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 256: #define GPD0 171
This community is not exposed or used by the kernel driver. […]
Checking the kernel and acpi code how they are handled previously.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 369: TSSTB
TRSTB?
Ack
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 374: #define GPP_MLK 277
I don't see this in the kernel driver.
I got this from the excel sheet shared by the hardware team. Will remove this if kernel does not have this pin.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 401: 5
How is this 5? There is GPIOCOM0 - GPIOCOM5 which is a total of 6? Is this because GPIOCOM3 is missi […]
Yes. Because of the missing community i kept the total number as 5.
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
As per EDS version 1, this is not correct. I see PMC GPP values being different based on DWx.
My understanding was this is used for programming MISCCFG. In EDS MISCCFG values are not changing form one DW to another.Can you please confirm.