Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit till ramstage ......................................................................
Patch Set 15:
(53 comments)
https://review.coreboot.org/c/coreboot/+/36087/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36087/8//COMMIT_MSG@9 PS8, Line 9: Clone entirely from Icelake
[…]
Done
https://review.coreboot.org/c/coreboot/+/36087/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36087/15//COMMIT_MSG@22 PS15, Line 22:
Yes Furquan, this list is valid but its till yesterday, so what i will do after ICL https://review. […]
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... PS10, Line 73: FSP_USES_CB_STACK
Done
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... PS10, Line 215: ENABLE_DISPLAY_OVER_EXT_PCIE_GFX
oh yes, this has added to enable dGPU over PCIE usecase on icelake RVP which might be applicable for […]
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Ma... File src/soc/intel/tigerlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Ma... PS10, Line 1: feq ($(CONFIG_SOC_INTEL_TIGERLAKE),y)
one question. is that all means all stages like […]
Done
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.
Done
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?
needed for google_ec_running_ro
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 188: }
missing newline
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 203: gnvs->pcnt = dev_count_cpu();
useless code, there's no user of PCNT
will analysis with kernel team
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 207: gnvs->cbmc = (uintptr_t)cbmem_find(CBMEM_ID_CONSOLE);
useless code, there's no user of CBMC.
isn't it used by chromeos ?
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.
will evaluate with TGL code ?
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 226: gnvs->u2we = config->usb2_wake_enable_bitmap;
seems only be used on skylake
will check with kernel team ?
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. […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 25:
Please use tabs.
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 53: : #if 0 : Method(_PRW, 0) : { : : } : #endif
?
Done
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?
Done
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?
Done
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} : }) : }
CL to move into common […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 59: /* Set flag to enable USB charging in S3 */ : Method (S3UE) : { : Store (One, \S3U0) : } : : /* Set flag to disable USB charging in S3 */ : Method (S3UD) : { : Store (Zero, \S3U0) : } : : /* Set flag to enable USB charging in S5 */ : Method (S5UE) : { : Store (One, \S5U0) : } : : /* Set flag to disable USB charging in S5 */ : Method (S5UD) : { : Store (Zero, \S5U0) : }
not required, already deleted in ICL
Done
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)
reasonable to do this but should we target any particular soc for moving towards ASL 2. […]
Done
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 ? […]
Done
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 : }
Done
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. […]
Done
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();
some work started in the same like we called that common code 2. […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... File src/soc/intel/tigerlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... PS8, Line 61: static uint8_t get_dev_revision(pci_devfn_t dev)
CL: https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/chi... PS8, Line 72: switch (dev->path.pci.devfn) {
which datasheet(s) was/were consultated to create this array?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/cpu... File src/soc/intel/tigerlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/cpu... PS8, Line 72: msr = rdmsr(IA32_MISC_ENABLE);
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/cpu... PS8, Line 130: /* Set PM1 timer IO port and enable*/
Done https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/cpu... PS8, Line 239: smm_lock();
why is it done here?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/fin... File src/soc/intel/tigerlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/fin... PS8, Line 45: pcr_or32(PID_ISCLK, CAMERA1_CLK, CAM_CLK_EN | MIPI_CLK);
why is it done in finalize?
after FSP-S programming is done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/fin... PS8, Line 51: if (config->pch_isclk)
what is handled here?
this depends on CAMERA controller
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/fin... PS8, Line 73: * point and hence removed from the root bus. pcidev_path_on_root thus
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/fin... PS8, Line 85: if (config->s0ix_enable) {
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fi... File src/soc/intel/tigerlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fi... PS10, Line 118: : BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, soc_finalize, NULL); : BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_EXIT, soc_finalize, NULL);
this is good feedback to handle this in . […]
will address this later in TGL changes ?
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fs... PS10, Line 42: __weak
let me start this non weak practice from TGL so we do force ppl to get used to it. SG? […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/gra... File src/soc/intel/tigerlake/graphics.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/gra... PS8, Line 73: * GFX PEIM module inside FSP binary is taking care of graphics
isn;t below code does the same check ? […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/gra... PS8, Line 78: * In case of non-FSP solution, SoC need to select VGA_ROM_RUN
well. […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/gsp... File src/soc/intel/tigerlake/gspi.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/gsp... PS8, Line 24: return PCH_DEVFN_GSPI0;
which datasheet(s) was/were consultated to create this array?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/i2c... File src/soc/intel/tigerlake/i2c.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/i2c... PS8, Line 23: case PCH_DEVFN_I2C0:
which datasheet(s) was/were consultated to create this array?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/gpe.h:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/inc... PS8, Line 20: #define GPE0_DW0_00 0
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 19: #include <soc/gpio_defs.h> : #include <intelblocks/gpio.h> : : #define CROS_GPIO_DEVICE_NAME "INT3455:00"
i believe this file is kind of wrapper for other mainboard and common code files to get gpio related […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/inc... PS8, Line 47: #define GPP_G0_IRQ 0x18
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/usb.h:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/inc... PS8, Line 22: /* Per Port HS Transmitter Emphasis */
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/p2s... File src/soc/intel/tigerlake/p2sb.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/p2s... PS8, Line 29: * Set p2sb PCI offset EPMASK5 [29, 28, 27, 26] to disable Sideband
which datasheet(s) was/were consultated to create this?
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/ro... PS10, Line 26: __weak
true. […]
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/smi... File src/soc/intel/tigerlake/smihandler.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/smi... PS8, Line 82: pch_disable_heci();
which datasheet(s) was/were consultated? Why it it done in SMM?
due to postboot SAI implementation since CNP PCH
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... File src/soc/intel/tigerlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... PS12, Line 191: tseg_base
@Arthur, need your view here ?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/spi... File src/soc/intel/tigerlake/spi.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/spi... PS8, Line 23: case PCH_DEVFN_SPI:
which datasheet(s) was/were consultated to create this array?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/sys... File src/soc/intel/tigerlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/sys... PS8, Line 31: { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH,
which datasheet(s) was/were consultated to create this array?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/uar... File src/soc/intel/tigerlake/uart.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/uar... PS8, Line 31: PAD_CFG_NF(GPP_C8, NONE, DEEP, NF1), /* UART0 RX */
which datasheet(s) was/were consultated to create this array?
Done
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/uar... PS8, Line 61: return pcidev_path_on_root(PCH_DEVFN_UART0);
which datasheet(s) was/were consultated to create this array?
Done