Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36273 )
Change subject: lib/cbmem_top: Add a common cbmem_top implementation ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36273/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36273/13//COMMIT_MSG@16 PS13, Line 16: if CONFIG_RAMSTAGE_CBMEM_TOP_ARG is not set the : same implementation as will be used as in romstage. :
I think the naming of your source of cbmem top is the issue. We shouldn't be using functions named _romstage() in stages other than romstage. It just causes confusion.
I see. What about a 'new' optional callback cbmem_top_ramstage that will in this case return cbmem_top_romstage? At the moment only RISCV (which does not even initialize cbmem in romstage on some targets) and NVIDIA tegra210 need it.
risc-v is broken there then (LATE_CBMEM_INIT is supposed to be gone). Need to look into tegra210 as well.
See CB:36446 for riscv. Tegra210 has a different core running the romstage than the ramstage. As they don't share registers passing via calling arguments is not possible.
I'm more concerned about the naming having _romstage() in it. Call it cbmem_top_chipset() instead? Or something else? The suffix may not be the best, but it's not tied to people's thinking around a stage.
Last thing: add your implementation of cbmem_top() to src/lib/imd_cbmem.c as that's where our cbmem API is implemented. I don't think we need a new file as this point. Or add it to cbmem_common.c (I'm not sure why we have that separated tbh).