| Message ID | 1438010223-124422-1-git-send-email-gabriele.paoloni@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bjorn Liviu and Rob already acked, do you think it is ok to merge this? Cheers Gab > -----Original Message----- > From: Gabriele Paoloni > Sent: Monday, July 27, 2015 4:17 PM > To: Gabriele Paoloni; arnd@arndb.de; lorenzo.pieralisi@arm.com; > Wangzhou (B); bhelgaas@google.com; robh+dt@kernel.org; > james.morse@arm.com; Liviu.Dudau@arm.com > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth) > Subject: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range > > From: gabriele paoloni <gabriele.paoloni@huawei.com> > > 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 > 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" > > 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 > > the patch is based on 4.2-rc1 > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> > Acked-by: Rob Herring <robh@kernel.org> > --- > 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
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. On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote: > From: gabriele paoloni <gabriele.paoloni@huawei.com> > > 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") > 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. > 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. 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. 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(). bus_addr: ? 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. > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> > Acked-by: Rob Herring <robh@kernel.org> Please un-indent your changelog. > --- > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@google.com] > Sent: Wednesday, July 29, 2015 6:21 PM > To: Gabriele Paoloni > Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; 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@huawei.com> > > > > 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@huawei.com> > > Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Acked-by: Rob Herring <robh@kernel.org> > > 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@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Andrew] On Wed, Jul 29, 2015 at 07:44:18PM +0000, Gabriele Paoloni wrote: > > -----Original Message----- > > From: Bjorn Helgaas [mailto:bhelgaas@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@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. 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. > 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; > > > };
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@google.com] >> Sent: Wednesday, July 29, 2015 6:21 PM >> To: Gabriele Paoloni >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> devicetree@vger.kernel.org; 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@huawei.com> >>> >>> 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@huawei.com> >>> Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> >>> Acked-by: Rob Herring <robh@kernel.org> >> >> 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@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > . >
> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Wednesday, July 29, 2015 10:47 PM > To: Gabriele Paoloni > Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@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@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@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; > > > > };
Hello, On Thu, Jul 30, 2015 at 09:30:41AM +0100, Gabriele Paoloni wrote: > > -----Original Message----- > > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > > Sent: Wednesday, July 29, 2015 10:47 PM > > To: Gabriele Paoloni > > Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > devicetree@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@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@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; > > > > > }; >
On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni <gabriele.paoloni@huawei.com> 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@google.com] >> Sent: Wednesday, July 29, 2015 6:21 PM >> To: Gabriele Paoloni >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> devicetree@vger.kernel.org; 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@huawei.com> >> > >> > 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 I think you mean "translated CPU address." The flow of addresses looks like this: CPU -> CPU bus address -> bus fabric address decoding -> bus address -> DW PCI -> PCI address This is quite common that an IP block does not see the full address. It is unusual that the IP block needs to know its full address on the slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping > here bus_addr after Rob Herring suggested the name...honestly I cannot think of a > different name Thinking about this some more, is this really a translation versus just a stripping of high bits? Does the DW IP have less than 32-bits address? If so, we could express differently and just mask the CPU address within the driver instead. Rob
> -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Rob Herring > Sent: Thursday, July 30, 2015 2:43 PM > To: Gabriele Paoloni > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > <gabriele.paoloni@huawei.com> 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@google.com] > >> Sent: Wednesday, July 29, 2015 6:21 PM > >> To: Gabriele Paoloni > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> devicetree@vger.kernel.org; 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@huawei.com> > >> > > >> > 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 > > I think you mean "translated CPU address." The flow of addresses looks > like this: > > CPU -> CPU bus address -> bus fabric address decoding -> bus address > -> DW PCI -> PCI address > > This is quite common that an IP block does not see the full address. > It is unusual that the IP block needs to know its full address on the > slave side. It is quite common for the master side and the kernel > deals with that all the time with DMA mapping > > > here bus_addr after Rob Herring suggested the name...honestly I > cannot think of a > > different name > > Thinking about this some more, is this really a translation versus > just a stripping of high bits? Does the DW IP have less than 32-bits > address? If so, we could express differently and just mask the CPU > address within the driver instead. I don’t think we should rely on PCI addresses...what if the intermediate translation layer changes the lower significant bits of the "bus address" to translate into a cpu address? In that case we're going to program the DW with a wrong address What I am saying is that the DW driver should not rely on the behavior of external HW....what do you think? > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Gabriele Paoloni > Sent: Thursday, July 30, 2015 2:52 PM > To: Rob Herring > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth) > Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > > > > -----Original Message----- > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > owner@vger.kernel.org] On Behalf Of Rob Herring > > Sent: Thursday, July 30, 2015 2:43 PM > > To: Gabriele Paoloni > > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > qiuzhenfa; Liguozhu (Kenneth) > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > of_pci_range > > > > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > > <gabriele.paoloni@huawei.com> 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@google.com] > > >> Sent: Wednesday, July 29, 2015 6:21 PM > > >> To: Gabriele Paoloni > > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > linux- > > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > >> devicetree@vger.kernel.org; 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@huawei.com> > > >> > > > >> > 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 > > > > I think you mean "translated CPU address." The flow of addresses > looks > > like this: > > > > CPU -> CPU bus address -> bus fabric address decoding -> bus address > > -> DW PCI -> PCI address > > > > This is quite common that an IP block does not see the full address. > > It is unusual that the IP block needs to know its full address on the > > slave side. It is quite common for the master side and the kernel > > deals with that all the time with DMA mapping > > > > > here bus_addr after Rob Herring suggested the name...honestly I > > cannot think of a > > > different name > > > > Thinking about this some more, is this really a translation versus > > just a stripping of high bits? Does the DW IP have less than 32-bits > > address? If so, we could express differently and just mask the CPU > > address within the driver instead. > > I don’t think we should rely on PCI addresses...what if the > intermediate > translation layer changes the lower significant bits of the "bus > address" > to translate into a cpu address? Sorry above I meant "I don’t think we should rely on CPU addresses" > > In that case we're going to program the DW with a wrong address > > What I am saying is that the DW driver should not rely on the > behavior of external HW....what do you think? > > > > > Rob > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > ??? ?&?~?&???+- > ?????w??????m?b??ir(?????}????z?&j:+v??? ????zZ+??+zf???h???~????i??? > z? ?w????????&?)?f
On Thu, Jul 30, 2015 at 08:42:53AM -0500, Rob Herring wrote: > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > <gabriele.paoloni@huawei.com> 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@google.com] > >> Sent: Wednesday, July 29, 2015 6:21 PM > >> To: Gabriele Paoloni > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> devicetree@vger.kernel.org; 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@huawei.com> > >> > > >> > 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 > > I think you mean "translated CPU address." The flow of addresses looks > like this: > > CPU -> CPU bus address -> bus fabric address decoding -> bus address > -> DW PCI -> PCI address > > This is quite common that an IP block does not see the full address. > It is unusual that the IP block needs to know its full address on the > slave side. It is quite common for the master side and the kernel > deals with that all the time with DMA mapping > > > here bus_addr after Rob Herring suggested the name...honestly I cannot think of a > > different name > > Thinking about this some more, is this really a translation versus > just a stripping of high bits? Does the DW IP have less than 32-bits > address? If so, we could express differently and just mask the CPU > address within the driver instead. I would like this much better if it would be sufficient. If I understand this correctly, this is all for the MMIO path, i.e., it has nothing to do with DMA addresses generated by the device being translated to main memory addresses. ACPI has a fairly good abstract model for describing the address translations the kernel and drivers need to know about. We should assume we'll need to describe this hardware via ACPI in addition to DT eventually, so I'm trying to figure out how we would map this into ACPI terms. If the driver really needs this intermediate address (the "bus address" above), I think that would mean adding a second ACPI bridge device. The first bridge would have a _TRA showing the offset from CPU physical address to bus address, and the second bridge would have a _TRA showing the offset from bus address to PCI address. If you only had a single ACPI bridge device, you'd only have one _TRA (from CPU physical to PCI bus address), and there'd be no way for the driver to learn about the intermediate bus address.
On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: > > > > -----Original Message----- > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > owner@vger.kernel.org] On Behalf Of Rob Herring > > Sent: Thursday, July 30, 2015 2:43 PM > > To: Gabriele Paoloni > > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > qiuzhenfa; Liguozhu (Kenneth) > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > of_pci_range > > > > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > > <gabriele.paoloni@huawei.com> 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@google.com] > > >> Sent: Wednesday, July 29, 2015 6:21 PM > > >> To: Gabriele Paoloni > > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > >> devicetree@vger.kernel.org; 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@huawei.com> > > >> > > > >> > 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 > > > > I think you mean "translated CPU address." The flow of addresses looks > > like this: > > > > CPU -> CPU bus address -> bus fabric address decoding -> bus address > > -> DW PCI -> PCI address > > > > This is quite common that an IP block does not see the full address. > > It is unusual that the IP block needs to know its full address on the > > slave side. It is quite common for the master side and the kernel > > deals with that all the time with DMA mapping > > > > > here bus_addr after Rob Herring suggested the name...honestly I > > cannot think of a > > > different name > > > > Thinking about this some more, is this really a translation versus > > just a stripping of high bits? Does the DW IP have less than 32-bits > > address? If so, we could express differently and just mask the CPU > > address within the driver instead. > > I don’t think we should rely on [CPU] addresses...what if the intermediate > translation layer changes the lower significant bits of the "bus address" > to translate into a cpu address? Is it really a possiblity that the lower bits could be changed? I think we're talking about MMIO here, and a bridge has an MMIO window. A window is a range of CPU physical addresses that the bridge forwards to PCI. The PCI core assumes that a CPU physical address range is linearly mapped to PCI bus addresses, i.e., if the window base is X and maps to PCI address Y, then as long as X+n is inside the window, it must map to Y+n. That means the low-order bits (the ones that are the offset into the window) cannot change.
> -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > Sent: Thursday, July 30, 2015 5:15 PM > To: Gabriele Paoloni > Cc: Rob Herring; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: > > > > > > > -----Original Message----- > > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > > owner@vger.kernel.org] On Behalf Of Rob Herring > > > Sent: Thursday, July 30, 2015 2:43 PM > > > To: Gabriele Paoloni > > > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; > Wangzhou > > > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > > qiuzhenfa; Liguozhu (Kenneth) > > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > > of_pci_range > > > > > > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > > > <gabriele.paoloni@huawei.com> 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@google.com] > > > >> Sent: Wednesday, July 29, 2015 6:21 PM > > > >> To: Gabriele Paoloni > > > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > linux- > > > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > >> devicetree@vger.kernel.org; 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@huawei.com> > > > >> > > > > >> > 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 > > > > > > I think you mean "translated CPU address." The flow of addresses > looks > > > like this: > > > > > > CPU -> CPU bus address -> bus fabric address decoding -> bus > address > > > -> DW PCI -> PCI address > > > > > > This is quite common that an IP block does not see the full address. > > > It is unusual that the IP block needs to know its full address on > the > > > slave side. It is quite common for the master side and the kernel > > > deals with that all the time with DMA mapping > > > > > > > here bus_addr after Rob Herring suggested the name...honestly I > > > cannot think of a > > > > different name > > > > > > Thinking about this some more, is this really a translation versus > > > just a stripping of high bits? Does the DW IP have less than 32- > bits > > > address? If so, we could express differently and just mask the CPU > > > address within the driver instead. > > > > I don’t think we should rely on [CPU] addresses...what if the > intermediate > > translation layer changes the lower significant bits of the "bus > address" > > to translate into a cpu address? > > Is it really a possiblity that the lower bits could be changed? I've checked all the current deignware users DTs except "pci-layerscape" that I could not find: spear1310.dtsi spear1340.dtsi dra7.dtsi imx6qdl.dtsi imx6sx.dtsi keystone.dtsi exynos5440.dtsi None of them modifies the lower bits. To be more precise the only guy that provides another translation layer is "dra7.dtsi": axi0 http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 axi1 http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 For this case masking the top 4bits (bits28 to 31) should make the job. Speaking in general terms so far I've always seen linear mappings that differ by bitmask offset, however linear does not mean that you cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design reasons it is much easier to remap directly using a bitmask...but I was not sure and I didn't think about the problems that can arise with ACPI. If you think the bitmask is Ok then I can directly define it in designware and we can drop this patch. Thanks Gab > > I think we're talking about MMIO here, and a bridge has an MMIO > window. A window is a range of CPU physical addresses that the > bridge forwards to PCI. The PCI core assumes that a CPU physical > address range is linearly mapped to PCI bus addresses, i.e., if the > window base is X and maps to PCI address Y, then as long as X+n is > inside the window, it must map to Y+n. > > That means the low-order bits (the ones that are the offset into the > window) cannot change. > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Jingoo, Pratyush] On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: > > -----Original Message----- > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > > Sent: Thursday, July 30, 2015 5:15 PM > > To: Gabriele Paoloni > > Cc: Rob Herring; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > qiuzhenfa; Liguozhu (Kenneth) > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > of_pci_range > > > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: > > > > > > > > > > -----Original Message----- > > > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > > > owner@vger.kernel.org] On Behalf Of Rob Herring > > > > Sent: Thursday, July 30, 2015 2:43 PM > > > > To: Gabriele Paoloni > > > > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; > > Wangzhou > > > > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > > > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > > > qiuzhenfa; Liguozhu (Kenneth) > > > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > > > of_pci_range > > > > > > > > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > > > > <gabriele.paoloni@huawei.com> 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@google.com] > > > > >> Sent: Wednesday, July 29, 2015 6:21 PM > > > > >> To: Gabriele Paoloni > > > > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > > > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > > linux- > > > > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > > >> devicetree@vger.kernel.org; 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@huawei.com> > > > > >> > > > > > >> > 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 > > > > > > > > I think you mean "translated CPU address." The flow of addresses > > looks > > > > like this: > > > > > > > > CPU -> CPU bus address -> bus fabric address decoding -> bus > > address > > > > -> DW PCI -> PCI address > > > > > > > > This is quite common that an IP block does not see the full address. > > > > It is unusual that the IP block needs to know its full address on > > the > > > > slave side. It is quite common for the master side and the kernel > > > > deals with that all the time with DMA mapping > > > > > > > > > here bus_addr after Rob Herring suggested the name...honestly I > > > > cannot think of a > > > > > different name > > > > > > > > Thinking about this some more, is this really a translation versus > > > > just a stripping of high bits? Does the DW IP have less than 32- > > bits > > > > address? If so, we could express differently and just mask the CPU > > > > address within the driver instead. > > > > > > I don’t think we should rely on [CPU] addresses...what if the > > intermediate > > > translation layer changes the lower significant bits of the "bus > > address" > > > to translate into a cpu address? > > > > Is it really a possiblity that the lower bits could be changed? > > I've checked all the current deignware users DTs except "pci-layerscape" > that I could not find: > spear1310.dtsi > spear1340.dtsi > dra7.dtsi > imx6qdl.dtsi > imx6sx.dtsi > keystone.dtsi > exynos5440.dtsi > > None of them modifies the lower bits. To be more precise the only guy > that provides another translation layer is "dra7.dtsi": > axi0 > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 > > axi1 > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 > > For this case masking the top 4bits (bits28 to 31) should make the job. > > Speaking in general terms so far I've always seen linear mappings > that differ by bitmask offset, however linear does not mean that you > cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to > <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design reasons > it is much easier to remap directly using a bitmask...but I was not sure > and I didn't think about the problems that can arise with ACPI. As I said, it wouldn't make sense for the bits that comprise the offset into the window to change, so the mapping only has to be linear inside the window. For the bits outside the window offset, I don't know enough to say whether masking is sufficient or safe. > If you think the bitmask is Ok then I can directly define it in > designware and we can drop this patch. It's OK by me, but I know nothing about the actual hardware involved. For the DesignWare question, I think you would just have to convince Jingoo and Pratyush (cc'd). > > I think we're talking about MMIO here, and a bridge has an MMIO > > window. A window is a range of CPU physical addresses that the > > bridge forwards to PCI. The PCI core assumes that a CPU physical > > address range is linearly mapped to PCI bus addresses, i.e., if the > > window base is X and maps to PCI address Y, then as long as X+n is > > inside the window, it must map to Y+n. > > > > That means the low-order bits (the ones that are the offset into the > > window) cannot change. > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: 30 July 2015 18:15 > To: Gabriele Paoloni > Cc: Rob Herring; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > [+cc Jingoo, Pratyush] > > On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: > > > -----Original Message----- > > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > > > Sent: Thursday, July 30, 2015 5:15 PM > > > To: Gabriele Paoloni > > > Cc: Rob Herring; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > (B); > > > robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; linux- > > > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > > qiuzhenfa; Liguozhu (Kenneth) > > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > > of_pci_range > > > > > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > > > > owner@vger.kernel.org] On Behalf Of Rob Herring > > > > > Sent: Thursday, July 30, 2015 2:43 PM > > > > > To: Gabriele Paoloni > > > > > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; > > > Wangzhou > > > > > (B); robh+dt@kernel.org; james.morse@arm.com; > Liviu.Dudau@arm.com; > > > > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > > > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > > > > > qiuzhenfa; Liguozhu (Kenneth) > > > > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > > > > of_pci_range > > > > > > > > > > On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni > > > > > <gabriele.paoloni@huawei.com> 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@google.com] > > > > > >> Sent: Wednesday, July 29, 2015 6:21 PM > > > > > >> To: Gabriele Paoloni > > > > > >> Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B); > > > > > >> robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > > > linux- > > > > > >> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > > > >> devicetree@vger.kernel.org; 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@huawei.com> > > > > > >> > > > > > > >> > 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 > > > > > > > > > > I think you mean "translated CPU address." The flow of addresses > > > looks > > > > > like this: > > > > > > > > > > CPU -> CPU bus address -> bus fabric address decoding -> bus > > > address > > > > > -> DW PCI -> PCI address > > > > > > > > > > This is quite common that an IP block does not see the full > address. > > > > > It is unusual that the IP block needs to know its full address > on > > > the > > > > > slave side. It is quite common for the master side and the > kernel > > > > > deals with that all the time with DMA mapping > > > > > > > > > > > here bus_addr after Rob Herring suggested the name...honestly > I > > > > > cannot think of a > > > > > > different name > > > > > > > > > > Thinking about this some more, is this really a translation > versus > > > > > just a stripping of high bits? Does the DW IP have less than 32- > > > bits > > > > > address? If so, we could express differently and just mask the > CPU > > > > > address within the driver instead. > > > > > > > > I don’t think we should rely on [CPU] addresses...what if the > > > intermediate > > > > translation layer changes the lower significant bits of the "bus > > > address" > > > > to translate into a cpu address? > > > > > > Is it really a possiblity that the lower bits could be changed? > > > > I've checked all the current deignware users DTs except "pci- > layerscape" > > that I could not find: > > spear1310.dtsi > > spear1340.dtsi > > dra7.dtsi > > imx6qdl.dtsi > > imx6sx.dtsi > > keystone.dtsi > > exynos5440.dtsi > > > > None of them modifies the lower bits. To be more precise the only guy > > that provides another translation layer is "dra7.dtsi": > > axi0 > > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 > > > > axi1 > > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 > > > > For this case masking the top 4bits (bits28 to 31) should make the job. > > > > Speaking in general terms so far I've always seen linear mappings > > that differ by bitmask offset, however linear does not mean that you > > cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to > > <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design > reasons > > it is much easier to remap directly using a bitmask...but I was not > sure > > and I didn't think about the problems that can arise with ACPI. > > As I said, it wouldn't make sense for the bits that comprise the > offset into the window to change, so the mapping only has to be linear > inside the window. > > For the bits outside the window offset, I don't know enough to say > whether masking is sufficient or safe. > > > If you think the bitmask is Ok then I can directly define it in > > designware and we can drop this patch. > > It's OK by me, but I know nothing about the actual hardware involved. > For the DesignWare question, I think you would just have to convince > Jingoo and Pratyush (cc'd). Yes actually looking at http://lxr.free-electrons.com/source/arch/arm/boot/dts/spear1310.dtsi#L99 I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so clearing the top 4 bits would alter the behaviour of the current designware SW framework...now I don't know if DW needs only the low 28b of that value or not.... Jingoo, Pratyush what do you think? > > > > I think we're talking about MMIO here, and a bridge has an MMIO > > > window. A window is a range of CPU physical addresses that the > > > bridge forwards to PCI. The PCI core assumes that a CPU physical > > > address range is linearly mapped to PCI bus addresses, i.e., if the > > > window base is X and maps to PCI address Y, then as long as X+n is > > > inside the window, it must map to Y+n. > > > > > > That means the low-order bits (the ones that are the offset into the > > > window) cannot change. > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni <gabriele.paoloni@huawei.com> wrote: >> -----Original Message----- >> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >> Sent: 30 July 2015 18:15 >> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: >> > > -----Original Message----- >> > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >> > > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas >> > > Sent: Thursday, July 30, 2015 5:15 PM >> > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: [...] >> > > > I don’t think we should rely on [CPU] addresses...what if the >> > > intermediate >> > > > translation layer changes the lower significant bits of the "bus >> > > address" >> > > > to translate into a cpu address? >> > > >> > > Is it really a possiblity that the lower bits could be changed? >> > >> > I've checked all the current deignware users DTs except "pci- >> layerscape" >> > that I could not find: >> > spear1310.dtsi >> > spear1340.dtsi >> > dra7.dtsi >> > imx6qdl.dtsi >> > imx6sx.dtsi >> > keystone.dtsi >> > exynos5440.dtsi >> > >> > None of them modifies the lower bits. To be more precise the only guy >> > that provides another translation layer is "dra7.dtsi": >> > axi0 >> > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 >> > >> > axi1 >> > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 >> > >> > For this case masking the top 4bits (bits28 to 31) should make the job. IMO, we should just fix this case. After further study, I don't think this is a DW issue, but rather an SOC integration issue. I believe you can just fixup the address in the pp->ops->host_init hook. In looking at this, I'm curious why only 2 ATU viewports are used by default as this causes switching on config accesses. Probably some configuration only has 2 viewports. This would not even work on SMP systems! Memory and i/o accesses do not have any lock. A config access on one core will temporarily disable the i/o or memory viewport which will cause an abort, dropped, or garbage data on an i/o or memory access from another core. You would have to be accessing cfg space on a bus other than the root bus, so maybe no one is doing that. >> > >> > Speaking in general terms so far I've always seen linear mappings >> > that differ by bitmask offset, however linear does not mean that you >> > cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to >> > <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design >> reasons >> > it is much easier to remap directly using a bitmask...but I was not >> sure >> > and I didn't think about the problems that can arise with ACPI. For higher speed buses, the h/w designs are not going to be doing complicated translations in order to meet timing requirements. At the top level only the highest order bits are going to be looked at. For downstream segments, the high order bits may get dropped to simplify routing. If an IP block has full address bits in this case, they could either be tied to 0 or tied off to the correct address. Either is reasonable, so I'm sure there is h/w doing both. That doesn't mean h/w designers haven't done something crazy, too. >> As I said, it wouldn't make sense for the bits that comprise the >> offset into the window to change, so the mapping only has to be linear >> inside the window. >> >> For the bits outside the window offset, I don't know enough to say >> whether masking is sufficient or safe. >> >> > If you think the bitmask is Ok then I can directly define it in >> > designware and we can drop this patch. >> >> It's OK by me, but I know nothing about the actual hardware involved. >> For the DesignWare question, I think you would just have to convince >> Jingoo and Pratyush (cc'd). > > Yes actually looking at > http://lxr.free-electrons.com/source/arch/arm/boot/dts/spear1310.dtsi#L99 > I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so clearing > the top 4 bits would alter the behaviour of the current designware > SW framework...now I don't know if DW needs only the low 28b of that > value or not.... Given that most cases have same cpu and bus address, masking is not the right solution in general. There is also a bug here BTW. pcie0 and pcie2 have the same CPU address for the memory range. Rob
[+cc Kishon] > -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Rob Herring > Sent: Thursday, July 30, 2015 9:42 PM > To: Gabriele Paoloni > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni > <gabriele.paoloni@huawei.com> wrote: > >> -----Original Message----- > >> From: Bjorn Helgaas [mailto:bhelgaas@google.com] > >> Sent: 30 July 2015 18:15 > >> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: > >> > > -----Original Message----- > >> > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > >> > > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > >> > > Sent: Thursday, July 30, 2015 5:15 PM > >> > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: > > [...] > > >> > > > I don’t think we should rely on [CPU] addresses...what if the > >> > > intermediate > >> > > > translation layer changes the lower significant bits of the > "bus > >> > > address" > >> > > > to translate into a cpu address? > >> > > > >> > > Is it really a possiblity that the lower bits could be changed? > >> > > >> > I've checked all the current deignware users DTs except "pci- > >> layerscape" > >> > that I could not find: > >> > spear1310.dtsi > >> > spear1340.dtsi > >> > dra7.dtsi > >> > imx6qdl.dtsi > >> > imx6sx.dtsi > >> > keystone.dtsi > >> > exynos5440.dtsi > >> > > >> > None of them modifies the lower bits. To be more precise the only > guy > >> > that provides another translation layer is "dra7.dtsi": > >> > axi0 > >> > http://lxr.free- > electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 > >> > > >> > axi1 > >> > http://lxr.free- > electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 > >> > > >> > For this case masking the top 4bits (bits28 to 31) should make the > job. > > IMO, we should just fix this case. After further study, I don't think > this is a DW issue, but rather an SOC integration issue. > > I believe you can just fixup the address in the pp->ops->host_init hook. > Yes I guess that I could just assign pp->(*)_mod_base to the CPU address in DW and mask it out in dra7xx_pcie_host_init()... Kishon, would you be ok with that? > In looking at this, I'm curious why only 2 ATU viewports are used by > default as this causes switching on config accesses. Probably some > configuration only has 2 viewports. This would not even work on SMP > systems! Memory and i/o accesses do not have any lock. A config access > on one core will temporarily disable the i/o or memory viewport which > will cause an abort, dropped, or garbage data on an i/o or memory > access from another core. You would have to be accessing cfg space on > a bus other than the root bus, so maybe no one is doing that. > > >> > > >> > Speaking in general terms so far I've always seen linear mappings > >> > that differ by bitmask offset, however linear does not mean that > you > >> > cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map > to > >> > <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design > >> reasons > >> > it is much easier to remap directly using a bitmask...but I was > not > >> sure > >> > and I didn't think about the problems that can arise with ACPI. > > For higher speed buses, the h/w designs are not going to be doing > complicated translations in order to meet timing requirements. At the > top level only the highest order bits are going to be looked at. For > downstream segments, the high order bits may get dropped to simplify > routing. If an IP block has full address bits in this case, they could > either be tied to 0 or tied off to the correct address. Either is > reasonable, so I'm sure there is h/w doing both. That doesn't mean h/w > designers haven't done something crazy, too. > > >> As I said, it wouldn't make sense for the bits that comprise the > >> offset into the window to change, so the mapping only has to be > linear > >> inside the window. > >> > >> For the bits outside the window offset, I don't know enough to say > >> whether masking is sufficient or safe. > >> > >> > If you think the bitmask is Ok then I can directly define it in > >> > designware and we can drop this patch. > >> > >> It's OK by me, but I know nothing about the actual hardware involved. > >> For the DesignWare question, I think you would just have to convince > >> Jingoo and Pratyush (cc'd). > > > > Yes actually looking at > > http://lxr.free- > electrons.com/source/arch/arm/boot/dts/spear1310.dtsi#L99 > > I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so > clearing > > the top 4 bits would alter the behaviour of the current designware > > SW framework...now I don't know if DW needs only the low 28b of that > > value or not.... > > Given that most cases have same cpu and bus address, masking is not > the right solution in general. > > There is also a bug here BTW. pcie0 and pcie2 have the same CPU > address for the memory range. > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
+Arnd Hi, On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote: > [+cc Kishon] > >> -----Original Message----- >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >> owner@vger.kernel.org] On Behalf Of Rob Herring >> Sent: Thursday, July 30, 2015 9:42 PM >> To: Gabriele Paoloni >> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou >> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; >> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; >> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >> of_pci_range >> >> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni >> <gabriele.paoloni@huawei.com> wrote: >>>> -----Original Message----- >>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >>>> Sent: 30 July 2015 18:15 >>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: >>>>>> -----Original Message----- >>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>>>>> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas >>>>>> Sent: Thursday, July 30, 2015 5:15 PM >>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: >> >> [...] >> >>>>>>> I don’t think we should rely on [CPU] addresses...what if the >>>>>> intermediate >>>>>>> translation layer changes the lower significant bits of the >> "bus >>>>>> address" >>>>>>> to translate into a cpu address? >>>>>> >>>>>> Is it really a possiblity that the lower bits could be changed? >>>>> >>>>> I've checked all the current deignware users DTs except "pci- >>>> layerscape" >>>>> that I could not find: >>>>> spear1310.dtsi >>>>> spear1340.dtsi >>>>> dra7.dtsi >>>>> imx6qdl.dtsi >>>>> imx6sx.dtsi >>>>> keystone.dtsi >>>>> exynos5440.dtsi >>>>> >>>>> None of them modifies the lower bits. To be more precise the only >> guy >>>>> that provides another translation layer is "dra7.dtsi": >>>>> axi0 >>>>> http://lxr.free- >> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 >>>>> >>>>> axi1 >>>>> http://lxr.free- >> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 >>>>> >>>>> For this case masking the top 4bits (bits28 to 31) should make the >> job. >> >> IMO, we should just fix this case. After further study, I don't think >> this is a DW issue, but rather an SOC integration issue. >> >> I believe you can just fixup the address in the pp->ops->host_init hook. >> > > Yes I guess that I could just assign pp->(*)_mod_base to the CPU address > in DW and mask it out in dra7xx_pcie_host_init()... > > Kishon, would you be ok with that? Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) had this discussion [1] before we decided the current approach. It'll be good to check with Arnd too. [1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/253528.html Thanks Kishon
> -----Original Message----- > From: Kishon Vijay Abraham I [mailto:kishon@ti.com] > Sent: Friday, July 31, 2015 3:57 PM > To: Gabriele Paoloni; Rob Herring > Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou > (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand; Arnd > Bergmann; Arnd Bergmann > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > +Arnd > > Hi, > > On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote: > > [+cc Kishon] > > > >> -----Original Message----- > >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > >> owner@vger.kernel.org] On Behalf Of Rob Herring > >> Sent: Thursday, July 30, 2015 9:42 PM > >> To: Gabriele Paoloni > >> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; > Wangzhou > >> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; > >> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; > >> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > >> of_pci_range > >> > >> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni > >> <gabriele.paoloni@huawei.com> wrote: > >>>> -----Original Message----- > >>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com] > >>>> Sent: 30 July 2015 18:15 > >>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: > >>>>>> -----Original Message----- > >>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > >>>>>> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > >>>>>> Sent: Thursday, July 30, 2015 5:15 PM > >>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: > >> > >> [...] > >> > >>>>>>> I don’t think we should rely on [CPU] addresses...what if the > >>>>>> intermediate > >>>>>>> translation layer changes the lower significant bits of the > >> "bus > >>>>>> address" > >>>>>>> to translate into a cpu address? > >>>>>> > >>>>>> Is it really a possiblity that the lower bits could be changed? > >>>>> > >>>>> I've checked all the current deignware users DTs except "pci- > >>>> layerscape" > >>>>> that I could not find: > >>>>> spear1310.dtsi > >>>>> spear1340.dtsi > >>>>> dra7.dtsi > >>>>> imx6qdl.dtsi > >>>>> imx6sx.dtsi > >>>>> keystone.dtsi > >>>>> exynos5440.dtsi > >>>>> > >>>>> None of them modifies the lower bits. To be more precise the only > >> guy > >>>>> that provides another translation layer is "dra7.dtsi": > >>>>> axi0 > >>>>> http://lxr.free- > >> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 > >>>>> > >>>>> axi1 > >>>>> http://lxr.free- > >> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 > >>>>> > >>>>> For this case masking the top 4bits (bits28 to 31) should make > the > >> job. > >> > >> IMO, we should just fix this case. After further study, I don't > think > >> this is a DW issue, but rather an SOC integration issue. > >> > >> I believe you can just fixup the address in the pp->ops->host_init > hook. > >> > > > > Yes I guess that I could just assign pp->(*)_mod_base to the CPU > address > > in DW and mask it out in dra7xx_pcie_host_init()... > > > > Kishon, would you be ok with that? > > Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) > had > this discussion [1] before we decided the current approach. It'll be > good to > check with Arnd too. > > [1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014- > May/253528.html In this patch you use the mask into designware....instead the approach proposed by Rob means to have the mask declared in the dra7 driver and you modified the pp members in dra7xx_pcie_host_init by masking them... BTW good to have Arnd opinion too.. > > Thanks > Kishon
On Fri, Jul 31, 2015 at 9:57 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote: > +Arnd > > Hi, > > On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote: >> [+cc Kishon] >> >>> -----Original Message----- >>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>> owner@vger.kernel.org] On Behalf Of Rob Herring >>> Sent: Thursday, July 30, 2015 9:42 PM >>> To: Gabriele Paoloni >>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou >>> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; >>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>> devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; >>> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand >>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >>> of_pci_range >>> >>> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni >>> <gabriele.paoloni@huawei.com> wrote: >>>>> -----Original Message----- >>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >>>>> Sent: 30 July 2015 18:15 >>>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: >>>>>>> -----Original Message----- >>>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>>>>>> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas >>>>>>> Sent: Thursday, July 30, 2015 5:15 PM >>>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: >>> >>> [...] >>> >>>>>>>> I don’t think we should rely on [CPU] addresses...what if the >>>>>>> intermediate >>>>>>>> translation layer changes the lower significant bits of the >>> "bus >>>>>>> address" >>>>>>>> to translate into a cpu address? >>>>>>> >>>>>>> Is it really a possiblity that the lower bits could be changed? >>>>>> >>>>>> I've checked all the current deignware users DTs except "pci- >>>>> layerscape" >>>>>> that I could not find: >>>>>> spear1310.dtsi >>>>>> spear1340.dtsi >>>>>> dra7.dtsi >>>>>> imx6qdl.dtsi >>>>>> imx6sx.dtsi >>>>>> keystone.dtsi >>>>>> exynos5440.dtsi >>>>>> >>>>>> None of them modifies the lower bits. To be more precise the only >>> guy >>>>>> that provides another translation layer is "dra7.dtsi": >>>>>> axi0 >>>>>> http://lxr.free- >>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 >>>>>> >>>>>> axi1 >>>>>> http://lxr.free- >>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 >>>>>> >>>>>> For this case masking the top 4bits (bits28 to 31) should make the >>> job. >>> >>> IMO, we should just fix this case. After further study, I don't think >>> this is a DW issue, but rather an SOC integration issue. >>> >>> I believe you can just fixup the address in the pp->ops->host_init hook. >>> >> >> Yes I guess that I could just assign pp->(*)_mod_base to the CPU address >> in DW and mask it out in dra7xx_pcie_host_init()... >> >> Kishon, would you be ok with that? > > Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) had > this discussion [1] before we decided the current approach. It'll be good to > check with Arnd too. > > [1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/253528.html The problem I have here is the use of ranges does not necessarily mean fewer address bits are available. It can be used just for convenience of not putting the full address into every node's reg property. And vice versa, there are probably plenty of cases where we have the full address in the nodes, but really only some of the address bits are decoded at the IP block. Whether the address bits are present is rarely cared about or known by s/w folks until you hit a problem like this. Given this is an isolated case ATM, I would fix it in an isolated way. It does not affect the binding and could be changed in the kernel later if this becomes a common need. Rob
On 2015. 8. 1., at AM 12:09, Gabriele Paoloni <gabriele.paoloni@huawei.com> wrote: > >> -----Original Message----- >> From: Kishon Vijay Abraham I [mailto:kishon@ti.com] >> Sent: Friday, July 31, 2015 3:57 PM >> To: Gabriele Paoloni; Rob Herring >> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou >> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; >> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; >> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand; Arnd >> Bergmann; Arnd Bergmann >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >> of_pci_range >> >> +Arnd >> >> Hi, >> >>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote: >>> [+cc Kishon] >>> >>>> -----Original Message----- >>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>>> owner@vger.kernel.org] On Behalf Of Rob Herring >>>> Sent: Thursday, July 30, 2015 9:42 PM >>>> To: Gabriele Paoloni >>>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com; >> Wangzhou >>>> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com; >>>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>> devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; >>>> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand >>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >>>> of_pci_range >>>> >>>> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni >>>> <gabriele.paoloni@huawei.com> wrote: >>>>>> -----Original Message----- >>>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >>>>>> Sent: 30 July 2015 18:15 >>>>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote: >>>>>>>> -----Original Message----- >>>>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>>>>>>> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas >>>>>>>> Sent: Thursday, July 30, 2015 5:15 PM >>>>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote: >>>> >>>> [...] >>>> >>>>>>>>> I don’t think we should rely on [CPU] addresses...what if the >>>>>>>> intermediate >>>>>>>>> translation layer changes the lower significant bits of the >>>> "bus >>>>>>>> address" >>>>>>>>> to translate into a cpu address? >>>>>>>> >>>>>>>> Is it really a possiblity that the lower bits could be changed? >>>>>>> >>>>>>> I've checked all the current deignware users DTs except "pci- >>>>>> layerscape" >>>>>>> that I could not find: >>>>>>> spear1310.dtsi >>>>>>> spear1340.dtsi >>>>>>> dra7.dtsi >>>>>>> imx6qdl.dtsi >>>>>>> imx6sx.dtsi >>>>>>> keystone.dtsi >>>>>>> exynos5440.dtsi >>>>>>> >>>>>>> None of them modifies the lower bits. To be more precise the only >>>> guy >>>>>>> that provides another translation layer is "dra7.dtsi": >>>>>>> axi0 >>>>>>> http://lxr.free- >>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207 >>>>>>> >>>>>>> axi1 >>>>>>> http://lxr.free- >>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241 >>>>>>> >>>>>>> For this case masking the top 4bits (bits28 to 31) should make >> the >>>> job. >>>> >>>> IMO, we should just fix this case. After further study, I don't >> think >>>> this is a DW issue, but rather an SOC integration issue. >>>> >>>> I believe you can just fixup the address in the pp->ops->host_init >> hook. Yep, it is SOC specific code for dra7. This is NOT a DW issue. >>> >>> Yes I guess that I could just assign pp->(*)_mod_base to the CPU >> address >>> in DW and mask it out in dra7xx_pcie_host_init()... >>> >>> Kishon, would you be ok with that? >> >> Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) >> had >> this discussion [1] before we decided the current approach. It'll be >> good to >> check with Arnd too. >> >> [1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014- >> May/253528.html > > > In this patch you use the mask into designware....instead the approach > proposed by Rob means to have the mask declared in the dra7 driver and > you modified the pp members in dra7xx_pcie_host_init by masking them... I want to move that code to dra7 driver, because that code is dra7-specific. Best regards, Jingoo Han > BTW good to have Arnd opinion too.. >> >> Thanks >> Kishon
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; };