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 50:
(24 comments)
Great job, we are definitely getting there. I think we are almost there. I wish we had the patch split into smaller chunks. This way we would need less rounds.
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/Kcon... File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/Kcon... PS50, Line 59: select SOC_INTEL_COMMON_BLOCK_P2SB we are not using this. We could use it, if PCH id are added into block/p2sb/p2sb.c.
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/Kcon... PS50, Line 60: SOC_INTEL_COMMON_BLOCK_ITSS I do not see the code use ITSS
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... PS50, Line 516: acpi_create_serialio_ssdt(ssdt2); : if (ssdt2->length) { : current += ssdt2->length; : acpi_add_table(rsdp, ssdt2); : printk(BIOS_DEBUG, "ACPI: * SSDT2 @ %p Length %x\n", ssdt2, : ssdt2->length); : current = (ALIGN(current, 16)); : } else { : ssdt2 = NULL; : printk(BIOS_DEBUG, "ACPI: * SSDT2 not generated.\n"); : } this code does nothing, doesn't it? so lets drop it
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... PS50, Line 720: uint16_t hpet_capid = *(volatile u16 *)(unsigned int)(HPET_BASE_ADDRESS); replace with read16()
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... PS50, Line 721: uint16_t num_hpets = (hpet_capid >> 0x08) & 0x1F; // Bits [8:12] has hpet count same
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... PS50, Line 726: *((volatile uint32_t *)(uint32_t)(HPET_BASE_ADDRESS + 0x100)) read32()
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... PS50, Line 760: uint64_t vtd_mmio_cap = *(volatile uint64_t *)(unsigned int)(vtd_base + read64()
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... PS50, Line 1041: __weak void acpi_create_serialio_ssdt(acpi_header_t *ssdt) {} not needed
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/boot... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/boot... PS41, Line 33: .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_CBFS_SIZE), : .CodeRegionLength = (UINT32)CONFIG_CBFS_SIZE,
We are going to figure out why this preferred method degrades boot stability. […]
I just realized we have CONFIG_ROM_SIZE for that. Since there is regression we may want to use this instead:
.CodeRegionBase = 0xffffffff - CONFIG_ROM_SIZE + 1; .CodeRegionLength = CONFIG_ROM_SIZE;
this may not be the perfect way though, because we cover whole flash. And I believe only BIOS region is memory mapped. This is just a suggestion in case we may have hard time fixing the hang issue that was mentioned.
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 22: /// Use below for functions from PCH GPIO Lib which : /// require GpioGroup as argument what PCH GPIO Lib? please remove this
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 39: /// same here
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 40: #define UART_BASE_SIZE 0x1000 : #define UART_BASE_0_ADDRESS 0xfe030000 : /* Both UART BAR 0 and 1 are 4KB in size */ : #define UART_BASE_0_ADDR(x) (UART_BASE_0_ADDRESS + (2 * UART_BASE_SIZE * (x))) : #define UART_BASE(x) UART_BASE_0_ADDR(x) there is no memory-mapped UART on this platform
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/itss.h:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 20: #define GPIO_IRQ_START 50 : #define GPIO_IRQ_END ITSS_MAX_IRQ : : #define ITSS_MAX_IRQ 119 : #define IRQS_PER_IPC 32 : #define NUM_IPC_REGS ((ITSS_MAX_IRQ + IRQS_PER_IPC - 1)/IRQS_PER_IPC) : : #define PCR_ITSS_PIR00 0x3140 : #define PCR_ITSS_PIR01 0x3142 : #define PCR_ITSS_PIR02 0x3144 : #define PCR_ITSS_PIR03 0x3146 : : #define PCH_PCR_ITSS_IPC0 0x3200 ///< Interrupt Polarity Control 0 : #define PCH_PCR_ITSS_IPC1 0x3204 ///< Interrupt Polarity Control 1 : #define PCH_PCR_ITSS_IPC2 0x3208 ///< Interrupt Polarity Control 2 : #define PCH_PCR_ITSS_IPC3 0x320C we are not using these values anywhere. Since we are not using that, lets drop the whole file?
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 33: #define PID_SERIALIO 0xCB I don't see serialio in C620 datasheet. What data sheet this is from? (I couldn't find c621 datasheet only c620)
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 36: define PCH_PCR_ADDRESS(Pid, Offset) \ : (P2SB_BAR | ((uint8_t)(Pid) << 16) | (uint16_t)(Offset)) this macro is not used and should be dropped
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/soc_config.h:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 20: #define IIO_UPLINK_PORT_INDEX 5 : #define MAX_TAD_RULES 20 : #define MAX_TAD_WAYS 3 : #define MAX_DPC 2 : #define MAX_SPD_BYTES 512 these are not used anywhere in the patch. Lets just drop the whole file
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/lpc.... File src/soc/intel/xeon_sp/lpc.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/lpc.... PS50, Line 57: 0xfd00 why this index? doesn't look like it has to be this value
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/lpc.... PS50, Line 62: res = new_resource(dev, IOINDEX_SUBTRACTIVE(io_index++, 0)); : res->base = FIRMWARE_BASE_ADDRESS; : res->size = FIRMWARE_BASE_SIZE; /* 16 MB for flash */ lets avoid hardcoding flash size. chip code should not depend on board-specific bits. also: why are we adding BIOS region as a resource?
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/lpc.... PS50, Line 67: LOG_MEM_RESOURCE("[MEM] subtractive_res", dev, io_index-1, I think it may be good idea to refactor LOG_MEM_RESOURCE macro to take dev and res. This way you can drop this "io_index-1"
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/lpc.... PS50, Line 71: res = new_resource(dev, 0xda); : res->base = P2SB_BAR; : res->size = P2SB_SIZE; : res->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED | : IORESOURCE_ASSIGNED; : LOG_MEM_RESOURCE("P2SB PCR config space BAR", dev, 0xda, : (res->base >> 10), (res->size >> 10)); so this region [P2SB_BAR;P2SB_BAR+P2SB_SIZE] already covers region [PCH_BASE_ADDRESS; PCH_BASE_ADDRESS+PCH_BASE_SIZE]. So the latter one is redundant and should not be removed. However, we already have common code for that. Please add p2sb pci device ids into block/p2sb/p2sb.c and we can drop this code as well.
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/soc_... File src/soc/intel/xeon_sp/soc_util.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/soc_... PS50, Line 27: #include <intelblocks/itss.h> we are not using this
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/soc_... PS50, Line 31: #include <soc/itss.h> neither this
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/unco... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/unco... PS50, Line 257: 0xa0000 VGA_BASE_ADDRESS
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/unco... PS50, Line 258: (0xc0000 - 0xa0000) >> 10; VGA_BASE_SIZE