Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )
Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 61: ROMSTAGE_SPD_CBFS Wouldn't it be easier to add a Kconfig ROMSTAGE_SPD_SMBUS instead of ROMSTAGE_SPD_CBFS. Then this can be set only by boards that obtain SPD using SMBus and you also don't need to compile out the romstage.c file.
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 23: CONFIG_ROMSTAGE_SPD_CBFS I think including this file can be helpful even without SPD being in CBFS. See comments on file.
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_cbfs.c:
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 44: mainboard_memory_init_params This function will have to be re-organized a little bit.
1. Call variant_memory_params()
2. Get single channel info using GPIO_MEM_CH_SEL
3. If ROMSTAGE_SPD_SMBUS is set, call memory_init_spd_smbus() which will set read_type to READ_SPD_SMBUS and set the addresses as required.
4. If #3 is not true, call memory_init_spd_cbfs() which will set read_type to READ_SPD_CBFS and get the mem_sku using variant_memory_sku().
https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 81: This function can just return early if ROMSTAGE_SPD_SMBUS is set.