Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40415 )
Change subject: mb/google/puff: add a region to cache SPD data ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40415/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/40415/4/src/mainboard/google/hatch/... PS4, Line 40: get_spd_sn(blk.addr_map[i], SPD_DRAM_DDR4, sn);
Should dram type read from SPD? If hard code here, can't work on DDR3 right?
my 2 cents I think the code only cache and get the SPD SN portion. So, the dram type is not saved or read. But, i think it would be a good idea if we can cache and read dram type as well. In doing so, we don't need to hard code the dram type here.
https://review.coreboot.org/c/coreboot/+/40415/4/src/mainboard/google/hatch/... PS4, Line 41: 0x00 == 0
Also, isn't sn[0] good enough to check?
https://review.coreboot.org/c/coreboot/+/40415/4/src/mainboard/google/hatch/... PS4, Line 85: (uintptr_t)blk.spd_array[0]; i guess you can add few more tabs to get closer to the `=` in previous line
https://review.coreboot.org/c/coreboot/+/40415/4/src/mainboard/google/hatch/... PS4, Line 96: (uintptr_t)blk.spd_array[1]; i guess you can add few more tabs to get closer to the `=` in previous line
-
Kane Chen (Code Review)