Attention is currently required from: Martin L Roth, Nicholas Chin.
Julius Werner has posted comments on this change by Martin L Roth. ( https://review.coreboot.org/c/coreboot/+/87186?usp=email )
Change subject: Documentation/lib: Update Timestamp documentation ......................................................................
Patch Set 4:
(10 comments)
File Documentation/lib/timestamp.md:
https://review.coreboot.org/c/coreboot/+/87186/comment/8d44da9f_e4732495?usp... : PS4, Line 79: (default 30, configurable via : the Kconfig option `CONFIG_MAX_TIMESTAMPS`) This seems incorrect — the current code just has the number 192 hardcoded.
https://review.coreboot.org/c/coreboot/+/87186/comment/248a80e3_2e9a42c4?usp... : PS4, Line 86: uint64_t Looks like this was changed to `int64_t` with the addition of pre-CPU timestamps.
https://review.coreboot.org/c/coreboot/+/87186/comment/df603849_859f7646?usp... : PS4, Line 112: enum { I think this entire enum doesn't exist anymore. It seems we just maintain a global pointer to the currently active table now that is changed when CBMEM is initialized.
https://review.coreboot.org/c/coreboot/+/87186/comment/43467640_bc18dbed?usp... : PS4, Line 119: if the cache is stored in local stash (bss area) I think this is out of date, I can't find that in the code anymore. I think we used to do that in early ramstage (or am I confusing that with CBMEM console?) but I guess we don't anymore. Nowadays it seems we just don't log any timestamps before CBMEM reinit in ramstage anymore. The only two remaining backends are `_timestamp` and CBMEM.
https://review.coreboot.org/c/coreboot/+/87186/comment/a6d4178b_141ee56e?usp... : PS4, Line 141: CONFIG_MAX_TIMESTAMP_CACHE_ENTRIES This Kconfig doesn't seem to exist anymore? I'm not sure if this section is talking about pre-RAM or CBMEM timestamps. Pre-RAM the size is determined by platform depending on how much space is allocated in memlayout. For CBMEM, the size seems to be hardcoded to 192 entries.
https://review.coreboot.org/c/coreboot/+/87186/comment/cfbd6a66_05e98290?usp... : PS4, Line 192: ### Core Functions The problem with directly listing functions or structures in the documentation folder is that they will inevitably go out of date again eventually when people change the code and don't realize there's separate documentation somewhere. We already have (or should have) good docstrings inside the header files in many cases which people usually do remember to update when they make changes. I wonder if it would be better to just link to the appropriate headers here? (I don't suppose markdown has a way to just pull in a whole header file into the document, or parse out the function prototypes and docstrings or something like that?)
https://review.coreboot.org/c/coreboot/+/87186/comment/ad7fec2b_11847702?usp... : PS4, Line 200: On x86 with CAR backed memory in romstage, this means : romstage before memory init No, it's actually bootblock for all architectures nowadays. (If you want, you could also mention how platform setup code on x86 or the decompressor on Arm can measure some timestamps earlier and pass them in to `bootblock_main_with_timestamp()`.)
https://review.coreboot.org/c/coreboot/+/87186/comment/b51fe47a_0a4f1aab?usp... : PS4, Line 252: - 100-115: Miscellaneous coreboot operations (e.g., I'm not sure if there really exists a difference between 1-99 and 100-199? I think that's all just the general coreboot kitchen sink range.
https://review.coreboot.org/c/coreboot/+/87186/comment/da4ec2dd_a33670e2?usp... : PS4, Line 254: - 500-600: Google/ChromeOS specific (e.g., `TS_VBOOT_START`, nit: maybe mention that many of the existing timestamps here are no longer Google-specific since many features originally added for Google vendorcode have since been migrated into general coreboot code.
https://review.coreboot.org/c/coreboot/+/87186/comment/5f1a4ccf_16066802?usp... : PS4, Line 327: . = ALIGN(8);\n _etimestamp = .;\n} nit: should usually use the `TIMESTAMP()` macro from `memlayout.h`, i.e. `TIMESTAMP(., 0x200)`
-
Julius Werner (Code Review)