Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
AGESA,binaryPI: Split romstage_main() to BSP and AP parts
BSP and AP have two distinct execution paths for romstage.
Change-Id: Id013b165f1345509fe6b74cef2bf8c3b420f84a4 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/amd/agesa/romstage.c M src/include/cpu/amd/car.h 2 files changed, 29 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/37326/1
diff --git a/src/drivers/amd/agesa/romstage.c b/src/drivers/amd/agesa/romstage.c index 0ecfeb2..5a40ba8 100644 --- a/src/drivers/amd/agesa/romstage.c +++ b/src/drivers/amd/agesa/romstage.c @@ -40,7 +40,7 @@ agesa_set_interface(cb); }
-void *asmlinkage romstage_main(unsigned long bist) +static void bsp_romstage_main(unsigned long bist) { struct postcar_frame pcf; struct sysinfo romstage_state; @@ -53,7 +53,7 @@
fill_sysinfo(cb);
- if ((initial_apic_id == 0) && boot_cpu()) { + if (initial_apic_id == 0) {
timestamp_init(timestamp_get()); timestamp_add_now(TS_START_ROMSTAGE); @@ -101,5 +101,30 @@
run_postcar_phase(&pcf); /* We do not return. */ - return NULL; +} + +static void __noreturn ap_romstage_main(void) +{ + struct sysinfo romstage_state; + struct sysinfo *cb = &romstage_state; + + /* Enable PCI MMIO configuration. */ + amd_initmmio(); + + fill_sysinfo(cb); + + agesa_execute_state(cb, AMD_INIT_RESET); + + agesa_execute_state(cb, AMD_INIT_EARLY); + + /* Not reached. */ + halt(); +} + +void asmlinkage romstage_main(unsigned long bist) +{ + if (boot_cpu()) + bsp_romstage_main(bist); + else + ap_romstage_main(); } diff --git a/src/include/cpu/amd/car.h b/src/include/cpu/amd/car.h index 46f7e1d..61c2388 100644 --- a/src/include/cpu/amd/car.h +++ b/src/include/cpu/amd/car.h @@ -5,6 +5,6 @@
void cache_as_ram_main(unsigned long bist, unsigned long cpu_init_detectedx);
-void *asmlinkage romstage_main(unsigned long bist); +void asmlinkage romstage_main(unsigned long bist);
#endif
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 124: void asmlinkage romstage_main(unsigned long bist) storage class 'asmlinkage' should be located before type 'void'
https://review.coreboot.org/c/coreboot/+/37326/1/src/include/cpu/amd/car.h File src/include/cpu/amd/car.h:
https://review.coreboot.org/c/coreboot/+/37326/1/src/include/cpu/amd/car.h@8 PS1, Line 8: void asmlinkage romstage_main(unsigned long bist); storage class 'asmlinkage' should be located before type 'void'
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 56: if (initial_apic_id == 0) { Is this check still needed?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 106: ap_romstage_main no bist checking?
Hello Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37326
to look at the new patch set (#2).
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
AGESA,binaryPI: Split romstage_main() to BSP and AP parts
BSP and AP have two distinct execution paths for romstage.
Change-Id: Id013b165f1345509fe6b74cef2bf8c3b420f84a4 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/amd/agesa/romstage.c M src/include/cpu/amd/car.h 2 files changed, 32 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/37326/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 2: Code-Review+2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 2: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 56: if (initial_apic_id == 0) {
Is this check still needed?
Good question. I believe this originates from multi-socket platforms and the intention was for only one core to execute this initialisation path. For each socket, one core would reports as boot_cpu() from what I remember.
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 106: ap_romstage_main
no bist checking?
Ack
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 56: if (initial_apic_id == 0) {
Good question. I believe this originates from multi-socket platforms and the intention was for only one core to execute this initialisation path. For each socket, one core would reports as boot_cpu() from what I remember.
Only core0 (BSC) on BSP (BKDG terminology) should have bit8 set (bsc bit) in lapic_base according to the fam 15h 00-0fh BKDG. It could be different on other families/variants or in actual silicon...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/37326/1/src/drivers/amd/agesa/romst... PS1, Line 56: if (initial_apic_id == 0) {
Good question. […]
CB:18624 has some history, it was originally called cpu_init_detected. It could be that AGESA uses it to detect if we re-entered romstage another time after system reset. If that still happens, it's a bit nasty for verstage and we might need to move the test to bootblock.
It needs some further investigation but would be followup and not change this commit.
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37326 )
Change subject: AGESA,binaryPI: Split romstage_main() to BSP and AP parts ......................................................................
AGESA,binaryPI: Split romstage_main() to BSP and AP parts
BSP and AP have two distinct execution paths for romstage.
Change-Id: Id013b165f1345509fe6b74cef2bf8c3b420f84a4 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37326 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/amd/agesa/romstage.c M src/include/cpu/amd/car.h 2 files changed, 32 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved
diff --git a/src/drivers/amd/agesa/romstage.c b/src/drivers/amd/agesa/romstage.c index 0ecfeb2..f9a8c97 100644 --- a/src/drivers/amd/agesa/romstage.c +++ b/src/drivers/amd/agesa/romstage.c @@ -40,7 +40,7 @@ agesa_set_interface(cb); }
-void *asmlinkage romstage_main(unsigned long bist) +static void bsp_romstage_main(unsigned long bist) { struct postcar_frame pcf; struct sysinfo romstage_state; @@ -53,7 +53,7 @@
fill_sysinfo(cb);
- if ((initial_apic_id == 0) && boot_cpu()) { + if (initial_apic_id == 0) {
timestamp_init(timestamp_get()); timestamp_add_now(TS_START_ROMSTAGE); @@ -101,5 +101,33 @@
run_postcar_phase(&pcf); /* We do not return. */ - return NULL; +} + +static void __noreturn ap_romstage_main(unsigned long bist) +{ + struct sysinfo romstage_state; + struct sysinfo *cb = &romstage_state; + + /* Enable PCI MMIO configuration. */ + amd_initmmio(); + + fill_sysinfo(cb); + + /* Halt if there was a built in self test failure */ + report_bist_failure(bist); + + agesa_execute_state(cb, AMD_INIT_RESET); + + agesa_execute_state(cb, AMD_INIT_EARLY); + + /* Not reached. */ + halt(); +} + +asmlinkage void romstage_main(unsigned long bist) +{ + if (boot_cpu()) + bsp_romstage_main(bist); + else + ap_romstage_main(bist); } diff --git a/src/include/cpu/amd/car.h b/src/include/cpu/amd/car.h index be7b69a..7e2fccd 100644 --- a/src/include/cpu/amd/car.h +++ b/src/include/cpu/amd/car.h @@ -3,6 +3,6 @@
#include <arch/cpu.h>
-void *asmlinkage romstage_main(unsigned long bist); +asmlinkage void romstage_main(unsigned long bist);
#endif
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Michał Żygowski (Code Review)