Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38715 )
Change subject: mb/google/volteer: use new tigerlake memory config ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38715/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/romstage.c:
https://review.coreboot.org/c/coreboot/+/38715/5/src/mainboard/google/voltee... PS5, Line 27: memset(&board_cfg, 0, sizeof(board_cfg)); This can also be avoided if you go with the comment on SoC CL.
https://review.coreboot.org/c/coreboot/+/38715/5/src/mainboard/google/voltee... PS5, Line 37: #ifdef GPIO_MEM_CH_SEL I don't think this check is required. This is provided in baseboard. In the future, if there is a variant that doesn't need this, we can add the check.
https://review.coreboot.org/c/coreboot/+/38715/5/src/mainboard/google/voltee... PS5, Line 41: board_cfg.spd.read_type = READ_SPD_CBFS; : board_cfg.spd.spd_spec.spd_index = variant_memory_sku(); This can be changed to:
const struct spd_info spd = { .read_type = READ_SPD_CBFS, .spd_spec.spd_index = variant_memory_sku(); };
https://review.coreboot.org/c/coreboot/+/38715/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/baseboard/memory.c:
https://review.coreboot.org/c/coreboot/+/38715/5/src/mainboard/google/voltee... PS5, Line 47: void variant_memory_params(struct mb_lpddr4x_cfg *bcfg) : { : memcpy(bcfg->dq_map, baseboard_memcfg.dq_map, : sizeof(baseboard_memcfg.dq_map)); : : memcpy(bcfg->dqs_map, baseboard_memcfg.dqs_map, : sizeof(baseboard_memcfg.dqs_map)); : : bcfg->ect = baseboard_memcfg.ect; : bcfg->half_populated = baseboard_memcfg.half_populated; : } See my comment on the SoC CL. If you go with that change, then this function can simply be reduced to: struct mb_lpddr4x_cfg *variant_memory_params(void) { return &baseboard_memcfg; }