[coreboot-gerrit] 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.



-- 
To view, visit https://review.coreboot.org/28932
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8b14ae2ae4d97e184906dec468b405134d590da
Gerrit-Change-Number: 28932
Gerrit-PatchSet: 2
Gerrit-Owner: Nick Vaccaro <nvaccaro at google.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Lijian Zhao <lijian.zhao at intel.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro at google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Fri, 05 Oct 2018 06:30:13 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181005/fca398a0/attachment.html>