Edward O'Callaghan 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36492/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36492/1//COMMIT_MSG@9 PS1, Line 9: unreasonably
Edward, makes sense. I was trying to point how I was reading the various efforts and wording. […]
Np just my usual poor grammar from dyslexia don't read too deeply into my wording however thanks for the feedback Aaron.
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 […]
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.
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:
- it is super easy to do, - makes progress with puff, - is easy to reason about without pre-processor and linker tricks and, - I think most importantly the least invasive way forwards.
We pick a path and compile in the appropriate object file that provides the correct worker function, specifically - `void mainboard_memory_init_params(FSPM_UPD *memupd)`, fixes to one path then do not run the risk of breaking the other so easily.
At the very core of this I think it comes down to where to bifurcate, be it at the source/runtime level or at the build time level. In essence it would be my view to slice and dice with the kconfig knob at build time. Apologies in advance if I didn't explain myself well, hopefully it all makes sense?
However, thank you for taking the time to look over both solutions!