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:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 85: return sn == 0x00000000, if addr is 0x0. : return sn == 0xffffffff, if dimm is not present. */
How about just return values from the function?
In new patchset, it will return a CB_ERR and sn will become u32 pointer.
return CB_SUCCESS, sn is the serial number and sn=0xffffffff if the dimm is not present. return CB_ERR, if dram_type is not supported or addr is a zero.
Is it good for you?
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 87: u8 *sn
This still looks weird. […]
In new patchset, it will return a CB_ERR and sn will become u32 pointer.
return CB_SUCCESS, sn is the serial number and sn=0xffffffff if the dimm is not present. return CB_ERR, if dram_type is not supported or addr is a zero.
Is it good for you?
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 92: /* smbus will return 0xff if addr is zero */ : if (addr == 0x00) { : memset(sn, 0, SPD_SN_LEN); : return; : }
do_smbus_read_byte not check the address 0x00 but will return 0xff.. […]
As Eric mentioned, I saw return value -1(0xffffffff) when I removed the DIMM. And it returns 0x000000ff when addr is a zero.
Now I added the code for checking if addr is a zero, but I also thought if we can just put assert here.
assert(addr != 0x00);
How do you think?
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 99: dram_type
This is a u8, but do_dmbus_read_byte returns an int (where negative numbers are the error values), s […]
Done
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 120: Unsupport
Unsupported
Done