Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39111 )
Change subject: soc/intel/tigerlake: Add Jasper lake GPIO support ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/gpio_op.asl:
PS15: I think this file should be converted to ASL 2.0 syntax as well. Feel free to do that as a follow-up.
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/gp... File src/soc/intel/tigerlake/gpio_jsl.c:
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/gp... PS15, Line 50: INTEL_GPP_BASE Just a note: This works currently based on the kernel driver implementation that is being pushed for JSL. I had posted a comment on the kernel driver that we should align the implementation with TGL. So, this driver in coreboot will have to be updated to match the kernel expectations once it changes.
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio.h:
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... PS15, Line 24: #define CROS_GPIO_DEVICE_NAME "GCM0" This is not correct. There are multiple GPIO devices for TGL.
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... PS15, Line 41: /* Common macro definition for gpio_op.asl file */ Aren't these macros already defined in some header file?
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio_soc_defs.h:
PS15: Shouldn't this file be renamed as gpio_soc_defs_tgl.h just like the JSL one?
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... PS15, Line 135: #define PMC_GPP_B 0x1 : #define PMC_GPP_A 0x0 : #define PMC_GPP_R 0x4 : #define PMC_GPP_S 0x6 : #define PMC_GPD 0x3 : #define PMC_GPP_H 0xA : #define PMC_GPP_D 0x7 : #define PMC_GPP_F 0x2 : #define PMC_GPP_C 0x8 : #define PMC_GPP_E 0xF Shouldn't these be ordered by number?
-
Furquan Shaikh (Code Review)