Change in coreboot[master]: soc/amd/common: Add weak function definitions to get ioapic IDs

[Jason Glenesk (Code Review)]

Hello Jason Glenesk,

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

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

to review the following change.

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

soc/amd/common: Add weak function definitions to get ioapic IDs

Add weak definitions for soc_get_fch_ioapic_id() and
soc_get_gnb_ioapic_id(). If not overriddden return CONFIG_MAX_CPUS and
CONFIG_MAX_CPUS +1 respectively.

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

Signed-off-by: Jason Glenesk 
Change-Id: Ib6778bf5141df580759e8158010a63d6af77e8ae
---
M src/soc/amd/common/block/cpu/Makefile.inc
A src/soc/amd/common/block/cpu/cpu.c
A src/soc/amd/common/block/include/amdblocks/cpu.h
3 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/46326/1

diff --git a/src/soc/amd/common/block/cpu/Makefile.inc b/src/soc/amd/common/block/cpu/Makefile.inc
index 395ab08..51e322d 100644
--- a/src/soc/amd/common/block/cpu/Makefile.inc
+++ b/src/soc/amd/common/block/cpu/Makefile.inc
@@ -6,3 +6,5 @@

romstage-$(CONFIG_SOC_AMD_COMMON_BLOCK_CAR) += car/ap_exit_car.S
 romstage-$(CONFIG_SOC_AMD_COMMON_BLOCK_CAR) += car/exit_car.S
+
+ramstage-y += cpu.c
diff --git a/src/soc/amd/common/block/cpu/cpu.c b/src/soc/amd/common/block/cpu/cpu.c
new file mode 100644
index 0000000..7d2d5c9
--- /dev/null
+++ b/src/soc/amd/common/block/cpu/cpu.c
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <types.h>
+#include <amdblocks/cpu.h>
+
+__weak uint8_t soc_get_fch_ioapic_id(void)
+{
+	return CONFIG_MAX_CPUS;
+}
+
+__weak uint8_t soc_get_gnb_ioapic_id(void)
+{
+	return CONFIG_MAX_CPUS + 1;
+}
diff --git a/src/soc/amd/common/block/include/amdblocks/cpu.h b/src/soc/amd/common/block/include/amdblocks/cpu.h
new file mode 100644
index 0000000..7282161
--- /dev/null
+++ b/src/soc/amd/common/block/include/amdblocks/cpu.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __AMDBLOCKS_CPU_H__
+#define __AMDBLOCKS_CPU_H__
+
+#include <types.h>
+
+uint8_t soc_get_fch_ioapic_id(void);
+uint8_t soc_get_gnb_ioapic_id(void);
+
+#endif

[Jason Glenesk (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 1:

Please help to review these changes.

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@8 
PS1, Line 8: 
Please elaborate, why these functions might be needed. Also, why can’t this be configured in the devicetree or as a Kconfig option?

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@10 
PS1, Line 10: overriddden
overridden

[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/+/46326 )

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

soc/amd/common: Add weak function definitions to get ioapic IDs

Add weak definitions for soc_get_fch_ioapic_id() and
soc_get_gnb_ioapic_id(). If not overridden return CONFIG_MAX_CPUS and
CONFIG_MAX_CPUS +1 respectively.

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

Signed-off-by: Jason Glenesk 
Change-Id: Ib6778bf5141df580759e8158010a63d6af77e8ae
---
M src/soc/amd/common/block/cpu/Makefile.inc
A src/soc/amd/common/block/cpu/cpu.c
A src/soc/amd/common/block/include/amdblocks/cpu.h
3 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/46326/2

[Jason Glenesk (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@8 
PS1, Line 8:
> Please elaborate, why these functions might be needed. […]

The helper function helps with the common code, that will not use the program specific Kconfig. It will give the flexibility in the future of moving more ACPI code into common without affecting the older generations.

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@10 
PS1, Line 10: overriddden
> overridden

Done.

[Nico Huber (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@15 
PS1, Line 15: MADT.
How can you tell if the values were taken from these functions and were
not hardcoded somewhere else?

[Jason Glenesk (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@15 
PS1, Line 15: MADT.
> How can you tell if the values were taken from these functions and were […]

In my test image, i set the Kconfig used in the strong override to something other than the hardcoded values in the weak def. The defaults today are functionally the same, but allow the flexibility to specify a different ID for the ioapics for picasso and newer programs.

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46326/1/src/soc/amd/common/block/cp... 
File src/soc/amd/common/block/cpu/cpu.c:

https://review.coreboot.org/c/coreboot/+/46326/1/src/soc/amd/common/block/cp... 
PS1, Line 6: soc_get_fch_ioapic_id
It feels like this one goes under common/block/smbus instead of here.

[Jason Glenesk (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Marshall Dawson, Jason Glenesk, Eric Peers, Rob Barnes,

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

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

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

soc/amd/common: Add weak function definitions to get ioapic IDs

Add weak definitions for soc_get_fch_ioapic_id() and
soc_get_gnb_ioapic_id(). If not overridden return CONFIG_MAX_CPUS and
CONFIG_MAX_CPUS +1 respectively.

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

Signed-off-by: Jason Glenesk 
Change-Id: Ib6778bf5141df580759e8158010a63d6af77e8ae
---
M src/soc/amd/common/block/cpu/Makefile.inc
A src/soc/amd/common/block/cpu/cpu.c
A src/soc/amd/common/block/include/amdblocks/cpu.h
A src/soc/amd/common/block/include/amdblocks/smbus.h
M src/soc/amd/common/block/smbus/smbus.c
5 files changed, 37 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/46326/3

[Jason Glenesk (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 3:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@8 
PS1, Line 8:
> The helper function helps with the common code, that will not use the program specific Kconfig. […]

marking resolved

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@10 
PS1, Line 10: overriddden
> Done.

Done

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@15 
PS1, Line 15: MADT.
> In my test image, i set the Kconfig used in the strong override to something other than the hardcode […]

marking resolved

https://review.coreboot.org/c/coreboot/+/46326/1/src/soc/amd/common/block/cp... 
File src/soc/amd/common/block/cpu/cpu.c:

https://review.coreboot.org/c/coreboot/+/46326/1/src/soc/amd/common/block/cp... 
PS1, Line 6: soc_get_fch_ioapic_id
> It feels like this one goes under common/block/smbus instead of here.

moved it

[Nico Huber (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 3:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@8 
PS1, Line 8:
> marking resolved

I'm a bit confused by the answer. If these functions are supposed to
return constant values, Kconfig is the usual way to solve this. We
try to avoid weak symbols as much as possible as they often hide
errors in code that would otherwise not build at all.

Please not that we are talking about Kconfig options that are _not_
visible to the user. These would get their defaults overridden by
SoC specific code.

https://review.coreboot.org/c/coreboot/+/46326/1//COMMIT_MSG@15 
PS1, Line 15: MADT.
> marking resolved

If I understand it correctly you tested that the follow up patch doesn't
regress non-picasso builds? If you tested a different or altered commit,
please always mention that in the commit message. With this commit alone,
there is nothing testable as the new functions are not called yet, AFAICS.

[Felix Held (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 3:

wouldn't a #define AMD_SOC_FCH_IOAPIC_ID CONFIG_MAX_CPUS and #define AMD_SOC_GNB_IOAPIC_ID (CONFIG_MAX_CPUS + 1) somewhere in a soc-specific header file also do the trick? If there's a reasonably easy way to avoid weak functions, I'd use that instead. just tried that assigning MAX_CPUS + 1 to PICASSO_GNB_IOAPIC_ID won't work due to kconfig not supporting arithmetic for the default values, so that wouldn't be an option here

[Felix Held (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Patch Set 3: Code-Review-1

thought a bit more about this, and since this will only return a per-soc constant and doesn't do anything that that would require it being a function, weak functions aren't the way to go here imho

[Jason Glenesk (Code Review)]

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

Change subject: soc/amd/common: Add weak function definitions to get ioapic IDs
......................................................................

Abandoned

Agree with Felix and Nico that this isn't the right solution