[coreboot-gerrit] [L] Change in coreboot[main]: Documentation/lib: Update Timestamp documentation

[Martin L Roth (Code Review)]

Attention is currently required from: Julius Werner, Nicholas Chin, Paul Menzel.

Martin L Roth 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/2362d70d_0db150b8?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.

Thanks. Fixed.

https://review.coreboot.org/c/coreboot/+/87186/comment/9cab99b7_76c886f9?usp... :
PS4, Line 86: uint64_t
> Looks like this was changed to `int64_t` with the addition of pre-CPU timestamps.

Done

https://review.coreboot.org/c/coreboot/+/87186/comment/7d101144_39f62b11?usp... :
PS4, Line 112: enum {
> I think this entire enum doesn't exist anymore. […]

Done

https://review.coreboot.org/c/coreboot/+/87186/comment/4715c79e_bc0393f5?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. […]

Updated.

https://review.coreboot.org/c/coreboot/+/87186/comment/eaf4f550_2b9996ce?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 […]

Done

https://review.coreboot.org/c/coreboot/+/87186/comment/ad041852_19cb2704?usp... :
PS4, Line 192: ### Core Functions
> Thanks, that sounds great! I don't really have the expertise to help you review this, but I hope we  […]

As I recall, we discarded the use of doxygen 5 or 6 years ago. We could pick it up again, but it just wasn't kept up to date, and was very confusing when vendorcode was pulled in. Vendorcode has doxygen documentation that was designed for those projects, not as a part of coreboot.

If we want to use doxygen again (which I'm not opposed to), we'd have to do a bunch of work to get it in shape.

Another alternative is to set up a tool that checks specific things to keep them in sync and give a warning in jenkins if the code no longer matches the documentation. We'd have to standardize some formats, but then it'd be easy to maintain:

```c
struct timestamp_entry {
        uint32_t        entry_id;
        uint64_t        entry_stamp;
} __packed;
```
> > > _Source: `src/commonlib/include/commonlib/timestamp_serialized.h`_

https://review.coreboot.org/c/coreboot/+/87186/comment/985f792a_dc8d6676?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. […]

Done

https://review.coreboot.org/c/coreboot/+/87186/comment/953312b7_204b234e?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 t […]

Done

https://review.coreboot.org/c/coreboot/+/87186/comment/7e930828_3df450d2?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 man […]

Done.

Thoughts on reorganizing all of these codes at some point? We've got lots of space in some areas, along with large gaps, but then the ME postcodes have to be split into two different sections.

Since it's a 32-bit value, we could do something like using the top 8 or 16 bits to identify the area (Intel/AMD/Google/coreboot), then the lower 16 bits for the actual ID in that area.

https://review.coreboot.org/c/coreboot/+/87186/comment/b66da8f6_f2eb4c01?usp... :
PS4, Line 327:     . = ALIGN(8);\n    _etimestamp = .;\n}
> nit: should usually use the `TIMESTAMP()` macro from `memlayout.h`, i.e. `TIMESTAMP(. […]

Done.