Re: [PATCH v5 08/12] PCI: imx6: Config look up table(LUT) to support MSI ITS and IOMMU for i.MX95

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-05-31 12:08 am, Bjorn Helgaas wrote:
[+cc IOMMU and pcie-apple.c folks for comment]

On Tue, May 28, 2024 at 03:39:21PM -0400, Frank Li wrote:
For the i.MX95, configuration of a LUT is necessary to convert Bus Device
Function (BDF) to stream IDs, which are utilized by both IOMMU and ITS.
This involves examining the msi-map and smmu-map to ensure consistent
mapping of PCI BDF to the same stream IDs. Subsequently, LUT-related
registers are configured. In the absence of an msi-map, the built-in MSI
controller is utilized as a fallback.

Additionally, register a PCI bus notifier to trigger imx_pcie_add_device()
upon the appearance of a new PCI device and when the bus is an iMX6 PCI
controller. This function configures the correct LUT based on Device Tree
Settings (DTS).

This scheme is pretty similar to apple_pcie_bus_notifier().  If we
have to do this, I wish it were *more* similar, i.e., copy the
function names, bitmap tracking, code structure, etc.

I don't really know how stream IDs work, but I assume they are used on
most or all arm64 platforms, so I'm a little surprised that of all the
PCI host drivers used on arm64, only pcie-apple.c and pci-imx6.c need
this notifier.

This is one of those things that's mostly at the mercy of the PCIe root complex implementation. Typically the SMMU StreamID and/or GIC ITS DeviceID is derived directly from the PCI RID, sometimes with additional high-order bits hard-wired to disambiguate PCI segments. I believe this RID-translation LUT is a particular feature of the the Synopsys IP - I know there's also one on the NXP Layerscape platforms, but on those it's programmed by the bootloader, which also generates the appropriate "msi-map" and "iommu-map" properties to match. Ideally that's what i.MX should do as well, but hey.

There's this path, which is pretty generic and does at least the
of_map_id() part of what you're doing in imx_pcie_add_device():

     __driver_probe_device
       really_probe
         pci_dma_configure                       # pci_bus_type.dma_configure
           of_dma_configure
             of_dma_configure_id
               of_iommu_configure
                 of_pci_iommu_init
                   of_iommu_configure_dev_id
                     of_map_id
                     of_iommu_xlate
                       ops = iommu_ops_from_fwnode
                       iommu_fwspec_init
                       ops->of_xlate(dev, iommu_spec)

Maybe this needs to be extended somehow with a hook to do the
device-specific work like updating the LUT?  Just speculating here,
the IOMMU folks will know how this is expected to work.

Note that that particular code path has fundamental issues and much of it needs to go away (I'm working on it, but it's a rich ~8-year-old pile of technical debt...). IOMMU configuration needs to be happening at device_add() time via the IOMMU layer's own bus notifier.

If it's really necessary to do this programming from Linux, then there's still no point in it being dynamic - the mappings cannot ever change, since the rest of the kernel believes that what the DT said at boot time was already a property of the hardware. It would be a lot more logical, and likely simpler, for the driver to just read the relevant map property and program the entire LUT to match, all in one go at controller probe time. Rather like what's already commonly done with the parsing of "dma-ranges" to program address-translation LUTs for inbound windows.

Plus that would also give a chance of safely dealing with bad DTs specifying invalid ID mappings (by refusing to probe at all). As it is, returning an error from a child's BUS_NOTIFY_ADD_DEVICE does nothing except prevent any further notifiers from running at that point - the device will still be added, allowed to bind a driver, and able to start sending DMA/MSI traffic without the controller being correctly programmed, which at best won't work and at worst may break the whole system.

Thanks,
Robin.




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux