Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Make ENV_ROMSTAGE_OR_BEFORE improvements ......................................................................
timestamps: Make ENV_ROMSTAGE_OR_BEFORE improvements
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we just switch the pointer to CBMEM and copy the data before CAR gets torn down.
TBD: This probably breaks FSP1_0 which returns to coreboot proper only after tearing down CAR. I have an idea how to fix it though.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/timestamp.c 3 files changed, 24 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index d19818c..25b667c 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -60,17 +60,16 @@ . += 32; _epdpt = .; #endif - _car_relocatable_data_start = .; - /* The timestamp implementation relies on this storage to be around - * after migration. One of the fields indicates not to use it as the - * backing store once cbmem comes online. Therefore, this data needs - * to reside in the migrated area (between _car_relocatable_data_start - * and _car_relocatable_data_end). */ + TIMESTAMP(., 0x200) + _car_ehci_dbg_info_start = .; /* Reserve sizeof(struct ehci_dbg_info). */ . += 80; _car_ehci_dbg_info_end = .; + + _car_relocatable_data_start = .; + /* _car_global_start and _car_global_end provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram * cbmem console. This is useful for clearing this area on a per-stage diff --git a/src/include/timestamp.h b/src/include/timestamp.h index 04d5c12..1fa568c 100644 --- a/src/include/timestamp.h +++ b/src/include/timestamp.h @@ -22,11 +22,10 @@ #if CONFIG(COLLECT_TIMESTAMPS) /* * timestamp_init() needs to be called once for each of these cases: - * 1. __PRE_RAM__ (bootblock, romstage, verstage, etc) and - * 2. !__PRE_RAM__ (ramstage) + * 1. ENV_ROMSTAGE_OR_BEFORE (bootblock, romstage, verstage, etc) and + * 2. ENV_RAMSTAGE (ramstage) * The latter is taken care of by the generic coreboot infrastructure so - * it's up to the chipset/arch to call timestamp_init() in *one* of - * the __PRE_RAM__ stages. If multiple calls are made timestamps will be lost. + * it's up to the chipset/arch to call timestamp_init() in the former stages. */ void timestamp_init(uint64_t base); /* diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 89152fd..3e9914e 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -41,7 +41,7 @@
DECLARE_OPTIONAL_REGION(timestamp);
-#if defined(__PRE_RAM__) +#if ENV_ROMSTAGE_OR_BEFORE #define USE_TIMESTAMP_REGION (REGION_SIZE(timestamp) > 0) #else #define USE_TIMESTAMP_REGION 0 @@ -58,6 +58,10 @@ */ static struct timestamp_cache timestamp_cache;
+/* This points to the active timestamp_table and can change within a stage. + as CBMEM comes available. */ +static struct timestamp_table *glob_ts_table CAR_GLOBAL; + enum { TIMESTAMP_CACHE_UNINITIALIZED = 0, TIMESTAMP_CACHE_INITIALIZED, @@ -87,7 +91,7 @@ } else if (USE_TIMESTAMP_REGION) { if (REGION_SIZE(timestamp) < sizeof(*ts_cache)) BUG(); - ts_cache = car_get_var_ptr((void *)_timestamp); + ts_cache = (void *)_timestamp; }
return ts_cache; @@ -128,30 +132,21 @@
static struct timestamp_table *timestamp_table_get(void) { - MAYBE_STATIC_BSS struct timestamp_table *ts_table = NULL; + struct timestamp_table *ts_table; struct timestamp_cache *ts_cache;
+ ts_table = car_get_var(glob_ts_table); if (ts_table != NULL) return ts_table;
ts_cache = timestamp_cache_get(); + if (ts_cache) + ts_table = &ts_cache->table;
- if (ts_cache == NULL) { - if (HAS_CBMEM) - ts_table = cbmem_find(CBMEM_ID_TIMESTAMP); - return ts_table; - } + if (ts_table == NULL && HAS_CBMEM) + ts_table = cbmem_find(CBMEM_ID_TIMESTAMP);
- /* Cache is required. */ - if (ts_cache->cache_state != TIMESTAMP_CACHE_NOT_NEEDED) - return &ts_cache->table; - - /* Cache shouldn't be used but there's no backing store. */ - if (!HAS_CBMEM) - return NULL; - - ts_table = cbmem_find(CBMEM_ID_TIMESTAMP); - + car_set_var(glob_ts_table, ts_table); return ts_table; }
@@ -237,7 +232,7 @@ uint32_t i; struct timestamp_cache *ts_cache; struct timestamp_table *ts_cache_table; - struct timestamp_table *ts_cbmem_table = NULL; + struct timestamp_table *ts_cbmem_table;
if (!timestamp_should_run()) return; @@ -270,6 +265,7 @@
if (ts_cbmem_table == NULL) { printk(BIOS_ERR, "ERROR: No timestamp table allocated\n"); + car_set_var(glob_ts_table, ts_cbmem_table); return; }
@@ -309,6 +305,7 @@ ts_cbmem_table->tick_freq_mhz = timestamp_tick_freq_mhz();
/* Cache no longer required. */ + car_set_var(glob_ts_table, ts_cbmem_table); ts_cache_table->num_entries = 0; ts_cache->cache_state = TIMESTAMP_CACHE_NOT_NEEDED; }
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Make ENV_ROMSTAGE_OR_BEFORE improvements ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35032/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35032/1//COMMIT_MSG@7 PS1, Line 7: timestamps: Make ENV_ROMSTAGE_OR_BEFORE improvements Improve ENV_ROMSTAGE_OR_BEFORE
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35032
to look at the new patch set (#2).
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/timestamp.c 3 files changed, 24 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 2:
(1 comment)
Werner, Arthur: Any chances of getting this tested with FSP1.0. Pay attention timestamps from late romstage are present (before and after this commit). Same for CBMEM console.
https://review.coreboot.org/c/coreboot/+/35032/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35032/1//COMMIT_MSG@7 PS1, Line 7: timestamps: Make ENV_ROMSTAGE_OR_BEFORE improvements
Improve ENV_ROMSTAGE_OR_BEFORE
Done
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 2: Code-Review+1
Will hold off on +2 and hope someone can test FSP 1.0.
Hello Werner Zeh, Aaron Durbin, Julius Werner, Arthur Heymans, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35032
to look at the new patch set (#3).
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/timestamp.c 3 files changed, 24 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35032/3/src/include/timestamp.h File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/35032/3/src/include/timestamp.h@28 PS3, Line 28: * it's up to the chipset/arch to call timestamp_init() in the former stages. This doesn't really make it clear anymore that it should only be called in *one* of those stages (like the old text).
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@51 PS3, Line 51: #define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR) So... dumb question: does this even still do anything? I think this was only used for LATE_CBMEM_INIT or something? Because the first timestamp I see being added in ramstage is *after* cbmem_initialize(), so it should already go to CBMEM. But even if you did try to log one earlier, timestamp_init() is also called after cbmem_init(), and before that the BSS cache is uninitialized... so there's no window where you could possible log into the cache there either. (For postcar I think the same is true, and I don't see it calling timestamp_init() at all.)
If that's true, we should just get rid of anything implying there was a BSS cache at all (which simplifies a bunch of more stuff here).
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@146 PS3, Line 146: if (ts_table == NULL && HAS_CBMEM) I don't think this would ever be hit?
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@224 PS3, Line 224: ts_cache->cache_state != TIMESTAMP_CACHE_UNINITIALIZED) nit: Somewhat tangential, but I think we could get rid of the whole cache_state thing if you just check the glob_ts_table here (and also initialize it in timestamp_cache_init())? All we need is an indication that any sort of timestamp stuff has been set up already, and checking that for NULL should be good enough.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@51 PS3, Line 51: #define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR)
So... […]
In its current state this commit enables the follow-ups that would simplify car.ld, assuming the parent commit for FSP1.0 CAR migration also works (work-in-progress).
I would delay further changes to this file until we have more details for amd/picasso. They may actually want to have timestamps in .bss; declaring TIMESTAMP_REGION in DRAM seems pointless if nothing prior to romstage will produce timestamps and CBMEM may not be available from the start of romstage. I think I read somewhere AGESA would not give out suitable cbmem_top() until PCIe training/enumeration, I just do not have enough details for the romstage in DRAM approach wrt. how it will be linked.
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@94 PS3, Line 94: ts_cache = (void *)_timestamp; Can't use car_get_var_ptr() after _timestamp is moved outside _car_relocatable. Currently FSP1.0 calls timestamp_init() after CAR teardown, maybe easiest to start collection from start of romstage (if parent commit with relative pointer works) instead.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 4:
(1 comment)
Sorry, didn't have time for a full review just now, but saw "picasso" and wanted to comment...
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@51 PS3, Line 51: #define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR)
In its current state this commit enables the follow-ups that would simplify car. […]
It's not an insurmountable problem, but I would prefer initializing timestamps in picasso before cbmem is up. Otherwise my alternative is to keep them noted until cbmem is ready and then initialize TSs. The reason is that we don't know the memory map until after fsp_memory_init() runs. Although FSP-M isn't turning on DRAM, the top of low memory isn't easily discoverable like in Intel -- the hard part is when UMA is <4GB, so we rely on AGESA to report it to us.
Re. PCIe training and enumeration, that's an open question. Definitely not my goal... However IIRC AGESA has always wanted the topology information early, and AMD has still baked in a DEPEX for that info. Right now I don't know whether it's a real dependency or artificial. However, cbmem_top() is valid in romstage. (FWIW, the very first prototype-ish implementation put 100% of functionality into FSP-M, but that was due to a bug that would hang FSP-S if it was runexecuted, so I don't know if that's clouding any perception.)
Hello Werner Zeh, Aaron Durbin, Julius Werner, Arthur Heymans, Marshall Dawson, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35032
to look at the new patch set (#5).
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Remove timestamp_cache from postcar and ramstate, as CBMEM is available early on.
Modify intel/fsp_broadwell_de such that timestamp_init() is called while REGION(timestamp) is available, adding two new early timestamps. Other FSP1.0 platforms fsp_baytrail and fsp_rangeley already did it this way.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/timestamp.c M src/soc/intel/fsp_broadwell_de/romstage/romstage.c 4 files changed, 75 insertions(+), 105 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35032/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35032/5/src/arch/x86/car.ld@69 PS5, Line 69: _car_ehci_dbg_info_end = .; We're also moving the location of ehci info in this patch to be outside of relocatable window. Do it in another patch?
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@55 PS5, Line 55: static struct timestamp_cache timestamp_cache; Given that TIMESTAMP_CACHE_IN_BSS is 0 why isn't that as well as this object removed?
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@168 PS5, Line 168: timestamp_add_table_entry(timestamp_table_get(), id, ts_time); Below you check the return value of timestamp_table_get() for NULL but you don't here. Why?
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@246 PS5, Line 246: static void timestamp_sync_cache_to_cbmem(int is_recovery) Why did this function move? Can we put it back to its original location for easier reviewing?
https://review.coreboot.org/c/coreboot/+/35032/5/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/soc/intel/fsp_broadwell... PS5, Line 108: timestamp_init(get_initial_timestamp()); Put these changes in another patch? Also add some comments as to why this should be placed here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 5:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@51 PS3, Line 51: #define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR)
It's not an insurmountable problem, but I would prefer initializing timestamps in picasso before cbm […]
Ack
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@94 PS3, Line 94: ts_cache = (void *)_timestamp;
Can't use car_get_var_ptr() after _timestamp is moved outside _car_relocatable. Currently FSP1. […]
Done
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@146 PS3, Line 146: if (ts_table == NULL && HAS_CBMEM)
I don't think this would ever be hit?
Done
https://review.coreboot.org/c/coreboot/+/35032/3/src/lib/timestamp.c@224 PS3, Line 224: ts_cache->cache_state != TIMESTAMP_CACHE_UNINITIALIZED)
nit: Somewhat tangential, but I think we could get rid of the whole cache_state thing if you just ch […]
Done
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@55 PS5, Line 55: static struct timestamp_cache timestamp_cache;
Given that TIMESTAMP_CACHE_IN_BSS is 0 why isn't that as well as this object removed?
Patchset #3 comments. I don't know whether amd/picasso will use .bss or REGION(timestamp) for romstage.
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@168 PS5, Line 168: timestamp_add_table_entry(timestamp_table_get(), id, ts_time);
Below you check the return value of timestamp_table_get() for NULL but you don't here. […]
Done
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@246 PS5, Line 246: static void timestamp_sync_cache_to_cbmem(int is_recovery)
Why did this function move? Can we put it back to its original location for easier reviewing?
Done
Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, Marshall Dawson, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35032
to look at the new patch set (#6).
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Fix comments about requirements of timestamp_init() usage.
Remove timestamp_cache from postcar and ramstage, as CBMEM is available early on.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/timestamp.c 3 files changed, 46 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/6
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35032/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35032/5/src/arch/x86/car.ld@69 PS5, Line 69: _car_ehci_dbg_info_end = .;
We're also moving the location of ehci info in this patch to be outside of relocatable window. […]
Done
https://review.coreboot.org/c/coreboot/+/35032/3/src/include/timestamp.h File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/35032/3/src/include/timestamp.h@28 PS3, Line 28: * it's up to the chipset/arch to call timestamp_init() in the former stages.
This doesn't really make it clear anymore that it should only be called in *one* of those stages (li […]
Done
https://review.coreboot.org/c/coreboot/+/35032/5/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/soc/intel/fsp_broadwell... PS5, Line 108: timestamp_init(get_initial_timestamp());
Put these changes in another patch? Also add some comments as to why this should be placed here.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35032/7/src/include/timestamp.h File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/35032/7/src/include/timestamp.h@26 PS7, Line 26: * to make *one* call per stage, otherwise some timestamps will be lost. So is this an intentional change in behavior now? Previously this wasn't one call *per stage*, this was one call *in total*, in the earliest stage where you want to use timestamps. I don't think we should change that, requiring chipset code to do extra work in every stage is cumbersome and seems unnecessary in this case. You should be able to just keep setting up the pointer to the cache or CBMEM the first time it is needed in every stage.
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@124 PS7, Line 124: return car_get_ptr(glob_ts_table); I think this should keep doing a cbmem_find() and/or a timestamp_cache_get() if the pointer is currently NULL, so we don't have to init it explicitly in every stage.
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@172 PS7, Line 172: timestamp_add_table_entry(ts_table, id, ts_time); Why are we removing the error message? I think that was useful (since this points to a programming problem).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35032/7/src/include/timestamp.h File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/35032/7/src/include/timestamp.h@26 PS7, Line 26: * to make *one* call per stage, otherwise some timestamps will be lost.
So is this an intentional change in behavior now? Previously this wasn't one call *per stage*, this […]
Not intentional, just confusions after looking into if cbmem_init() is necessary for ramstage and making wrong choices.
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@124 PS7, Line 124: return car_get_ptr(glob_ts_table);
I think this should keep doing a cbmem_find() and/or a timestamp_cache_get() if the pointer is curre […]
Why would he have cbmem_find() here now, see patchset #3 comment? I thought you analyzed for ramstage/postcar timestamp_add() before cbmem_initialize() does not need to be supported and CBMEM_HOOK would always have done car_set_ptr() already. That was the logic for .bss cache removal.
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@172 PS7, Line 172: timestamp_add_table_entry(ts_table, id, ts_time);
Why are we removing the error message? I think that was useful (since this points to a programming p […]
I'll put it back.
Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, Marshall Dawson, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35032
to look at the new patch set (#8).
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Fix comments about requirements of timestamp_init() usage.
Remove timestamp_cache from postcar and ramstage, as CBMEM is available early on.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/timestamp.c 3 files changed, 50 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/8
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 8: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@124 PS7, Line 124: return car_get_ptr(glob_ts_table);
Why would he have cbmem_find() here now, see patchset #3 comment? I thought you analyzed for ramstag […]
Right, I guess we don't need cbmem_find() because CBMEM recovery always sets the pointer already.
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@74 PS8, Line 74: static struct timestamp_cache *timestamp_cache_get(void) nit: Now that there are no other relevant members in struct timestamp_cache, you could change this to return &ts_cache->table...
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@131 PS8, Line 131: ts_cache = timestamp_cache_get(); ...and then simplify this to just
ts_table = timestamp_cache_get();
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@203 PS8, Line 203: if (!ENV_ROMSTAGE_OR_BEFORE) nit: Should this maybe just be an assert()? Because doing this is just incorrect now, right? We can fix the code that does this instead.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35032/7/src/include/timestamp.h File src/include/timestamp.h:
https://review.coreboot.org/c/coreboot/+/35032/7/src/include/timestamp.h@26 PS7, Line 26: * to make *one* call per stage, otherwise some timestamps will be lost.
Not intentional, just confusions after looking into if cbmem_init() is necessary for ramstage and ma […]
Done
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@124 PS7, Line 124: return car_get_ptr(glob_ts_table);
Right, I guess we don't need cbmem_find() because CBMEM recovery always sets the pointer already.
Ack
https://review.coreboot.org/c/coreboot/+/35032/7/src/lib/timestamp.c@172 PS7, Line 172: timestamp_add_table_entry(ts_table, id, ts_time);
I'll put it back.
Done
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@203 PS8, Line 203: if (!ENV_ROMSTAGE_OR_BEFORE)
nit: Should this maybe just be an assert()? Because doing this is just incorrect now, right? We can […]
(was supposed to be non-blocking)
Hello Werner Zeh, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, Marshall Dawson, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35032
to look at the new patch set (#9).
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Fix comments about requirements of timestamp_init() usage.
Remove timestamp_cache from postcar and ramstage, as CBMEM is available early on.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/hardwaremain.c M src/lib/timestamp.c 4 files changed, 47 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35032/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@74 PS8, Line 74: static struct timestamp_cache *timestamp_cache_get(void)
nit: Now that there are no other relevant members in struct timestamp_cache, you could change this t […]
Done
https://review.coreboot.org/c/coreboot/+/35032/8/src/lib/timestamp.c@131 PS8, Line 131: ts_cache = timestamp_cache_get();
...and then simplify this to just […]
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/5/src/lib/timestamp.c@55 PS5, Line 55: static struct timestamp_cache timestamp_cache;
Patchset #3 comments. I don't know whether amd/picasso will use . […]
Follow-up shall deal with this.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 10: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c@52 PS10, Line 52: /* : * Storage of cache entries prior to cbmem coming online. This can be a single line comment now.
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c@135 PS10, Line 135: car_set_ptr(glob_ts_table, ts_table); You could call here timestamp_table_set(ts_table);
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c@52 PS10, Line 52: /* : * Storage of cache entries prior to cbmem coming online.
This can be a single line comment now.
Followup will remove .bss cache entirely (once I rebase it).
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c@135 PS10, Line 135: car_set_ptr(glob_ts_table, ts_table);
You could call here […]
I kind of like the get_ptr/set_ptr symmetry here.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
Patch Set 10: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c@52 PS10, Line 52: /* : * Storage of cache entries prior to cbmem coming online.
Followup will remove .bss cache entirely (once I rebase it).
Done
https://review.coreboot.org/c/coreboot/+/35032/10/src/lib/timestamp.c@135 PS10, Line 135: car_set_ptr(glob_ts_table, ts_table);
I kind of like the get_ptr/set_ptr symmetry here.
OK, fine with me.
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35032 )
Change subject: timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE ......................................................................
timestamps: Improve collection for ENV_ROMSTAGE_OR_BEFORE
Keep track of the active timestamp table location using a CAR_GLOBAL variable. Done this way, the entire table can be located outside _car_relocatable_data and we only switch the pointer to CBMEM and copy the data before CAR gets torn down.
Fix comments about requirements of timestamp_init() usage.
Remove timestamp_cache from postcar and ramstage, as CBMEM is available early on.
Change-Id: I87370f62db23318069b6fd56ba0d1171d619cb8a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35032 Reviewed-by: Werner Zeh werner.zeh@siemens.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/car.ld M src/include/timestamp.h M src/lib/hardwaremain.c M src/lib/timestamp.c 4 files changed, 47 insertions(+), 72 deletions(-)
Approvals: build bot (Jenkins): Verified Werner Zeh: Looks good to me, approved
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 9d5ab28..2fbdb71 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -56,17 +56,16 @@ . += 32; _epdpt = .; #endif - _car_relocatable_data_start = .; - /* The timestamp implementation relies on this storage to be around - * after migration. One of the fields indicates not to use it as the - * backing store once cbmem comes online. Therefore, this data needs - * to reside in the migrated area (between _car_relocatable_data_start - * and _car_relocatable_data_end). */ + TIMESTAMP(., 0x200) + + _car_relocatable_data_start = .; + _car_ehci_dbg_info_start = .; /* Reserve sizeof(struct ehci_dbg_info). */ . += 80; _car_ehci_dbg_info_end = .; + /* _car_global_start and _car_global_end provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram * cbmem console. This is useful for clearing this area on a per-stage diff --git a/src/include/timestamp.h b/src/include/timestamp.h index 04d5c12..951032c 100644 --- a/src/include/timestamp.h +++ b/src/include/timestamp.h @@ -21,12 +21,10 @@
#if CONFIG(COLLECT_TIMESTAMPS) /* - * timestamp_init() needs to be called once for each of these cases: - * 1. __PRE_RAM__ (bootblock, romstage, verstage, etc) and - * 2. !__PRE_RAM__ (ramstage) - * The latter is taken care of by the generic coreboot infrastructure so - * it's up to the chipset/arch to call timestamp_init() in *one* of - * the __PRE_RAM__ stages. If multiple calls are made timestamps will be lost. + * timestamp_init() needs to be called once in *one* of the ENV_ROMSTAGE_OR_BEFORE + * stages (bootblock, romstage, verstage, etc). It's up to the chipset/arch + * to make the call in the earliest stage, otherwise some timestamps will be lost. + * For x86 ENV_ROMSTAGE call must be made before CAR is torn down. */ void timestamp_init(uint64_t base); /* diff --git a/src/lib/hardwaremain.c b/src/lib/hardwaremain.c index a4c154e..2ff2b8c 100644 --- a/src/lib/hardwaremain.c +++ b/src/lib/hardwaremain.c @@ -461,9 +461,6 @@ */ cbmem_initialize();
- /* Record current time, try to locate timestamps in CBMEM. */ - timestamp_init(timestamp_get()); - timestamp_add_now(TS_START_RAMSTAGE); post_code(POST_ENTRY_RAMSTAGE);
diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 89152fd..bb5c114 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -31,7 +31,6 @@ #define MAX_BSS_TIMESTAMP_CACHE 16
struct __packed timestamp_cache { - uint32_t cache_state; struct timestamp_table table; /* The struct timestamp_table has a 0 length array as its last field. * The following 'entries' array serves as the storage space for the @@ -41,28 +40,23 @@
DECLARE_OPTIONAL_REGION(timestamp);
-#if defined(__PRE_RAM__) +#if ENV_ROMSTAGE_OR_BEFORE #define USE_TIMESTAMP_REGION (REGION_SIZE(timestamp) > 0) #else #define USE_TIMESTAMP_REGION 0 #endif
-/* The cache location will sit in BSS when in ramstage/postcar. */ -#define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR) - -#define HAS_CBMEM (ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_POSTCAR) +/* Currently we never store timestamp cache in .bss. */ +#define TIMESTAMP_CACHE_IN_BSS 0
/* - * Storage of cache entries during ramstage/postcar prior to cbmem coming - * online. + * Storage of cache entries prior to cbmem coming online. */ static struct timestamp_cache timestamp_cache;
-enum { - TIMESTAMP_CACHE_UNINITIALIZED = 0, - TIMESTAMP_CACHE_INITIALIZED, - TIMESTAMP_CACHE_NOT_NEEDED, -}; +/* This points to the active timestamp_table and can change within a stage. + as CBMEM comes available. */ +static struct timestamp_table *glob_ts_table CAR_GLOBAL;
static void timestamp_cache_init(struct timestamp_cache *ts_cache, uint64_t base) @@ -70,7 +64,6 @@ ts_cache->table.num_entries = 0; ts_cache->table.max_entries = MAX_BSS_TIMESTAMP_CACHE; ts_cache->table.base_time = base; - ts_cache->cache_state = TIMESTAMP_CACHE_INITIALIZED;
if (USE_TIMESTAMP_REGION) ts_cache->table.max_entries = (REGION_SIZE(timestamp) - @@ -87,7 +80,7 @@ } else if (USE_TIMESTAMP_REGION) { if (REGION_SIZE(timestamp) < sizeof(*ts_cache)) BUG(); - ts_cache = car_get_var_ptr((void *)_timestamp); + ts_cache = (void *)_timestamp; }
return ts_cache; @@ -128,33 +121,26 @@
static struct timestamp_table *timestamp_table_get(void) { - MAYBE_STATIC_BSS struct timestamp_table *ts_table = NULL; + struct timestamp_table *ts_table; struct timestamp_cache *ts_cache;
- if (ts_table != NULL) + ts_table = car_get_ptr(glob_ts_table); + if (ts_table) return ts_table;
ts_cache = timestamp_cache_get(); + if (ts_cache) + ts_table = &ts_cache->table;
- if (ts_cache == NULL) { - if (HAS_CBMEM) - ts_table = cbmem_find(CBMEM_ID_TIMESTAMP); - return ts_table; - } - - /* Cache is required. */ - if (ts_cache->cache_state != TIMESTAMP_CACHE_NOT_NEEDED) - return &ts_cache->table; - - /* Cache shouldn't be used but there's no backing store. */ - if (!HAS_CBMEM) - return NULL; - - ts_table = cbmem_find(CBMEM_ID_TIMESTAMP); - + car_set_ptr(glob_ts_table, ts_table); return ts_table; }
+static void timestamp_table_set(struct timestamp_table *ts) +{ + car_set_ptr(glob_ts_table, ts); +} + static const char *timestamp_name(enum timestamp_id id) { int i; @@ -212,6 +198,8 @@ { struct timestamp_cache *ts_cache;
+ assert(ENV_ROMSTAGE_OR_BEFORE); + if (!timestamp_should_run()) return;
@@ -222,36 +210,19 @@ return; }
- /* Timestamps could have already been recovered. - * In those circumstances honor the cache which sits in BSS - * as it has already been initialized. */ - if (TIMESTAMP_CACHE_IN_BSS && - ts_cache->cache_state != TIMESTAMP_CACHE_UNINITIALIZED) - return; - timestamp_cache_init(ts_cache, base); + timestamp_table_set(&ts_cache->table); }
static void timestamp_sync_cache_to_cbmem(int is_recovery) { uint32_t i; - struct timestamp_cache *ts_cache; struct timestamp_table *ts_cache_table; - struct timestamp_table *ts_cbmem_table = NULL; + struct timestamp_table *ts_cbmem_table;
if (!timestamp_should_run()) return;
- ts_cache = timestamp_cache_get(); - - /* No timestamp cache found */ - if (ts_cache == NULL) { - printk(BIOS_ERR, "ERROR: No timestamp cache found\n"); - return; - } - - ts_cache_table = &ts_cache->table; - /* cbmem is being recovered. */ if (is_recovery) { /* x86 resume path expects timestamps to be reset. */ @@ -270,6 +241,19 @@
if (ts_cbmem_table == NULL) { printk(BIOS_ERR, "ERROR: No timestamp table allocated\n"); + timestamp_table_set(NULL); + return; + } + + /* Seed the timestamp tick frequency in ENV_PAYLOAD_LOADER. */ + if (ENV_PAYLOAD_LOADER) + ts_cbmem_table->tick_freq_mhz = timestamp_tick_freq_mhz(); + + ts_cache_table = timestamp_table_get(); + if (!ts_cache_table) { + if (ENV_ROMSTAGE) + printk(BIOS_ERR, "ERROR: No timestamp cache found\n"); + timestamp_table_set(ts_cbmem_table); return; }
@@ -294,6 +278,7 @@ * If timestamps only get initialized in ramstage, the base_time from * timestamp_init() will get ignored and all timestamps will be 0-based. */ + for (i = 0; i < ts_cache_table->num_entries; i++) { struct timestamp_entry *tse = &ts_cache_table->entries[i]; timestamp_add_table_entry(ts_cbmem_table, tse->entry_id, @@ -304,13 +289,9 @@ if (ts_cbmem_table->base_time == 0) ts_cbmem_table->base_time = ts_cache_table->base_time;
- /* Seed the timestamp tick frequency in ENV_PAYLOAD_LOADER. */ - if (ENV_PAYLOAD_LOADER) - ts_cbmem_table->tick_freq_mhz = timestamp_tick_freq_mhz(); - /* Cache no longer required. */ + timestamp_table_set(ts_cbmem_table); ts_cache_table->num_entries = 0; - ts_cache->cache_state = TIMESTAMP_CACHE_NOT_NEEDED; }
void timestamp_rescale_table(uint16_t N, uint16_t M)
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Paul Menzel (Code Review)
-
Werner Zeh (Code Review)