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 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@13 PS16, Line 13: Once you have verified with the kernel driver, I would recommend adding all the details about what kernel driver version was used for testing, what the expectations are, etc.
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.
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.
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? Local2 = PCRB(Local0) + PAD_CFG_BASE + Local1 * 16 Return Local2
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.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 124: >>= I don't think this is correct. This should just be >>
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 124: ( Are the braces around the and ops really required?
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 129: 1 extra blank line not required.
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/ac... PS16, Line 160: 2 extra blank lines not required.
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?
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?
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. Why are these removed?
https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in... PS16, Line 119: * space before *
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?
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:
#define COMM_0 0 #define COMM_1 1 #define COMM_2 2 /* GPIO community 3 is not exposed to be used and hence is skipped. */ #define COMM_4 3 #define COMM_5 4
#define TOTAL_GPIO_COMM (COMM_5 + 1)
And then use these COMM_* macros in gpio.c as index for the various communities. This will ensure that if we switch to using COMM_3 in the future, mainboard and SoC code wouldn't break unexpectedly.
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. At best, this should be added as a separate CL if they are really required.
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 374: #define GPP_MLK 277
I had asked kernel team. Waiting for their reply.
Any update here?
-
Furquan Shaikh (Code Review)