Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
I've left some inline comments. But before we contiue discussing the implementation, IMHO, it's better to discuss some design decisions around it.
First of all, I don't think we should make the number of segment groups follow the SA_PCIEX_LENGTH Kconfig. Instead, I would prefer another Kconfig, to be set by the mainboard or soc code, that sets the number of segment groups. If that is > 1, then SA_PCIEX_LENGTH should be increased automatically. Let's call this PCI_SEGMENT_GROUPS.
We can then add additional segment groups to the DSDT, according to PCI_SEGMENT_GROUPS. If we should do that with an SSDT generator, or in static .asl files, at the soc level or at the mainboard level, depends a lot on the actual use case. Which I know next to nothing about.
Only when patches to add additional segment groups to the DSDT, along with their _SEG numbers have landed, this change should land. cf. PCI Firmware Specification "MCFG Table Description"
Most of design i agree with except that we need a dedicated Kconfig for knowing PCI segment PCI_SEGMENT_GROUPS because EDS never talks about how many segment present, its deduced based on PCI BUS number SA_PCIEX_LENGTH we have.
The EDS only describes one register and doesn't tell the whole story. PCIEXBAR is just a tool to access the PCI devices' config space, it doesn't decide on which segment/bus the devices are.
In my experience, things turn out much easier if you reflect reality in Kconfig. We need a bigger SA_PCIEX_LENGTH if there is a second segment group. Not the other way around.
Can't we use SA_PCIEX_LENGTH to calculate segment number dynamically (rather doing opposite?) as we are doing now and then pass those information from .c file .asl (northbridge.asl) to create additional capability for new segment (for _SEG 1).
I just fear we'd end up repeating the /256 calculation everywhere. But if that happens depends a lot on the actual implementation. You can just try.
The reason is knowing SA_PCIEX_LENGTH and PCI_SEGMENT_GROUPS are too much information we expect from soc/mb user to define SoC programming logic.
That's why I suggested to increase SA_PCIEX_LENGTH automatically based on other Kconfigs.
The BIOS Spec mentions some dependencies, so it should be decided per mainboard, AFAICS. Overriding SA_PCIEX_LENGTH at the mainboard level wouldn't look good. So I guess we need an additional Kconfig anyway.
Can you please review this design proposal made based on your feedback above
@Nico: Anything here https://review.coreboot.org/c/coreboot/+/40722