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 10:
Patch Set 10:
That doesn't mean we shouldn't review the Tiger Lake addition. And I think we all agreed already that a 8k LOC patch isn't reviewable. And of course, if we had more common code already, it would be much easier... and we would have a better code base if 3ee54bb hadn't slipped in. Let's not repeat that error.
Sure, i believe Arthur has done great work of taking pain to review this CL. we have address majority of his feedback and few more is there. will continue to address also will see if we can submit this CL into 4 part
- Bootblock (soc/intel/tigerlake/bootblock/)
- romstage (soc/intel/tigerlake/romstage/)
- ramstage (soc/intel/tigerlake/)
- acpi (soc/intel/tigerlake/acpi/)
I still don't think "this CL" is a good idea, even if split up. Any commit history that first copies stale code and then adapts it in later commits (instead of adding the adapted code step by step) will lose lots of information.
For instance, I assume some developers have already looked at the copied code and identified chunks that can stay the same. A copy first, adapt later commit history will not contain this information. It would contain details about what was changed, but not about what wasn't. OTOH, if the commit that adds (pieces of) unchanged code targets a single topic, and the commit message states that this was confirmed to work on the new platform, nobody will have to doubt if that was already checked. And the information will be in the Git history and can help future maintenance.
i understand you are suggesting for incremental patches to create TGL code in upstream. but the philosophy i'm referring is that, we have already reviewed ICL code many time and should't we allow ICL code as is when we are renaming as TGL (this is copy patch logic) and then add only 8 CL to make TGL code complete. Rather we have many CL (definitely more than 8) to even reach at any stage in incremental approach. I agree that incremental approach is easy to review entire code but do we really like to review ICL code again in TGL development?
Well, I don't think ICL code was much reviewed either. Every line got a +2 at some point, yes. But that doesn't mean half of it wasn't rubber stamped. And you seem to completely ignore that reviewing includes to check available documentation (for the newer silicon and FSP).
Yes, you are right in terms of ICL code because of not active chrome product out. But ICL is kind of base for next generation platform hence targeting ICL code. Thanks to all reviewers help to make ICL code in shape, really appreciate all help here.
If you want to catch subtle difference between ICL and TGL silicon and FSP interface early, the incremental approach seems so much easier. If you copy first, please mention in the commit message which parts were identified to behave exactly the same and which will be adapted in future commits. Then, reviewers can confirm that. However, if that requires to look ahead in the patch queue for every other line of code, you'll likely scare the best reviewers away.
yes, will capture those increment changes expectation in commit msg so we know what we are expecting to get at each CL when we split this one in multiple smaller CL
-
Subrata Banik (Code Review)