Attention is currently required from: Arthur Heymans, Jason Glenesk, Furquan Shaikh, Marshall Dawson, Patrick Rudolph, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55067 )
Change subject: arch/x86: Add a common romstage entry ......................................................................
Patch Set 5:
(9 comments)
File src/arch/x86/romstage.c:
https://review.coreboot.org/c/coreboot/+/55067/comment/011bb3fb_1bdb4a7b PS2, Line 7: static void _romstage_main(void)
Does this need to be a separate function?
Done
File src/arch/x86/romstage.c:
https://review.coreboot.org/c/coreboot/+/55067/comment/f53726cd_ca7173a0 PS5, Line 3: #include <arch/cpu.h> : #include <console/console.h> : #include <timestamp.h> : #include <romstage_common.h> nit: order includes alphabetically?
File src/include/romstage_common.h:
https://review.coreboot.org/c/coreboot/+/55067/comment/77d6a3c4_6ca35e19 PS5, Line 8: __ nit: remove the leading underscores
File src/include/romstage_common.h:
https://review.coreboot.org/c/coreboot/+/55067/comment/85f4b9da_084f8ade PS2, Line 3: __ROMSTAGE_COMMON_H
Identifiers starting with two leading underscores are reserved. […]
Done
File src/soc/amd/cezanne/romstage.c:
https://review.coreboot.org/c/coreboot/+/55067/comment/cdff986d_af79a6fe PS5, Line 4: #include <acpi/acpi.h> : #include <amdblocks/acpimmio.h> : #include <amdblocks/memmap.h> : #include <amdblocks/pmlib.h> : #include <arch/cpu.h> : #include <console/console.h> : #include <fsp/api.h> : #include <program_loading.h> : #include <timestamp.h> Looks like these includes are sorted alphabetically. Would be great if you could preserve this ordering.
https://review.coreboot.org/c/coreboot/+/55067/comment/5f0cc28a_f2f954fd PS5, Line 16: timestamp_add_now(TS_START_ROMSTAGE); This needs to be removed
File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/55067/comment/b72f8f45_80cbe59c PS5, Line 3: #include <acpi/acpi.h> : #include <amdblocks/memmap.h> : #include <amdblocks/pmlib.h> : #include <arch/cpu.h> : #include <commonlib/helpers.h> : #include <console/console.h> : #include <fsp/api.h> : #include <program_loading.h> : #include <timestamp.h> : #include <types.h> Looks like these includes are sorted alphabetically. Would be great if you could preserve this ordering.
https://review.coreboot.org/c/coreboot/+/55067/comment/69ee4592_ed3b68b1 PS5, Line 17: timestamp_add_now(TS_START_ROMSTAGE); This needs to be removed
File src/soc/example/min86/romstage.c:
https://review.coreboot.org/c/coreboot/+/55067/comment/8f0ec8bd_2593eab0 PS5, Line 4: #include <arch/cpu.h> No longer needed