Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id.

[Jason Glenesk (Code Review)]

Hello Jason Glenesk,

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

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

to review the following change.

Change subject: soc/amd/picasso: Override weak functions for ioapic_id.
......................................................................

soc/amd/picasso: Override weak functions for ioapic_id.

Override soc_get_fch_ioapic_id() and soc_get_gnb_ioapic_id() for
picasso. Return CONFIG_PICASSO_FCH_IOAPIC_ID and
CONFIG_PICASSO_GNB_IOAPIC_ID.

BUG=b:167421913
TEST=Boot morphius to shell. Confirm overrides return values by dumping
MADT.
BRANCH=Zork

Signed-off-by: Jason Glenesk 
Change-Id: I7f0403678d7b221e7f6e76c72b701a095f233c97
---
M src/soc/amd/common/block/smbus/sm.c
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/cpu.c
3 files changed, 16 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/46327/1

diff --git a/src/soc/amd/common/block/smbus/sm.c b/src/soc/amd/common/block/smbus/sm.c
index c5c1ed8..d10167e 100644
--- a/src/soc/amd/common/block/smbus/sm.c
+++ b/src/soc/amd/common/block/smbus/sm.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <amdblocks/acpimmio_map.h>
+#include <amdblocks/cpu.h>
 #include <device/device.h>
 #include <device/pci.h>
 #include <device/pci_ids.h>
@@ -16,7 +17,7 @@

static void sm_init(struct device *dev)
 {
-	setup_ioapic(VIO_APIC_VADDR, CONFIG_MAX_CPUS);
+	setup_ioapic(VIO_APIC_VADDR, soc_get_fch_ioapic_id());
 }

static u32 get_sm_mmio(struct device *dev)
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c
index 17940e9..645968f 100644
--- a/src/soc/amd/picasso/acpi.c
+++ b/src/soc/amd/picasso/acpi.c
@@ -19,6 +19,7 @@
 #include <device/pci.h>
 #include <amdblocks/acpimmio.h>
 #include <amdblocks/acpi.h>
+#include <amdblocks/cpu.h>
 #include <soc/acpi.h>
 #include <soc/pci_devs.h>
 #include <soc/cpu.h>
@@ -52,10 +53,10 @@
    current = acpi_create_madt_lapics(current);

current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current,
-			CONFIG_PICASSO_FCH_IOAPIC_ID, IO_APIC_ADDR, 0);
+			soc_get_fch_ioapic_id(), IO_APIC_ADDR, 0);

current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current,
-			CONFIG_PICASSO_GNB_IOAPIC_ID, GNB_IO_APIC_ADDR, IO_APIC_INTERRUPTS);
+			soc_get_gnb_ioapic_id(), GNB_IO_APIC_ADDR, IO_APIC_INTERRUPTS);

/* 0: mean bus 0--->ISA */
    /* 0: PIC 0 */
diff --git a/src/soc/amd/picasso/cpu.c b/src/soc/amd/picasso/cpu.c
index 4d6e98d..5e01e9e 100644
--- a/src/soc/amd/picasso/cpu.c
+++ b/src/soc/amd/picasso/cpu.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <amdblocks/cpu.h>
 #include <cpu/cpu.h>
 #include <cpu/x86/mp.h>
 #include <cpu/x86/mtrr.h>
@@ -139,3 +140,13 @@
    .ops      = &cpu_dev_ops,
    .id_table = cpu_table,
 };
+
+uint8_t soc_get_fch_ioapic_id(void)
+{
+	return CONFIG_PICASSO_FCH_IOAPIC_ID;
+}
+
+uint8_t soc_get_gnb_ioapic_id(void)
+{
+	return CONFIG_PICASSO_GNB_IOAPIC_ID;
+}

[Jason Glenesk (Code Review)]

Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id.
......................................................................

Patch Set 1:

Please help to review these changes.

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso: Override weak functions for ioapic_id.
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG@7 
PS1, Line 7: .
nit: no trailing period here

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/picasso: Override weak functions for ioapic_id.
......................................................................

Patch Set 1:

(2 comments)

Can’t this be done all in Kconfig?

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG@11 
PS1, Line 11: CONFIG_PICASSO_GNB_IOAPIC_ID.
Why are other values needed?

https://review.coreboot.org/c/coreboot/+/46327/1/src/soc/amd/common/block/sm... 
File src/soc/amd/common/block/smbus/sm.c:

https://review.coreboot.org/c/coreboot/+/46327/1/src/soc/amd/common/block/sm... 
PS1, Line 20: 	setup_ioapic(VIO_APIC_VADDR, soc_get_fch_ioapic_id());
Shouldn’t this go into the other commit?

[Jason Glenesk (Code Review)]

Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id.
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (2 comments)
>
> Can’t this be done all in Kconfig?

Before Picasso, these values were hardcoded to MAX_CPU and MAX_CPU +1. Adding the helper function will help with any common code where the older program won't utilize the Kconfig option. The more i've been thinking about this, i'd like to look at the design for some of this ACPI stuff and redesign into common code so we don't have to copy paste each generation.

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Jason Glenesk (Code Review)]

Jason Glenesk has uploaded a new patch set (#2) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

soc/amd/picasso: Override weak functions for ioapic_id

Override soc_get_fch_ioapic_id() and soc_get_gnb_ioapic_id() for
picasso. Return CONFIG_PICASSO_FCH_IOAPIC_ID and
CONFIG_PICASSO_GNB_IOAPIC_ID.

BUG=b:167421913
TEST=Boot morphius to shell. Confirm overrides return values by dumping
MADT.
BRANCH=Zork

Signed-off-by: Jason Glenesk 
Change-Id: I7f0403678d7b221e7f6e76c72b701a095f233c97
---
M src/soc/amd/common/block/smbus/sm.c
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/cpu.c
3 files changed, 16 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/46327/2

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Jason Glenesk (Code Review)]

Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG@7 
PS1, Line 7: .
> nit: no trailing period here

I'll remove it.

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG@11 
PS1, Line 11: CONFIG_PICASSO_GNB_IOAPIC_ID.
> Why are other values needed?

This new design allows coreboot to dictate what the ioapic IDs will be and communicate to the FSP what it wants to use. It will put coreboot in the driverseat and allow more flexibility.

https://review.coreboot.org/c/coreboot/+/46327/1/src/soc/amd/common/block/sm... 
File src/soc/amd/common/block/smbus/sm.c:

https://review.coreboot.org/c/coreboot/+/46327/1/src/soc/amd/common/block/sm... 
PS1, Line 20: 	setup_ioapic(VIO_APIC_VADDR, soc_get_fch_ioapic_id());
> Shouldn’t this go into the other commit?

i could have. when i submitted it made sense to make the change for all consumers of the helper function together.

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

Patch Set 2:

I believe fsp_assign_ioapic_upds() in fsp_params.c is another place that should use the new call.

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Jason Glenesk (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Jason Glenesk, Angel Pons,

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

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

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

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

soc/amd/picasso: Override weak functions for ioapic_id

Override soc_get_fch_ioapic_id() and soc_get_gnb_ioapic_id() for
picasso. Return CONFIG_PICASSO_FCH_IOAPIC_ID and
CONFIG_PICASSO_GNB_IOAPIC_ID.

BUG=b:167421913
TEST=Boot morphius to shell. Confirm overrides return values by dumping
MADT.
BRANCH=Zork

Signed-off-by: Jason Glenesk 
Change-Id: I7f0403678d7b221e7f6e76c72b701a095f233c97
---
M src/soc/amd/common/block/smbus/sm.c
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/agesa_acpi.c
M src/soc/amd/picasso/cpu.c
M src/soc/amd/picasso/fsp_params.c
A src/soc/amd/picasso/smbus.c
7 files changed, 29 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/46327/3

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46327/3/src/soc/amd/picasso/smbus.c 
File src/soc/amd/picasso/smbus.c:

https://review.coreboot.org/c/coreboot/+/46327/3/src/soc/amd/picasso/smbus.c... 
PS3, Line 8: }
adding a line without newline at end of file

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Jason Glenesk (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Angel Pons,

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

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

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

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

soc/amd/picasso: Override weak functions for ioapic_id

Override soc_get_fch_ioapic_id() and soc_get_gnb_ioapic_id() for
picasso. Return CONFIG_PICASSO_FCH_IOAPIC_ID and
CONFIG_PICASSO_GNB_IOAPIC_ID.

BUG=b:167421913
TEST=Boot morphius to shell. Confirm overrides return values by dumping
MADT.
BRANCH=Zork

Signed-off-by: Jason Glenesk 
Change-Id: I7f0403678d7b221e7f6e76c72b701a095f233c97
---
M src/soc/amd/common/block/smbus/sm.c
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/agesa_acpi.c
M src/soc/amd/picasso/cpu.c
M src/soc/amd/picasso/fsp_params.c
A src/soc/amd/picasso/smbus.c
7 files changed, 29 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/46327/4

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Jason Glenesk (Code Review)]

Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46327/1//COMMIT_MSG@11 
PS1, Line 11: CONFIG_PICASSO_GNB_IOAPIC_ID.
> This new design allows coreboot to dictate what the ioapic IDs will be and communicate to the FSP wh […]

marking resolved

https://review.coreboot.org/c/coreboot/+/46327/1/src/soc/amd/common/block/sm... 
File src/soc/amd/common/block/smbus/sm.c:

https://review.coreboot.org/c/coreboot/+/46327/1/src/soc/amd/common/block/sm... 
PS1, Line 20: 	setup_ioapic(VIO_APIC_VADDR, soc_get_fch_ioapic_id());
> i could have. […]

marking resolved

Change in coreboot[master]: soc/amd/picasso: Override weak functions for ioapic_id

[Jason Glenesk (Code Review)]

Jason Glenesk has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/46327 )

Change subject: soc/amd/picasso: Override weak functions for ioapic_id
......................................................................

Abandoned