Change in coreboot[master]: [RFC,POC]arch/arm: Pass cbmem_top to ramstage via calling argument
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36145 )
Change subject: [RFC,POC]arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
[RFC,POC]arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/mainboard/emulation/qemu-armv7/Makefile.inc 3 files changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/1
diff --git a/src/arch/arm/include/arch/stages.h b/src/arch/arm/include/arch/stages.h index 3841265..16cfbbc 100644 --- a/src/arch/arm/include/arch/stages.h +++ b/src/arch/arm/include/arch/stages.h @@ -16,6 +16,6 @@
#include <main_decl.h>
-void stage_entry(void); +void stage_entry(void *stage_arg);
#endif diff --git a/src/arch/arm/stages.c b/src/arch/arm/stages.c index c9f5744..50526c4 100644 --- a/src/arch/arm/stages.c +++ b/src/arch/arm/stages.c @@ -22,13 +22,28 @@ * .text.stage_entry section created by -ffunction-sections). */
+#include <cbmem.h> #include <arch/stages.h> #include <arch/cache.h>
/** * generic stage entry point. override this if board specific code is needed. */ -__weak void stage_entry(void) +#if ENV_RAMSTAGE +static uintptr_t cbmem_top_ptr; +#endif + +__weak void stage_entry(void *stage_arg) { +#if ENV_RAMSTAGE + cbmem_top_ptr = (uintptr_t)stage_arg; +#endif main(); } + +#if ENV_RAMSTAGE +void *cbmem_top(void) +{ + return (void *)cbmem_top_ptr; +} +#endif diff --git a/src/mainboard/emulation/qemu-armv7/Makefile.inc b/src/mainboard/emulation/qemu-armv7/Makefile.inc index d5742e1..c62915b 100644 --- a/src/mainboard/emulation/qemu-armv7/Makefile.inc +++ b/src/mainboard/emulation/qemu-armv7/Makefile.inc @@ -15,7 +15,6 @@ romstage-y += romstage.c
romstage-y += cbmem.c -ramstage-y += cbmem.c
bootblock-y += media.c romstage-y += media.c
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36145 )
Change subject: [RFC,POC]arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
Patch Set 2:
(1 comment)
Hmmm... in general I agree it would be good to do something here, since this is a problem that annoys us a lot on Arm platforms (have to figure out a new way to pass this stuff for every new SoC). But just passing cbmem_top() wouldn't really solve it, because we still need to know the full DRAM size to write the correct memory tables (with ram_resource()). We can calculate cbmem_top() easily when we know the size of DRAM (it's essentially MIN(4GB, _dram + dram_size)), but not the other way round.
Would it make more sense to instead pass the size of DRAM as an argument?
https://review.coreboot.org/c/coreboot/+/36145/2/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/2/src/arch/arm/stages.c@34 PS2, Line 34: #endif This sort of stuff shouldn't be here. You'll probably want to centralize it in src/lib/imd_cbmem.c somehow, and call a function here that would call into there (or pass it through to main() and call it from there, that would probably be even cleaner).
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36145 )
Change subject: [RFC,POC]arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
Hmmm... in general I agree it would be good to do something here, since this is a problem that annoys us a lot on Arm platforms (have to figure out a new way to pass this stuff for every new SoC). But just passing cbmem_top() wouldn't really solve it, because we still need to know the full DRAM size to write the correct memory tables (with ram_resource()). We can calculate cbmem_top() easily when we know the size of DRAM (it's essentially MIN(4GB, _dram + dram_size)), but not the other way round.
Would it make more sense to instead pass the size of DRAM as an argument?
You can pass on every information you want from romstage (I assume you can compute the total amount of dram more easily here?) to ramstage via cbmem, but for that you need to know where cbmem is in ramstage. So all you'd need to pass from romstage to ramstage(s) is the pointer to cbmem.
https://review.coreboot.org/c/coreboot/+/36145/2/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/2/src/arch/arm/stages.c@34 PS2, Line 34: #endif
This sort of stuff shouldn't be here. You'll probably want to centralize it in src/lib/imd_cbmem.c somehow, and call a function here that would call into there (or pass it through to main() and call it from there, that would probably be even cleaner).
Sure. This is just a dirty hack/POC to see if it works on qemu arm. The idea is indeed to have one common implementation covering all architectures.
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#3).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Nvidia Tegra will be handled in a separate patch because it has a custom ramstage entry.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be removed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/cbmem.c M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/mainboard/emulation/qemu-armv7/cbmem.c M src/soc/qualcomm/ipq40xx/cbmem.c M src/soc/qualcomm/ipq806x/cbmem.c M src/soc/rockchip/common/cbmem.c M src/soc/samsung/exynos5250/cbmem.c M src/soc/samsung/exynos5420/cbmem.c 11 files changed, 21 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/3
Julius Werner 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 3:
Would it make more sense to instead pass the size of DRAM as an argument?
You can pass on every information you want from romstage (I assume you can compute the total amount of dram more easily here?) to ramstage via cbmem, but for that you need to know where cbmem is in ramstage. So all you'd need to pass from romstage to ramstage(s) is the pointer to cbmem.
Well, it just seems unnecessarily complicated. Pretty much every non-x86 platform calculates cbmem_top() in the same way based on the end of DRAM. If you're passing something anyway, you might as well pass the DRAM size and then implement cbmem_top() with a common one-liner for all of them. If you're passing cbmem_top(), then they all need to add this extra CBMEM section to pass the DRAM size as well. If we're standardizing on something, I believe standardizing on DRAM size would just work better for those architectures.
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 3:
Patch Set 3:
Would it make more sense to instead pass the size of DRAM as an argument?
You can pass on every information you want from romstage (I assume you can compute the total amount of dram more easily here?) to ramstage via cbmem, but for that you need to know where cbmem is in ramstage. So all you'd need to pass from romstage to ramstage(s) is the pointer to cbmem.
Well, it just seems unnecessarily complicated. Pretty much every non-x86 platform calculates cbmem_top() in the same way based on the end of DRAM. If you're passing something anyway, you might as well pass the DRAM size and then implement cbmem_top() with a common one-liner for all of them. If you're passing cbmem_top(), then they all need to add this extra CBMEM section to pass the DRAM size as well. If we're standardizing on something, I believe standardizing on DRAM size would just work better for those architectures.
cbmem_top is implemented now is one one-liner for every arch, with very few LOC to fetch it from the romstage call argument. Also the code for passing on the argument is now common for every arch. Doing things differently per arch means introducing a new callback. If you always know the _dram at compile time then the size of dram is simply cbmem_top() - _dram, assuming you need it for computing resources? The _dram symbol is not a symbol available on all targets so it looses universality. Note that passing multiple arguments is likely too messy.
Julius Werner 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 4:
If you always know the _dram at compile time then the size of dram is simply cbmem_top() - _dram, assuming you need it for computing resources?
No, because CBMEM is never placed above 4GB. So if the system DRAM goes above 0xffffffff, CBMEM is clipped there and you can't easily figure out the end of DRAM from it anymore. That's why I'm saying it's trivial to calculate cbmem_top from DRAM size, but not the other way round.
The _dram symbol is not a symbol available on all targets so it looses universality. Note that passing multiple arguments is likely too messy.
It should be on all architectures this is affecting? For x86 it probably needs to be handled separately anyway because it seems their cbmem_top calculation is not that simple.
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 4:
Patch Set 2:
(1 comment)
Hmmm... in general I agree it would be good to do something here, since this is a problem that annoys us a lot on Arm platforms (have to figure out a new way to pass this stuff for every new SoC). But just passing cbmem_top() wouldn't really solve it, because we still need to know the full DRAM size to write the correct memory tables (with ram_resource()). We can calculate cbmem_top() easily when we know the size of DRAM (it's essentially MIN(4GB, _dram + dram_size)), but not the other way round.
Would it make more sense to instead pass the size of DRAM as an argument?
One could call the dram size on x86 the current thing that cbmem_top returns and pass that as argument. However the 4G max rule does not seem to be applied on all SOC, so you can't implement cbmem_top based on total dram size alone in a platform independent way.
I'm in the process of writing some common code to make passing information from rom- to ram-stage more easy. If one needs to pass on total dram without recomputing it would boil down to:
save_romstage_info(&total_dram, sizeof(total)) and get_romstage_info(&total_dram).
which with my current implementation works regardless of whether cbmem is already initialized or not.
Julius Werner 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 4:
Okay, it's not a big deal either way. I agree that doing the same for all architectures is a bit nicer. I guess we can just provide some simple helpers that platforms can use to pass their DRAM size via CBMEM.
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#5).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Nvidia Tegra will be handled in a separate patch because it has a custom ramstage entry.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/cbmem.c M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/mainboard/emulation/qemu-armv7/cbmem.c M src/soc/qualcomm/ipq40xx/cbmem.c M src/soc/qualcomm/ipq806x/cbmem.c M src/soc/rockchip/common/cbmem.c M src/soc/samsung/exynos5250/cbmem.c M src/soc/samsung/exynos5420/cbmem.c 11 files changed, 21 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/5
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#6).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Nvidia Tegra will be handled in a separate patch because it has a custom ramstage entry.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/cbmem.c M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/mainboard/emulation/qemu-armv7/cbmem.c M src/soc/nvidia/tegra124/verstage.c M src/soc/qualcomm/ipq40xx/cbmem.c M src/soc/qualcomm/ipq806x/cbmem.c M src/soc/rockchip/common/cbmem.c M src/soc/samsung/exynos5250/cbmem.c M src/soc/samsung/exynos5420/cbmem.c 12 files changed, 22 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/6
Julius Werner 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:
(2 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@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).
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?
Aaron Durbin 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:
(2 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@25 PS15, Line 25: #include <cbmem.h> This compilation unit doesn't technically need this header.
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?
Julius Werner 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:
(1 comment)
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@35 PS15, Line 35: { I think this needs an ENV_RAMSTAGE check like the arm64 version?
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.
Aaron Durbin 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:
(1 comment)
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? […]
It doesn't need a similar function. It just needs to set the variable. You are allocating and setting the variable in the assembly when you only need the latter. The cbmem_top.c compilation unit can hold the object itself, right?
Julius Werner 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:
(2 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@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 […]
Not really sure I understand the second part of what you said, maybe that would be clearer if you upload a patch. In general I'm just trying to move as much of this out of the architectures and into common code as possible. For boards not passing the argument from romstage, stage_arg would just be undefined here and passed through undefined to main(), which would then have to know not to use it (based on Kconfig, probably).
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)
This patch comes before the aarch64 one, so some guarding was needed.
Oh, I see. Can we take this back out in the arm64 one, then?
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#16).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/Makefile.inc M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/soc/nvidia/tegra124/Makefile.inc M src/soc/nvidia/tegra124/verstage.c M src/soc/rockchip/rk3288/Makefile.inc M src/soc/samsung/exynos5250/Makefile.inc M src/soc/samsung/exynos5420/Makefile.inc 10 files changed, 7 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/16
Julius Werner 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 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36145/16/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/16/src/arch/arm/stages.c@34 PS16, Line 34: if (!ENV_ROMSTAGE) This needs to be either ENV_RAMSTAGE or !ENV_ROMSTAGE_OR_BEFORE. We don't want it in pre-RAM verstage.
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#18).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/Makefile.inc M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/soc/nvidia/tegra124/Makefile.inc M src/soc/nvidia/tegra124/verstage.c M src/soc/rockchip/rk3288/Makefile.inc M src/soc/samsung/exynos5250/Makefile.inc M src/soc/samsung/exynos5420/Makefile.inc 10 files changed, 7 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/18
Julius Werner 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 18: Code-Review+2
(1 comment)
Did we abandon the idea of passing through to main() now? I'd consider this is okay as is, but I still think passing through to main() would be a bit cleaner.
https://review.coreboot.org/c/coreboot/+/36145/18/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/18/src/arch/arm/stages.c@32 PS18, Line 32: void nit: If you want this as a uintptr_t, just make it a uintptr_t.
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 18:
Patch Set 18: Code-Review+2
(1 comment)
Did we abandon the idea of passing through to main() now? I'd consider this is okay as is, but I still think passing through to main() would be a bit cleaner.
I thought about doing that. It's a bit cleaner on platforms where the entry is just C code, but on platforms where the entry is written in assembly it's more involving as you have to deal with the ABI, which can change depending on compiler version, flags, ... Also on x86 the ramstage entry would be different than the postcar stage, where making the calling argument the argument of the main() function is not really an option.
It seems to me that the balance is slightly in favour of doing it in the stage entry than in the main(), although I might be biased towards making it easier on x86.
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#19).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/Makefile.inc M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/soc/nvidia/tegra124/Makefile.inc M src/soc/nvidia/tegra124/verstage.c M src/soc/rockchip/rk3288/Makefile.inc M src/soc/samsung/exynos5250/Makefile.inc M src/soc/samsung/exynos5420/Makefile.inc 10 files changed, 7 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/19
Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36145
to look at the new patch set (#20).
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/Makefile.inc M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/soc/nvidia/tegra124/Makefile.inc M src/soc/nvidia/tegra124/verstage.c M src/soc/rockchip/rk3288/Makefile.inc M src/soc/samsung/exynos5250/Makefile.inc M src/soc/samsung/exynos5420/Makefile.inc 10 files changed, 8 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36145/20
Aaron Durbin 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 20: Code-Review+2
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 20:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36145/2/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/2/src/arch/arm/stages.c@34 PS2, Line 34: #endif
This sort of stuff shouldn't be here. You'll probably want to centralize it in src/lib/imd_cbmem. […]
Done
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@25 PS15, Line 25: #include <cbmem.h>
This compilation unit doesn't technically need this header.
Now it does with the extern symbol declared in there.
https://review.coreboot.org/c/coreboot/+/36145/15/src/arch/arm/stages.c@32 PS15, Line 32: uintptr_t _cbmem_top_ptr;
It doesn't need a similar function. It just needs to set the variable. […]
Done
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? […]
Done
https://review.coreboot.org/c/coreboot/+/36145/15/src/arch/arm/stages.c@36 PS15, Line 36: _cbmem_top_ptr = (uintptr_t)stage_arg;
Not really sure I understand the second part of what you said, maybe that would be clearer if you up […]
Done
https://review.coreboot.org/c/coreboot/+/36145/16/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/16/src/arch/arm/stages.c@34 PS16, Line 34: if (!ENV_ROMSTAGE)
This needs to be either ENV_RAMSTAGE or !ENV_ROMSTAGE_OR_BEFORE. […]
Done
https://review.coreboot.org/c/coreboot/+/36145/18/src/arch/arm/stages.c File src/arch/arm/stages.c:
https://review.coreboot.org/c/coreboot/+/36145/18/src/arch/arm/stages.c@32 PS18, Line 32: void
nit: If you want this as a uintptr_t, just make it a uintptr_t.
Done
Julius Werner 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 20: Code-Review+2
Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36145 )
Change subject: arch/arm: Pass cbmem_top to ramstage via calling argument ......................................................................
arch/arm: Pass cbmem_top to ramstage via calling argument
This solution is very generic and can in principle be implemented on all arch/soc.
Instead trying to figure out which files can be removed from stages and which cbmem_top implementations need with preprocessor, rename all cbmem_top implementation to cbmem_top_romstage.
Mechanisms set in place to pass on information from rom- to ram-stage will be placed in a followup commit.
Change-Id: If31f0f1de17ffc92c9397f32b26db25aff4b7cab Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36145 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/arch/arm/Kconfig M src/arch/arm/include/arch/stages.h M src/arch/arm/stages.c M src/cpu/ti/am335x/Makefile.inc M src/mainboard/emulation/qemu-armv7/Makefile.inc M src/soc/nvidia/tegra124/Makefile.inc M src/soc/nvidia/tegra124/verstage.c M src/soc/rockchip/rk3288/Makefile.inc M src/soc/samsung/exynos5250/Makefile.inc M src/soc/samsung/exynos5420/Makefile.inc 10 files changed, 8 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/arch/arm/Kconfig b/src/arch/arm/Kconfig index 47c333b..9e10378 100644 --- a/src/arch/arm/Kconfig +++ b/src/arch/arm/Kconfig @@ -17,6 +17,7 @@ config ARCH_RAMSTAGE_ARM bool select ARCH_ARM + select RAMSTAGE_CBMEM_TOP_ARG
source src/arch/arm/armv4/Kconfig source src/arch/arm/armv7/Kconfig diff --git a/src/arch/arm/include/arch/stages.h b/src/arch/arm/include/arch/stages.h index 3841265..795a3a3 100644 --- a/src/arch/arm/include/arch/stages.h +++ b/src/arch/arm/include/arch/stages.h @@ -14,8 +14,9 @@ #ifndef __ARCH_STAGES_H #define __ARCH_STAGES_H
+#include <stdint.h> #include <main_decl.h>
-void stage_entry(void); +void stage_entry(uintptr_t stage_arg);
#endif diff --git a/src/arch/arm/stages.c b/src/arch/arm/stages.c index c9f5744..fc2ebdb 100644 --- a/src/arch/arm/stages.c +++ b/src/arch/arm/stages.c @@ -22,13 +22,16 @@ * .text.stage_entry section created by -ffunction-sections). */
+#include <cbmem.h> #include <arch/stages.h> #include <arch/cache.h>
/** * generic stage entry point. override this if board specific code is needed. */ -__weak void stage_entry(void) +__weak void stage_entry(uintptr_t stage_arg) { + if (!ENV_ROMSTAGE_OR_BEFORE) + _cbmem_top_ptr = stage_arg; main(); } diff --git a/src/cpu/ti/am335x/Makefile.inc b/src/cpu/ti/am335x/Makefile.inc index 24a79dd..d3ef970 100644 --- a/src/cpu/ti/am335x/Makefile.inc +++ b/src/cpu/ti/am335x/Makefile.inc @@ -10,7 +10,6 @@ ramstage-y += dmtimer.c ramstage-y += monotonic_timer.c ramstage-y += nand.c -ramstage-y += cbmem.c
bootblock-y += uart.c romstage-y += uart.c diff --git a/src/mainboard/emulation/qemu-armv7/Makefile.inc b/src/mainboard/emulation/qemu-armv7/Makefile.inc index d5742e1..c62915b 100644 --- a/src/mainboard/emulation/qemu-armv7/Makefile.inc +++ b/src/mainboard/emulation/qemu-armv7/Makefile.inc @@ -15,7 +15,6 @@ romstage-y += romstage.c
romstage-y += cbmem.c -ramstage-y += cbmem.c
bootblock-y += media.c romstage-y += media.c diff --git a/src/soc/nvidia/tegra124/Makefile.inc b/src/soc/nvidia/tegra124/Makefile.inc index fb5389f..e80125e 100644 --- a/src/soc/nvidia/tegra124/Makefile.inc +++ b/src/soc/nvidia/tegra124/Makefile.inc @@ -46,7 +46,6 @@ romstage-y += cache.c romstage-y += uart.c
-ramstage-y += cbmem.c ramstage-y += clock.c ramstage-y += display.c ramstage-y += dma.c diff --git a/src/soc/nvidia/tegra124/verstage.c b/src/soc/nvidia/tegra124/verstage.c index 2495351..7ecf31a 100644 --- a/src/soc/nvidia/tegra124/verstage.c +++ b/src/soc/nvidia/tegra124/verstage.c @@ -45,7 +45,7 @@ early_mainboard_init(); }
-void stage_entry(void) +void stage_entry(uintptr_t unused) { asm volatile ("bl arm_init_caches" : : : "r0", "r1", "r2", "r3", "r4", "r5", "ip"); diff --git a/src/soc/rockchip/rk3288/Makefile.inc b/src/soc/rockchip/rk3288/Makefile.inc index 7e4c5b4..e7982f7 100644 --- a/src/soc/rockchip/rk3288/Makefile.inc +++ b/src/soc/rockchip/rk3288/Makefile.inc @@ -18,7 +18,6 @@ IDBTOOL = util/rockchip/make_idb.py
bootblock-y += bootblock.c -bootblock-y += ../common/cbmem.c bootblock-y += ../common/uart.c bootblock-y += timer.c bootblock-y += clock.c @@ -55,7 +54,6 @@ romstage-y += ../common/i2c.c
ramstage-y += soc.c -ramstage-y += ../common/cbmem.c ramstage-y += timer.c ramstage-y += ../common/i2c.c ramstage-$(CONFIG_SOFTWARE_I2C) += software_i2c.c diff --git a/src/soc/samsung/exynos5250/Makefile.inc b/src/soc/samsung/exynos5250/Makefile.inc index a6eb9ee..6a595f4 100644 --- a/src/soc/samsung/exynos5250/Makefile.inc +++ b/src/soc/samsung/exynos5250/Makefile.inc @@ -40,7 +40,6 @@ ramstage-y += dp-reg.c ramstage-y += fb.c ramstage-y += usb.c -ramstage-y += cbmem.c
CPPFLAGS_common += -Isrc/soc/samsung/exynos5250/include/
diff --git a/src/soc/samsung/exynos5420/Makefile.inc b/src/soc/samsung/exynos5420/Makefile.inc index b41e9f9..dc25919 100644 --- a/src/soc/samsung/exynos5420/Makefile.inc +++ b/src/soc/samsung/exynos5420/Makefile.inc @@ -40,7 +40,6 @@ ramstage-y += i2c.c ramstage-y += dp.c dp_lowlevel.c fimd.c ramstage-y += usb.c -ramstage-y += cbmem.c
rmodules_$(ARCH-ROMSTAGE-y)-y += timer.c
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)