[S] Change in coreboot[main]: soc/intel/common/block/acpi: Add GPE1 blocks to ACPI FADT table

[Subrata Banik (Code Review)]

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

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

Change subject: soc/intel/common/block/acpi: Add GPE1 blocks to ACPI FADT table
......................................................................

Patch Set 7:

(2 comments)

File src/soc/intel/common/block/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/84103/comment/0957515b_441482b6?usp... :
PS7, Line 121: 	if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1)) {
> Subrata,
>
> | fadt->gpe1_blk =  GPE1_STS(0) ? (pmbase + GPE1_STS(0)) :  GPE1_STS(0);
>
> in the case that _HAVE_GPE1 = true ; _USE_GPE1 = false, we need to set 
> gpe1_blk=0, gpe1_blk_len=0, and gpe1_base=0 in FADT. GPE1_STS(0) has none-zero vaule and we will need to check _USE_GPE1 flag instead of using GPE1_STS(0) check, isn't?

The `GPE1_STS(0)` macro definition in the ptl soc pm.h header file must be guarded by the `_USE_GPE1` Kconfig option.

In this case, the following will occur:

1. If `!_USE_GPE1`, then `GPE1_STS(0)=0`. This is the default behavior for all SoC generations prior to PTL.
2. PTL can select `_HAVE_GPE1` and only implement the `GPE1_STS(0)` macro inside the PTL SOC when `_USE_GPE1` is enabled. In this case, assuming that PTL selects `_HAVE_GPE1=true` but `_USE_GPE1=false`, the behavior will be the same as in #1, with `GPE1_STS(0)=0`.
3. If PTL selects `_HAVE_GPE1=true` and `_USE_GPE1=true`, then the `GPE1_STS` macro can have a non-zero value.

I am now wondering if we really need two Kconfig options, `_USE_GPE1` and `_HAVE_GPE1`. What is the point of selecting `_HAVE_GPE1` if we are not intending to select `_USE_GPE1`?

if you just implement one Kconfig and ignore (https://review.coreboot.org/c/coreboot/+/84103/comment/58095ae5_e462c778/) then things will be much simpler where, platform w/o `_USE_GPE1` will set GPE1_STS as zero and platform w/ `_USE_GPE1` will set `GPE1_STS` with non-zero value.

Does that make sense to you ?

https://review.coreboot.org/c/coreboot/+/84103/comment/ba8118e7_52cf4986?usp... :
PS7, Line 128:  8
> The unit for gpe1_base is bit and GPE0 block size is in bytes. The resulting is 0x80 (128), which is the next bit of GPE0 [127-0].

are you saying that you are multiplying with 8 to convert bytes to bit ?