Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 68: true
nit: for clarity, maybe set this to `dimm_changed`. […]
IMO, that's actually less clear, and I don't want to diverge the implementation here and in google/hatch unnecessarily if we're going to end up unifying it at the SoC or common level
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 104: cannonlake_memcfg_init(mem_cfg, &memcfg);
Wouldn't it make more sense to move the spd cache code into `cannonlake_memcfg_init`?
possibly, but one would at least need to pass in the channel config, SPD address map, etc. But open to ideas