Change in coreboot[master]: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42576 )

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant

All other variants have no mptable.c, but HAVE_MP_TABLE was selected for
all variants, which should only be selected if they provided a
mptable.c.

Change-Id: I7bd6e774f847289a2b51392b72be7cd55fe98c63
Signed-off-by: Felix Held 
---
M src/mainboard/asus/p2b/Kconfig
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/42576/1

diff --git a/src/mainboard/asus/p2b/Kconfig b/src/mainboard/asus/p2b/Kconfig
index 00295be..f1b3d40 100644
--- a/src/mainboard/asus/p2b/Kconfig
+++ b/src/mainboard/asus/p2b/Kconfig
@@ -5,7 +5,6 @@
 config BASE_ASUS_P2B_D
    def_bool n
    select SDRAMPWR_4DIMM
-	select HAVE_MP_TABLE
    select IOAPIC
    select SMP

@@ -20,6 +19,7 @@
    select SDRAMPWR_4DIMM if BOARD_ASUS_P2B_LS || BOARD_ASUS_P3B_F
    select HAVE_ACPI_TABLES if BOARD_ASUS_P2B || BOARD_ASUS_P2B_LS
    select BASE_ASUS_P2B_D if BOARD_ASUS_P2B_D || BOARD_ASUS_P2B_DS
+	select HAVE_MP_TABLE if BASE_ASUS_P2B_D

config MAX_CPUS
    int

[Angel Pons (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1: Code-Review-1

Um, this changes nothing?

[Felix Held (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
>
> Um, this changes nothing?

oh...

[Felix Held (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1: Code-Review-2

the issue here is a different one; likely during making the different boards variants of each other the mptable.c got moved to the variant directory where the build system doesn't look for it. So the file wasn't included, but the build system didn't complain due to the issue CB:42572 fixes

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42576 )

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1: Code-Review-1

Looks like I was late to the game, but this was never an issue.

In line 23 of mb/asus/p2b/Kconfig:
        select BASE_ASUS_P2B_D if BOARD_ASUS_P2B_D || BOARD_ASUS_P2B_DS

HAVE_MP_TABLE is only selected if BASE_ASUS_P2B_D is selected, ie. only D and DS variants.

I just ran make olddefconfig on a p3b-f config and *_MP_TABLE is not present in .config.

[Felix Held (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1:

the comments on https://review.coreboot.org/c/coreboot/+/42572/ might point where this should be going. Do you want to take over those two patches? feel free to do so. i put a -2 on this patch, because it doesn't fix the issue; misread how this was selecting things

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42576 )

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1:

Still a non-issue, because GENERATE_MP_TABLE depends on HAVE_MP_TABLE which this Kconfig fragment only sets for P2B-D[S].

Even if I wanted to, I probably am unable to because I don't have two matching slot 1 CPUs and my only P2B-DS board seems to have failed.

I say abandon this patch and move on.

[Felix Held (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Still a non-issue, because GENERATE_MP_TABLE depends on HAVE_MP_TABLE which this Kconfig fragment only sets for P2B-D[S].

This patch is wrong at the moment and doesn't fix the issue. The real issue is that build system doesn't find the mptable.c since it's not in the mainboard directory, but in the variant directory where the build system doesn't expect it. At the moment the mptable.c files aren't included in the build due to that. This needs to be fixed in order for me to land an updated version of https://review.coreboot.org/c/coreboot/+/42572/ which prevents such things in the future. Probably the way to go is to allow the mainboard to specify the path to the mptable.c file as an overide for the default location, but still fail the build if that also results in no file

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42576 )

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1:
> > Still a non-issue, because GENERATE_MP_TABLE depends on HAVE_MP_TABLE which this Kconfig fragment only sets for P2B-D[S].
>
> This patch is wrong at the moment and doesn't fix the issue. The real issue is that build system doesn't find the mptable.c since it's not in the mainboard directory, but in the variant directory where the build system doesn't expect it. At the moment the mptable.c files aren't included in the build due to that. This needs to be fixed in order for me to land an updated version of https://review.coreboot.org/c/coreboot/+/42572/ which prevents such things in the future. Probably the way to go is to allow the mainboard to specify the path to the mptable.c file as an overide for the default location, but still fail the build if that also results in no file

When I did the variant conversion, I add mptable.c as a ramstage compile unit via Makefile.inc based on GENERATE_MP_TABLE, following the pattern of other boards. If you just call write_smp_table(), and it isn't defined (in mptable.c or elsewhere) the build will fail. No override needed. Let mainboard be responsible for providing mptable.c. (This does not preclude sharing if appropriate.) I hope this points you in the right direction.

But yes, this patch is still wrong.

[Felix Held (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Abandoned

since the new patch CB:42604 does something entirely different, I'll abandon the patch as suggested

[Felix Held (Code Review)]

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

Change subject: mb/asus/p2b: only select HAVE_MP_TABLE for D and DS variant
......................................................................

Patch Set 1:
> When I did the variant conversion, I add mptable.c as a ramstage compile unit via Makefile.inc based on GENERATE_MP_TABLE, following the pattern of other boards. If you just call write_smp_table(), and it isn't defined (in mptable.c or elsewhere) the build will fail. No override needed. Let mainboard be responsible for providing mptable.c. (This does not preclude sharing if appropriate.) I hope this points you in the right direction.

Oh, i should have looked if there are new comments after i re-did the change. Yeah, you're right; didn't look at the Makefile.inc
> But yes, this patch is still wrong.

I agree.