On 2015/7/30 3:44, Gabriele Paoloni wrote: > 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 > Hi Gab, I can merge this patch in my series if this make it clearer to understand. Thanks, Zhou > >> >>> 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
