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:
(3 comments)
Patch Set 8:
(15 comments)
Patch Set 8:
Patch Set 8:
Patch Set 8:
Just to share the thought process behind the copy patch logic and creating newer directory for Intel latest SOC
- ICL and TGL are different SOC target for different launch year, what does that mean is that, FSP source tree version would be different in both cases so growing TGL inside ICL might be needs to deal with so many #IFDEF/if/#IF logic to guard between ICL and TGL specific changes if we consider UPD alone (which might be ~50% SOC/MB code touch point)
I don't think anyone has a problem with UPD's being handled separately.
its been problem for validation across team (as we have seen in past developed might break other soc without proper knowledge of usage), when multiple soc are getting hooks inside as parent soc (unnecessarily like ICL and TGL), Folks who are using ICL coreboot code might bother about TGL code pieces and vice versa without any reason as these are 2 completely different generation product. we always like to maintain to same generation product under same parent soc directory like CNL/CFL/WHL as an example
Coreboot has one tree for all targets and encourages common code. coreboot is not a 'copy and forget about' UEFI. If you intend to have support in coreboot without breaking other platforms, the solution is not to copy code again and again because of being overscrupulous about breaking things, but set up a way to get your platforms validated if needed.
its not completely different i agree but don't fully agree that copy code might not be a solution here.
- This SOC is quite similar to ICL and a lot of code can be reused. Copying all the code and making small modifications is not the best strategy for reasons Nico explained.
its not "small" for that matter either but we can assured that we have visibility of amount of code went in by copy and how do we plan to use those codes.
I don't understand this. Could you rephrase it in a different way?
the view we have about common code and the effort might require to comprehend the same might not meet the timeline what we are looking at to enable TGL at upsteam as per customer request.
I can assure you that reviewing and merging a single 8K LOC commit will take longer than going through the effort of reducing non SOC specific code in here! Have you considered that asking to review such a large and non trivial commit is not a reasonable demand to make?
I only skimmed through the first files.
working on to make lpc.asl common across all intel soc also few common ASL declaration moved into common placeholder to avoid duplicate code. ETA tomorrow to pus CL for review
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/cnvi.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 1: /* this file doesn't exist inside icl soc directory. please don't any file why is not present because we would like to have "only" copy patch from ICL
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 42: Store (^^PCRB (PID_GPIOCOM0), BAS0) : Store (GPIO_BASE_SIZE, LEN0)
Could you please move to the ASL 2.0 syntax? E.g. LEN0 = GPIO_BASE_SIZE.
reasonable to do this but should we target any particular soc for moving towards ASL 2.0 syntax ?
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
You have a fixed mapping of PIRA-H to either 10 for PIRB and 11 for all the rest. […]
didn't get ur thought ?
-
Subrata Banik (Code Review)