On Fri, Jan 17, 2025 at 11:30:31AM -0600, Bjorn Helgaas wrote: > On Fri, Jan 17, 2025 at 10:53:01AM +0000, Conor Dooley wrote: > > On Thu, Jan 16, 2025 at 12:02:55PM -0600, Bjorn Helgaas wrote: > > > On Thu, Jan 16, 2025 at 05:45:33PM +0000, Conor Dooley wrote: > > > > On Thu, Jan 16, 2025 at 11:07:22AM -0600, Bjorn Helgaas wrote: > > > > > [+cc Frank, original patch at > > > > > https://lore.kernel.org/r/20241011140043.1250030-2-daire.mcnamara@xxxxxxxxxxxxx] > > > > > > > > > > On Thu, Jan 16, 2025 at 04:46:19PM +0000, Conor Dooley wrote: > > > > > > On Thu, Jan 16, 2025 at 09:42:53AM -0600, Bjorn Helgaas wrote: > > > > > > > On Tue, Jan 14, 2025 at 06:13:10PM -0600, Bjorn Helgaas wrote: > > > > > > > > On Fri, Oct 11, 2024 at 03:00:41PM +0100, daire.mcnamara@xxxxxxxxxxxxx wrote: > > > > > > > > > From: Daire McNamara <daire.mcnamara@xxxxxxxxxxxxx> > > > > > > > > > > > > > > > > > > On Microchip PolarFire SoC (MPFS) the PCIe Root Port can be behind one of > > > > > > > > > three general-purpose Fabric Interface Controller (FIC) buses that > > > > > > > > > encapsulate an AXI-M interface. That FIC is responsible for managing > > > > > > > > > the translations of the upper 32-bits of the AXI-M address. On MPFS, > > > > > > > > > the Root Port driver needs to take account of that outbound address > > > > > > > > > translation done by the parent FIC bus before setting up its own > > > > > > > > > outbound address translation tables. In all cases on MPFS, > > > > > > > > > the remaining outbound address translation tables are 32-bit only. > > > > > > > > > > > > > > > > > > Limit the outbound address translation tables to 32-bit only. > > > > > > > > > > > > > > > > I don't quite understand what this is saying. It seems like the code > > > > > > > > keeps only the low 32 bits of a PCI address and throws away any > > > > > > > > address bits above the low 32. > > > > > > > > > > > > > > > > If that's what the FIC does, I wouldn't describe the FIC as > > > > > > > > "translating the upper 32 bits" since it sounds like the translation > > > > > > > > is just truncation. > > > > > > > > > > > > > > > > I guess it must be more complicated than that? I assume you can still > > > > > > > > reach BARs that have PCI addresses above 4GB using CPU loads/stores? > > > > > > > > > > > > > > > > The apertures through the host bridge for MMIO access are described by > > > > > > > > DT ranges properties, so this must be something that can't be > > > > > > > > described that way? > > > > > > > > > > > > Daire's been having some issues getting onto the corporate VPN to send > > > > > > his reply, I've pasted it below on his behalf: > > > > > > > > > > > > There are 3 Fabric Inter Connect (FIC) buses on PolarFire SoC - each of > > > > > > these FIC buses contain an AXI master bus and are 64-bits wide. These > > > > > > AXI-Masters (each with an individual 64-bit AXI base address – for example > > > > > > FIC1’s AXI Master has a base address of 0x2000000000) are connected to > > > > > > general purpose FPGA logic. This FPGA logic is, in turn, connected to a > > > > > > 2nd 32-bit AXI master which is attached to the PCIe block in RootPort mode. > > > > > > Conceptually, on the other side of this configurable logic, there is a > > > > > > 32-bit bus to a hard PCIe rootport. So, again conceptually, outbound address > > > > > > translation looks like this: > > > > > > > > > > > > Processor Complex à FIC (64-bit AXI-M) à Configurable Logic à 32-bit AXI-M à PCIe Rootport > > > > > > (This how it came to me from Daire, I think the á is meant to > > > > > > be an arrow) > > I'm trying to match this up with the DT snippet you included earlier: > > fabric-pcie-bus@3000000000 { > compatible = "simple-bus"; > #address-cells = <2>; > #size-cells = <2>; > ranges = <0x00 0x40000000 0x00 0x40000000 0x00 0x20000000>, > <0x30 0x00000000 0x30 0x00000000 0x10 0x00000000>; Sorry, I jump into this thread. Look like fabric-pcie-bus trim down to 32 bit address if I understand correctly and try reuse my previous works. <0x30 0x00000000 0x30 0x00000000 0x10 0x00000000>; should be <0 0x00000000, 0x30 0x00000000, 0, 0x40000000>; <0 0x60000000, 0x30 0x60000000, 0, 0xa0000000>; 32bit only 4G size, [parent 0x30_0000_0000..0x30_3fff_ffff] -> [child 0x0000_0000..0x3fff_ffff] [parent 0x30_6000_0000..0x30_ffff_ffff] -> [child 0x6000_0000..0xffff_ffff] 0x4000_0000..0x6000_0000 look like use for controller register access. > > IIUC, this describes these regions, so there's no address translation > at this point: > > [parent 0x00_40000000-0x00_5fffffff] -> [child 0x00_40000000-0x00_5fffffff] > [parent 0x30_00000000-0x3f_ffffffff] -> [child 0x30_00000000-0x3f_ffffffff] > > Here's the PCI controller: > > pcie: pcie@3000000000 { > compatible = "microchip,pcie-host-1.0"; > #address-cells = <0x3>; > #size-cells = <0x2>; > device_type = "pci"; > > reg = <0x30 0x00000000 0x0 0x08000000>, 0x30 is impossible here! > <0x00 0x43008000 0x0 0x00002000>, > <0x00 0x4300a000 0x0 0x00002000>; > > which has this register space (in the fabric-pcie-bus@3000000000 > address space): > > [0x30_00000000-0x30_07ffffff] (128MB) > [0x00_43008000-0x00_43009fff] (8KB) > [0x00_4300a000-0x00_4300bfff] (8KB) > > So if I'm reading this right (and I'm not at all sure I am), the PCI > controller a couple 8KB register regions below 4GB, and also 128MB of > register space at [0x30_00000000-0x30_07ffffff] (maybe ECAM?). I > don't know how to reconcile this one with the 32-bit AXI-M bus leading > to it. > > And it has these ranges, which *do* look like they translate > addresses: > > ranges = <0x43000000 0x0 0x09000000 0x30 0x09000000 0x0 0x0f000000>, > <0x01000000 0x0 0x08000000 0x30 0x08000000 0x0 0x01000000>, > <0x03000000 0x0 0x18000000 0x30 0x18000000 0x0 0x70000000>; All 0x30 should be 0x00 remove here ranges = <0x43000000 0x0 0x09000000 0x00 0x09000000 0x0 0x0f000000>, <0x01000000 0x0 0x08000000 0x00 0x00000000 0x0 0x01000000>, <0x03000000 0x0 0x18000000 0x00 0x18000000 0x0 0x70000000>; > > [parent 0x30_09000000-0x30_17ffffff] -> [pci 0x09000000-0x17ffffff pref 64bit mem] > [parent 0x30_08000000-0x30_08ffffff] -> [pci 0x08000000-0x08ffffff io] > [parent 0x30_18000000-0x30_87ffffff] -> [pci 0x18000000-0x87ffffff 64bit mem] [parent 0x0900_0000-0x17ff_ffff] -> [pci 0x09000000-0x17ffffff pref 64bit mem] [parent 0x0800_0000-0x08ff_ffff] -> [pci 0x00000000-0x00ffffff io] [parent 0x1800_0000-0x87ff_ffff] -> [pci 0x18000000-0x87ffffff 64bit mem] So whole translate for example 0x30_1000_0000 from CPU to PCI Bus CPU address -> fabric-pcie-bus@3000000000 -> PCI controller -> PCI BUS 0x30_0800_0000 [0x30_0800_0000 -> 0x0800_0000] [0x0800_0000 -> 0x00000000 (IO)] 0x00000000 (IO) > > }; > } > > These look like three apertures to PCI, all of which are below 4GB on > PCI (I'm not sure why the space code is 11b, which indicates 64-bit > memory space). But all of these are *above* 4GB on the upstream side > of the PCI controller, so I have the same question about the 32-bit > AXI-M bus. > > Maybe the translation in the pcie@3000000000 'ranges' should be in the > fabric-pcie-bus@3000000000 'ranges' instead? both needs ranges, ranges in fabric-pcie-bus@3000000000, convert CPU address to local bus address, such as trim high 32bits. ranges in pcie@3000000000, convert local bus address for difference PCI transfer type such as config/io/mem space. > > > > So is this patch a symptom that is telling us we're not paying > > > attention to 'ranges' correctly? > > > > Sounds to me like there's something missing core wise, if you've got > > several drivers having to figure it out themselves. > > Yeah, this doesn't seem like something each driver should have to do > by itself. > > > Daire seems to think what Frank's done should work here, but it'd need > > to be looked into of course. Devicetree should look the same in both > > cases, do you want it as a new version or as a follow up? > > I'd prefer if we could sort this out before merging this if we can. > I'm not sure we can squeeze Frank's work in this cycle; it seems like > we might be able to massage it and figure out some sort of common > strategy for this situation even if DesignWare, Cadence, Microchip, > etc need slightly different implementations. At least first two patches are needed before other people can start work. of: address: Add parent_bus_addr to struct of_pci_range PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback Frank > > Bjorn
