Lean Sheng Tan 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:
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.
Hi Nico, Sorry for late reply, got tied up with other projects only have time recently to resume my upstreaming work again. There are many differences on PCH side for EHL & JSL. Just for your understanding, we used to plan to merge in TGL, JSL & EHL into one SOC folder, but google folks found there are too many config params hence they decided to split them out. That's why we have 3 separate platforms now.
We use JSL folder as the skeleton code base because it has the less differences among all projects.
Good point, let me upstream the mainboard folder right away, will take note of this in future.
The main reason we are using JSL as our base model also based on our agreed model with Google folks and the community, that we will follow the model used by TGL: https://review.coreboot.org/q/topic:%22TGL_UPSTREAM%22+(status:open%20OR%20s...) This model is being used across all latest Intel platforms (JSL, ADL etc).
Please let me know if you have any concern otherwise, hope these answer your questions.