Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )
Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................
Patch Set 1: Code-Review-1
Well, it's nice to have these timestamps. Different (de)compression algorithms need different amounts of time to decompress things.
Sorry, I think you may misunderstand the core problem here: these timestamps cannot accurately reflect decompression speed on memory-mapped SPI devices. On an Arm device, when decompressing an LZMA compressed stage, we first load all the compressed data into CBFS_CACHE (this is the rdev_mmap() in cbfs_load_and_decompress()) and then we run the decompression algorithm. By putting a timestamp in-between those two calls, we can tell how much of the total time was spent on loading and how much on decompression.
But on an x86 device with memory-mapped SPI, rdev_mmap() is pretty much a no-op that just returns a pointer into the memory-mapped region. The data is only actually loaded from SPI at the time the processor accesses that pointer, which it does as part of the decompression function. That's why it's impossible to cleanly separate the time spent on loading and time spent on decompressing, because they're interlocked with each other. That is why this says "ignore on x86" -- because on x86 (at least the memory-mapped ones), the time between TS_START_ULZMA and TS_END_ULZMA includes all the SPI loading time as well (and the start/end of loading stage timestamps don't list any significant extra time on top of that. So there's no value in having the extra decompression timestamps and it's best to ignore them..
I agree with Patrick that just not logging the timestamp at all would probably be a better move, and that it's cleaner to key off CONFIG_BOOT_DEVICE_MEMORY_MAPPED instead of just the architecture. So I think a change in that direction would be useful. But this patch as it is seems wrong -- the reason that remark is there is still very much true today.
(BTW, I think for LZ4 the timings are actually correct because LZ4 does a full rdev_read() before the decompression. But that is inefficient for x86, so the proper fix there would be to change that to use rdev_mmap() on CONFIG_BOOT_DEVICE_MEMORY_MAPPED devices instead.)