Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 3:
(4 comments)
I wonder, is it necessary to manually set the size of the CAR/SRAM FMAP cache via a config option? Or could we just set it to the exact size of the known FMAP and just have the config as a on/off? (And if there isn't enough room, it'll still know at link time and fail.)
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@35 PS3, Line 35: #define STR(x) XSTR(x) : #define XSTR(x) #x I don't doubt that this is necessary... but what does it accomplish?
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@35 PS3, Line 35: print_once Not sure I like this name... it sounds like the function should take care of setting fmap_print_once itself. On the other hand, I can't think of anything better at the moment.
Will this end up printing once per stage?
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@305 PS3, Line 305: const ssize_t ret = rdev_readat(&fmrd, fmap, 0, s); So is this re-reading FMAP when CBMEM comes up, rather than copying it from CAR/SRAM if it is available?
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... File src/soc/mediatek/mt8173/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... PS3, Line 44: 12K Should probably note this reduction in the commit message.