Support for PCIe on Annapurna Alpine

[mst_semihalf.com]

Summary

Add driver for PCIe root complex on Annapurna Alpine platform.

The driver subclasses pci-host-generic and additionally performs configuration of vendor-specific PCIe registers.
The Alpine MSI-X driver (review here: D7579) is separate from the PCIe RC driver.

[nwhitehorn]

sys/arm/annapurna/alpine/alpine_pci.c

110

Why not inherit ofwpci.c instead? It's slightly more complete than generic_pci and otherwise the same.

257

Why not just encode this during interrupt allocation time? This already fabricates an OF interrupt specifier, which can be passed to ofw_bus_map_intr() in alloc_msi()/alloc_msix() after D7493 is merged. Then you don't need this whole function. The MSI allocation process becomes slightly more robust in this case, to boot (the next comment becomes obsolete, for instance).

260

Is this true for devices with MSIs but no LSIs? What about child devices with multiple non-MSI interrupts?

sys/arm/annapurna/alpine/alpine_pci.h

33 ↗

(On Diff #19468)

Are these functions defined anywhere?

[nwhitehorn]

sys/boot/fdt/dts/arm/annapurna-alpine.dts

178

This information should be obtained from the OFW_PCI_PHYS_HI_SPACE_CONFIG parts of "ranges" rather than reg. (In general, we shouldn't be modifying DTS). If you use ofwpci, this is accessible in the sc_ranges member of the parent softc.

[andrew]

sys/arm/annapurna/alpine/alpine_pci.c

70

Why add non-INTRNG support? I's not needed for arm nor arm64.

[andrew]

sys/arm/annapurna/alpine/alpine_pci.c

110

It will make it more difficult to reuse this driver with an ACPI attachment. I would like it if the ofwpci drive was reworked to be a subclass of the generic_pci driver as this would simplify this case on arm64.

[mst_semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.h

33 ↗

(On Diff #19468)

I will remove this and other non-INTRNG parts in the next revision of the patch.

[mst_semihalf.com]

Remove all no-INTRNG parts of PCI driver.

[mst_semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

70

Removed.

[nwhitehorn]

sys/arm/annapurna/alpine/alpine_pci.c

110

That would be hard, since there are many PCI bridge drivers that inherit from ofwpci but that look nothing like pci_generic. So I don't think it's possible to do it that way.

As far as I understand, and please correct me, pci_generic implements a PCI host bridge driver that:

1. Uses the standard OF properties for memory and IO ranges and interrupts (+MSIs, if that MSI spec gets adopted).
2. Has a probe() method that selects a few notable devices.
3. Has its config space mapped 1:1 as the first memory register bank of the device.

ofwpci just implements (1) and doesn't try to do (2) or (3), leaving that up to subclasses as the mechanisms for (2) and (3) aren't specified by the standard (i.e. it doesn't do anything by itself). If you made the existing code replace its (1) by ofwpci, it would both lose no functionality (assuming we made ofwpci handle the MSI bits, which is a good idea) and become more bus-agnostic, so you could have both ACPI- and OFW- bus attachments without too much trouble. It's worth noting here as well that the Annapurna device is exactly one of the cases I mentioned at the beginning. It doesn't actually work with (2) or (3) from generic_pci: it has its own probe() method and has to modify the DTS for (3) to work.

It would probably help for my understanding to see what you had in mind for ACPI. Are you planning on doing something beyond what's in dev/acpica/acpi_pcib_acpi.c? I didn't see a lot of obviously x86-specific code there at first blush.

[mst_semihalf.com]

Use device entries instead of SOC option.

[mst_semihalf.com]

Remove implementation of bus_extend_resource() which is no longer in tree. Alpine MSI-X interrupts work fine without it after D7493 and a small change in GICv3.

[mst_semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

257

Thanks, I removed bus_extend_resource implementation and reworked the MSIX driver to suit D7493. It is much simpler now and less hackish.

[nwhitehorn]

Looks good. I had two remaining comments, neither of which should block merging:

1. I think this would be better to inherit from ofwpci than pci_generic, since the code just works around all the things pci_generic adds to ofwpci.
2. The DTS bindings being used don't match the vendor ones. Barring some compelling reason to diverge, I think we should use the vendor's device trees.

[mst_semihalf.com]

Move al_pci device entry from sys/conf/files to files.arm and files.arm64. Add fdt dependency.

[wma]

In general, pci_generic contains one feature which ofw_pci lacks: it's able to enumerate devices which are present in DTS under the PCI-controller node, which are not PCI devices. I know this is a mess and does not comply with standard nor logic, but some arm64-vendors are using this type of DTSes and by now it is hard to convince them to fix DTS once it's already been put into production.

I'd leave Alpine PCI controller being a child of generic_pci like all others arm64 systems until we come up with a solution that can flawlessly merge both arm64 and x86 worlds not loosing any functionality.

sys/boot/fdt/dts/arm/annapurna-alpine.dts

178

MST, is this fixed?

[nwhitehorn]

In D7571#161529, @wma wrote:

In general, pci_generic contains one feature which ofw_pci lacks: it's able to enumerate devices which are present in DTS under the PCI-controller node, which are not PCI devices. I know this is a mess and does not comply with standard nor logic, but some arm64-vendors are using this type of DTSes and by now it is hard to convince them to fix DTS once it's already been put into production.

That is quite strange, but is supported by ofwpci as well, at least if used in conjunction with ofw_pcibus.c. However, I now see that ofw_pcibus.c and ofw_pcib_pci.c were never moved from /sys/powerpc/ofw like ofwpci.c was. Let's leave this as pcie_generic for the time being and then move over the remainder of the PowerPC PCI support code, which is all actually machine-independent.

[mmel]

it's able to enumerate devices which are present in DTS under the PCI-controller node, which are not PCI devices

I don't think that this is right. The nodes under PCI-controller node describes configuration for individual ports of PCIe root complex, these are part of PCIe-controller configuration.
These are not a independent devices at all so instantiation new newbus devices for them looks like pure nonsense.

[nwhitehorn]

In D7571#161544, @meloun-miracle-cz wrote:

it's able to enumerate devices which are present in DTS under the PCI-controller node, which are not PCI devices

I don't think that this is right. The nodes under PCI-controller node describes configuration for individual ports of PCIe root complex, these are part of PCIe-controller configuration.
These are not a independent devices at all so instantiation new newbus devices for them looks like pure nonsense.

Per the spec, device tree children are supposed to be PCI devices. I assume these are (potentially fictional) PCI<->PCI bridges for the root ports? Those are what I've always seen, anyway.

[mmel]

Yes and no :) .
These nodes doesn't have 'compatible' property, so doesn't have driver. It's job of PCI(e) driver to read and use them (assign lanes to ports, allocate memory for memory mapped configuration space ...)

Please see one (documented) example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pci/nvidia%2Ctegra20-pcie.txt#L113
and
https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/tegra124.dtsi#L53

[mst_semihalf.com]

sys/boot/fdt/dts/arm/annapurna-alpine.dts

178

It seems that the 'reg' property was erased by mistake before. It's there in the DTS in Linux, so I think we should keep it in.