Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45059 )
Change subject: soc/amd/picasso: pass verstage timestamps to x86 ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 123: #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
No particular reason, I just followed what's done in bootblock_soc_init() below.
That shouldn't be guarded by an #if either.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 125: CONFIG_PSP_SHAREDMEM_BASE;
Are you referring to line 132? Or did you mean "(struct transfer_info_struct *)(uintptr_t)CONFIG_PSP […]
CONFIG_PSP_SHAREDMEM is a #define that is an int to the type system. ints should not be casted as a pointer for code compatibility. They should be cast through a pointer type that has the correct length. Your suggestion of using (uintptr_t) would be correct, but it might need to go through (void *) as well. i.e. (struct transfer_info_struct *)(void *)(uintptr_t)CONFIG_PSP_SHAREDMEM_BASE;
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 130: uint32_t tick_freq_mhz = timestamp_tick_freq_mhz();
It's done in line 145-149. […]
We should normalize to use 1us tick regardless of environment. Then there's no funny math required.
https://review.coreboot.org/c/coreboot/+/45059/2/src/soc/amd/picasso/bootblo... PS2, Line 149: base_timestamp = psp_ts_table->base_time * tick_freq_mhz; None of this ensures the timestamp from x86 is monotonically increasing relative to psp. What are you doing to ensure a consistent timeline?