Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33759 )
Change subject: soc/amd/picasso: Create a hybrid romstage to begin in DRAM ......................................................................
Patch Set 13:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33759/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33759/13//COMMIT_MSG@22 PS13, Line 22: Remove all postcar support. At first appearance >50% of code line changes is about MTRRs and not about the entry to romstage like it should be.
Could you please make first a separate commit that removes anything postcar related, while not adding any set_var_mtrr() here. Delay all that set_var_mtrr() (those are not merge-critical, just speed optimisations?) to be done much later in the patchtrain.
I would really like to see mock emulation/qemu-picasso that build-tests these soc/amd/picasso changes until CRB board has landed on master.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/Kconfi... File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/Kconfi... PS13, Line 71: config POSTCAR_STAGE Hmm.. maybe POSTCAR_STAGE should have 'depends on !RESET_VECTOR_IN_RAM̈́' but we can deal with that separately.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/Makefi... PS13, Line 42: romstage-y += hybrid_romstage.c I see little point in renaming the file, we just start asking questions why you link in romstage-y class... It also makes gerrit introduce it as separate file now, it could not detect romstage.c being renamed to hybrid_romstage.c, too many changes.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/hybrid... File src/soc/amd/picasso/hybrid_romstage.c:
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/hybrid... PS13, Line 4: * Copyright (C) 2019 Advanced Micro Devices, Inc.
In romstage.h you placed this line after previous copyright, here you placed it before. […]
I believe there is decision to no longer add these, but refer to AUTHORS and git blame.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/hybrid... PS13, Line 66: CONFIG_ROM_SIZE, MTRR_TYPE_WRPROT); Sorry.. all the caps makes my eyes bleed, and I did not check how all these macros are defined now. This is what I meant about delaying any set_var_mtrr() to be done later.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/hybrid... PS13, Line 125: romstage_soc_early_init(); PCI MMCONF setup needs to happen earlier.