Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42278 )
Change subject: post_code: add defines for missing postcode values ......................................................................
Patch Set 13:
(34 comments)
I find it hard to see if these postcodes make sense without seeing how they are going to be used.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... File src/include/console/post_codes.h:
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 72: POST code emitted Use `POSTed` which is shorter? One should know this is a post code.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 81: * wait_for_sipi state missing trailing period
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 86: Clears Clear
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 88: * The POST code is emitted after successful clearing of fixed MTRRs missing trailing period
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 93: Clears Clear
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 95: * The POST code is emitted after successful clearing of variable MTRRs missing trailing period
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 85: /** : * \brief Clears fixed MTRR : * : * The POST code is emitted after successful clearing of fixed MTRRs : */ : #define POST_CAR_FIXED_MTRR 0x16 : : /** : * \brief Clears variable MTRR : * : * The POST code is emitted after successful clearing of variable MTRRs : */ : #define POST_CAR_VARIABLE_MTRR 0x17 Is there a reason to have these two postcodes? Why not a single postcode POST_CAR_AFTER_MTRR_CLEAR?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 100: Sets Set
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 102: Denotes the setting of CAR base address Maybe:
POSTed before writing the CAR base address into the MTRR MSRs.
In any case, missing trailing period
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 107: Entry to enabling MTRR I'd say:
About to enable CAR MTRR
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 109: POST code emitted before enabling MTRR and finishing of setting the CAR mask I would say:
POSTed right before enabling the previously-programmed CAR MTRR.
In any case, missing trailing period
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 116: The POST code emitted marks the beginning of clearance of the cache : * memory region I'm not sure if this description is accurate.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 119: 0x20 what about 0x1a .. 0x1f codes?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 126: POST_CAR_CACHE_EVICTION I'd add "DISABLE" somewhere
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 131: This is followed by enabling cache : * in certain CPUs If not everyone does this, why call it `POST_CAR_INIT_CACHE` instead of `POST_CAR_INIT_DONE`?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 146: used by few CPUs Then, why have it? I only see it in one place
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 148: X lowercase `x`
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 158: /** : * \brief Disable Cache : * : * Disables cache to enable CAR mode : */ : #define POST_CAR_CACHE_DISABLE 0x26 Ugh... CB:42503 is using this when setting up *and* when tearing down CAR.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 168: signifying the : * beginning of CPU initialization I thought we had started initializing things earlier?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 178: #define POST_ROM_SOC_EARLY_INIT 0x31 I see this being used on CB:42503 in only three places, one is not in a SoC, another is in bootblock and the last one is in car_stage_entry. Rather ugly.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 183: remaining I don't think so. RAM initializattion is part of SoC initialization too, among many other things.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 190: before and after Please decide. Before or after?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 173: /** : * \brief SoC early init : * : * POSTed before the early init of the SoC/mainboard : */ : #define POST_ROM_SOC_EARLY_INIT 0x31 : : /** : * \brief Entry to SoC pre-ram init : * : * Entry to soc_pre_ram_init which performs remaining SoC initialization : */ : #define POST_SOC_RAM_PREINIT_ENTRY 0x32 : : /** : * \brief Entry to mainboard_pre_raminit() : * : * Initialization of mainboard before and after RAM is enabled in romstage : */ : #define POST_BOARD_PREINIT_ENTRY 0x33 : : /** : * \brief Pre-memory init preparation start : * : * Post code emitted in romstage before making callbacks to allow SoC/mainboard : * to prepare params for FSP memory init. : */ : #define POST_MEM_PREINIT_PREP_START 0x34 I fail to see the difference between those four postcode definitions.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 202: /** : * \brief Mainboard callback after Memory Init : * : * POST code emitted before Mainboard callback run after Memory init done : */ : #define POST_MAINBOARD_SOC_INIT_ENTRY 0x35 If this is after memory init, why is it a smaller value than the surrounding stuff?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 217: /** : * \brief Entry before call to amdinitreset : * : * POSTed before the call to AMDinitreset : */ : #define POST_AGESA_SOC_INIT_RESET 0x37 Do we need an AGESA-specific postcode?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 238: /** : * \brief Entry before PEI data is filled : * : * POST code emitted before PEI data is filled among north/south bridges, : * device trees and mainboard : */ : #define POST_ROM_PEI_DATA_FILL 0x3b Duplicates POST_MEM_PREINIT_PREP_START
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 246: /** : * \brief Executes the PEI executable : * : * POST code sent before PEI executable is executed from the coreboot filesystem : */ : #define POST_ROM_SDRAM_INIT 0x3c Duplicates POST_RAM_INIT
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 260: /** : * \brief Initialization of RCBA register : * : * POST code emitted before initialization of the southbridge and mainboard : * RCBA register : */ : #define POST_ROM_RCBA_CONFIG 0X3e RCBA is Intel-specific, and it's just part of chipset initialization. Nothing special.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 268: /** : * \brief ROM stage handoff : * : * POST code emitted if current boot in a S3 resume for relocatable ramstage : */ : #define POST_ROM_STAGE_HANDOFF 0x3f Is this used anywhere? The description is rather confusing.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 282: Where is 0x41?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 290: /** : * \brief Notifies PSP that DRAM is available : * : * POST code is emitted before PSP is notified DRAM is present : */ : #define POST_PSP_DRAM_NOTIFY 0x43 Intel ME also needs to be notified of DRAM being available. Currently only one platform needs coreboot to notify the PSP that DRAM is available.
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 344: 0x50 What about 0x4a .. 0x4f?
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 349: existense existence
https://review.coreboot.org/c/coreboot/+/42278/13/src/include/console/post_c... PS13, Line 373: POST_ENABLING_CACHE How many caches are we enabling?