Jonathan Zhang 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 51:
(36 comments)
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.
Done
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
Done
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
Done
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()
Done
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
This can not be replaced with read16()
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()
Done
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()
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi/pci_irq.asl:
PS50:
please indent with tabs
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi/uncore.asl:
PS50:
please indent uniformly with tabs
Done
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,
I just realized we have CONFIG_ROM_SIZE for that. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... File src/soc/intel/xeon_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 165: npr == 0
npr == NULL
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 166: assign_resource_to_stack
add_res_to_stack()
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 197: bool first = 1
int first? Or if you prefer to keep it as a bool, use true/false instead of 1/0.
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 284: continue;
Join lines 281-283 so that the parens terminate after the condition. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 317: 0
NULL
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 516: 0
NULL
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 518: struct bus
nit: use the object's size in case the type ever changes, i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/chip... PS50, Line 519: struct bus
ditto
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/cpu.... File src/soc/intel/xeon_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/cpu.... PS50, Line 92: *
nit: indentation
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/cpu.... PS50, Line 188:
nit: extra line
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/incl... PS50, Line 39: ///
same here
Done
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
Done
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. […]
Done
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. […]
Done
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
Done
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. […]
Done
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
Done
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. […]
Done
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. […]
Done
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_ADDRE […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/soc_... PS50, Line 31: #include <soc/itss.h>
neither this
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38548/50/src/soc/intel/xeon_sp/unco... PS50, Line 258: (0xc0000 - 0xa0000) >> 10;
VGA_BASE_SIZE
Done