Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 48: memcfg.spd[0].read_type = READ_SMBUS;
Why are we marking this is as READ_SMBUS when get_spd_smbus() is doing the reading and filling in th […]
So I realised now that FSP has a parallel mechanism internally for reading SPD data aside from that of the native coreboot version of get_spd_smbus(). I have conflated them while looking at other Intel boards and I should choice just the one mechanism.
Thanks for catching this, I wonder if we should make a patch to ensure other platforms do this in a consistent manner as a separate thing?
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 49: memcfg.spd[0].spd_spec.spd_smbus_address = (uintptr_t)blk.spd_array[0];
spd_smbus_address is expecting an smbus address. 0xa0 (0x50 shifted) or 0xa4 (0x54 shifted). […]
Your absolutely correct, I mixed this up with 'MemorySpdPtr00' too much CamelCase for my dyslexic brain thanks for the catch!!
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 52: 1
I'm not sure this should be 1. […]
Interesting, noted thanks. I will rework this implementation based on this and the above. Thanks very much for looking it over as this is my first FSP experience so its very helpful for the second more informed eye.