Change in coreboot[master]: acpi: Reverse logic for setting PCIEXP_WAKE_DIS bit in PM1_EN_STS

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/28932 )

Change subject: acpi: Reverse logic for setting PCIEXP_WAKE_DIS bit in PM1_EN_STS
......................................................................

Patch Set 2:

(5 comments)

https://review.coreboot.org/#/c/28932/2//COMMIT_MSG
Commit Message:

https://review.coreboot.org/#/c/28932/2//COMMIT_MSG@9
PS2, Line 9: The logic for setting PCIEXP_WAKE_DIS bit is backwards
The function you are changing does not enable or disable PCIEXP_WAKE_DIS.

https://review.coreboot.org/#/c/28932/2//COMMIT_MSG@21
PS2, Line 21: TEST
I am really surprised that the changes in this CL actually help.

https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c
File src/soc/intel/skylake/acpi.c:

https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c@691
PS2, Line 691: soc_fill_acpi_wake
This function, as the comment says above, is used to only identify the wake source during resume. It does not actually write to PM1_EN_STS register to enable/disable any wake bits.

https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c@719
PS2, Line 719: pm1_en |= PCIEXPWAK_STS;
The intent of this code is to check if DSX_EN_WAKE_PIN was enabled for wake and if yes, then set PCIEXPWAK_STS to mask the pm1_sts bits in order to identify the true wake source. It is not used to enable or disable wake using the WAKE# pin. That is the reason why it is using PCIEXPWAK_STS and not PCIEXPWAK_DIS macro.

https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c@722
PS2, Line 722: *pm1 = ps->pm1_sts & pm1_en;
             : 
This is where it masks the status read from PM1_STS using the valid bits that were set in pm1_en.

The logic here is that some bits in PM1_EN get lost when waking from Deep Sx. This is because the bits reside in different power wells as indicated by the comment in line 710. Hence, the code in this function:

1. Reads current pm1_en from ps->pm1_en into pm1_en
2. ORs pm1_en with PWRBTN_STS and PCIEXPWAK_STS if DSX_EN_WAKE_PIN is set.

This mask obtained using #1 and #2 is used to identify the wake source from pm1_sts.