[coreboot-gerrit] [L] Change in coreboot[main]: mb/google/fatcat: Add minimal code support for fatcat

[Nick Vaccaro (Code Review)]

Attention is currently required from: Subrata Banik.

Nick Vaccaro has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83197?usp=email )

Change subject: mb/google/fatcat: Add minimal code support for fatcat
......................................................................

Patch Set 3:

(12 comments)

File src/mainboard/google/fatcat/Kconfig:

https://review.coreboot.org/c/coreboot/+/83197/comment/6c8fd3f3_7c85ffe3?usp... :
PS3, Line 52: 	select GBB_FLAG_FORCE_DEV_BOOT_USB
NIT - Given these are alphabetical, this should be moved up 1 line

https://review.coreboot.org/c/coreboot/+/83197/comment/f2dad971_b39f61b0?usp... :
PS3, Line 94: DRIVER_TPM_I2C_ADDR
NIT - should this be above DRIVER_TPM_I2C_BUS to be in alphabetical order, or should it stay here given one thinks of I2C bus first, then the address on that bus?

I'm fine if this stays like it is if the deviation from alphabetising was on purpose due to what I just noted.

File src/mainboard/google/fatcat/variants/baseboard/fatcat/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/83197/comment/9c992354_b9ee78e6?usp... :
PS3, Line 1: meteorlake
Just to confirm, this is for SOC N-1, yes?

File src/mainboard/google/fatcat/variants/baseboard/fatcat/include/baseboard/ec.h:

https://review.coreboot.org/c/coreboot/+/83197/comment/dcedf294_b0345b1a?usp... :
PS3, Line 8: #include <baseboard/gpio.h>
NIT - alphabetically, this should be first

https://review.coreboot.org/c/coreboot/+/83197/comment/207f8d7a_acbd3235?usp... :
PS3, Line 11: 	(EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_CLOSED)        |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_OPEN)          |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_CONNECTED)      |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED)   |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_LOW)       |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_CRITICAL)  |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY)           |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_STATUS)    |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_THERMAL_THRESHOLD) |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_START)    |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_STOP)     |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_MKBP)              |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)            |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_MODE_CHANGE)       |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_USB_MUX))
NIT - should these be alphabetized?

https://review.coreboot.org/c/coreboot/+/83197/comment/2b93cfe2_bef66014?usp... :
PS3, Line 47: 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_CRITICAL) |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_SHUTDOWN))
            : 
NIT - alphabetize?

https://review.coreboot.org/c/coreboot/+/83197/comment/b13b2925_b19a8520?usp... :
PS3, Line 51: 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_USB_MUX)         |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_HANG_DETECT))
NIT - alphabetize?

https://review.coreboot.org/c/coreboot/+/83197/comment/f420cdf6_3bfd49b0?usp... :
PS3, Line 55: 	(EC_HOST_EVENT_MASK(EC_HOST_EVENT_THERMAL_SHUTDOWN) |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_SHUTDOWN) |\
            : 	 EC_HOST_EVENT_MASK(EC_HOST_EVENT_PANIC))
NIT - alphabetize?

https://review.coreboot.org/c/coreboot/+/83197/comment/2290c438_152e1cc7?usp... :
PS3, Line 68: #define EC_ENABLE_LID_SWITCH
            : #define EC_ENABLE_WAKE_PIN      GPE_EC_WAKE
            : /* Enable EC backed PD MCU device in ACPI */
            : #define EC_ENABLE_PD_MCU_DEVICE
            : #define SIO_EC_MEMMAP_ENABLE     /* EC Memory Map Resources */
            : #define SIO_EC_HOST_ENABLE       /* EC Host Interface Resources */
            : #define SIO_EC_ENABLE_PS2K       /* Enable PS/2 Keyboard */
            : 
            : #define EC_ENABLE_SYNC_IRQ      /* Enable tight timestamp / wake support */
            : #define EC_SYNC_IRQ_WAKE_CAPABLE /* Let the OS know ec_sync is wake capable */
NIT - alphabetize?

File src/mainboard/google/fatcat/variants/baseboard/fatcat/include/baseboard/gpio.h:

https://review.coreboot.org/c/coreboot/+/83197/comment/f320bd62_91831845?usp... :
PS3, Line 11: #define GPIO_PCH_WP	0
            : /* GPIO IRQ for tight timestamps / wake support */
            : #define EC_SYNC_IRQ	0
            : /* eSPI virtual wire reporting */
            : #define EC_SCI_GPI	0
            : 
NIT - alphabetize?

File src/mainboard/google/fatcat/variants/baseboard/fatcat/memory.c:

https://review.coreboot.org/c/coreboot/+/83197/comment/631031c6_8973b48d?usp... :
PS3, Line 22: variant_is_half_populated(void)
            : {
Does PTL have two channels?  My understanding is that WCL will only have 1, in which case I wouldn't think this would be needed, but not sure about PTL.

File src/mainboard/google/fatcat/variants/fatcat/gpio.c:

https://review.coreboot.org/c/coreboot/+/83197/comment/7c0250fe_deef9ebb?usp... :
PS3, Line 12: #include <soc/gpio.h>
NIT - alphabetize