Maulik V Vaghela 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 2:
(15 comments)
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/gpio_jsl.asl:
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/acp... PS2, Line 20:
Can we merge the ASLs for TGL and JSL into one? The difference as highlighted by Tim in other CL is […]
Yes Karthik, I'll check this and work on it.
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/acp... PS2, Line 40: /* GPIO Community 0 */ : CreateDWordField (^RBUF, ^COM0._BAS, BAS0) : CreateDWordField (^RBUF, ^COM0._LEN, LEN0) : Store (^^PCRB (PID_GPIOCOM0), BAS0) : Store (GPIO_BASE_SIZE, LEN0) : : /* GPIO Community 1 */ : CreateDWordField (^RBUF, ^COM1._BAS, BAS1) : CreateDWordField (^RBUF, ^COM1._LEN, LEN1) : Store (^^PCRB (PID_GPIOCOM1), BAS1) : Store (GPIO_BASE_SIZE, LEN1) : : /* GPIO Community 2 */ : CreateDWordField (^RBUF, ^COM2._BAS, BAS2) : CreateDWordField (^RBUF, ^COM2._LEN, LEN2) : Store (^^PCRB (PID_GPIOCOM2), BAS2) : Store (GPIO_BASE_SIZE, LEN2) : : /* GPIO Community 4 */ : CreateDWordField (^RBUF, ^COM4._BAS, BAS4) : CreateDWordField (^RBUF, ^COM4._LEN, LEN4) : Store (^^PCRB (PID_GPIOCOM4), BAS4) : Store (GPIO_BASE_SIZE, LEN4) : : /* GPIO Community 5 */ : CreateDWordField (^RBUF, ^COM5._BAS, BAS5) : CreateDWordField (^RBUF, ^COM5._LEN, LEN5) : Store (^^PCRB (PID_GPIOCOM5), BAS5) : Store (GPIO_BASE_SIZE, LEN5)
Why is RBUF being modified at runtime? Why can't the base addresses and lengths just go into RBUF ab […]
Let me check this. since I am on vacation I may take a day or two to correct it.
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/gpi... File src/soc/intel/tigerlake/gpio_jsl.c:
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/gpi... PS2, Line 105:
Nit: Remove this space.
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/gpi... PS2, Line 203: GPP_GPD
line this up with the other GPP_* ?
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/gpio.h:
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 36:
Nit: No need for indentation here and below.
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/gpio_defs_jsl.h:
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 261:
No need to define VGPIO*_IRQ? Couple of VGPIOs have their IRQ routed to APIC. […]
Ack
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 266: 0xb0
0xc0
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 268: 0x110
0x120 as per the EDS.
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 270: 0x1A0
Nit: keep the alphabets in hex small just to be consistent with the rest of the file.
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 273: #define GPIORXSTATE_MASK 0x1 : #define GPIORXSTATE_SHIFT 1
Redundant definition. It is already defined in gpio.h file to be included in gpio_ops. […]
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/gpio_defs_tgl.h:
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 315: #define GPIORXSTATE_MASK 0x1 : #define GPIORXSTATE_SHIFT 1 : #define GPIOTXSTATE_MASK 0x1
Redundant definitions. These are already defined in gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/gpio_soc_defs_jsl.h:
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 44: : /* Group B */
Group F
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 66:
/* Group B */
Done
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 67: GPIO_RSVD_0
Do you want to name the GPIO as reserved? Can the name be mentioned explicitly?
These are soc reserved gpio pins that's why name is reserved.
https://review.coreboot.org/c/coreboot/+/39111/2/src/soc/intel/tigerlake/inc... PS2, Line 344: COMM_3 3 : #define COMM_4
Should it be mentioned as COMM_3/4 or COMM_4/5 just to be consistent with the rest of the definition […]
Done