Attention is currently required from: Cliff Huang, Felix Held, Fred Reitberger, Jason Glenesk, Lance Zhao, Matt DeVillier, Patrick Rudolph, Tim Wawrzynczak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79877?usp=email )
Change subject: device: Add support for multiple PCI segments ......................................................................
Patch Set 1: Code-Review+1
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/79877/comment/12c8ec3d_df8b7a63 : PS1, Line 7: PCI segments I wouldn't call this segments.
Linux calls this "PCI domains", ACPI calls it "PCI segment groups". A segment is just the (sub)tree below any node. So this could be confused.
Patchset:
PS1: Finally somebody does it :)
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/79877/comment/e1420dea_04355813 : PS1, Line 157: i * CONFIG_ECAM_MMCONF_BUS_NUMBER Shouldn't this still be 0? In each segment group we start from 0, I guess.
File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/79877/comment/136de95f_c22cf55f : PS1, Line 89: uint8_t segment; /* PCI segment */ We also discussed lately if it wouldn't be easier to make it part of the bus number, i.e. `secondary` which is already 16 bits. That would spare us a lot of manual handling (and potential bugs if it's missed somewhere). It also fits the idea of a consecutive ECAM space. We could still split the numbers in console output if that's preferred.
WDYT?
File src/soc/amd/common/block/data_fabric/domain.c:
https://review.coreboot.org/c/coreboot/+/79877/comment/af419df6_36d72190 : PS1, Line 23: : /* TODO: Implement support for more than one PCI segment group in coreboot */ : if (segment_group) { : printk(BIOS_ERR, "coreboot currently only supports one PCI segment group.\n"); : return; : } Should this stay?
-
Nico Huber (Code Review)