Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel Xeon Scalable Processor support ......................................................................
Patch Set 47:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38548/47/src/soc/intel/xeon_sp/unco... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/38548/47/src/soc/intel/xeon_sp/unco... PS47, Line 208: /* Mark FSP region */ : base_kb = (range_entry_base(&fsp_mem) >> 10); : size_kb = (range_entry_end(&fsp_mem) - range_entry_base(&fsp_mem) + 1) >> 10; : LOG_MEM_RESOURCE("mmio_fsp", dev, index, base_kb, size_kb); : mmio_resource(dev, index++, base_kb, size_kb); : you don't need to explicitly mark FSP's region. This is done automagically in fsp_memory_init() as part of cbmem initialization()
https://review.coreboot.org/c/coreboot/+/38548/47/src/soc/intel/xeon_sp/unco... PS47, Line 214: /* Mark coreboot region as reserved */ : base_kb = (range_entry_base(&fsp_mem) + (range_entry_end(&fsp_mem) - : range_entry_base(&fsp_mem) + 1)) >> 10; : size_kb = (mc_values[TSEG_BASE_REG] - (base_kb << 10)) >> 10; : LOG_MEM_RESOURCE("mmio_coreboot", dev, index, base_kb, size_kb); : mmio_resource(dev, index++, base_kb, size_kb); I do not understand what is the "coreboot region". Could you please explain what is being added as MMIO resource?
https://review.coreboot.org/c/coreboot/+/38548/47/src/soc/intel/xeon_sp/unco... PS47, Line 225: mmio_resource(dev, index++, base_kb, size_kb); I think that we mark SMM as reserved resource, not MMIO.
https://review.coreboot.org/c/coreboot/+/38548/47/src/soc/intel/xeon_sp/unco... PS47, Line 232: mmio_resource(dev, index++, base_kb, size_kb); I believe this is not MMIO as well