Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(8 comments)
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... PS6, Line 60: for (int i = 0; i < ARRAY_SIZE(bcfg->spd); i++) {
If bcfg->spd has more than 4 elements, the calculation to index into bcfg->channel_empty[] will read […]
Removed channel_empty.
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... PS6, Line 90: for (int i = 0; i < ARRAY_SIZE(bcfg->spd); i++) {
Same comment as for baseboard/memory. […]
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 77: break;
see comment in cnl_memcfg_init. […]
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 82: printk(BIOS_EMERG, "nonexistent memory slot: %u", mem_slot); : die("");
Same thing here, just pass that message to die(); I don't think the slot number is necessary.
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 100: mem_slot
add a define to the header for the amount of slots
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 144: No valid way to read mem info.
Just put this in die?
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 67: struct spd_info spd[4];
The #define that Patrick Rudolph requested in cnl_memcfg_init. […]
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 130: * to indicate that the channel is populated. For example,
Are you saying we can just remove channel_empty[] here? […]
I see. Thanks for the explanation.