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:
(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.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 21: #include <ec/google/chromeec/ec.h> This should not be here.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 32: #include <vendorcode/google/chromeos/gnvs.h> Can you work on making GNVS common and move this out of here?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 209: if (CONFIG(CHROMEOS)) { : /* Initialize Verified Boot data */ : chromeos_init_chromeos_acpi(&(gnvs->chromeos)); : if (CONFIG(EC_GOOGLE_CHROMEEC)) { : gnvs->chromeos.vbt2 = google_ec_running_ro() ? : ACTIVE_ECFW_RO : ACTIVE_ECFW_RW; : } else : gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO; : } This does not belong here.
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 25: Please use tabs.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 53: : #if 0 : Method(_PRW, 0) : { : : } : #endif ?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/espi.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 23: Device (FWH) : { : Name (_HID, EISAID ("INT0800")) : Name (_DDN, "Firmware Hub") : Name (_CRS, ResourceTemplate () : { : Memory32Fixed (ReadOnly, 0xff000000, 0x01000000) : }) : } Should this always be created? It might be a good idea to look at the boot straps before doing so?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 81: IO (Decode16, 0x2e, 0x2e, 0x1, 0x02) // First SuperIO : IO (Decode16, 0x4e, 0x4e, 0x1, 0x02) // Second SuperIO : IO (Decode16, 0x61, 0x61, 0x1, 0x01) // NMI Status : IO (Decode16, 0x63, 0x63, 0x1, 0x01) // CPU Reserved : IO (Decode16, 0x65, 0x65, 0x1, 0x01) // CPU Reserved : IO (Decode16, 0x67, 0x67, 0x1, 0x01) // CPU Reserved : IO (Decode16, 0x80, 0x80, 0x1, 0x01) // Port 80 Post : IO (Decode16, 0x92, 0x92, 0x1, 0x01) // CPU Reserved : IO (Decode16, 0xb2, 0xb2, 0x1, 0x02) // SWSMI : IO (Decode16, ACPI_BASE_ADDRESS, ACPI_BASE_ADDRESS, : 0x1, 0xff) Should this not depend on what is actually programmed in registers?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 95: Device (RTC) : { : Name (_HID, EISAID ("PNP0B00")) : Name (_DDN, "Real Time Clock") : Name (_CRS, ResourceTemplate () : { : IO (Decode16, 0x70, 0x70, 1, 8) : }) : } : : Device (TIMR) : { : Name (_HID, EISAID ("PNP0100")) : Name (_DDN, "8254 Timer") : Name (_CRS, ResourceTemplate () : { : IO (Decode16, 0x40, 0x40, 0x01, 0x04) : IO (Decode16, 0x50, 0x50, 0x10, 0x04) : IRQNoFlags () {0} : }) : } This seems very unspecific to this SOC. Move to a common place?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 30: GNVS Could you work on aligning those? Every SoC creating nearly identical GNVS is a maintenance burden and complicates handling of this code.
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.
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. wouldn't it be better to make that explicit here?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 22: : OperationRegion (APMP, SystemIO, 0xb2, 2) : Field (APMP, ByteAcc, NoLock, Preserve) : { : APMC, 8, // APM command : APMS, 8 // APM status : } Is this used? This is also very common on Intel SoC.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/sleepstates.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 17: : Name (_S0, Package () { 0x0, 0x0, 0x0, 0x0 }) : Name (_S3, Package () { 0x5, 0x5, 0x0, 0x0 }) : Name (_S5, Package () { 0x7, 0x7, 0x0, 0x0 }) This has been the same for all Intel targets since a long time. Please put it in a common location like southbridge/intel/common/acpi/sleepstates.asl.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... File src/soc/intel/tigerlake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... PS8, Line 30: : bootblock_systemagent_early_init(); : bootblock_pch_early_init(); : bootblock_cpu_init(); : pch_early_iorange_init(); : if (CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) : uart_bootblock_init(); This seems very generic. Please put it in a common location. I recommend you reuse cpu/intel/common/car/bootblock + callbacks.
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... File src/soc/intel/tigerlake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... PS8, Line 24: BOOT_DEVICE_SPI_FLASH This already select BOOT_DEVICE_MEMORY_MAPPED on x86.
-
Arthur Heymans (Code Review)