Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready
This can be used in romstage in particular to know if dram is ready.
Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cbmem.h M src/lib/imd_cbmem.c 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/1
diff --git a/src/include/cbmem.h b/src/include/cbmem.h index cf79f41..a68d65f 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -81,6 +81,9 @@ */ void *cbmem_top_chipset(void);
+/* Returns 1 if cbmem is initialized, 0 otherwise */ +int cbmem_ready(void); + /* Add a cbmem entry of a given size and id. These return NULL on failure. The * add function performs a find first and do not check against the original * size. */ diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 5be7dc4..864d73e 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -44,6 +44,22 @@ dead_code(); }
+static int cbmem_initialized; + +static void set_cbmem_ready(int unused) +{ + cbmem_initialized = 1; +} + +ROMSTAGE_CBMEM_INIT_HOOK(set_cbmem_ready); +POSTCAR_CBMEM_INIT_HOOK(set_cbmem_ready); +RAMSTAGE_CBMEM_INIT_HOOK(set_cbmem_ready); + +int cbmem_ready(void) +{ + return cbmem_initialized; +} + static inline const struct cbmem_entry *imd_to_cbmem(const struct imd_entry *e) { return (const struct cbmem_entry *)e;
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h File src/include/cbmem.h:
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@85 PS1, Line 85: int cbmem_ready(void); I think you should add some commentary that the CBMEM_HOOKs won't necessarily see a consistent value here because of the current implementation. While it is implicit in the calling of the hooks themselves we should also note it here in case there's some shared code and people don't realize they might not see this reflected. If you do want always consistent assertion of this value then you shouldn't use the cbmem hooks to mark the variable in the positive.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h File src/include/cbmem.h:
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@84 PS1, Line 84: /* Returns 1 if cbmem is initialized, 0 otherwise */ Would probably make sense to make this a static inline and call cbmem_possibly_online() first, so that it allows for compile-time elimination in early stages. (For consistency it might also be better to call this cbmem_online().)
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@85 PS1, Line 85: int cbmem_ready(void);
I think you should add some commentary that the CBMEM_HOOKs won't necessarily see a consistent value […]
Would probably be better to just set this either at the top or the bottom of cbmem_run_init_hooks()?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38736
to look at the new patch set (#2).
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready
This can be used in romstage in particular to know if dram is ready.
Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cbmem.h M src/lib/imd_cbmem.c 2 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38736
to look at the new patch set (#3).
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready
This can be used in romstage in particular to know if dram is ready.
Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cbmem.h M src/lib/imd_cbmem.c 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/3
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38736
to look at the new patch set (#4).
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready
This can be used in romstage in particular to know if dram is ready.
Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cbmem.h M src/lib/imd_cbmem.c 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h File src/include/cbmem.h:
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@84 PS1, Line 84: /* Returns 1 if cbmem is initialized, 0 otherwise */
Would probably make sense to make this a static inline and call cbmem_possibly_online() first, so that it allows for compile-time elimination in early stages. (For consistency it might also be better to call this cbmem_online().)
More or less done. Is static inline needed in the last implementation?
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@85 PS1, Line 85: int cbmem_ready(void);
Would probably be better to just set this either at the top or the bottom of cbmem_run_init_hooks()?
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h File src/include/cbmem.h:
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@84 PS1, Line 84: /* Returns 1 if cbmem is initialized, 0 otherwise */
Would probably make sense to make this a static inline and call cbmem_possibly_online() first, so […]
It makes a big difference for compile time elimination. If you did something like
if (cbmem_online()) huge_function();
your current implementation would always pull huge_function() (and other code only referenced from there) into the binary. If you make it a static inline, then stages where cbmem_possibly_online() is false can completely eliminate it. This is both useful for size and sometimes to make it compile at all (e.g. huge_function() can then refer to symbols that are only available in stages where cbmem_possibly_online() is true, it won't cause undefined reference errors in the stages where it's not going to be used anyway).
See vboot_logic_executed() for an example of how to make this a static inline without exposing the cbmem_initialized global directly.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38736
to look at the new patch set (#6).
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready
This can be used in romstage in particular to know if dram is ready.
Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cbmem.h M src/lib/imd_cbmem.c 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/6
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h File src/include/cbmem.h:
https://review.coreboot.org/c/coreboot/+/38736/1/src/include/cbmem.h@84 PS1, Line 84: /* Returns 1 if cbmem is initialized, 0 otherwise */
It makes a big difference for compile time elimination. If you did something like […]
Thanks for explain and the pointer.
Done.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38736 )
Change subject: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready ......................................................................
lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready
This can be used in romstage in particular to know if dram is ready.
Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/38736 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/include/cbmem.h M src/lib/imd_cbmem.c 2 files changed, 20 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/include/cbmem.h b/src/include/cbmem.h index b548cd9..7e1da17 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -155,4 +155,18 @@ return 1; }
+/* Returns 1 after running cbmem init hooks, 0 otherwise. */ +static inline int cbmem_online(void) +{ + extern int cbmem_initialized; + + if (!cbmem_possibly_online()) + return 0; + + if (ENV_ROMSTAGE) + return cbmem_initialized; + + return 1; +} + #endif /* _CBMEM_H_ */ diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 7638e0e..4b7c412 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -30,6 +30,8 @@ dead_code(); }
+int cbmem_initialized; + static inline const struct cbmem_entry *imd_to_cbmem(const struct imd_entry *e) { return (const struct cbmem_entry *)e; @@ -79,6 +81,8 @@
/* Complete migration to CBMEM. */ cbmem_run_init_hooks(no_recovery); + + cbmem_initialized = 1; }
int cbmem_initialize(void) @@ -112,6 +116,8 @@ /* Complete migration to CBMEM. */ cbmem_run_init_hooks(recovery);
+ cbmem_initialized = 1; + /* Recovery successful. */ return 0; }