Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Hannah Williams, Jamie Ryu.
Felix Held has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/84104?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: soc/intel/common/block/pmc: Add GPE1 functions ......................................................................
Patch Set 11:
(4 comments)
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/84104/comment/69d5a39a_643c18f7?usp... : PS11, Line 239: based on standard GPE0 EN bits hmm, this sounds odd to me
https://review.coreboot.org/c/coreboot/+/84104/comment/ecda8374_54345401?usp... : PS11, Line 241: gpe0_mask shouldn't this be gpe1_mask? same below
File src/soc/intel/common/block/pmc/pmclib.c:
PS11: are the gpe1 registers for the same event as the gpe0 register with the same number? the code using the same masks for the gpe0 and gpe1 suggest that to me, but i have some doubts that this is actually the case
https://review.coreboot.org/c/coreboot/+/84104/comment/b5a20cdb_29759aed?usp... : PS11, Line 68: /* SoC overrides for GPE1 when SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 is enabled */ : __weak const char *const *soc_std_gpe1_sts_array(int idx, size_t *a) : { : return NULL; : } : : /* disable the corresponding GPE1 bits based on standard GPE0 bits */ : __weak void soc_pmc_disable_std_gpe1(uint32_t gpe0_mask) : { : } : : /* enable the corresponding GPE1 bits based on standard GPE0 bits */ : __weak void soc_pmc_enable_std_gpe1(uint32_t gpe0_mask) : { : } are those weak functions needed? i'd assume that if a soc selects SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, it needs to implement those and if it doesn't select that option, those function implementations won't be needed.