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:
(30 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
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@13 PS12, Line 13: I am looking at this as reference: https://github.com/torvalds/linux/tree/master/drivers/pinctrl/intel/pinctrl-...
Please let me know if this is not the right kernel driver to review your code.
https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@14 PS12, Line 14: none b:144680462
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. Just update the offsets that you want to change.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 101: * space before *
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 101: 05 "5" to keep the naming consistent.
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.0 syntax in the entire file?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/ac... PS12, Line 159: extra blank lines
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?
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 [COMM_3] which is not correct.
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?
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 of INTEL_GPP_BASE.
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? INTEL_GPP(GPP_B0, GPP_B0, GSPI1_CLK_LOOPBK)
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.
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
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?
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?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 119: * space before *
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 130: Group D This is not GPP Group D.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 255: iIRQ extra i
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 devicetree user doesn't have to care about GPP_C being different in DW0 v/s DW1 v/s DW2. But, behind the scenes SoC code can take care of mapping GPP_C to its right value depending upon whether it is used in DW0 or DW1 or DW2. That way even the mapping of PMC to GPIO GPE can be simplified.
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. GPP_B24 or something similar?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 135: ESPI_CLK_LOOPBK No GPP#?
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?
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. If GPIO #s are assigned consecutively here after COM1, wouldn't it mess up the mapping in kernel v/s coreboot? I think in the past platforms, this was handled by assigning numbers to GPD group towards the end.
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 369: TSSTB TRSTB?
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.
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 missing?
https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/in... PS12, Line 407: pin mux setting What exactly does that mean? Is it doing anything more than setting DW0 and DW1?
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.
-
Furquan Shaikh (Code Review)