Aaron Durbin 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();
I think we are all reaching the same conclusion here.
In regards to the case of `variant_memory_sku()` being a nop - in the follow up patch I moved it to be close to the call site. I did that in a separate patch to deal with cornering it out. I am not sure 100% about the is_single_ch_mem discovery logic is actually needed for Puff, although the schematic does mention the strap so I will look further. I have to fill in the gap in my understanding of whats right to do there however it seems simple enough.
I don't think single channel needs/requires a gpio if the memory subsystem is using dimms. That can be inferred from the presence of the dimms.
Ultimately I think it best to drop this route of exploration and focus on the simple slice down the middle of having a romstage.c that is appropriate for down and seated dimms respectively. My reasoning is that:
Are you abandoning this patchset then?