Change in coreboot[master]: soc/intel/denverton_ns: enable Denverton to use common systemagent code
Attention is currently required from: Jeff Daly, Mariusz Szafrański, Suresh Bellampalli, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61015 )
Change subject: soc/intel/denverton_ns: enable Denverton to use common systemagent code ......................................................................
Patch Set 10: Code-Review+1
(11 comments)
File src/soc/intel/denverton_ns/chip.h:
https://review.coreboot.org/c/coreboot/+/61015/comment/2fe64739_6a53b7c5 PS10, Line 67: uint32_t Why `uint32_t`? `bool` should suffice
File src/soc/intel/denverton_ns/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/61015/comment/0b8a5fbc_92228737 PS10, Line 15: /* no DMI for DNV */ : #define DMI_BASE_ADDRESS 0 : #define DMI_BASE_SIZE 0 : : /* no EP windows for DNV */ : #define EP_BASE_ADDRESS 0 : #define EP_BASE_SIZE 0 : : /* no EDRAM for DNV */ : #define EDRAM_BASE_ADDRESS 0 : #define EDDRAM_BASE_SIZE 0 : : /* no GDXC for DNV */ : #define GDXC_BASE_ADDRESS 0 : #define GDXC_BASE_SIZE 0 : : /* no iGFX for DNV */ : #define GFXVT_BASE_ADDRESS 0 : #define GFXVT_BASE_SIZE 0 Does common code need these defines? If it doesn't, I would drop them.
https://review.coreboot.org/c/coreboot/+/61015/comment/2c8abb9e_48bfcff6 PS10, Line 53: #define MCH_BASE_ADDRESS 0xfed10000 : #define MCH_BASE_SIZE 0x8000 Same as lines 12-13
https://review.coreboot.org/c/coreboot/+/61015/comment/85835471_e2693850 PS10, Line 56: #define HPET_BASE_ADDRESS 0xfed00000 This is now defined in a central location, src/arch/x86/include/arch/hpet.h
https://review.coreboot.org/c/coreboot/+/61015/comment/c9bb586c_54f1754a PS10, Line 67: #define DEFAULT_HPET_ADDR CONFIG_HPET_ADDRESS The Kconfig was removed, I'd drop this.
File src/soc/intel/denverton_ns/include/soc/nvs.h:
PS10: Most of the NVS values only make sense on "client" platforms. Also, you would need to update the definition in the .asl file accordingly
File src/soc/intel/denverton_ns/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/61015/comment/935dc307_def975f2 PS10, Line 25: #if 0 : #define TOUUD_LO 0xa8 /* Top of Upper Usable DRAM - Low */ : #define MASK_TOUUD_LO 0xFFF00000 : #define TOUUD_HI 0xac /* Top of Upper Usable DRAM - High */ : #define MASK_TOUUD_HI 0x0000007F : #define TOUUD TOUUD_LO /* Top of Upper Usable DRAM */ : #define MASK_TOUUD 0x7FFFF00000 : #endif Remove?
https://review.coreboot.org/c/coreboot/+/61015/comment/7ef606bd_7966b3cb PS10, Line 55: /* DNV has different offsets than other SoCs */ : #ifdef MCH_PKG_POWER_LIMIT_LO : #undef MCH_PKG_POWER_LIMIT_LO : #define MCH_PKG_POWER_LIMIT_LO 0x70a8 : #undef MCH_PKG_POWER_LIMIT_HI : #define MCH_PKG_POWER_LIMIT_HI 0x70ac : #undef MCH_DDR_POWER_LIMIT_LO : #define MCH_DDR_POWER_LIMIT_LO 0x7040 : #undef MCH_DDR_POWER_LIMIT_HI : #define MCH_DDR_POWER_LIMIT_HI 0x7048 : #endif These defines are only used in `src/soc/intel/common/block/power_limit/power_limit.c`. The code is guarded by `SOC_INTEL_COMMON_BLOCK_POWER_LIMIT`.
https://review.coreboot.org/c/coreboot/+/61015/comment/8e488b22_48208cf7 PS10, Line 69: static const struct sa_mmio_descriptor soc_vtd_mmio_descriptor = { : VTDBAR, : VTD_BASE_ADDRESS, : VTD_BASE_SIZE, : "VTDBAR" : }; This declaration should be inside a C file (e.g. denverton_ns/systemagent.c), not a header.
File src/soc/intel/denverton_ns/systemagent.c:
https://review.coreboot.org/c/coreboot/+/61015/comment/f3164259_1b7a2619 PS10, Line 64: /* Enable BIOS Reset CPl */ This is quite redundant, no?
https://review.coreboot.org/c/coreboot/+/61015/comment/1ea16b22_7d71d6e1 PS10, Line 68: BGSM Weird that DNV-NS has BGSM but doesn't have an iGPU.
-
Angel Pons (Code Review)