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:
(1 comment)
Patch Set 9:
Sorry, but all companies and developers have to go through the process of submitting quality software stated in the coreboot trademark license agreement as well. Be prepared that this code only goes into the tree if the common code stuff is done before.
I have to side with Intel on the matter of common code. While I totally agree that we shouldn't duplicate too much code, we never made maximizing common code a requirement for new platforms. And it seems harsh to arbitrarily add that requirement when people feel time pressure.
Thanks Nico for understanding the problem what we are dealing with against timeline.
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
1. Bootblock (soc/intel/tigerlake/bootblock/) 2. romstage (soc/intel/tigerlake/romstage/) 3. ramstage (soc/intel/tigerlake/) 4. acpi (soc/intel/tigerlake/acpi/)
Also i was just looking at CLOC inside TGL directory with this newer patchset. Looks like we have only ~4K LOC inside each soc but that doesn't mean we can't do further common code. common SMM is something we are doing and common report platform implementation also there. Let me revive those CL. But i will hope to get your support after i split this CL into 4 CL as mentioned above for further review and possibility merge by this week (please)
src/soc/intel/tigerlake$ cloc . 73 text files. 73 unique files. 14 files ignored.
http://cloc.sourceforge.net v 1.60 T=0.28 s (211.0 files/s, 21877.3 lines/s) ------------------------------------------------------------------------------- Language files blank comment code ------------------------------------------------------------------------------- C 30 460 957 2228 C/C++ Header 28 285 610 1559 Pascal 1 1 0 18 ------------------------------------------------------------------------------- SUM: 59 746 1567 3805 -------------------------------------------------------------------------------
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/loc... File src/soc/intel/tigerlake/lockdown.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/loc... PS8, Line 20:
You can still reference the document number and chapter, even if it's not published yet. […]
sounds good. i can write a document with those document number and chapter with file details. Will that help?