Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36492 )
Change subject: mainboard/google: Move variant_memory_sku() to baseboard/ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36492/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/c/coreboot/+/36492/2/src/mainboard/google/hatch/... PS2, Line 39: mem_sku = variant_memory_sku();
When it is a nop, what value would this return so that things continue to work in both memory down and dimms?
If we decide to share the same code for SPDs and DIMMs, then it would be better to reorganize the function such that the paths that do not matter for one type of memory are not taken at all by the other path.
Thus, this function should look like:
void mainboard_memory_init_params(FSPM_UPD *memupd) { is_single_ch_mem = gpio_get(GPIO_MEM_CH_SEL); variant_memory_params(&memcfg);
if (CONFIG(...SMBUS)) memory_init_params_smbus(&memcfg, is_single_ch_mem); else memory_init_params_spd(&memcfg, is_single_ch_mem); }
Along w/ my first question I'm wondering about this is_single_ch_mem discovery and if that makes sense.
I see the single channel GPIO on Puff, but I am really not sure if that is a valid case.
I think I may like the other approach, fwiw, as there's lots of specifics that become odd in comparing hatch to puff -- largely this memory topology.
That sounds okay to me. If the overall changes diverge too much, it would be better to just separate them out as Edward did in his other CL.