Edward O'Callaghan 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 3:
Patch Set 3:
Patch Set 2:
Patch Set 2:
Patch Set 2:
There is also this change to make SPDs optional in CBFS: https://review.coreboot.org/c/coreboot/+/36068
From inspection I don't think that is actually workable in its current form. variant boards are assumed to define GPIO straps that wouldn't exist for a smbus romstage type board. Frankly there is too much love of pre-processor going on and it makes a tangled mess for no apparent value. The weak symbols only make it worse by making runtime fragile as well as compile time.
I would strongly suggest we make a split between romstages that are appropriate for each path instead of trying to conflate them and hack around with ifdefs. The former solution is simple and robust.
In order to handle the GPIO straps problem, variant_memory_sku() just needs to move into variants/baseboard/ which provides default GPIO straps. Also, check my latest comment about dealing with read_type differently. In that case, variant_memory_sku() will not even get compiled in and will get dropped by the linker.
You can use runtime "if CONFIG(...)" without having to use #ifdefs. Splitting romstage file is just going to end up duplicating most of the code in that file. Any ways, if that seems cleaner, feel free to go ahead. Please ensure that you add Shelley and Tim on the CLs you are pushing.
I agree with furquan here that we should avoid the code duplication inherent in forking romstage.c. Comparing this CL with CL:36449 shows there is much overlap between the two.
Are you sure there is overlap? The functions are actually very different.