Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit ......................................................................
Patch Set 8:
Patch Set 8:
(1 comment)
Patch Set 8:
Patch Set 8:
Oooops, I think people are talking past each other here.
Subrata, nobody suggested that we shouldn't create a tigerlake/ directory, we should!
Arthur, nobody suggested to review this patch. I assume Subrata deliberately ignores our development guidelines and wants this rubber-stamped.
Our problem is not with the introduction of 8k LOC for Tigerlake. Sure, more common code would be nice, but we understand that it's not the best time. The problem is that you want to skip review. Which multiplies future maintenance effort, just to save somebody some days to write proper commits. That's very harmful for the project as a whole. Without a reasonable commit history (from which one can tell if a piece of code was reviewed for a particular piece of silicon) maintenance cost increase so heavily that it delays future work like more common code and eventually also the next SoC generation. It also increases the regression rate, it wastes resources on bug hunting, ... A broken Git history is a technical debt that can only be repaid by starting from scratch. I doubt that is the plan here.
So, instead of adding 8k LOC at once and unreviewed, we want you to add the code topic by topic, and let it be reviewed commit by commit. That shouldn't be hard. Just take the current state you have achieved, throw everything away that isn't validated yet, and put what is left into smaller, reviewable commits. Please!
its reasonable to add code block by block but end of 50+ CL then we will see ultimately we have same 8K LOC in TGL as existed today in ICL. rather can we have areas identified as arthur has mentioned should be good to fix in ICL code itself then copy into TGL ?
I don't this approach is beneficial for your timeline. I only skimmed through and found some superficial issues. It will take you too much time to get this 8k LOC reviewed in *one* CL. If you split things up, the review process will be quickened a lot! Going back and forward with fixing ICL and copying the improved code will likely even slow the process as it will be hard to track progress.
Arthur got ur point but as ICL and TGL won't have that significant delta in coreboot code as per our study hence at end of the day, u might have ended up reviewing ICL code again as you have done so many time by now. i saw the current feedback, its more over two broad category
1. Review can be addressed into ICL itself (feedback from you majorly), Status : WIP [https://review.coreboot.org/c/coreboot/+/36431], will find folks around to work on this in parallel
2. Review depends on EDS or datasheet (feedback from patrick R majorly). As its early development of TGL i'm afraid we might not have open EDS at this moment but i can say you will find almost similar registers in ICL datasheet
will you bear time to improve ICL code in mean time, i'm expecting to address all review in this CL by tomorrow EOD then we will again regenerate TGL copy patch ?