Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48729 )
Change subject: Add SPI trace map support ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/em100/+/48729/2/em100.c File em100.c:
https://review.coreboot.org/c/em100/+/48729/2/em100.c@1247 PS2, Line 1247: ( nit: space before (
https://review.coreboot.org/c/em100/+/48729/2/map.c File map.c:
https://review.coreboot.org/c/em100/+/48729/2/map.c@33 PS2, Line 33: load_trace_mapfile We can get a little fancy with this using a structure that stores pointers to child and parent and organize this like a tree. That will have a few benefits: * Order in the map file is not important. * Duplicate entries can be caught and handled. * No need to have special logic for handling first entry as being "FLASH" - Keep dropping levels from the top of the tree if the tree has a single node. This will eliminate any redundant parents in the beginning or anywhere in the map file. * Reduces search space for each transaction.
Parent pointer is not strictly required, but can be helpful if we have to traverse back up.
https://review.coreboot.org/c/em100/+/48729/2/map.c@89 PS2, Line 89: void Would it make sense to update this function to also combine multiple occurrences of an operation to the same region into a single print? Basically, doing the same thing that the go tool is doing. It can also be done based on a command line arg if required.
It probably might be easier to handle this at the caller rather than within this function i.e. requires more helper functions for the map file.
1. void *map_get_matching_entry(uint64_t addr); 2. void map_get_path(void *entry, const char *str, size_t len); 3. bool map_entry_has_addr(void *entry, uint64_t addr);
The caller can then save what operation is being performed and map entry. If the next operation is the same as last one and going to the same entry, then it can just club the address together and print out a single entry. Something like:
SI_BIOS > UNIFIED_MRC_CACHE > RW_MRC_CACHE: 0xbb (dual I/O read): 0x01868c00.........0x01868d00
We can keep printing '.' for each operation to the same region and finally if the next operation is to a different region or after a certain timeout (~2-3 seconds), print out the last address that was accessed in the region.
And with the command line option we can control if the addresses should be combined together or a new line should be printed for each transaction.
https://review.coreboot.org/c/em100/+/48729/2/map.c@92 PS2, Line 92: GCC decided that it is a good idea to warn : * if strncpy is used to potentially only copy a substring. Is it complaining for: strncat(path, " ‣ ", 5);