Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40414 )
Change subject: lib/spd_bin: add get_spd_sn function ......................................................................
Patch Set 6:
(17 comments)
Patch Set 2:
(4 comments)
Just some minor things I noticed Jamie that you could address on your next iteration.
Thank you.
https://review.coreboot.org/c/coreboot/+/40414/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40414/2//COMMIT_MSG@9 PS2, Line 9: added
adds
Done
https://review.coreboot.org/c/coreboot/+/40414/2//COMMIT_MSG@10 PS2, Line 10: number.
This is only implemented for Intel, isn’t it? Please mention that in the commit message.
Done
https://review.coreboot.org/c/coreboot/+/40414/2//COMMIT_MSG@11 PS2, Line 11:
Where and how should this be used? To fill SMBIOS tables?
It's for SPD cache.
https://review.coreboot.org/c/coreboot/+/40414/3/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/40414/3/src/include/spd_bin.h@51 PS3, Line 51: void get_spd_sn(u8 addr, u8 dram_type, u8 *sn);
There is SPD_SN_LEN, but that is very implicit. […]
I checked the SPD serial number lengths are the same in DDR3 and DDR4. In the future, if there is new type added and the length is not 4 bytes. We can add it with new patch. It likes the SPD_PAGE_LEN and SPD_PAGE_LEN_DDR4 case.
https://review.coreboot.org/c/coreboot/+/40414/3/src/include/spd_bin.h@51 PS3, Line 51: dram_type
There's enum spd_memory_type in spd. […]
Thanks for the info. I will remove the dram_type parameter and use SPD data to detect dram_type.
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 79: int
unsigned
Done
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 80: if ((do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) & 0xff) == 0xff) {
Don’t we already know from RAM training/initialization, what slots are populated?
I think that it can use SPD byte 2 to identify the Dram type. So I will do it in the new pathset.
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 80: if ((do_smbus_read_byte(SMBUS_IO_BASE, addr, 0) & 0xff) == 0xff) {
oh, typo.. […]
Done
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 82: addr << 1);
I think you can fit this on the line above.
Done
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 94:
s/\t/ /
Done
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 100:
s/\t/ /
Done
https://review.coreboot.org/c/coreboot/+/40414/2/src/soc/intel/common/block/... PS2, Line 101: }
Print an error, if it’s neither DDR4 nor DDR3.
Done
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 41: dimm_preset
dimm_present.
It will be removed in next patchset.
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 43: < 0
So even if the host controller is busy, it will still return < 0. […]
I checked the return value is -1 (CB_ERR) when dimm is not present. I will change it to return a CB_ERR when addr is a zero and dram_type is not supported. Also it reported dimm is not present by sn=0xffffffff when smbus return value < 0.
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 44: addr << 1
If it's passed in as addr, I don't see a reason why would you print it out shifted left 1. […]
Done
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 91: void
suggestion: this should return a bool, false for the !dimm_present or unsupported enum spd_memory_ty […]
I will do this at the next patchset.
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 95: if (!dimm_preset(addr)) {
Please add add check for non-zero like #77. If addr is 0, smbus will return 0xff.
Add the code to check if addr is a zero in new patchset.