Attention is currently required from: Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Jonathan Zhang, Nico Huber, Patrick Rudolph, Paul Menzel, Shuo Liu, TangYiwei, Tim Chu.

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

View Change

8 comments:

• File src/soc/intel/xeon_sp/Makefile.mk:

  • Patch Set #56, Line 8: ## GNR IBL codes are initially reused from EBG, will update later.

    nit: Add a "TODO: " prefix?

• File src/soc/intel/xeon_sp/chip_gen6.c:

  • Patch Set #56, Line 35: static void iio_pci_domain_read_resources(struct device *dev)

    I'd recommend using the constructors in https://github.com/coreboot/coreboot/blob/90e835db2d2ef75ea7d0999090d1806cf7f85a4a/src/include/device/device.h instead of manually specifying `IORESOURCE_` flags.

    Also, shouldn't all these resources be `IORESOURCE_FIXED` and `IORESOURCE_STORED`? https://github.com/coreboot/coreboot/blob/90e835db2d2ef75ea7d0999090d1806cf7f85a4a/src/include/device/resource.h#L33

  • Patch Set #56, Line 56: res->flags = IORESOURCE_IO | IORESOURCE_SUBTRACTIVE | IORESOURCE_ASSIGNED;

    I asked on IRC, this resource should be `IORESOURCE_FIXED` so that the allocator doesn't try to allocate it. And `IORESOURCE_SUBTRACTIVE` shouldn't matter (the allocator doesn't allocate fixed resources).

• File src/soc/intel/xeon_sp/gnr/Kconfig:

  • Patch Set #56, Line 17: select FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND

    I don't remember if I mentioned this, but it would be great to fix FSP

  • Patch Set #56, Line 34: default 255

    Given that previous gen platforms (e.g. ibm/sbp1) have more cores/threads than that, I imagine this is a placeholder. Probably embargo-related.

• File src/soc/intel/xeon_sp/gnr/romstage.c:

  • Patch Set #56, Line 21: switch (base_addr) {

    OK, these are a bit tricky to handle because decimals, but how about:

    ```
    static uint8_t get_mmcfg_base_upd_index(const uint64_t base_addr)
    {
    	switch (base_addr) {
    	case 1UL * GiB:			// 1G
    		return 0;
    	case 1UL * GiB + 512UL * MiB:	// 1.5G
    		return 0x1;
    	case 1UL * GiB + 768UL * MiB:	// 1.75G
    		return 0x2;
    	case 2UL * GiB:			// 2G
    		return 0x3;
    	case 2UL * GiB + 256UL * MiB:	// 2.25G
    		return 0x4;
    	case 3UL * GiB:			// 3G
    		return 0x5;
    	default:			// Auto
    		return 0x6;
    	}
    }
    ```

    I haven't tried, but this should compile fine. I know the comment alignment is wrong, Gerrit uses 4 spaces for tabs.

• File src/soc/intel/xeon_sp/spr/cpu.c:

  • Patch Set #56:

    nit: Maybe move these changes to a separate commit?

• File src/soc/intel/xeon_sp/spr/soc_acpi.c:

  • Patch Set #56, Line 158: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current)

    Where did this go?

To view, visit change 81316. To unsubscribe, or for help writing mail filters, visit settings.