Change in coreboot[master]: lib/imd_cbmem.c: Add a helper function to indicate that cbmem is ready

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Arthur Heymans (Code Review)

6 Feb 2020 6 Feb '20

6:06 p.m.

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;

Attachments:

attachment.htm (text/html — 4.1 KB)

Show replies by date

Aaron Durbin (Code Review)

6 Feb 6 Feb

6:09 p.m.

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 (Code Review)

8 Feb 8 Feb

12:54 a.m.

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()?

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 08 Feb 2020 00:54:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Arthur Heymans (Code Review)

7 Aug 7 Aug

3:40 p.m.

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.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Arthur Heymans (Code Review)

3:44 p.m.

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.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Arthur Heymans (Code Review)

3:52 p.m.

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.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Arthur Heymans (Code Review)

4:22 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 07 Aug 2020 16:22:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Julius Werner (Code Review)

9:27 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 07 Aug 2020 21:27:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: comment

Arthur Heymans (Code Review)

9:41 p.m.

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.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/38736/6

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Arthur Heymans (Code Review)

9:42 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 07 Aug 2020 21:42:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: comment

Julius Werner (Code Review)

10:45 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 07 Aug 2020 22:45:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

8 Aug 8 Aug

8:26 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 08 Aug 2020 20:26:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

17 Aug 17 Aug

6:22 a.m.

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; }

-- To view, visit https://review.coreboot.org/c/coreboot/+/38736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0231ab9c0b78a69faa762e0a97378bf0b50eebaf Gerrit-Change-Number: 38736 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1505

days inactive

1698

days old

coreboot-gerrit@coreboot.org

12 comments

5 participants

tags (0)

participants (5)

Aaron Durbin (Code Review)
Angel Pons (Code Review)
Arthur Heymans (Code Review)
Julius Werner (Code Review)
Patrick Georgi (Code Review)