Jamie 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 7:
(11 comments)
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 15: void
This should probably return a CB_ERR value and that can be used down on line 123.
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 19: int
unsigned
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 34: spd
SPD
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 45: int
Please use CB_ERR and friends.
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 56: *spd_cache = rdev_mmap_full(&rdev);
Shouldn't you check the CRC above to make sure the cache is valid? The update_spd_cache() can fail a […]
In new patchset, there is a function called spd_cache_is_valid, it can use to check if the data is valid.
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 64: BIOS_DEBUG
BIOS_INFO fits better in my opinion.
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 84: int
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 97: BIOS_INFO
BIOS_WARN
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 102: time
last system boot
Done
https://review.coreboot.org/c/coreboot/+/40415/2/src/mainboard/google/hatch/... PS2, Line 109: BIOS_INFO
BIOS_NOTICE
Done
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 41: 0x00
== 0 […]
In new patchset, it changed to sn == 0xffffffff when dimm is not present.