[PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Liviu Dudau
Liviu.Dudau at arm.com
Thu Jul 30 04:20:38 PDT 2015
Hello,
On Thu, Jul 30, 2015 at 09:30:41AM +0100, Gabriele Paoloni wrote:
> > -----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?
I can try to do that, as I worked with Andrew Murray when he did the patch.
- pci_space is indeed the range.flags variable and it was trying to keep
the original value from the DT mainly to try to identify the config space
in the ranges described. It has now become clear that declaring config
space in the ranges area is wrong even if one supports ECAM so pci_space
could be removed as suggested by Bjorn.
- pci_addr is the address that goes on the PCI(e) bus.
- cpu_addr is the translated address that the CPU uses. It does not necessarily
means it is the same address that the Root Complex sees when requested to
do Address Translation between host side and bus side. Also, what gets stored
in the cpu_addr is not equal to what gets declared in the ranges property if
there are other address translation parents between the Root Complex and the
CPU.
- bus_addr is meant to be the un-translated cpu_addr that DesignWare needs
in order to setup its ATS service. The reason for putting it in the of_pci_range
is because the struct resource does not have the concept of an untranslated
address.
Best regards,
Liviu
> >
> > > > 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;
> > > > > };
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
More information about the linux-arm-kernel
mailing list