Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44799 )
Change subject: soc/intel/elkhartlake/bootblock: Do initial SoC commit until bootblock ......................................................................
Patch Set 7:
Hi, I know I'm pretty late for review but would like to drop some comments anyway.
Without having looked into documentation, I wonder how different are Jasper Lake and Elkhart Lake actually? Are the differences really big enough to need another copy of the code?
If they differ much and you just need a skeleton to start with, Jasper Lake might not be a good starting point. Because the community usually cleans up and enhances the code starting with older platforms, the latest copies, in this case Jasper Lake, are usually the most stale, with the lowest quality.
If they don't differ much, it would be better to support them in a single directory.
I see no mainboard selecting the code. This means that the code is not build tested and if one doesn't know if the code compiles the review usually turns to be super- ficial. Because this has caused trouble in the past, we agreed that it is best to add a board along with the very first patch train, so nothing needs to be reviewed before it is build tested. This doesn't have to be a real board, just something that hooks the code up for building.
It looks like this is just a copy with names replaced. This way of adding a new platform has caused many discus- sions in the past. There is a big disadvantage: The code will never be properly reviewed. Because no reviewer will ever know when it is the right time to review if the code matches Elkhart Lake. Only changes that are made later can be reviewed, but all the code that stays as it is after the initial copy will be left unchecked, causing bugs and a maintenance burden in case it doesn't do anything useful for the new platform. IMHO, this is the wrong way to add new platforms. If you want to have a look at a good example how it can be done:
git log -p --reverse -- src/soc/intel/cannonlake/
This code was added step-by-step and was reviewed for Cannon Lake from the beginning.