Change in coreboot[master]: mb/google/zork: C-state IO base address aligment

[chris wang (Code Review)]

Hello Chris Wang,

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

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

to review the following change.

Change subject: mb/google/zork: C-state IO base address aligment
......................................................................

mb/google/zork: C-state IO base address aligment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/include/soc/iomap.h
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44135/1

diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index 526900a..039e3c2 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -257,6 +257,12 @@
    help
      Base address for the ACPI registers.

+config PICASSO_CSTATE_IO_BASE_ADDR
+	hex
+	default 0x413
+	help
+	  Base address for the CC6 State.
+
 config PICASSO_CONSOLE_UART
    bool "Use Picasso UART controller for console"
    default n
diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h
index 6b9ad2a..360b348 100644
--- a/src/soc/amd/picasso/include/soc/iomap.h
+++ b/src/soc/amd/picasso/include/soc/iomap.h
@@ -64,12 +64,12 @@
 /* I/O Ranges */
 #define ACPI_SMI_CTL_PORT		0xb2
 #define PICASSO_ACPI_IO_BASE	CONFIG_PICASSO_ACPI_IO_BASE
+#define  ACPI_CPU_CONTROL	CONFIG_PICASSO_CSTATE_IO_BASE_ADDR
 #define  ACPI_PM_EVT_BLK	(PICASSO_ACPI_IO_BASE + 0x00)     /* 4 bytes */
 #define  ACPI_PM1_STS		(ACPI_PM_EVT_BLK + 0x00)	  /* 2 bytes */
 #define  ACPI_PM1_EN		(ACPI_PM_EVT_BLK + 0x02)	  /* 2 bytes */
 #define  ACPI_PM1_CNT_BLK	(PICASSO_ACPI_IO_BASE + 0x04)     /* 2 bytes */
 #define  ACPI_PM_TMR_BLK	(PICASSO_ACPI_IO_BASE + 0x08)     /* 4 bytes */
-#define  ACPI_CPU_CONTROL	(PICASSO_ACPI_IO_BASE + 0x0c)     /* 6 bytes */
 /* doc says 0x14 for GPE0_BLK but FT5 only works with 0x20 */
 #define  ACPI_GPE0_BLK		(PICASSO_ACPI_IO_BASE + 0x20)     /* 8 bytes */
 #define  ACPI_GPE0_STS		(ACPI_GPE0_BLK + 0x00)		  /* 4 bytes */

Change in coreboot[master]: mb/google/zork: C-state IO base address alignment

[chris wang (Code Review)]

Hello Chris Wang,

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

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

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

Change subject: mb/google/zork: C-state IO base address alignment
......................................................................

mb/google/zork: C-state IO base address alignment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
Signed-off-by: Chris Wang 
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/include/soc/iomap.h
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44135/2

Change in coreboot[master]: mb/google/zork: C-state IO base address alignment

[chris wang (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Chris Wang,

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

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

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

Change subject: mb/google/zork: C-state IO base address alignment
......................................................................

mb/google/zork: C-state IO base address alignment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/include/soc/iomap.h
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44135/3

Change in coreboot[master]: mb/google/zork: C-state IO base address alignment

[Marshall Dawson (Code Review)]

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

Change subject: mb/google/zork: C-state IO base address alignment
......................................................................

Patch Set 3:

I think a better approach is to establish that coreboot owns this register and AGESA should leave it alone.  So rather than adjust coreboot to match AGESA, how about moving the write of the BSP (in sb_init_acpi_ports()) to model_17_init() to write all cores?  I'll push a change for AGESA to skip the register for FSP.

Change in coreboot[master]: mb/google/zork: C-state IO base address alignment

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: C-state IO base address alignment
......................................................................

Patch Set 3: Code-Review+1

Not sure this particular Kconfig needs to be exactly aligned w/ io base address as I think the cpu is just trapping io to these addresses

Change in coreboot[master]: mb/google/zork: C-state IO base address alignment

[Paul Menzel (Code Review)]

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

Change subject: mb/google/zork: C-state IO base address alignment
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44135/3//COMMIT_MSG@7 
PS3, Line 7: mb/google/zork: C-state IO base address alignment
Please make this a statement by adding a verb (in imperative mood).

Change in coreboot[master]: mb/google/zork: C-state IO base address alignment

[chris wang (Code Review)]

chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: C-state IO base address alignment
......................................................................

Patch Set 3:
> Patch Set 3:
>
> I think a better approach is to establish that coreboot owns this register and AGESA should leave it alone.  So rather than adjust coreboot to match AGESA, how about moving the write of the BSP (in sb_init_acpi_ports()) to model_17_init() to write all cores?  I'll push a change for AGESA to skip the register for FSP.

Yes, agree with you. that's better for program MSR only in coreboot or AGESA.

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[chris wang (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Chris Wang, Aaron Durbin, Felix Held,

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

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

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

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

mb/google/zork: keep the c-state IO base address alignment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/cpu.c
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
4 files changed, 18 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44135/4

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Felix Held (Code Review)]

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

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/Kconfig... 
PS4, Line 260: PICASSO_CSTATE_IO_BASE_ADDR
Does this need to be in the I/O range specified by CONFIG_PICASSO_ACPI_IO_BASE? If so, this should probably be an offset to that base address instead.
Does this need to be changed by some board? If not (which is the case I'd assume without having looked closely into this), I'd prefer to have the offset defined in a header file and use that in all places instead of adding another option, that will need to have a constant value

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/include... 
PS4, Line 72: 0x0c
to my other comment: change this to 0x13 to align things with the AGESA side and don't introduce CONFIG_PICASSO_CSTATE_IO_BASE_ADDR?

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Marshall Dawson (Code Review)]

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

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/cpu.c 
File src/soc/amd/picasso/cpu.c:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/cpu.c@5... 
PS4, Line 56: 
Remove blank line

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[chris wang (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Chris Wang, Aaron Durbin, Felix Held,

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

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

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

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

mb/google/zork: keep the c-state IO base address alignment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
---
M src/soc/amd/picasso/cpu.c
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
3 files changed, 11 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44135/5

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/44135/5/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/44135/5/src/soc/amd/picasso/include... 
PS5, Line 66: 0x413
I think Felix wanted this to be the offset from PICASSO_ACPI_IO_BASE:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/include...

changing the original line 72's offset of 0x0c to 0x13 would suffice in conjunction w/ the other .c file changes.

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[chris wang (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Chris Wang, Aaron Durbin, Felix Held,

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

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

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

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

mb/google/zork: keep the c-state IO base address alignment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
---
M src/soc/amd/picasso/cpu.c
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
3 files changed, 11 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44135/6

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[chris wang (Code Review)]

chris wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 6:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/44135/3//COMMIT_MSG@7 
PS3, Line 7: mb/google/zork: C-state IO base address alignment
> Please make this a statement by adding a verb (in imperative mood).

Done

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/cpu.c 
File src/soc/amd/picasso/cpu.c:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/cpu.c@5... 
PS4, Line 56:
> Remove blank line

Done

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/include... 
PS4, Line 72: 0x0c
> to my other comment: change this to 0x13 to align things with the AGESA side and don't introduce CON […]

Done

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/Kconfig 
File src/soc/amd/picasso/Kconfig:

https://review.coreboot.org/c/coreboot/+/44135/4/src/soc/amd/picasso/Kconfig... 
PS4, Line 260: PICASSO_CSTATE_IO_BASE_ADDR
> Does this need to be in the I/O range specified by CONFIG_PICASSO_ACPI_IO_BASE? If so, this should p […]

I suspect this is just a trap address, but it doesn't hurt to make it an offset.

https://review.coreboot.org/c/coreboot/+/44135/5/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/44135/5/src/soc/amd/picasso/include... 
PS5, Line 66: 0x413
> I think Felix wanted this to be the offset from PICASSO_ACPI_IO_BASE: […]

Done

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Aaron Durbin (Code Review)]

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

mb/google/zork: keep the c-state IO base address alignment

Align the C-state MSR value of BSP with AGESA.

BUG=b:162705221
BRANCH=none
TEST=Check the MSR value is correct and BSP can enter CC6 with AVT tool

Signed-off-by: Chris Wang 
Change-Id: Ib98d34af518439d338326446c20601867ad31690
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44135
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
---
M src/soc/amd/picasso/cpu.c
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
3 files changed, 11 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved

diff --git a/src/soc/amd/picasso/cpu.c b/src/soc/amd/picasso/cpu.c
index c42f400..4d6e98d 100644
--- a/src/soc/amd/picasso/cpu.c
+++ b/src/soc/amd/picasso/cpu.c
@@ -46,6 +46,15 @@
    return 1 + (cpuid_ecx(0x80000008) & 0xff);
 }

+static void set_cstate_io_addr(void)
+{
+	msr_t cst_addr;
+
+	cst_addr.hi = 0;
+	cst_addr.lo = ACPI_CPU_CONTROL;
+	wrmsr(MSR_CSTATE_ADDRESS, cst_addr);
+}
+
 static void fill_in_relocation_params(struct smm_relocation_params *params)
 {
    uintptr_t tseg_base;
@@ -109,6 +118,7 @@
 {
    check_mca();
    setup_lapic();
+	set_cstate_io_addr();

amd_update_microcode_from_cbfs();
 }
diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h
index 6b9ad2a..8256836 100644
--- a/src/soc/amd/picasso/include/soc/iomap.h
+++ b/src/soc/amd/picasso/include/soc/iomap.h
@@ -69,7 +69,7 @@
 #define  ACPI_PM1_EN		(ACPI_PM_EVT_BLK + 0x02)	  /* 2 bytes */
 #define  ACPI_PM1_CNT_BLK	(PICASSO_ACPI_IO_BASE + 0x04)     /* 2 bytes */
 #define  ACPI_PM_TMR_BLK	(PICASSO_ACPI_IO_BASE + 0x08)     /* 4 bytes */
-#define  ACPI_CPU_CONTROL	(PICASSO_ACPI_IO_BASE + 0x0c)     /* 6 bytes */
+#define  ACPI_CPU_CONTROL	(PICASSO_ACPI_IO_BASE + 0x13)
 /* doc says 0x14 for GPE0_BLK but FT5 only works with 0x20 */
 #define  ACPI_GPE0_BLK		(PICASSO_ACPI_IO_BASE + 0x20)     /* 8 bytes */
 #define  ACPI_GPE0_STS		(ACPI_GPE0_BLK + 0x00)		  /* 4 bytes */
diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c
index 4cd24dd..54d7640 100644
--- a/src/soc/amd/picasso/southbridge.c
+++ b/src/soc/amd/picasso/southbridge.c
@@ -233,7 +233,6 @@
 static void sb_init_acpi_ports(void)
 {
    u32 reg;
-	msr_t cst_addr;

/* We use some of these ports in SMM regardless of whether or not
     * ACPI tables are generated. Enable these ports indiscriminately.
@@ -244,11 +243,6 @@
    pm_write16(PM_TMR_BLK, ACPI_PM_TMR_BLK);
    pm_write16(PM_GPE0_BLK, ACPI_GPE0_BLK);

-	/* CpuControl is in _SB.CP00, 6 bytes */
-	cst_addr.hi = 0;
-	cst_addr.lo = ACPI_CPU_CONTROL;
-	wrmsr(MSR_CSTATE_ADDRESS, cst_addr);
-
    if (CONFIG(HAVE_SMI_HANDLER)) {
    	/* APMC - SMI Command Port */
    	pm_write16(PM_ACPI_SMI_CMD, APM_CNT);

Change in coreboot[master]: mb/google/zork: keep the c-state IO base address alignment

[Felix Held (Code Review)]

Attention is currently required from: chris wang.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44135 )

Change subject: mb/google/zork: keep the c-state IO base address alignment
......................................................................

Patch Set 7:

(1 comment)

File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/44135/comment/53572231_b1239479 
PS7, Line 72: 0x13
the 0x13 does look a bit odd to me and the reference code has an offset of 0x10 here in version 1.0.0.c. i'd say that it would be good to align this with the reference code, but i'm not 100% sure how to test it this still works properly after the change