[M] Change in coreboot[main]: soc/intel/common/block/pmc: Add GPE1 functions

[Subrata Banik (Code Review)]

Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Hannah Williams, Jamie Ryu.

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

Change subject: soc/intel/common/block/pmc: Add GPE1 functions
......................................................................

Patch Set 7:

(10 comments)

File src/soc/intel/common/block/pmc/pmclib.c:

https://review.coreboot.org/c/coreboot/+/84104/comment/3e19191d_60628e63?usp... :
PS3, Line 406: pmc_clear_gpi_gpe0_status
> gpi_gpe0 means the general GPI event in GPE0. GPE1 only contains Intel's std events.

ideally you have to clean event status register for all GPEs (irrespective of if it's GPE0 and GPE1). This is required to avoid a fake wake during system sleep or S5 entry.

Additionally, you are not cleaning the Enable bit hence, it should be okay. Status bit is asserted by the hardware and only be cleared upon writing 1 to it. The idea to clear the GPE status to avoid fake wake due to persistent status bit (which often due to poor hardware)

File src/soc/intel/common/block/pmc/pmclib.c:

https://review.coreboot.org/c/coreboot/+/84104/comment/a6c86904_4df4a535?usp... :
PS7, Line 25: #if !CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1)
            : 
            : /* NOTE: For platform doesn't have support for GPE1, adding dummy entries here for common code
            :  */
            : #ifndef GPE1_STS
            : #define GPE1_STS(x) (0x0 + ((x) * 4))
            : #endif
            : #ifndef GPE1_REG_MAX
            : #define GPE1_REG_MAX 0
            : #endif
            : 
            : #endif
we should avoid defining it across multiple files. Please find a file in common code and implement it once.

https://review.coreboot.org/c/coreboot/+/84104/comment/f12b9950_9aca6023?usp... :
PS7, Line 306: 0
this should be NULL ? as you are returning `char *`

https://review.coreboot.org/c/coreboot/+/84104/comment/2af0f75f_c39193c1?usp... :
PS7, Line 341: pmc_clear_gpi_gpe0_status
can I request you to perform this renaming as part of the base CL and mention the motivation about why are you changing the GPE to GPE0

https://review.coreboot.org/c/coreboot/+/84104/comment/d262a2c9_188d8864?usp... :
PS7, Line 379: gpe_sts
check the NULL pointer at line 381

https://review.coreboot.org/c/coreboot/+/84104/comment/6fb2799a_3581cc98?usp... :
PS7, Line 381: 	int i;
drop

https://review.coreboot.org/c/coreboot/+/84104/comment/bdab28f7_9c847597?usp... :
PS7, Line 383: i
```int i```

https://review.coreboot.org/c/coreboot/+/84104/comment/71a5b78f_9b9b9232?usp... :
PS7, Line 393: 	int i;
same

https://review.coreboot.org/c/coreboot/+/84104/comment/84dfcf5b_6839ebb9?usp... :
PS7, Line 396: sts_arr =
missing NULL check ?

https://review.coreboot.org/c/coreboot/+/84104/comment/1f4d14fc_79017452?usp... :
PS7, Line 406: 	if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1)) {
WDYT

```
if (!CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1))
  return;

uint32_t gpe1_sts[GPE1_REG_MAX];

reset_std_gpe1_status(gpe1_sts);
print_std_gpe1_sts(gpe1_sts);
```