Hi Bjorn Many Thanks for your reply I have commented back inline with resolutions from my side. If you're ok with them I'll send it out a new version in the appropriate patchset Cheers Gab > -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > Sent: Wednesday, July 29, 2015 6:21 PM > To: Gabriele Paoloni > Cc: arnd@xxxxxxxx; lorenzo.pieralisi@xxxxxxx; Wangzhou (B); > robh+dt@xxxxxxxxxx; james.morse@xxxxxxx; Liviu.Dudau@xxxxxxx; linux- > pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > Hi Gabriele, > > As far as I can tell, this is not specific to PCIe, so please use "PCI" > in > the subject as a generic term that includes both PCI and PCIe. sure agreed > > On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote: > > From: gabriele paoloni <gabriele.paoloni@xxxxxxxxxx> > > > > This patch is needed port PCIe designware to new DT parsing API > > As discussed in > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015- > January/317743.html > > in designware we have a problem as the PCI addresses in the PCIe > controller > > address space are required in order to perform correct HW > operation. > > > > In order to solve this problem commit f4c55c5a3 "PCI: designware: > > Program ATU with untranslated address" added code to read the > PCIe > > Conventional reference is 12-char SHA1, like this: > > f4c55c5a3f7f ("PCI: designware: Program ATU with untranslated > address") Agreed, will change this > > > controller start address directly from the DT ranges. > > > > In the new DT parsing API of_pci_get_host_bridge_resources() > hides the > > DT parser from the host controller drivers, so it is not possible > > for drivers to parse values directly from the DT. > > > > In http://www.spinics.net/lists/linux-pci/msg42540.html we > already tried > > to use the new DT parsing API but there is a bug (obviously) in > setting > > the <*>_mod_base addresses > > Applying this patch we can easily set "<*>_mod_base = win- > >__res.start" > > By itself, this patch adds something. It would help me understand it > if > the *user* of this new something were in the same patch series. the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support" I will ask Zhou Wang to include this patch in his patchset > > > 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? sure I can add comments inline in the code > > 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 > > 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: 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. > > > the patch is based on 4.2-rc1 > > You can put this after the "---" line because it's not relevant in the > permanent changelog. Agreed > > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx> > > Acked-by: Liviu Dudau <Liviu.Dudau@xxxxxxx> > > Acked-by: Rob Herring <robh@xxxxxxxxxx> > > Please un-indent your changelog. Ok agreed > > > --- > > drivers/of/address.c | 2 ++ > > drivers/of/of_pci.c | 4 ++++ > > include/linux/of_address.h | 1 + > > 3 files changed, 7 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index 8bfda6a..23a5793 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -253,6 +253,7 @@ struct of_pci_range > *of_pci_range_parser_one(struct of_pci_range_parser *parser, > > struct of_pci_range *range) > > { > > const int na = 3, ns = 2; > > + const int p_ns = of_n_size_cells(parser->node); > > > > if (!range) > > return NULL; > > @@ -265,6 +266,7 @@ struct of_pci_range > *of_pci_range_parser_one(struct of_pci_range_parser *parser, > > range->pci_addr = of_read_number(parser->range + 1, ns); > > range->cpu_addr = of_translate_address(parser->node, > > parser->range + na); > > + range->bus_addr = of_read_number(parser->range + na, p_ns); > > range->size = of_read_number(parser->range + parser->pna + na, > ns); > > > > parser->range += parser->np; > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > > index 5751dc5..fe57030 100644 > > --- a/drivers/of/of_pci.c > > +++ b/drivers/of/of_pci.c > > @@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct > device_node *dev, > > > > pr_debug("Parsing ranges property...\n"); > > for_each_of_pci_range(&parser, &range) { > > + struct resource_entry *entry; > > /* Read next ranges element */ > > if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) > > snprintf(range_type, 4, " IO"); > > @@ -240,6 +241,9 @@ int of_pci_get_host_bridge_resources(struct > device_node *dev, > > } > > > > pci_add_resource_offset(resources, res, res->start - > range.pci_addr); > > + entry = list_last_entry(resources, struct resource_entry, > node); > > + /* we are using __res for storing the PCI controller > address */ > > + entry->__res.start = range.bus_addr; > > } > > > > return 0; > > 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; > > }; > > -- > > 1.9.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
