Change in coreboot[master]: soc/amd/picasso: Notify PSP system is going to sleep state

[Felix Held (Code Review)]

Hello Marshall Dawson,

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

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

to review the following change.

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

soc/amd/picasso: Notify PSP system is going to sleep state

Signed-off-by: Marshall Dawson 
Change-Id: Ic72bd5f5710181ca4f282feba5f7531b098c907a
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/40298/1

diff --git a/src/soc/amd/picasso/smihandler.c b/src/soc/amd/picasso/smihandler.c
index e4d1c86..f29fffb 100644
--- a/src/soc/amd/picasso/smihandler.c
+++ b/src/soc/amd/picasso/smihandler.c
@@ -145,6 +145,30 @@
    smi_write32(SMI_REG_SMISTS4, smi_read32(SMI_REG_SMISTS4));
 }

+static void send_sx_info(uint8_t slptyp)
+{
+	uint8_t sx;
+
+	switch (slptyp) {
+	case ACPI_S0:
+		sx = 0;
+		break;
+	case ACPI_S3:
+		sx = 3;
+		break;
+	case ACPI_S4:
+		sx = 4;
+		break;
+	case ACPI_S5:
+		sx = 5;
+		break;
+	default:
+		return;
+	}
+
+	psp_notify_sx_info(sx);
+}
+
 static void sb_slp_typ_handler(void)
 {
    uint32_t pci_ctrl, reg32;
@@ -220,12 +244,16 @@
    					reg32);
    	} /* if (CONFIG(ELOG_GSMI)) */

+		send_sx_info(slp_typ);
+
    	/*
    	 * An IO cycle is required to trigger the STPCLK/STPGNT
    	 * handshake when the Pm1 write is reissued.
    	 */
    	outw(pm1cnt | SLP_EN, pm_read16(PM1_CNT_BLK));
    	hlt();
+	} else {
+		send_sx_info(slp_typ);
    }
 }

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
File src/soc/amd/picasso/smihandler.c:

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
PS1, Line 148: slptyp
nit: code below uses `slp_typ`, maybe use it here for consistency?

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
PS1, Line 152: 	switch (slptyp) {
Given that `ACPI_Sx == x`, this looks odd. Is it intentional?

[Felix Held (Code Review)]

Hello build bot (Jenkins), Marshall Dawson, Angel Pons,

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

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

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

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

soc/amd/picasso: Notify PSP system is going to sleep state

BUG=b:153677737

Signed-off-by: Marshall Dawson 
Change-Id: Ic72bd5f5710181ca4f282feba5f7531b098c907a
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/40298/2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
File src/soc/amd/picasso/smihandler.c:

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
PS1, Line 148: slptyp
> nit: code below uses `slp_typ`, maybe use it here for consistency?

Done

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
PS1, Line 152: 	switch (slptyp) {
> Given that `ACPI_Sx == x`, this looks odd. […]

I think this is to ignore invalid sleep types

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded a new patch set (#5) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

soc/amd/picasso: Notify PSP system is going to sleep state

BUG=b:153677737

Signed-off-by: Marshall Dawson 
Change-Id: Ic72bd5f5710181ca4f282feba5f7531b098c907a
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/40298/5

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded a new patch set (#6) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

soc/amd/picasso: Notify PSP system is going to sleep state

BUG=b:153677737

Signed-off-by: Marshall Dawson 
Change-Id: Ic72bd5f5710181ca4f282feba5f7531b098c907a
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/40298/6

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
File src/soc/amd/picasso/smihandler.c:

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
PS1, Line 148: slptyp
> Done

N/A now.  Have pushed CB:40338 and CB:40339

https://review.coreboot.org/c/coreboot/+/40298/1/src/soc/amd/picasso/smihand... 
PS1, Line 152: 	switch (slptyp) {
> Given that `ACPI_Sx == x`, this looks odd. […]

It was intentional but I never quite liked it.  We really need to ensure we're using the actual values and not enum'ed ones.  This all goes away with CB:40338 and CB:40339 though.

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded a new patch set (#7) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

soc/amd/picasso: Notify PSP system is going to sleep state

BUG=b:153677737

Signed-off-by: Marshall Dawson 
Change-Id: Ic72bd5f5710181ca4f282feba5f7531b098c907a
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/40298/7

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 7: Code-Review+2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 7: Code-Review-1

the other patches need to be ready in order for this to get merged

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40298 )

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 8: -Code-Review

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Notify PSP system is going to sleep state
......................................................................

Patch Set 8: Code-Review+2