Change in coreboot[master]: acpi: Reverse logic for setting PCIEXP_WAKE_DIS bit in PM1_EN_STS
Nick Vaccaro 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:
(6 comments)
Based on Furquan's explanation, I'm abandoning this change as this code was not doing what I though it was doing. Sorry for the thrash.
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.
It looks like that's where it sets the PCIEXP_WAKE_DIS bit in pm1_en.
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.
They haven't shown to help, which is why I have it set to -1.
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. […]
Ack
https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c@718 PS2, Line 718: if ((config->deep_sx_config & DSX_EN_WAKE_PIN) == 0)
if (!(config->deep_sx_config & DSX_EN_WAKE_PIN))
Ack
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 PCI […]
Ack
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. […]
Ack
-
Nick Vaccaro (Code Review)