Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31744
Change subject: x86/car: Fix incorrect config usage ......................................................................
x86/car: Fix incorrect config usage
This IS_ENABLED(XXX) line should've clearly been IS_ENABLED(CONFIG_XXX). This patch can fix that. However, I don't have (and don't plan to acquire) an affected system to test, so approve at your own risk (or let me know if I should just remove that check instead).
Change-Id: I74e0ed4d04471ee521ff5c69a74a6f4c949e5847 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/cpu/x86/car.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/31744/1
diff --git a/src/cpu/x86/car.c b/src/cpu/x86/car.c index 1b02f8b..1a99c36 100644 --- a/src/cpu/x86/car.c +++ b/src/cpu/x86/car.c @@ -142,7 +142,7 @@
static void car_migrate_variables(int is_recovery) { - if (!IS_ENABLED(PLATFORM_USES_FSP1_0)) + if (!IS_ENABLED(CONFIG_PLATFORM_USES_FSP1_0)) do_car_migrate_variables(); } ROMSTAGE_CBMEM_INIT_HOOK(car_migrate_variables)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31744 )
Change subject: x86/car: Fix incorrect config usage ......................................................................
Patch Set 1: Code-Review+1
If I read this correctly, FSP1_0 platforms called do_car_migrate_variables() when it should not have. However, as the call is made after FSP1_0 already has torn CAR down, car_migrated variable would return non-zero on line 127.
The code on FSP1_0 CAR migration was originally tested on intel/minnowmax.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31744 )
Change subject: x86/car: Fix incorrect config usage ......................................................................
Patch Set 1: Code-Review+2
I have had a closer look at this issue on mc_bdx1 (fsp_broadwell_de) and mc_tcu3 (fsp_baytrail) which are both fsp1.0 based. Currently (with this error still there) do_car_migrate_variables() is exited on line 127 (as Kyösti pointed out already). So there is no real work that is done here and "removing" it for fsp 1.0 based platforms should not harm.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31744 )
Change subject: x86/car: Fix incorrect config usage ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31744 )
Change subject: x86/car: Fix incorrect config usage ......................................................................
Patch Set 1: Code-Review+1
Maybe the commit message can be updated with Kyösti’s and Werner’s findings.
Hello Kyösti Mälkki, Werner Zeh, Aaron Durbin, Piotr Król, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31744
to look at the new patch set (#2).
Change subject: x86/car: Fix incorrect config usage ......................................................................
x86/car: Fix incorrect config usage
This IS_ENABLED(XXX) line should've clearly been IS_ENABLED(CONFIG_XXX). This patch fixes the issue. Not tested on a real board, but looking at the affected code paths suggests that this will result in no effective change anywhere (since CAR should already be torn down by the time this is called on FSP1.0 boards, so do_car_migrate_variables() would have immediately exited anyway).
Change-Id: I74e0ed4d04471ee521ff5c69a74a6f4c949e5847 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/cpu/x86/car.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/31744/2
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31744 )
Change subject: x86/car: Fix incorrect config usage ......................................................................
x86/car: Fix incorrect config usage
This IS_ENABLED(XXX) line should've clearly been IS_ENABLED(CONFIG_XXX). This patch fixes the issue. Not tested on a real board, but looking at the affected code paths suggests that this will result in no effective change anywhere (since CAR should already be torn down by the time this is called on FSP1.0 boards, so do_car_migrate_variables() would have immediately exited anyway).
Change-Id: I74e0ed4d04471ee521ff5c69a74a6f4c949e5847 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/31744 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Werner Zeh werner.zeh@siemens.com Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/cpu/x86/car.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve Werner Zeh: Looks good to me, approved
diff --git a/src/cpu/x86/car.c b/src/cpu/x86/car.c index 1b02f8b..1a99c36 100644 --- a/src/cpu/x86/car.c +++ b/src/cpu/x86/car.c @@ -142,7 +142,7 @@
static void car_migrate_variables(int is_recovery) { - if (!IS_ENABLED(PLATFORM_USES_FSP1_0)) + if (!IS_ENABLED(CONFIG_PLATFORM_USES_FSP1_0)) do_car_migrate_variables(); } ROMSTAGE_CBMEM_INIT_HOOK(car_migrate_variables)
-
Julius Werner (Code Review) -
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)
-
Werner Zeh (Code Review)