Attention is currently required from: Tarun Tuli, Subrata Banik, Tim Wawrzynczak, Angel Pons, Nick Vaccaro. Eran Mitrani has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64677 )
Change subject: soc/intel/adl: Add missing claimed memory regions ......................................................................
Patch Set 13:
(22 comments)
File src/soc/intel/alderlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/64677/comment/0264cd69_0f35bf68 PS9, Line 16: #define EPBAR 0x40 : #define DMIBAR 0x68 : #define CAPID0_A 0xe4 : #define VTD_DISABLE (1 << 23) : : /* MCHBAR offsets */ : #define GFXVTBAR 0x5400 : #define EDRAMBAR 0x5408 : #define VTVC0BAR 0x5410 : #define REGBAR 0x5420 : #define MCH_DDR_POWER_LIMIT_LO 0x58e0 : #define MCH_DDR_POWER_LIMIT_HI 0x58e4 : #define MCH_PKG_POWER_LIMIT_LO 0x59a0 : #define MCH_PKG_POWER_LIMIT_HI 0x59a4 : #define BIOS_RESET_CPL 0x5da8 : #define IMRBASE 0x6a40 : #define IMRLIMIT 0x6a48 : #define IPUVTBAR 0x7880 : #define TBTxBAR(x) (0x7888 + (x) * 8) : : #define MAX_TBT_PCIE_PORT 4 : : #define VTBAR_ENABLED 0x01 : #define VTBAR_MASK 0x7ffffff000ull : : static const struct sa_mmio_descriptor soc_vtd_resources[] = { : { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR" }, : { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" }, : { TBTxBAR(0), TBT0_BASE_ADDRESS, TBT0_BASE_SIZE, "TBT0BAR" }, : { TBTxBAR(1), TBT1_BASE_ADDRESS, TBT1_BASE_SIZE, "TBT1BAR" }, : { TBTxBAR(2), TBT2_BASE_ADDRESS, TBT2_BASE_SIZE, "TBT2BAR" }, : { TBTxBAR(3), TBT3_BASE_ADDRESS, TBT3_BASE_SIZE, "TBT3BAR" }, : { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR" }, : }; : : #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
we use tabs for indentation in coreboot, please leave these as tabs
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/4fd6ab71_4d8596af PS9, Line 59: 0x01000000 // 16MB
`16 * MiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/3d22f32d_d31f5c9c PS9, Line 61: 0x80000 // 512KB
`512 * KiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/514a6305_581b85c0 PS9, Line 63: 0x10000 //64KB
`64 * KiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/d5b1ceed_f9149bd0 PS9, Line 66: 0x20000 // 128KB
`128 * KiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/301fc594_9d5236dd PS9, Line 67: #define APIC_BASE_ADDRESS 0xFEC00000
see `IO_APIC_ADDR` in src/arch/x86/include/arch/ioapic. […]
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/cb9754dd_d52dec2a PS9, Line 68: 0x00100000
etc.
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/2f5070c8_9122a769 PS9, Line 70: 0x00100000
etc.
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/6887d5dd_b1d630a8 PS9, Line 72: 0x00100000
etc.
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/e9863a8b_7b4f9722 PS9, Line 76: : #define GGC_REG 0x50
Already defined as `GGC` in src/soc/intel/common/block/systemagent/systemagent_def. […]
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/5d58fb96_4ef5d2c0 PS9, Line 58: #define MMSPI_FLASH_BASE_ADDRESS 0xFF000000 : #define MMSPI_FLASH_SIZE 0x01000000 // 16MB : #define CRAB_ABORT_BASE_ADDRESS 0xEFB00000 : #define CRAB_ABORT_SIZE 0x80000 // 512KB : #define TPM_BASE_ADDRESS 0xFED40000 : #define TPM_SIZE 0x10000 //64KB : : #define LT_SECURITY_BASE_ADDRESS 0xFED50000 : #define LT_SECURITY_SIZE 0x20000 // 128KB : #define APIC_BASE_ADDRESS 0xFEC00000 : #define APIC_SIZE 0x00100000 // 1MB : #define LEGACY_MEM_BASE_ADDRESS 0x00000000 : #define LEGACY_MEM_SIZE 0x00100000 // 1MB : #define MSI_INTERRUPTS_BASE_ADDRESS 0xFEE00000 : #define MSI_INTERRUPTS_SIZE 0x00100000 // 1MB : : #define MASK_PCIEXBAR_LENGTH 0x0000000E // bits [3:1] : #define PCIEXBAR_LENGTH_LSB 1 // used to shift right : : #define GGC_REG 0x50 : #define DSM_BASE_ADDR_REG 0xB0 : #define MASK_DSM_LENGTH 0xFF00 // [15:8] : #define MASK_DSM_LENGTH_LSB 8 // used to shift right : #define MASK_GSM_LENGTH 0xC0 // [7:6] : #define MASK_GSM_LENGTH_LSB 6 // used to shift right : #define DPR_REG 0x5C : #define MASK_DPR_LENGTH 0xFF0 // [11:4] : #define MASK_DPR_LENGTH_LSB 4 // used to shift right
tabs for indentation please
Done
File src/soc/intel/alderlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/64677/comment/45a7d168_d123131e PS9, Line 39: // first field (sa_mmio_descriptor.index) is not used, setting to 0:
nit: this file is already using `/* */` style comments, would be good to keep consistency
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/49e580a0_1cb9d9d3 PS9, Line 197: 0x100000000
`4 * GiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/efa5b544_a67465b3 PS9, Line 200: 0x80000000
`2 * GiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/edad405f_340a7820 PS9, Line 203: 0x40000000
etc.
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/bc2df6d7_b3a85234 PS9, Line 234: 0x2000000
`32 * MiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/75cab8df_2a5947f1 PS9, Line 236: 0x400000
`4 * MiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/409dd66e_6ca68668 PS9, Line 240: 0x40000000
`1 * GiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/143e88ae_981a36dc PS9, Line 243: 0x60000000
`1536 * MiB`
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/fa03bfac_03541b88 PS9, Line 278: return size;
although the masks may prevent it now, it may be safest to ensure size is always initialized incase […]
Done
https://review.coreboot.org/c/coreboot/+/64677/comment/399225c7_726c40be PS9, Line 294: resource->base = base;
check/handle for resource being null
Done
File src/soc/intel/common/block/systemagent/systemagent_def.h:
https://review.coreboot.org/c/coreboot/+/64677/comment/a672a5f8_fda7d86a PS9, Line 25: #define PCIEXBAR_LENGTH_4096MB 6 : #define PCIEXBAR_LENGTH_2048MB 5 : #define PCIEXBAR_LENGTH_1024MB 4 : #define PCIEXBAR_LENGTH_512MB 3
use tabs to indent/align the right side
Done
-
Eran Mitrani (Code Review)