Message-ID: <Z4rLRpNt7I3sOkBT@lizhi-Precision-Tower-5810>
Date: Fri, 17 Jan 2025 16:27:34 -0500
From: Frank Li <Frank.li@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Conor Dooley <conor@...nel.org>, daire.mcnamara@...rochip.com,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	conor.dooley@...rochip.com, lpieralisi@...nel.org, kw@...ux.com,
	robh@...nel.org, bhelgaas@...gle.com, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, ilpo.jarvinen@...ux.intel.com,
	kevin.xie@...rfivetech.com
Subject: Re: [PATCH v10 1/3] PCI: microchip: Fix outbound address translation
 tables

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@microchip.com]
> > > > >
> > > > > 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@...rochip.com wrote:
> > > > > > > > > From: Daire McNamara <daire.mcnamara@...rochip.com>
> > > > > > > > >
> > > > > > > > > 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@...0000000 {
>     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@...0000000 {
>       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@...0000000
> 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@...0000000     ->      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@...0000000 'ranges' should be in the
> fabric-pcie-bus@...0000000 'ranges' instead?

both needs ranges,

ranges in fabric-pcie-bus@...0000000, convert CPU address to local bus
address, such as trim high 32bits.

ranges in pcie@...0000000, 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

Powered by blists - more mailing lists