Change in coreboot[master]: soc/amd/picasso: Add common PSP support

[Felix Held (Code Review)]

Hello Marshall Dawson, Marshall Dawson, Eric Peers,

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

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

to review the following change.

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

soc/amd/picasso: Add common PSP support

Add a new psp.c file so the base address can be determined, and select
the common/block/psp feature.

Change-Id: I322fd11a867a817375ff38a008219f9236c4f2ea
Signed-off-by: Marshall Dawson 
Reviewed-on: https://chromium-review.googlesource.com/2020368
Tested-by: Eric Peers 
Reviewed-by: Eric Peers 
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/Makefile.inc
A src/soc/amd/picasso/psp.c
3 files changed, 26 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/40296/1

diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index d9211b4..5ea02f1 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -47,6 +47,7 @@
    select SOC_AMD_COMMON_BLOCK_HDA
    select SOC_AMD_COMMON_BLOCK_SATA
    select SOC_AMD_COMMON_BLOCK_SMBUS
+	select SOC_AMD_COMMON_BLOCK_PSP
    select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
    select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH
    select PARALLEL_MP
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 2f4f00b..b79f627 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -44,6 +44,7 @@
 romstage-y += southbridge.c
 romstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c
 romstage-y += soc_util.c
+romstage-y += psp.c

verstage-y += gpio.c
 verstage-y += i2c.c
@@ -76,6 +77,7 @@
 ramstage-y += tsc_freq.c
 ramstage-y += finalize.c
 ramstage-y += soc_util.c
+ramstage-y += psp.c

all-y += reset.c

@@ -84,6 +86,7 @@
 smm-y += tsc_freq.c
 smm-$(CONFIG_DEBUG_SMI) += uart.c
 smm-y += gpio.c
+smm-y += psp.c

CPPFLAGS_common += -I$(src)/soc/amd/picasso
 CPPFLAGS_common += -I$(src)/soc/amd/picasso/include
diff --git a/src/soc/amd/picasso/psp.c b/src/soc/amd/picasso/psp.c
new file mode 100644
index 0000000..9780627
--- /dev/null
+++ b/src/soc/amd/picasso/psp.c
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* This file is part of the coreboot project. */
+
+#include <console/console.h>
+#include <cpu/x86/msr.h>
+#include <amdblocks/psp.h>
+
+#define PSP_MAILBOX_OFFSET		0x10570
+#define MSR_CU_CBBCFG			0xc00110a2
+
+void *soc_get_mbox_address(void)
+{
+	uintptr_t psp_mmio;
+
+	psp_mmio = rdmsr(MSR_CU_CBBCFG).lo;
+	if (psp_mmio == 0xffffffff) {
+		printk(BIOS_WARNING, "PSP: MSR value uninitialized\n");
+		return 0;
+	}
+
+	return (void *)(psp_mmio + PSP_MAILBOX_OFFSET);
+}

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/1/src/soc/amd/picasso/psp.c 
File src/soc/amd/picasso/psp.c:

https://review.coreboot.org/c/coreboot/+/40296/1/src/soc/amd/picasso/psp.c@1... 
PS1, Line 17: MSR value
Maybe mention which MSR this is?

[Felix Held (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Marshall Dawson, Angel Pons, Eric Peers,

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

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

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

soc/amd/picasso: Add common PSP support

Add a new psp.c file so the base address can be determined, and select
the common/block/psp feature.

BUG=b:153677737

Change-Id: I322fd11a867a817375ff38a008219f9236c4f2ea
Signed-off-by: Marshall Dawson 
Reviewed-on: https://chromium-review.googlesource.com/2020368
Tested-by: Eric Peers 
Reviewed-by: Eric Peers 
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/Makefile.inc
A src/soc/amd/picasso/psp.c
3 files changed, 26 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/40296/2

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/1/src/soc/amd/picasso/psp.c 
File src/soc/amd/picasso/psp.c:

https://review.coreboot.org/c/coreboot/+/40296/1/src/soc/amd/picasso/psp.c@1... 
PS1, Line 17: MSR value
> Maybe mention which MSR this is?

Done

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig... 
PS3, Line 50: 	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
Huh, that selecting this doesn't cause any linker errors about `soc_fill_smm_trig_info` and `soc_fill_smm_reg_info` probably means it's not being build-tested, or am I mistaken?

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig... 
PS3, Line 50: 	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
> Huh, that selecting this doesn't cause any linker errors about `soc_fill_smm_trig_info` and `soc_fil […]

soc/amd/picasso is currently not boot tested by upstream Jenkins, since mb/amd/mandolin hasn't landed yet; that depends on some blobs being upstreamed and the mainboard code cleaned up. I did a local build&boot test on this whole patch train though; didn't try every single commit though. Had a quick look into this and yes, this commit likely needs to be swapped with the next one

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig... 
PS3, Line 50: 	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
> soc/amd/picasso is currently not boot tested by upstream Jenkins, since mb/amd/mandolin hasn't lande […]

ok, it's not that easy. any objections to squashing this change with the next one? not sure how to properly fix this without having significantly more sort-of useless work

[Raul Rangel (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig... 
PS3, Line 50: 	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
> ok, it's not that easy. […]

Just move this select into a later CL.

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig... 
PS3, Line 50: 	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
> Just move this select into a later CL.

reordered patches; will push the fixed follow-up patches later

[Raul Rangel (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 4: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/40296/3/src/soc/amd/picasso/Kconfig... 
PS3, Line 50: 	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
> reordered patches; will push the fixed follow-up patches later

I don't mind, but wanted to know if it was because no board uses the code yet

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40296 )

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

soc/amd/picasso: Add common PSP support

Add a new psp.c file so the base address can be determined, and select
the common/block/psp feature.

BUG=b:153677737

Change-Id: I322fd11a867a817375ff38a008219f9236c4f2ea
Signed-off-by: Marshall Dawson 
Reviewed-on: https://chromium-review.googlesource.com/2020368
Tested-by: Eric Peers 
Reviewed-by: Eric Peers 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40296
Reviewed-by: Raul Rangel 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/Makefile.inc
A src/soc/amd/picasso/psp.c
3 files changed, 26 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Raul Rangel: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index d9211b4..842ba0c 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -47,6 +47,7 @@
    select SOC_AMD_COMMON_BLOCK_HDA
    select SOC_AMD_COMMON_BLOCK_SATA
    select SOC_AMD_COMMON_BLOCK_SMBUS
+	select SOC_AMD_COMMON_BLOCK_PSP_GEN2
    select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
    select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH
    select PARALLEL_MP
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 2f4f00b..b79f627 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -44,6 +44,7 @@
 romstage-y += southbridge.c
 romstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c
 romstage-y += soc_util.c
+romstage-y += psp.c

verstage-y += gpio.c
 verstage-y += i2c.c
@@ -76,6 +77,7 @@
 ramstage-y += tsc_freq.c
 ramstage-y += finalize.c
 ramstage-y += soc_util.c
+ramstage-y += psp.c

all-y += reset.c

@@ -84,6 +86,7 @@
 smm-y += tsc_freq.c
 smm-$(CONFIG_DEBUG_SMI) += uart.c
 smm-y += gpio.c
+smm-y += psp.c

CPPFLAGS_common += -I$(src)/soc/amd/picasso
 CPPFLAGS_common += -I$(src)/soc/amd/picasso/include
diff --git a/src/soc/amd/picasso/psp.c b/src/soc/amd/picasso/psp.c
new file mode 100644
index 0000000..d6eb7d3
--- /dev/null
+++ b/src/soc/amd/picasso/psp.c
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* This file is part of the coreboot project. */
+
+#include <console/console.h>
+#include <cpu/x86/msr.h>
+#include <amdblocks/psp.h>
+
+#define PSP_MAILBOX_OFFSET		0x10570
+#define MSR_CU_CBBCFG			0xc00110a2
+
+void *soc_get_mbox_address(void)
+{
+	uintptr_t psp_mmio;
+
+	psp_mmio = rdmsr(MSR_CU_CBBCFG).lo;
+	if (psp_mmio == 0xffffffff) {
+		printk(BIOS_WARNING, "PSP: MSR_CU_CBBCFG uninitialized\n");
+		return 0;
+	}
+
+	return (void *)(psp_mmio + PSP_MAILBOX_OFFSET);
+}

[9elements QA (Code Review)]

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

Change subject: soc/amd/picasso: Add common PSP support
......................................................................

Patch Set 5:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2334
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2333
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2332

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