[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel: Allow zero values for PMC GPE0 DW registers

[Subrata Banik (Code Review)]

Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Eric Lai, Intel coreboot Reviewers, Jakub Czapiga, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Matt DeVillier, Nick Vaccaro, Pranava Y N, Sean Rhodes, Tarun, Werner Zeh.

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

Change subject: soc/intel: Allow zero values for PMC GPE0 DW registers
......................................................................

Patch Set 3:

(3 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/86164/comment/2b985d39_d269acd2?usp... :
PS2, Line 14: This prevents potential issues arising from a zeroed configuration
> The commit message still needs some update in my opinion:
> the describe logic is wrong and not aligned with the code

fixed
> It does not really explain the issue being fixed

I don't believe this is any problem in the existing code as in one platform can't configure all GPE DWs to zero value which is also a valid GPIO community. So, technically this is not fixing any problem. It avoids a assertion when platform
decides to not configure GPE and use all DWs set to zero value.
> It should probably point out to https://review.coreboot.org/c/coreboot/+/85161

sure

File src/soc/intel/alderlake/pmutil.c:

https://review.coreboot.org/c/coreboot/+/86164/comment/fc0a0de0_94a33963?usp... :
PS3, Line 164: override
> do we really need this here? there's already another printk in this case (PMC: Using default GPE route.)

`PMC: Using default GPE route` this often mislead as default value from baseboard but doesn't convey if we are using all zero value which is ideally what you wish to avoid here. All zero values for all dwords is not valid.

https://review.coreboot.org/c/coreboot/+/86164/comment/d2cf0a58_e1f06449?usp... :
PS3, Line 167: bool result = (config->pmc_gpe0_dw0 != config->pmc_gpe0_dw1) &&
             : 			 (config->pmc_gpe0_dw0 != config->pmc_gpe0_dw2) &&
             : 			 (config->pmc_gpe0_dw1 != config->pmc_gpe0_dw2) || all_zero;
             : 
             : 	assert(result);
> slightly cleaner? […]

Acknowledged