Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36068 )
Change subject: automation: update script and template files for new variant ......................................................................
Patch Set 5:
(3 comments)
Patch Set 3:
Patch Set 3:
Wonder if it would be better to allow an empty SPD_SOURCES and print an info instead of an error in spd/Makefile.inc.
Allowing an empty SPD_SOURCES means we need extra logic for when spd.bin doesn't exist (because it only gets built if SPD_SOURCES is non-empty), and then the C array that points to the SPD data has to have special logic as well. And then when you try to run your code, it doesn't work because you don't have any SPD for your DRAM so MRC doesn't know what to do.
On the other hand, if we start with an "empty" SPD, we have one slot filled, spd.bin gets built, the C array has some contents, it's just that (as before) your code won't work, though this time it's because the SPD doesn't work for the DRAM you have.
We have some new variants for Hatch coming up (Puff) that do not use SPD. So, we anyways need to re-factor the code to ensure that we allow for it. Change should as simple as : diff --git a/src/mainboard/google/hatch/spd/Makefile.inc b/src/mainboard/google/hatch/spd/Makefile.inc index 9ab7394b30..1f7bb35a9e 100644 --- a/src/mainboard/google/hatch/spd/Makefile.inc +++ b/src/mainboard/google/hatch/spd/Makefile.inc @@ -16,10 +16,7 @@ SPD_BIN = $(obj)/spd.bin
ifeq ($(SPD_SOURCES),) - SPD_DEPS := $(error SPD_SOURCES is not set. Variant must provide this) -else SPD_DEPS := $(foreach f, $(SPD_SOURCES), src/mainboard/$(MAINBOARDDIR)/spd/$(f).spd.hex) -endif
# Include spd ROM data $(SPD_BIN): $(SPD_DEPS) @@ -32,3 +29,5 @@ $(SPD_BIN): $(SPD_DEPS) cbfs-files-y += spd.bin spd.bin-file := $(SPD_BIN) spd.bin-type := spd + +endif
https://review.coreboot.org/c/coreboot/+/36068/5/util/mainboard/google/hatch... File util/mainboard/google/hatch/template/gpio.c:
https://review.coreboot.org/c/coreboot/+/36068/5/util/mainboard/google/hatch... PS5, Line 24: override_gpio_table This is already provided as a weak function in baseboard/gpio.c, so there is no need to add this unless the variant really requires it.
https://review.coreboot.org/c/coreboot/+/36068/5/util/mainboard/google/hatch... PS5, Line 40: variant_early_gpio_table I think we should just provide a weak implementation of this in baseboard/hatch. Then, this entire file can be skipped here.
https://review.coreboot.org/c/coreboot/+/36068/5/util/mainboard/google/hatch... File util/mainboard/google/hatch/template/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/36068/5/util/mainboard/google/hatch... PS5, Line 21: #define GPIO_MEM_CONFIG_0 GPP_F20
It is there just to make the code compile. […]
It would be worthwhile moving this to baseboard/gpio.h. Variants can always re-define those later if they don't use the same configuration as baseboard.