Change in coreboot[master]: soc/intel/mtl: Do initial Meteor Lake SoC commit till bootblock
Attention is currently required from: Felix Singer, Jamie Ryu, Subrata Banik, Ethan Tsao, Ravishankar Sarawadi, Raj Astekar. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62772 )
Change subject: soc/intel/mtl: Do initial Meteor Lake SoC commit till bootblock ......................................................................
Patch Set 7:
(13 comments)
File src/soc/intel/meteorlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/62772/comment/1600e492_f58f77b5 PS7, Line 56: ifd2 Why not "mtl" instead? ifdtool seems to know about it.
File src/soc/intel/meteorlake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/62772/comment/b79253fb_df72a49b PS7, Line 85: */ nit: add space before comment end
File src/soc/intel/meteorlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/62772/comment/3031443f_128514a5 PS7, Line 27: MeteorLake Hmmm, the other entries spell "Meteorlake" with the 'l' in lowercase. Which form is preferred?
File src/soc/intel/meteorlake/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/62772/comment/e1309db8_82f4db03 PS7, Line 17: */ nit: Add space before comment end
https://review.coreboot.org/c/coreboot/+/62772/comment/ddc68bae_bba74706 PS7, Line 18: */ nit: Add space before comment end
https://review.coreboot.org/c/coreboot/+/62772/comment/c84c95d7_cf2fe071 PS7, Line 24: #define PCCTL 0xE0 /* PCI Clock Control */ Does this register actually exist? It only seems to exist on platforms that have LPC.
File src/soc/intel/meteorlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/62772/comment/94b80dfd_b6298d82 PS7, Line 51: 4KB nit: Technically, 4 KiB
https://review.coreboot.org/c/coreboot/+/62772/comment/8a36b854_becb3e82 PS7, Line 73: /* : #define PID_IOM 0xAA : #define IOM_BASE_ADDR (IOE_PCR_ABOVE_4G_BASE_ADDR + (PID_IOM << 16)) : */ Why is this commented out?
File src/soc/intel/meteorlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/62772/comment/1253b53b_f3786178 PS7, Line 11: pcidev_path_on_root_debug Could we please use `pcidev_path_on_root` instead?
https://review.coreboot.org/c/coreboot/+/62772/comment/80fb56c3_2f982a9e PS7, Line 59: #define PCI_DEV_SLOT_GNA 0x08 : #define PCI_DEVFN_GNA _PCI_DEVFN(GNA, 0) : #define PCI_DEV_GNA _PCI_DEV(GNA, 0) nit: Move above `PCI_DEV_SLOT_TCSS` to keep ordering?
https://review.coreboot.org/c/coreboot/+/62772/comment/81fc5dd3_4f792931 PS7, Line 159: #define PCI_DEV_SLOT_PCIE_2 0x6 : #define PCI_DEVFN_PCIE9 _PCI_DEVFN(PCIE_2, 0) : #define PCI_DEVFN_PCIE10 _PCI_DEVFN(PCIE_2, 1) : #define PCI_DEVFN_PCIE11 _PCI_DEVFN(PCIE_2, 2) : #define PCI_DEV_PCIE9 _PCI_DEV(PCIE_2, 0) : #define PCI_DEV_PCIE10 _PCI_DEV(PCIE_2, 1) : #define PCI_DEV_PCIE11 _PCI_DEV(PCIE_2, 2) : : #define PCI_DEV_SLOT_PCIE_3 0x1 : #define PCI_DEVFN_PCIE12 _PCI_DEVFN(PCIE_3, 0) : #define PCI_DEV_PCIE12 _PCI_DEV(PCIE_3, 0) I'd prefer to keep the macros ordered as per device/function numbers.
Looking at ADL, I imagine these PCIe RPs are located on the "north" part of the SoC, whereas the PCIe RPs of device 0x1c are located on the "south" part of the SoC.
File src/soc/intel/meteorlake/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/62772/comment/e88232d7_bca99dc2 PS7, Line 29: #define PID_IOM 0xAA : #define PID_TC_IOM 0xAA Is it intentional that these defines have the same value?
File src/soc/intel/meteorlake/include/soc/pm.h:
https://review.coreboot.org/c/coreboot/+/62772/comment/f2eb619b_42d588e0 PS7, Line 150: /* Get base address PMC memory mapped registers. */ : uint8_t *pmc_mmio_regs(void); : : /* Get base address of TCO I/O registers. */ : uint16_t smbus_tco_regs(void); : : /* Set the DISB after DRAM init */ : void pmc_set_disb(void); : : /* Clear PMCON status bits */ : void pmc_clear_pmcon_sts(void); : : /* STM Support */ : uint16_t get_pmbase(void); These are unused
-
Angel Pons (Code Review)