Change in coreboot[master]: sio/winbond: Treat Immediate Power Down (IPD) as D3

[Nico Huber (Code Review)]

Hello Paul Menzel,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/39746

to review the following change.

Change subject: sio/winbond: Treat Immediate Power Down (IPD) as D3
......................................................................

sio/winbond: Treat Immediate Power Down (IPD) as D3

Change-Id: Ifa28a7c56575848e76e4a1c542866413b4c44d50
Signed-off-by: Nico Huber 
---
M src/superio/winbond/w83627dhg/acpi/superio.asl
M src/superio/winbond/w83627hf/acpi/superio.asl
M src/superio/winbond/w83977tf/acpi/superio.asl
3 files changed, 10 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/39746/1

diff --git a/src/superio/winbond/w83627dhg/acpi/superio.asl b/src/superio/winbond/w83627dhg/acpi/superio.asl
index cb6a4a7..f86f169 100644
--- a/src/superio/winbond/w83627dhg/acpi/superio.asl
+++ b/src/superio/winbond/w83627dhg/acpi/superio.asl
@@ -109,12 +109,12 @@
    #define PNP_EXIT_MAGIC_1ST	0xaa
    #include <superio/acpi/pnp_config.asl>

-	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D2 */
+	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D3 */
    Method (_PSC) {
    	ENTER_CONFIG_MODE (PNP_NO_LDN_CHANGE)
    	  Store (IPD, Local0)
    	EXIT_CONFIG_MODE ()
-		If (Local0) { Return (2) }
+		If (Local0) { Return (3) }
    	Else { Return (0) }
    }

@@ -125,8 +125,8 @@
    	EXIT_CONFIG_MODE ()
    }

-	/* PM: Switch to D2 by setting IPD high  */
-	Method (_PS2) {
+	/* PM: Switch to D3 by setting IPD high  */
+	Method (_PS3) {
    	ENTER_CONFIG_MODE (PNP_NO_LDN_CHANGE)
    	  Store (One, IPD)
    	EXIT_CONFIG_MODE ()
diff --git a/src/superio/winbond/w83627hf/acpi/superio.asl b/src/superio/winbond/w83627hf/acpi/superio.asl
index c1293ff..38791d3 100644
--- a/src/superio/winbond/w83627hf/acpi/superio.asl
+++ b/src/superio/winbond/w83627hf/acpi/superio.asl
@@ -166,12 +166,12 @@
    	Release (CRMX)
    }

-	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D2 */
+	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D3 */
    Method (_PSC) {
    	ENCM (0xFF)
    	Store (IPD, Local0)
    	EXCM ()
-		If (Local0) { Return (2) }
+		If (Local0) { Return (3) }
    	Else { Return (0) }
    }

@@ -182,8 +182,8 @@
    	EXCM ()
    }

-	/* PM: Switch to D2 by setting IPD high  */
-	Method (_PS2) {
+	/* PM: Switch to D3 by setting IPD high  */
+	Method (_PS3) {
    	ENCM (0xFF)
    	Store (One, IPD)
    	EXCM ()
diff --git a/src/superio/winbond/w83977tf/acpi/superio.asl b/src/superio/winbond/w83977tf/acpi/superio.asl
index e2ff2ef..c7a62cd 100644
--- a/src/superio/winbond/w83977tf/acpi/superio.asl
+++ b/src/superio/winbond/w83977tf/acpi/superio.asl
@@ -72,12 +72,12 @@
 #define PNP_EXIT_MAGIC_1ST      0xaa
 #include <superio/acpi/pnp_config.asl>

-/* PM: indicate IPD (Immediate Power Down) bit state as D0/D2 */
+/* PM: indicate IPD (Immediate Power Down) bit state as D0/D3 */
 Method (_PSC) {
    ENTER_CONFIG_MODE (0xFF)
    Store (IPD, Local0)
    EXIT_CONFIG_MODE ()
-	If (Local0) { Return (2) }
+	If (Local0) { Return (3) }
    Else { Return (0) }
 }

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Felix Held,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39746

to look at the new patch set (#2).

Change subject: sio/winbond: Treat Immediate Power Down (IPD) as D3
......................................................................

sio/winbond: Treat Immediate Power Down (IPD) as D3

Spec says if any object to control the power state exists, at least
D0 and D3 must be supported. And it seems Windows complains about the
missing D3 support: https://ticket.coreboot.org/issues/257

Change-Id: Ifa28a7c56575848e76e4a1c542866413b4c44d50
Signed-off-by: Nico Huber 
---
M src/superio/winbond/w83627dhg/acpi/superio.asl
M src/superio/winbond/w83627hf/acpi/superio.asl
M src/superio/winbond/w83977tf/acpi/superio.asl
3 files changed, 10 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/39746/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39746 )

Change subject: sio/winbond: Treat Immediate Power Down (IPD) as D3
......................................................................

Patch Set 2: Code-Review+1

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Angel Pons, Felix Held,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39746

to look at the new patch set (#3).

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

superio: Replace D1/D2 power states with D3

Spec says if any object to control the power state exists, at least
D0 and D3 must be supported. And it seems Windows complains about the
missing D3 support: https://ticket.coreboot.org/issues/257

Change-Id: Ifa28a7c56575848e76e4a1c542866413b4c44d50
Signed-off-by: Nico Huber 
---
M src/superio/acpi/pnp.asl
M src/superio/acpi/pnp_generic.asl
M src/superio/acpi/pnp_uart.asl
M src/superio/winbond/w83627dhg/acpi/superio.asl
M src/superio/winbond/w83627hf/acpi/superio.asl
M src/superio/winbond/w83977tf/acpi/superio.asl
6 files changed, 29 insertions(+), 29 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/39746/3

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39746 )

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39746/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39746/3//COMMIT_MSG@12 
PS3, Line 12: 
It’d be great if you added this as a tag.
> Closes: https://ticket.coreboot.org/issues/257

For people grep’ing the coreboot git history, could you also add the Windows error message?

Stop Error Code=0xA5 (as Before), Parameter1=0xD, Parameter2=0xFFFFFA8004815A00, Parameter3=0x000000003353505F, Parameter4=0x0

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39746 )

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Angel Pons, Felix Held,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39746

to look at the new patch set (#4).

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

superio: Replace D1/D2 power states with D3

Spec says if any object to control the power state exists, at least
D0 and D3 must be supported. And it seems Windows complains about the
missing D3 support: https://ticket.coreboot.org/issues/257

Windows reported `*** STOP: 0x000000A5` with the first parameter
`0x000000000000000D` (refers to a missing ACPI object) and the
third parameter `0x000000003353505F` which is the name of the
object in ASCII, little-endian (`_PS3`).

Change-Id: Ifa28a7c56575848e76e4a1c542866413b4c44d50
Signed-off-by: Nico Huber 
Closes: https://ticket.coreboot.org/issues/257
---
M src/superio/acpi/pnp.asl
M src/superio/acpi/pnp_generic.asl
M src/superio/acpi/pnp_uart.asl
M src/superio/winbond/w83627dhg/acpi/superio.asl
M src/superio/winbond/w83627hf/acpi/superio.asl
M src/superio/winbond/w83977tf/acpi/superio.asl
6 files changed, 29 insertions(+), 29 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/39746/4

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39746 )

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39746/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39746/3//COMMIT_MSG@12 
PS3, Line 12:
> It’d be great if you added this as a tag. […]

Done

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39746 )

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

superio: Replace D1/D2 power states with D3

Spec says if any object to control the power state exists, at least
D0 and D3 must be supported. And it seems Windows complains about the
missing D3 support: https://ticket.coreboot.org/issues/257

Windows reported `*** STOP: 0x000000A5` with the first parameter
`0x000000000000000D` (refers to a missing ACPI object) and the
third parameter `0x000000003353505F` which is the name of the
object in ASCII, little-endian (`_PS3`).

Change-Id: Ifa28a7c56575848e76e4a1c542866413b4c44d50
Signed-off-by: Nico Huber 
Closes: https://ticket.coreboot.org/issues/257
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39746
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Menzel 
Reviewed-by: Angel Pons 
---
M src/superio/acpi/pnp.asl
M src/superio/acpi/pnp_generic.asl
M src/superio/acpi/pnp_uart.asl
M src/superio/winbond/w83627dhg/acpi/superio.asl
M src/superio/winbond/w83627hf/acpi/superio.asl
M src/superio/winbond/w83977tf/acpi/superio.asl
6 files changed, 29 insertions(+), 29 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/superio/acpi/pnp.asl b/src/superio/acpi/pnp.asl
index 1f607eb..bb71c9b 100644
--- a/src/superio/acpi/pnp.asl
+++ b/src/superio/acpi/pnp.asl
@@ -69,7 +69,7 @@

/*
  * Current power state (returns the chip's state, if it's in
- * power saving mode, 1 if this LDN is in power saving mode,
+ * power saving mode, 3 if this LDN is in power saving mode,
  * 0 else)
  *
  * PM_REG	Identifier of a register which powers down the device
@@ -82,7 +82,7 @@
    ENTER_CONFIG_MODE (PM_LDN)\
      Store (PM_REG, Local0)\
    EXIT_CONFIG_MODE ()\
-	If (LEqual(Local0, PM_VAL)) { Return (1) }\
+	If (LEqual(Local0, PM_VAL)) { Return (3) }\
    Else { Return (0) }\

/* Disable power saving mode */
@@ -92,7 +92,7 @@
    EXIT_CONFIG_MODE ()

/* Enable power saving mode */
-#define PNP_GENERIC_PS1(PM_REG, PM_VAL, PM_LDN) \
+#define PNP_GENERIC_PS3(PM_REG, PM_VAL, PM_LDN) \
    ENTER_CONFIG_MODE (PM_LDN)\
      Store (PM_VAL, PM_REG)\
    EXIT_CONFIG_MODE ()
diff --git a/src/superio/acpi/pnp_generic.asl b/src/superio/acpi/pnp_generic.asl
index 482d73e..cb92a5d 100644
--- a/src/superio/acpi/pnp_generic.asl
+++ b/src/superio/acpi/pnp_generic.asl
@@ -74,8 +74,8 @@
    	PNP_GENERIC_PS0(SUPERIO_PNP_PM_REG, SUPERIO_PNP_PM_VAL, SUPERIO_PNP_PM_LDN)
    }

-	Method (_PS1) {
-		PNP_GENERIC_PS1(SUPERIO_PNP_PM_REG, SUPERIO_PNP_PM_VAL, SUPERIO_PNP_PM_LDN)
+	Method (_PS3) {
+		PNP_GENERIC_PS3(SUPERIO_PNP_PM_REG, SUPERIO_PNP_PM_VAL, SUPERIO_PNP_PM_LDN)
    }
 #else
    Method (_PSC) {
diff --git a/src/superio/acpi/pnp_uart.asl b/src/superio/acpi/pnp_uart.asl
index e727889..859430e 100644
--- a/src/superio/acpi/pnp_uart.asl
+++ b/src/superio/acpi/pnp_uart.asl
@@ -57,8 +57,8 @@
    	PNP_GENERIC_PS0(SUPERIO_UART_PM_REG, SUPERIO_UART_PM_VAL, SUPERIO_UART_PM_LDN)
    }

-	Method (_PS1) {
-		PNP_GENERIC_PS1(SUPERIO_UART_PM_REG, SUPERIO_UART_PM_VAL, SUPERIO_UART_PM_LDN)
+	Method (_PS3) {
+		PNP_GENERIC_PS3(SUPERIO_UART_PM_REG, SUPERIO_UART_PM_VAL, SUPERIO_UART_PM_LDN)
    }
 #else
    Method (_PSC) {
diff --git a/src/superio/winbond/w83627dhg/acpi/superio.asl b/src/superio/winbond/w83627dhg/acpi/superio.asl
index cb6a4a7..f86f169 100644
--- a/src/superio/winbond/w83627dhg/acpi/superio.asl
+++ b/src/superio/winbond/w83627dhg/acpi/superio.asl
@@ -109,12 +109,12 @@
    #define PNP_EXIT_MAGIC_1ST	0xaa
    #include <superio/acpi/pnp_config.asl>

-	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D2 */
+	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D3 */
    Method (_PSC) {
    	ENTER_CONFIG_MODE (PNP_NO_LDN_CHANGE)
    	  Store (IPD, Local0)
    	EXIT_CONFIG_MODE ()
-		If (Local0) { Return (2) }
+		If (Local0) { Return (3) }
    	Else { Return (0) }
    }

@@ -125,8 +125,8 @@
    	EXIT_CONFIG_MODE ()
    }

-	/* PM: Switch to D2 by setting IPD high  */
-	Method (_PS2) {
+	/* PM: Switch to D3 by setting IPD high  */
+	Method (_PS3) {
    	ENTER_CONFIG_MODE (PNP_NO_LDN_CHANGE)
    	  Store (One, IPD)
    	EXIT_CONFIG_MODE ()
diff --git a/src/superio/winbond/w83627hf/acpi/superio.asl b/src/superio/winbond/w83627hf/acpi/superio.asl
index c1293ff..d5c5ec9 100644
--- a/src/superio/winbond/w83627hf/acpi/superio.asl
+++ b/src/superio/winbond/w83627hf/acpi/superio.asl
@@ -166,12 +166,12 @@
    	Release (CRMX)
    }

-	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D2 */
+	/* PM: indicate IPD (Immediate Power Down) bit state as D0/D3 */
    Method (_PSC) {
    	ENCM (0xFF)
    	Store (IPD, Local0)
    	EXCM ()
-		If (Local0) { Return (2) }
+		If (Local0) { Return (3) }
    	Else { Return (0) }
    }

@@ -182,8 +182,8 @@
    	EXCM ()
    }

-	/* PM: Switch to D2 by setting IPD high  */
-	Method (_PS2) {
+	/* PM: Switch to D3 by setting IPD high  */
+	Method (_PS3) {
    	ENCM (0xFF)
    	Store (One, IPD)
    	EXCM ()
@@ -220,7 +220,7 @@
    		ENCM (0xFF)
    		Store (FDPW, Local0)
    		EXCM ()
-			If (Local0) { Return (1) }
+			If (Local0) { Return (3) }
    		Else { Return (0) }
    	}
    	/* Disable power saving mode */
@@ -230,7 +230,7 @@
    		EXCM ()
    	}
    	/* Enable power saving mode */
-		Method (_PS1) {
+		Method (_PS3) {
    		ENCM (0xFF)
    		Store (Zero, FDPW)
    		EXCM ()
@@ -441,7 +441,7 @@
    		ENCM (0xFF)
    		Store (PRPW, Local0)
    		EXCM ()
-			If (Local0) { Return (1) }
+			If (Local0) { Return (3) }
    		Else { Return (0) }
    	}
    	Method (_PS0) {
@@ -449,7 +449,7 @@
    		Store (One, PRPW)
    		EXCM ()
    	}
-		Method (_PS1) {
+		Method (_PS3) {
    		ENCM (0xFF)
    		Store (Zero, PRPW)
    		EXCM ()
@@ -618,7 +618,7 @@
    		ENCM (0xFF)
    		Store (UAPW, Local0)
    		EXCM ()
-			If (Local0) { Return (1) }
+			If (Local0) { Return (3) }
    		Else { Return (0) }
    	}
    	Method (_PS0) {
@@ -626,7 +626,7 @@
    		Store (One, UAPW)
    		EXCM ()
    	}
-		Method (_PS1) {
+		Method (_PS3) {
    		ENCM (0xFF)
    		Store (Zero, UAPW)
    		EXCM ()
@@ -743,7 +743,7 @@
    		ENCM (0xFF)
    		Store (UBPW, Local0)
    		EXCM ()
-			If (Local0) { Return (1) }
+			If (Local0) { Return (3) }
    		Else { Return (0) }
    	}
    	Method (_PS0) {
@@ -751,7 +751,7 @@
    		Store (One, UBPW)
    		EXCM ()
    	}
-		Method (_PS1) {
+		Method (_PS3) {
    		ENCM (0xFF)
    		Store (Zero, UBPW)
    		EXCM ()
@@ -868,7 +868,7 @@
    		ENCM (0xFF)
    		Store (UBPW, Local0)
    		EXCM ()
-			If (Local0) { Return (1) }
+			If (Local0) { Return (3) }
    		Else { Return (0) }
    	}
    	Method (_PS0) {
@@ -876,7 +876,7 @@
    		Store (One, UBPW)
    		EXCM ()
    	}
-		Method (_PS1) {
+		Method (_PS3) {
    		ENCM (0xFF)
    		Store (Zero, UBPW)
    		EXCM ()
@@ -1391,7 +1391,7 @@
    		ENCM (0xFF)
    		Store (HWPW, Local0)
    		EXCM ()
-			If (Local0) { Return (1) }
+			If (Local0) { Return (3) }
    		Else { Return (0) }
    	}

@@ -1402,7 +1402,7 @@
    		EXCM ()
    	}

-		Method (_PS1)
+		Method (_PS3)
    	{
    		ENCM (0xFF)
    		Store (Zero, HWPW)
diff --git a/src/superio/winbond/w83977tf/acpi/superio.asl b/src/superio/winbond/w83977tf/acpi/superio.asl
index e2ff2ef..c7a62cd 100644
--- a/src/superio/winbond/w83977tf/acpi/superio.asl
+++ b/src/superio/winbond/w83977tf/acpi/superio.asl
@@ -72,12 +72,12 @@
 #define PNP_EXIT_MAGIC_1ST      0xaa
 #include <superio/acpi/pnp_config.asl>

-/* PM: indicate IPD (Immediate Power Down) bit state as D0/D2 */
+/* PM: indicate IPD (Immediate Power Down) bit state as D0/D3 */
 Method (_PSC) {
    ENTER_CONFIG_MODE (0xFF)
    Store (IPD, Local0)
    EXIT_CONFIG_MODE ()
-	If (Local0) { Return (2) }
+	If (Local0) { Return (3) }
    Else { Return (0) }
 }

Change in coreboot[master]: superio: Replace D1/D2 power states with D3

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39746 )

Change subject: superio: Replace D1/D2 power states with D3
......................................................................

Patch Set 5:

Automatic boot test returned (PASS/FAIL/TOTAL): 5/0/5
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1877
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1876
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1875
Non-emulation targets:
HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1879
HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1878

Please note: This test is under development and might not be accurate at all!