Arthur Heymans 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:
(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.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 100: 11
didn't get ur thought ?
In PICB IRQ16-23 (A-H) get mapped to 11 or 10 (IRQ17/B). Since you know the IRQ information at compile time you could easily generate this whole file in ssdt. See southbridge/intel/common/acpi_pirq_gen.c. I could work on implementing this on cannonlake or icelake if you want (and can test it).