Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36145 )
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36145/15/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/15/src/arch/arm/stages.c@32 PS15, Line 32: uintptr_t _cbmem_top_ptr;
Why can't the x86 stages do this as well? Why put it in assembler?
I guess the x86 c_entry.S could call into a similar function. I'm considering calling main() with the stage argument?
https://review.coreboot.org/c/coreboot/+/36145/15/src/arch/arm/stages.c@35 PS15, Line 35: {
I think this needs an ENV_RAMSTAGE check like the arm64 version?
I think no harm is done, but that would be cleaner indeed.
https://review.coreboot.org/c/coreboot/+/36145/15/src/arch/arm/stages.c@36 PS15, Line 36: _cbmem_top_ptr = (uintptr_t)stage_arg;
Wouldn't it be cleaner to pass the argument through to main() and handle it in there? Then you don't have to duplicate everything for every arch (and it would be easier for custom entrypoints to tie into it, too).
That would be an option indeed. On the platforms still lacking the argument passing the 'custom' cbmem_top, could also be renamed to cbmem_top_romstage and passed on as an argument to main, so the call of main would look like main(cbmem_top_romstage()), effectively getting rid of the Kconfig option to guard the common cbmem_top implementation. What do you think about this?
https://review.coreboot.org/c/coreboot/+/36145/15/src/soc/rockchip/common/cb... File src/soc/rockchip/common/cbmem.c:
https://review.coreboot.org/c/coreboot/+/36145/15/src/soc/rockchip/common/cb... PS15, Line 28: #if !CONFIG(RAMSTAGE_CBMEM_TOP_ARG)
When would this ever happen?
This patch comes before the aarch64 one, so some guarding was needed.