[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block/acpi: Add provision for multiple PCI segments

[Nico Huber (Code Review)]

Nico Huber 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:

(2 comments)

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"

https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... 
PS5, Line 43: seg_nr = pciex_bus / 256;
> Sorry, i didn't get your point here

_SEG is an object in the DSDT below the PCI root. Currently, we have
something like this in coreboot:

Scope (_SB) {
            Device (PCI0) {
                    Name (_SEG, 0)
                    ...
            }
    }

This means _SB.PCI0 forms segment group number 0.

What you need for a second segment group on TGL, is a second root with
_SEG 1, for instance:

Scope (_SB) {
            Device (PCI0) {
                    Name (_SEG, 0)
                    ...
            }
            Device (PCI1) { // or TBT0?
                    Name (_SEG, 1)
                    ...
            }
    }

https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... 
PS5, Line 46: acpi_create_mcfg_mmconfig((void *)current,
            : 			CONFIG_MMCONF_BASE_ADDRESS, seg_nr, 0, bus_end_nr);
> As per ACPI MCFG table i believe we are good as i don't see any ACPI entry for multi segment

acpi_create_mcfg_mmconfig() is supposed to be called once for each segment
group.

The `seg_nr` parameter of acpi_create_mcfg_mmconfig() means the id (or
number) of the given segment group, _not_ the overall count (number) of
segment groups.