Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45850 )
Change subject: soc/intel/xeon_sp: Move common northbridge ACPI to nb_acpi.c ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45850/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45850/13//COMMIT_MSG@12 PS13, Line 12: #if (CONFIG(SOC_INTEL_*_SP)).
I think callbacks with in code linked in cpx/skx/... would be nicer. LGTM, this can be done later.
Agree, I have those in the works.
https://review.coreboot.org/c/coreboot/+/45850/13/src/soc/intel/xeon_sp/nb_a... File src/soc/intel/xeon_sp/nb_acpi.c:
https://review.coreboot.org/c/coreboot/+/45850/13/src/soc/intel/xeon_sp/nb_a... PS13, Line 385: CBMEM_ID_STORAGE_DATA
This is used for sdhci controller information?? Probably should get rid of it here?
OK.
https://review.coreboot.org/c/coreboot/+/45850/13/src/soc/intel/xeon_sp/nb_a... PS13, Line 448: stack <= PSTACK2
Does this work on skx?
It was the same code on skx, but I think I have seen the conflicts you mention below.
https://review.coreboot.org/c/coreboot/+/45850/13/src/soc/intel/xeon_sp/nb_a... PS13, Line 442: for (int iio = 1; iio <= hob->PlatformData.numofIIO; ++iio) { : int socket = iio; : if (socket == hob->PlatformData.numofIIO) // socket 0 should be last DRHD entry : socket = 0; : : if (socket == 0) { : for (int stack = 1; stack <= PSTACK2; ++stack) : current = acpi_create_drhd(current, socket, stack, hob); : current = acpi_create_drhd(current, socket, CSTACK, hob); : } else { : for (int stack = 0; stack <= PSTACK2; ++stack) : current = acpi_create_drhd(current, socket, stack, hob); : } : }
Sidenote: This way of looping over stacks to create IO APIC DMAR entries conflicts with the MADT gen […]
Yes, I'm tracking that change, too. There is a patch to de-duplicate the MADT.