Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit till ramstage ......................................................................
Patch Set 12:
HI Arthur/Philipp,
Will you please consider your -2 with this new sets of split CL in place and we have icl base clean up cl also merged ? This will help us to land TGL SOC copy code ?
This still has received no real review. It references many docs that I cannot find so how do you expect proper review to happen?
I have access to the documents and would be willing to review. However, I want to be fully honest: a review of copy-first patches will take weeks if not months. It pushes a lot of work that should be done by the authors to the reviewers and will create a bottle-neck. I estimate at least five times more work for reviewers this way. Doesn't seem fair, and due to the overhead of copy-first patch-later, it might even be less work for me if I write the patches by myself.
Again, document said to have TGL specific changes which are yet to land. this copy patches are placeholder from previous soc to help review only TGL specific changes as per logic.
Again, again, again, it eases the review of differences, yes; but it also increases the effort to confirm that everything else really stayed the same by one or two orders of magnitude.
Shouldn't the same we can confirm based on TGL specific CL's for an example: GPIO driver for TGL. If its an incremental change over original ICL gpio code then why should I write again a fresh code? thats the argument i have.
I don't say you should. But I don't see how this information, that it was an incremental change, is valuable in our Git history.
Given that we don't have trust on SOC vendors that they know what they are doing with new soc. in terms of IP. We are just asking help to make the code land and its matter to have 8-10 CL to make entire ICL to TGL transition.
That 8-10 sounds bad to me. Few, big commits are 10 times harder to review than many, small commits.
And i have shared the document number so u can verify whatever coming new makes sense or not.
I think we should also verify that everything that we assume is unchanged in hardware really didn't change. Otherwise, we'll create hard to track bugs for the future.
Let's stick to your GPIO example. In the current patch set, `gpio.c`. How is one supposed to review this file?
* To see if this is the final state of the file, I'd have to look ahead on the patch branch. Time wasted. * It is not the final state, so I'd have to figure out for every line of the file if that line is in its final state. Incredibly much time wasted. * If I have to comment on one line but surrounding lines are patched later, on which change shall I comment? => messy review, more time wasted.
Another example `graphics.c`. This file is flawed, but not changed later it seems. If we don't go through the pain of reviewing it as part of the messy review, the errors will stick. Now, in a year or so, somebody might run into a bug there, they might check the Git history for clues about the code:
* How is one supposed to know if the code was already checked to be Tiger Lake compatible? * If somebody assumes that it was checked but it wasn't, more time gets wasted.