[PATCH v6] PCI: Store PCIe bus address in struct of_pci_range

[Gabriele Paoloni]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas at google.com]
> Sent: Wednesday, July 29, 2015 10:47 PM
> To: Gabriele Paoloni
> Cc: arnd at arndb.de; lorenzo.pieralisi at arm.com; Wangzhou (B);
> robh+dt at kernel.org; james.morse at arm.com; Liviu.Dudau at arm.com; linux-
> pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth); Andrew Murray
> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> of_pci_range
> 
> [+cc Andrew]
> 
> On Wed, Jul 29, 2015 at 07:44:18PM +0000, Gabriele Paoloni wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:bhelgaas at google.com]
> > > Sent: Wednesday, July 29, 2015 6:21 PM
> > > To: Gabriele Paoloni
> 
> > > On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > > > From: gabriele paoloni <gabriele.paoloni at huawei.com>
> 
> > > >     This patch adds a new field in "struct of_pci_range" to store
> the
> > > >     pci bus start address; it fills the field in
> > > of_pci_range_parser_one();
> > > >     in of_pci_get_host_bridge_resources() it retrieves the
> resource
> > > entry
> > > >     after it is created and added to the resource list and uses
> > > >     entry->__res.start to store the pci controller address
> > >
> > > struct of_pci_range is starting to get confusing to non-OF folks
> like
> > > me.
> > > It now contains:
> > >
> > >   u32 pci_space;
> > >   u64 pci_addr;
> > >   u64 cpu_addr;
> > >   u64 bus_addr;
> > >
> > > Can you explain what all these things mean, and maybe even add one-
> line
> > > comments to the structure?
> 
> > > pci_space: The only uses I see are to determine whether to print
> > > "Prefetch".  I don't see any real functionality that uses this.
> >
> > Looking at the code I agree. it's seems to be used only in powerpc
> > and microblaze to print out.
> > However from my understanding pci_space is the phys.hi field of the
> > ranges property: it defines the properties of the address space
> associated
> > to the PCI address. if you're curious you can find a nice and quick
> to read
> > "guide" in http://devicetree.org/MPC5200:PCI
> 
> I think pci_space should be removed and the users should test
> "range.flags & IORESOURCE_PREFETCH" instead.  That's already set by
> of_bus_pci_get_flags().  This is separate from your current patch, of
> course.

Ok so I'll do nothing in my patch about this; maybe I can propose a separate
patch for this, but I cannot test it (I've got no microblaze and powerpc 
neither....)

> 
> 29b635c00f3e ("of/pci: Provide support for parsing PCI DT ranges
> property") added struct of_pci_range, and even at the time,
> of_bus_pci_get_flags() set IORESOURCE_PREFETCH in of_pci_range.flags.
> 
> 654837e8fe8d ("powerpc/pci: Use of_pci_range_parser helper in
> pci_process_bridge_OF_ranges") converted powerpc to use
> of_pci_range_parser() instead of parsing manually.  It converted other
> references to look at struct of_pci_range.flags; I'm not sure why it
> didn't do that for the prefetch bit.
> 
> I copied Andrew in case there's some subtlety here.
> 
> > > pci_addr: I assume this is a PCI bus address, like what you would
> see
> > > if
> > > you put an analyzer on the bus/link.  This address could go in a
> BAR.
> >
> > Yes, this is the PCI start address of the range: phys.mid + phys.low
> in the
> > guide mentioned above
> >
> > > cpu_addr: I assume this is a CPU physical address, like what you
> would
> > > see
> > > in /proc/iomem and what you would pass to ioremap().
> >
> > Yes correct
> >
> > > bus_addr: ?
> >
> > According to the guide above, this is the address into which the
> pci_address
> > get translated to and that is passed to the root complex. Between the
> root
> > complex and the CPU there can be intermediate translation layers:
> 
> I can't quite parse this, but I do understand how a host bridge can
> translate CPU physical addresses to a different range of PCI bus
> addresses.  What I don't understand is the difference between
> "pci_addr"
> and the "bus_addr" you're adding.

For example see:
http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6qdl.dtsi#L148

ranges = <0x00000800 0 0x01f00000 0x01f00000 0 0x00080000   /* configuration space */
          0x81000000 0 0          0x01f80000 0 0x00010000   /* downstream I/O */
          0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable memory *
               /\          /\          /\
           pci_space    pci_addr     bus_addr            


The host bridge performs the first translation from pci_addr to bus_addr.
If there are other ranges in the parents nodes in the DT bus_addr gets 
translated further till you get the cpu_addr.

Hope it is a bit clearer now....

> 
> > see that to
> > get pci_address we call "of_translate_address"; this will apply all
> the
> > translation layers (ranges in the DT) that it finds till it comes to
> the root
> > node of the DT (thus retrieving the CPU address).
> > Now said that, for designware we need the first translated PCI
> address, that we call
> > here bus_addr after Rob Herring suggested the name...honestly I
> cannot think of a
> > different name
> >
> >
> >
> > > I'm trying to imagine how this might be expressed in ACPI.  A host
> > > bridge
> > > ACPI _CRS contains a CPU physical address and applying a _TRA
> > > (translation
> > > offset) to the CPU address gives you a PCI bus address.  I know
> this
> > > code
> > > is OF, not ACPI, but I assume that it should be possible to
> describe
> > > your
> > > hardware via ACPI as well as by OF.
> 
> > > > diff --git a/include/linux/of_address.h
> b/include/linux/of_address.h
> > > > index d88e81b..865f96e 100644
> > > > --- a/include/linux/of_address.h
> > > > +++ b/include/linux/of_address.h
> > > > @@ -16,6 +16,7 @@ struct of_pci_range {
> > > >  	u32 pci_space;
> > > >  	u64 pci_addr;
> > > >  	u64 cpu_addr;
> > > > +	u64 bus_addr;
> > > >  	u64 size;
> > > >  	u32 flags;
> > > >  };