Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45131 )
Change subject: lib/Makefile.inc: fail build when SPD would be empty ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45131/14/src/lib/Makefile.inc File src/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45131/14/src/lib/Makefile.inc@360 PS14, Line 360: test -n "$(SPD_SOURCES)" || \ : (echo "HAVE_SPD_BIN_IN_CBFS is set but SPD_SOURCES is empty" && exit 1)
I'd rather avoid getting such incomplete states of board variants into the tree. I expect boards that make it into master to be able to boot, even if half of the features (e.g. Wi-Fi, touchpad, touchscreen, suspend and resume...) don't work.
The initial commits are primarily to seed the variant. Example: the template file commits for different google variants that are added in the tree. It is to help anyone easily add a new variant without having to manually copy the same code around (most of the times, people end up copying the wrong bits which never get dropped until someone runs into an issue). Thus, the scripts to seed a new variant enable building it with bare minimum files. Almost every time, you need more follow-up code to actually have that variant boot -- it might come out of reset, but I don't think it is fair to assume that it will just boot all the way.
Even if we add a board manually, it is much easier to add and review smaller logical pieces of code than copy-pasting a big chunk. One good example is dedede - https://review.coreboot.org/c/coreboot/+/38277. That series broke down in small logical pieces the addition of a mainboard support. In my opinion, it made things very easy to review as well.
I understand your intention to ensure that we are carrying tested code. But, for a new board being added, this is very difficult (example: almost every time a new board is added, it's hardware is not available and so even a complete board support wouldn't be tested until the change lands and makes it to a build that is provided to factory).
The reason I brought this up is https://review.coreboot.org/c/coreboot/+/45746. That CL works fine to ensure that we start with an empty SPD. But the problem I see is in terms of maintenance. Example: Volteer supports variants which might have LP4x or DDR4 memories. To avoid confusion, the CL is changing ddr4-empty-spd.hex in tigerlake to empty.spd.hex so that the starting SPD doesn't mention any particular type. It works okay as long as the empty.spd.hex is not used later for filling in empty SPDs when some DRAM strap IDs are unused - else we need to ensure that the size of the empty SPDs are correct as per the memory type. I don't think it's difficult to get all of that correct. But, I think we are unnecessarily adding more manual work to ensure things always stay this way and don't silently cause other issues.
-
Furquan Shaikh (Code Review)