Arthur Heymans 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: Code-Review+2
(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.
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?
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?
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 generation. Fixed it in https://review.coreboot.org/c/coreboot/+/46503