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 16:
(17 comments)
https://review.coreboot.org/c/coreboot/+/37427/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37427/16//COMMIT_MSG@9 PS16, Line 9: tigerlake
Tiger Lake
Ack
https://review.coreboot.org/c/coreboot/+/37427/16//COMMIT_MSG@13 PS16, Line 13:
Once you have verified with the kernel driver, I would recommend adding all the details about what k […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 19:
nit: extra blank line not required.
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 33: GPIO_IRQ14
Along with this, you also need to set FSP param GpioIrqRoute.
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 107: Local2 = PCRB (Local0) : Local2 += PAD_CFG_BASE
This and the next match can be done in a single line? […]
Will check.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 109: /* Return (Add (Local2, Multiply (Local1, 16))) */
Commented code can be removed.
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 124: >>=
I don't think this is correct. […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 129:
1 extra blank line not required.
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 160:
2 extra blank lines not required.
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/gp... File src/soc/intel/tigerlake/gpio.c:
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/gp... PS16, Line 94: .gpi_int_sts_reg_0 = GPI_INT_STS_0, : .gpi_int_en_reg_0 = GPI_INT_EN_0,
Why are these removed?
Ack. As mentioned in gpio_def.h i will bring this part back.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/gp... PS16, Line 197: { PMC_GPP_G, GPP_G },
Why was this removed? Is it not possible to map that to GPE?
I dont see these groups in EDS.SO i would guess for this version of silicon its not possible. If in other version of silicon these groups come back we can put this back here with proper indexing.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 268: #define GPI_INT_STS_0 0x100 : #define GPI_INT_EN_0 0x110
I am confused. […]
This is removed in between version 9-10, not sure why.I see we would need these to clear the pms registers if needed. Will bring this one back.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 119: *
space before *
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 35: 26
Why is this set to 26?
This should be the max number of pins in any group we have. but i see VGPIO have 27. so this should be 27 actually.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 232: 171
I am still skeptical about the GPD group numbering here. […]
With the present kernel code i can still see only 67(0-66) pins.I will try with the 5.4 kernel pinctrl driver.But i guess that kernel problem is different from the GPD here as this is com2, if some mismatch hapens atleast kernel should have read the com 0 and 1. but agreed with you regarding this can break the kernel mapping. Will debug more.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 376: TOTAL_GPIO_COMM 5
If we are going with this, I would recommend at least having macros for the communities: […]
Ack
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 379: /*
I don't think we need the pinmux settings. […]
ACK.Will remove this part.
-
Shaunak Saha (Code Review)