[coreboot-gerrit] [L] Change in coreboot[main]: soc/intel/ptl: Define GPE1 macros and register fields

[Subrata Banik (Code Review)]

Attention is currently required from: Cliff Huang, Kapil Porwal, Paul Menzel, Pranava Y N.

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

Change subject: soc/intel/ptl: Define GPE1 macros and register fields
......................................................................

Patch Set 12:

(4 comments)

File src/soc/intel/pantherlake/include/soc/gpe.h:

https://review.coreboot.org/c/coreboot/+/84297/comment/8292243e_0e5e06f7?usp... :
PS12, Line 153: all
`All` and same for all below comments as well

File src/soc/intel/pantherlake/include/soc/gpe.h:

https://review.coreboot.org/c/coreboot/+/84297/comment/915a7dbc_3c8fa2f5?usp... :
PS4, Line 30: 146
> > okay. will it be okay if TCSS still contain the defines from gpe.h and pm.h for now? […]

Acknowledged

File src/soc/intel/pantherlake/include/soc/pm.h:

PS9:
> The actual event bits for GPE1 are PMC I/O registers, which are defines in pm.h. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/84297/comment/98d80fce_940b47d8?usp... :
PS9, Line 133: #if CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1)
> > Subrata, we will hit the GPE1_* marco redefined build error if CONFIG_SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 is not set in PTL. I recall I brought up this issue.
>
> yes, I remember. In such case we should introduce `SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1` and `SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1`
>
> PTL SoC to select SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 and mainboard can select SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1 if they wish to use it.
>
> this way, you don't run into any issue (macro definition of is still inside !SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, therefore, for PTL it will only include one/proper register definition)
> fadt->gpe1_blk = 0;
> if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1)) {
>     fadt->gpe1_blk = pmbase + GPE1_STS(0);
>     fadt->gpe1_blk_len = 2 * GPE1_REG_MAX * sizeof(uint32_t);
>     /*
>      * NOTE: gpe1 is after gpe0, which has _STS and _EN register sets.
>      * gpe1_base is the starting bit offset for GPE1.
>      */
>     fadt->gpe1_base = fadt->gpe0_blk_len / 2 * 8;
> }

can you drop this CPP guard now ?