[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 8:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/84104/comment/6b468cdd_af72321c?usp... :
PS7, Line 341: pmc_clear_gpi_gpe0_status
> I do a search and only the existing gpe event disabling functions need to cover GPE1 EN bits if _HAVE_GPE1. By doing this, no _gpe_function needs to be renamed outside of pmclib.c.
>
> In addition, I delete the GPE event enable functions

which one have deleted ?
> since not used. CB should not enable these events during boot, anyways.

without keeping the GPE enabled bit set, how can we wake the device with various wake sources as defined by the `General Purpose Event 0 Enable [127:96] 
(GPE0_EN[127:96])` like alarm, lan wake etc?

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

https://review.coreboot.org/c/coreboot/+/84104/comment/23f279e4_1d64c63e?usp... :
PS8, Line 321: 	soc_pmc_disable_std_gpe1(mask);
> Subrata,
>
> In PTL, in the case that we choose not to switch to use GPE1 events (i.e. not include GPE1 in FADT so that kernel won't see GPE1 event and include only ACPI GPE0 event methods in DSDT/SSDT), these GPE1 status bit might still be set by the HW and we need to clear it at boot. We might need to go back to use _HAVE_GPE1 and _USE_GPE1 approach.
>
> _USE_GPE1: if false:  _USE_GEP1: dummy GPE_STS defines. include GPE0 methods.
>        if true: add GPE1 blocks to FADT and include GPE1 methods
>
> _HAVE_GPE1: 
>             define GPE_STS() in SOC.
>             clear GPE1 STS at boot.
>
> acpi.c:
>      fadt->gpe1_blk = CONFIG(_USE_GPE1) ? (pmbase + GPE1_STS(0)) : 0;
>
> also, in acpi Kconfig
> _USE_GPE1 depends on _HAVE_GPE1 as you mentioned in one of comments.
>
> Any thought prior to https://review.coreboot.org/c/coreboot/+/84103/14 being merged?

I am having trouble understanding why we would not use GPE1 if the HW logic would use it for wake functionality. For example, why would we use a GPIO configured as GPE0 for CNVi if GPE1 already has native support? Could you please explain what you mean by "HW will set those bits"? Ideally, HW would only set those bits if the HW logic applies to GPE1 and not GPE0. What would happen if we were using GPE0? Would only the GPE1 status bit be set?

Additionally, if we decided not to use GPE1, then we would not be setting the GPE1 Enable PINs. HW logic can only set GPE1 status and not the enable bit. Unless both En and Status are set, there is no GPE event that would trigger. We do not need to even clear the GPE1 status unless we really plan to configure GPE1 enable bits.