Subrata Banik 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:
(1 comment)
Patch Set 12:
Patch Set 12:
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. 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. And i have shared the document number so u can verify whatever coming new makes sense or not.
So what you are really trying to ease, IMHO, is the conscience of whoever is supposed to rubber-stamp this.
trying to ease the review time of old platform and helping to have smooth review only on new SOC code given that ICL been reviewed properly (Arthur has spent significant to review almost all soc files from ICL to review achieving that)
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... File src/soc/intel/tigerlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... PS12, Line 1: /*
Is this file not copied 1 on 1 many times in the tree? Make it common?
yes, this file is a candidate for common code i had started sometime now https://review.coreboot.org/c/coreboot/+/26138