| Message ID | 1438010223-124422-1-git-send-email-gabriele.paoloni@huawei.com |
|---|---|
| State | Superseded |
| 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 -- 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 -- 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; > > > }; -- 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 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 > > . > -- 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; > > > > }; -- 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
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 -- 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 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. -- 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 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. -- 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
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogbGludXgtcGNpLW93bmVy QHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS0NCj4gb3duZXJAdmdlci5rZXJuZWwu b3JnXSBPbiBCZWhhbGYgT2YgQmpvcm4gSGVsZ2Fhcw0KPiBTZW50OiBUaHVyc2RheSwgSnVseSAz MCwgMjAxNSA1OjE1IFBNDQo+IFRvOiBHYWJyaWVsZSBQYW9sb25pDQo+IENjOiBSb2IgSGVycmlu ZzsgYXJuZEBhcm5kYi5kZTsgbG9yZW56by5waWVyYWxpc2lAYXJtLmNvbTsgV2FuZ3pob3UgKEIp Ow0KPiByb2JoK2R0QGtlcm5lbC5vcmc7IGphbWVzLm1vcnNlQGFybS5jb207IExpdml1LkR1ZGF1 QGFybS5jb207IGxpbnV4LQ0KPiBwY2lAdmdlci5rZXJuZWwub3JnOyBsaW51eC1hcm0ta2VybmVs QGxpc3RzLmluZnJhZGVhZC5vcmc7DQo+IGRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOyBZdWFu emhpY2hhbmc7IFpodWRhY2FpOyB6aGFuZ2p1a3VvOw0KPiBxaXV6aGVuZmE7IExpZ3Vvemh1IChL ZW5uZXRoKQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHY2XSBQQ0k6IFN0b3JlIFBDSWUgYnVzIGFk ZHJlc3MgaW4gc3RydWN0DQo+IG9mX3BjaV9yYW5nZQ0KPiANCj4gT24gVGh1LCBKdWwgMzAsIDIw MTUgYXQgMDE6NTI6MTNQTSArMDAwMCwgR2FicmllbGUgUGFvbG9uaSB3cm90ZToNCj4gPg0KPiA+ DQo+ID4gPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+ID4gRnJvbTogbGludXgtcGNp LW93bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS0NCj4gPiA+IG93bmVyQHZn ZXIua2VybmVsLm9yZ10gT24gQmVoYWxmIE9mIFJvYiBIZXJyaW5nDQo+ID4gPiBTZW50OiBUaHVy c2RheSwgSnVseSAzMCwgMjAxNSAyOjQzIFBNDQo+ID4gPiBUbzogR2FicmllbGUgUGFvbG9uaQ0K PiA+ID4gQ2M6IEJqb3JuIEhlbGdhYXM7IGFybmRAYXJuZGIuZGU7IGxvcmVuem8ucGllcmFsaXNp QGFybS5jb207DQo+IFdhbmd6aG91DQo+ID4gPiAoQik7IHJvYmgrZHRAa2VybmVsLm9yZzsgamFt ZXMubW9yc2VAYXJtLmNvbTsgTGl2aXUuRHVkYXVAYXJtLmNvbTsNCj4gPiA+IGxpbnV4LXBjaUB2 Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsNCj4g PiA+IGRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOyBZdWFuemhpY2hhbmc7IFpodWRhY2FpOyB6 aGFuZ2p1a3VvOw0KPiA+ID4gcWl1emhlbmZhOyBMaWd1b3podSAoS2VubmV0aCkNCj4gPiA+IFN1 YmplY3Q6IFJlOiBbUEFUQ0ggdjZdIFBDSTogU3RvcmUgUENJZSBidXMgYWRkcmVzcyBpbiBzdHJ1 Y3QNCj4gPiA+IG9mX3BjaV9yYW5nZQ0KPiA+ID4NCj4gPiA+IE9uIFdlZCwgSnVsIDI5LCAyMDE1 IGF0IDI6NDQgUE0sIEdhYnJpZWxlIFBhb2xvbmkNCj4gPiA+IDxnYWJyaWVsZS5wYW9sb25pQGh1 YXdlaS5jb20+IHdyb3RlOg0KPiA+ID4gPiBIaSBCam9ybg0KPiA+ID4gPg0KPiA+ID4gPiBNYW55 IFRoYW5rcyBmb3IgeW91ciByZXBseQ0KPiA+ID4gPg0KPiA+ID4gPiBJIGhhdmUgY29tbWVudGVk IGJhY2sgaW5saW5lIHdpdGggcmVzb2x1dGlvbnMgZnJvbSBteSBzaWRlLg0KPiA+ID4gPg0KPiA+ ID4gPiBJZiB5b3UncmUgb2sgd2l0aCB0aGVtIEknbGwgc2VuZCBpdCBvdXQgYSBuZXcgdmVyc2lv biBpbiB0aGUNCj4gPiA+IGFwcHJvcHJpYXRlIHBhdGNoc2V0DQo+ID4gPiA+DQo+ID4gPiA+IENo ZWVycw0KPiA+ID4gPg0KPiA+ID4gPiBHYWINCj4gPiA+ID4NCj4gPiA+ID4+IC0tLS0tT3JpZ2lu YWwgTWVzc2FnZS0tLS0tDQo+ID4gPiA+PiBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWlsdG86Ymhl bGdhYXNAZ29vZ2xlLmNvbV0NCj4gPiA+ID4+IFNlbnQ6IFdlZG5lc2RheSwgSnVseSAyOSwgMjAx NSA2OjIxIFBNDQo+ID4gPiA+PiBUbzogR2FicmllbGUgUGFvbG9uaQ0KPiA+ID4gPj4gQ2M6IGFy bmRAYXJuZGIuZGU7IGxvcmVuem8ucGllcmFsaXNpQGFybS5jb207IFdhbmd6aG91IChCKTsNCj4g PiA+ID4+IHJvYmgrZHRAa2VybmVsLm9yZzsgamFtZXMubW9yc2VAYXJtLmNvbTsgTGl2aXUuRHVk YXVAYXJtLmNvbTsNCj4gbGludXgtDQo+ID4gPiA+PiBwY2lAdmdlci5rZXJuZWwub3JnOyBsaW51 eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmc7DQo+ID4gPiA+PiBkZXZpY2V0cmVlQHZn ZXIua2VybmVsLm9yZzsgWXVhbnpoaWNoYW5nOyBaaHVkYWNhaTsgemhhbmdqdWt1bzsNCj4gPiA+ ID4+IHFpdXpoZW5mYTsgTGlndW96aHUgKEtlbm5ldGgpDQo+ID4gPiA+PiBTdWJqZWN0OiBSZTog W1BBVENIIHY2XSBQQ0k6IFN0b3JlIFBDSWUgYnVzIGFkZHJlc3MgaW4gc3RydWN0DQo+ID4gPiA+ PiBvZl9wY2lfcmFuZ2UNCj4gPiA+ID4+DQo+ID4gPiA+PiBIaSBHYWJyaWVsZSwNCj4gPiA+ID4+ DQo+ID4gPiA+PiBBcyBmYXIgYXMgSSBjYW4gdGVsbCwgdGhpcyBpcyBub3Qgc3BlY2lmaWMgdG8g UENJZSwgc28gcGxlYXNlDQo+IHVzZQ0KPiA+ID4gIlBDSSINCj4gPiA+ID4+IGluDQo+ID4gPiA+ PiB0aGUgc3ViamVjdCBhcyBhIGdlbmVyaWMgdGVybSB0aGF0IGluY2x1ZGVzIGJvdGggUENJIGFu ZCBQQ0llLg0KPiA+ID4gPg0KPiA+ID4gPiBzdXJlIGFncmVlZA0KPiA+ID4gPg0KPiA+ID4gPj4N Cj4gPiA+ID4+IE9uIE1vbiwgSnVsIDI3LCAyMDE1IGF0IDExOjE3OjAzUE0gKzA4MDAsIEdhYnJp ZWxlIFBhb2xvbmkgd3JvdGU6DQo+ID4gPiA+PiA+IEZyb206IGdhYnJpZWxlIHBhb2xvbmkgPGdh YnJpZWxlLnBhb2xvbmlAaHVhd2VpLmNvbT4NCj4gPiA+ID4+ID4NCj4gPiA+ID4+ID4gICAgIFRo aXMgcGF0Y2ggaXMgbmVlZGVkIHBvcnQgUENJZSBkZXNpZ253YXJlIHRvIG5ldyBEVA0KPiBwYXJz aW5nDQo+ID4gPiBBUEkNCj4gPiA+ID4+ID4gICAgIEFzIGRpc2N1c3NlZCBpbg0KPiA+ID4gPj4g PiAgICAgaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvcGlwZXJtYWlsL2xpbnV4LWFybS0NCj4g a2VybmVsLzIwMTUtDQo+ID4gPiA+PiBKYW51YXJ5LzMxNzc0My5odG1sDQo+ID4gPiA+PiA+ICAg ICBpbiBkZXNpZ253YXJlIHdlIGhhdmUgYSBwcm9ibGVtIGFzIHRoZSBQQ0kgYWRkcmVzc2VzIGlu DQo+IHRoZQ0KPiA+ID4gUENJZQ0KPiA+ID4gPj4gY29udHJvbGxlcg0KPiA+ID4gPj4gPiAgICAg YWRkcmVzcyBzcGFjZSBhcmUgcmVxdWlyZWQgaW4gb3JkZXIgdG8gcGVyZm9ybSBjb3JyZWN0IEhX DQo+ID4gPiA+PiBvcGVyYXRpb24uDQo+ID4gPiA+PiA+DQo+ID4gPiA+PiA+ICAgICBJbiBvcmRl ciB0byBzb2x2ZSB0aGlzIHByb2JsZW0gY29tbWl0IGY0YzU1YzVhMyAiUENJOg0KPiA+ID4gZGVz aWdud2FyZToNCj4gPiA+ID4+ID4gICAgIFByb2dyYW0gQVRVIHdpdGggdW50cmFuc2xhdGVkIGFk ZHJlc3MiIGFkZGVkIGNvZGUgdG8gcmVhZA0KPiB0aGUNCj4gPiA+ID4+IFBDSWUNCj4gPiA+ID4+ DQo+ID4gPiA+PiBDb252ZW50aW9uYWwgcmVmZXJlbmNlIGlzIDEyLWNoYXIgU0hBMSwgbGlrZSB0 aGlzOg0KPiA+ID4gPj4NCj4gPiA+ID4+ICAgZjRjNTVjNWEzZjdmICgiUENJOiBkZXNpZ253YXJl OiBQcm9ncmFtIEFUVSB3aXRoIHVudHJhbnNsYXRlZA0KPiA+ID4gPj4gYWRkcmVzcyIpDQo+ID4g PiA+DQo+ID4gPiA+IEFncmVlZCwgd2lsbCBjaGFuZ2UgdGhpcw0KPiA+ID4gPg0KPiA+ID4gPj4N Cj4gPiA+ID4+ID4gICAgIGNvbnRyb2xsZXIgc3RhcnQgYWRkcmVzcyBkaXJlY3RseSBmcm9tIHRo ZSBEVCByYW5nZXMuDQo+ID4gPiA+PiA+DQo+ID4gPiA+PiA+ICAgICBJbiB0aGUgbmV3IERUIHBh cnNpbmcgQVBJDQo+IG9mX3BjaV9nZXRfaG9zdF9icmlkZ2VfcmVzb3VyY2VzKCkNCj4gPiA+ID4+ IGhpZGVzIHRoZQ0KPiA+ID4gPj4gPiAgICAgRFQgcGFyc2VyIGZyb20gdGhlIGhvc3QgY29udHJv bGxlciBkcml2ZXJzLCBzbyBpdCBpcyBub3QNCj4gPiA+IHBvc3NpYmxlDQo+ID4gPiA+PiA+ICAg ICBmb3IgZHJpdmVycyB0byBwYXJzZSB2YWx1ZXMgZGlyZWN0bHkgZnJvbSB0aGUgRFQuDQo+ID4g PiA+PiA+DQo+ID4gPiA+PiA+ICAgICBJbiBodHRwOi8vd3d3LnNwaW5pY3MubmV0L2xpc3RzL2xp bnV4LXBjaS9tc2c0MjU0MC5odG1sIHdlDQo+ID4gPiA+PiBhbHJlYWR5IHRyaWVkDQo+ID4gPiA+ PiA+ICAgICB0byB1c2UgdGhlIG5ldyBEVCBwYXJzaW5nIEFQSSBidXQgdGhlcmUgaXMgYSBidWcN Cj4gKG9idmlvdXNseSkNCj4gPiA+IGluDQo+ID4gPiA+PiBzZXR0aW5nDQo+ID4gPiA+PiA+ICAg ICB0aGUgPCo+X21vZF9iYXNlIGFkZHJlc3Nlcw0KPiA+ID4gPj4gPiAgICAgQXBwbHlpbmcgdGhp cyBwYXRjaCB3ZSBjYW4gZWFzaWx5IHNldCAiPCo+X21vZF9iYXNlID0gd2luLQ0KPiA+ID4gPj4g Pl9fcmVzLnN0YXJ0Ig0KPiA+ID4gPj4NCj4gPiA+ID4+IEJ5IGl0c2VsZiwgdGhpcyBwYXRjaCBh ZGRzIHNvbWV0aGluZy4gIEl0IHdvdWxkIGhlbHAgbWUNCj4gdW5kZXJzdGFuZA0KPiA+ID4gaXQN Cj4gPiA+ID4+IGlmDQo+ID4gPiA+PiB0aGUgKnVzZXIqIG9mIHRoaXMgbmV3IHNvbWV0aGluZyB3 ZXJlIGluIHRoZSBzYW1lIHBhdGNoIHNlcmllcy4NCj4gPiA+ID4NCj4gPiA+ID4gdGhlIHVzZXIg aXM6ICJbUEFUQ0ggdjUgMi81XSBQQ0k6IGRlc2lnbndhcmU6IEFkZCBBUk02NCBzdXBwb3J0Ig0K PiA+ID4gPiBJIHdpbGwgYXNrIFpob3UgV2FuZyB0byBpbmNsdWRlIHRoaXMgcGF0Y2ggaW4gaGlz IHBhdGNoc2V0DQo+ID4gPiA+DQo+ID4gPiA+DQo+ID4gPiA+Pg0KPiA+ID4gPj4gPiAgICAgVGhp cyBwYXRjaCBhZGRzIGEgbmV3IGZpZWxkIGluICJzdHJ1Y3Qgb2ZfcGNpX3JhbmdlIiB0bw0KPiBz dG9yZQ0KPiA+ID4gdGhlDQo+ID4gPiA+PiA+ICAgICBwY2kgYnVzIHN0YXJ0IGFkZHJlc3M7IGl0 IGZpbGxzIHRoZSBmaWVsZCBpbg0KPiA+ID4gPj4gb2ZfcGNpX3JhbmdlX3BhcnNlcl9vbmUoKTsN Cj4gPiA+ID4+ID4gICAgIGluIG9mX3BjaV9nZXRfaG9zdF9icmlkZ2VfcmVzb3VyY2VzKCkgaXQg cmV0cmlldmVzIHRoZQ0KPiA+ID4gcmVzb3VyY2UNCj4gPiA+ID4+IGVudHJ5DQo+ID4gPiA+PiA+ ICAgICBhZnRlciBpdCBpcyBjcmVhdGVkIGFuZCBhZGRlZCB0byB0aGUgcmVzb3VyY2UgbGlzdCBh bmQNCj4gdXNlcw0KPiA+ID4gPj4gPiAgICAgZW50cnktPl9fcmVzLnN0YXJ0IHRvIHN0b3JlIHRo ZSBwY2kgY29udHJvbGxlciBhZGRyZXNzDQo+ID4gPiA+Pg0KPiA+ID4gPj4gc3RydWN0IG9mX3Bj aV9yYW5nZSBpcyBzdGFydGluZyB0byBnZXQgY29uZnVzaW5nIHRvIG5vbi1PRiBmb2xrcw0KPiA+ ID4gbGlrZQ0KPiA+ID4gPj4gbWUuDQo+ID4gPiA+PiBJdCBub3cgY29udGFpbnM6DQo+ID4gPiA+ Pg0KPiA+ID4gPj4gICB1MzIgcGNpX3NwYWNlOw0KPiA+ID4gPj4gICB1NjQgcGNpX2FkZHI7DQo+ ID4gPiA+PiAgIHU2NCBjcHVfYWRkcjsNCj4gPiA+ID4+ICAgdTY0IGJ1c19hZGRyOw0KPiA+ID4g Pj4NCj4gPiA+ID4+IENhbiB5b3UgZXhwbGFpbiB3aGF0IGFsbCB0aGVzZSB0aGluZ3MgbWVhbiwg YW5kIG1heWJlIGV2ZW4gYWRkDQo+IG9uZS0NCj4gPiA+IGxpbmUNCj4gPiA+ID4+IGNvbW1lbnRz IHRvIHRoZSBzdHJ1Y3R1cmU/DQo+ID4gPiA+DQo+ID4gPiA+IHN1cmUgSSBjYW4gYWRkIGNvbW1l bnRzIGlubGluZSBpbiB0aGUgY29kZQ0KPiA+ID4gPg0KPiA+ID4gPj4NCj4gPiA+ID4+IHBjaV9z cGFjZTogVGhlIG9ubHkgdXNlcyBJIHNlZSBhcmUgdG8gZGV0ZXJtaW5lIHdoZXRoZXIgdG8gcHJp bnQNCj4gPiA+ID4+ICJQcmVmZXRjaCIuICBJIGRvbid0IHNlZSBhbnkgcmVhbCBmdW5jdGlvbmFs aXR5IHRoYXQgdXNlcyB0aGlzLg0KPiA+ID4gPg0KPiA+ID4gPiBMb29raW5nIGF0IHRoZSBjb2Rl IEkgYWdyZWUuIGl0J3Mgc2VlbXMgdG8gYmUgdXNlZCBvbmx5IGluDQo+IHBvd2VycGMNCj4gPiA+ ID4gYW5kIG1pY3JvYmxhemUgdG8gcHJpbnQgb3V0Lg0KPiA+ID4gPiBIb3dldmVyIGZyb20gbXkg dW5kZXJzdGFuZGluZyBwY2lfc3BhY2UgaXMgdGhlIHBoeXMuaGkgZmllbGQgb2YNCj4gdGhlDQo+ ID4gPiA+IHJhbmdlcyBwcm9wZXJ0eTogaXQgZGVmaW5lcyB0aGUgcHJvcGVydGllcyBvZiB0aGUg YWRkcmVzcyBzcGFjZQ0KPiA+ID4gYXNzb2NpYXRlZA0KPiA+ID4gPiB0byB0aGUgUENJIGFkZHJl c3MuIGlmIHlvdSdyZSBjdXJpb3VzIHlvdSBjYW4gZmluZCBhIG5pY2UgYW5kDQo+IHF1aWNrDQo+ ID4gPiB0byByZWFkDQo+ID4gPiA+ICJndWlkZSIgaW4gaHR0cDovL2RldmljZXRyZWUub3JnL01Q QzUyMDA6UENJDQo+ID4gPiA+DQo+ID4gPiA+Pg0KPiA+ID4gPj4gcGNpX2FkZHI6IEkgYXNzdW1l IHRoaXMgaXMgYSBQQ0kgYnVzIGFkZHJlc3MsIGxpa2Ugd2hhdCB5b3UNCj4gd291bGQNCj4gPiA+ IHNlZQ0KPiA+ID4gPj4gaWYNCj4gPiA+ID4+IHlvdSBwdXQgYW4gYW5hbHl6ZXIgb24gdGhlIGJ1 cy9saW5rLiAgVGhpcyBhZGRyZXNzIGNvdWxkIGdvIGluIGENCj4gQkFSLg0KPiA+ID4gPg0KPiA+ ID4gPiBZZXMsIHRoaXMgaXMgdGhlIFBDSSBzdGFydCBhZGRyZXNzIG9mIHRoZSByYW5nZTogcGh5 cy5taWQgKw0KPiBwaHlzLmxvdw0KPiA+ID4gaW4gdGhlDQo+ID4gPiA+IGd1aWRlIG1lbnRpb25l ZCBhYm92ZQ0KPiA+ID4gPg0KPiA+ID4gPj4NCj4gPiA+ID4+IGNwdV9hZGRyOiBJIGFzc3VtZSB0 aGlzIGlzIGEgQ1BVIHBoeXNpY2FsIGFkZHJlc3MsIGxpa2Ugd2hhdCB5b3UNCj4gPiA+IHdvdWxk DQo+ID4gPiA+PiBzZWUNCj4gPiA+ID4+IGluIC9wcm9jL2lvbWVtIGFuZCB3aGF0IHlvdSB3b3Vs ZCBwYXNzIHRvIGlvcmVtYXAoKS4NCj4gPiA+ID4NCj4gPiA+ID4gWWVzIGNvcnJlY3QNCj4gPiA+ ID4NCj4gPiA+ID4+DQo+ID4gPiA+PiBidXNfYWRkcjogPw0KPiA+ID4gPj4NCj4gPiA+ID4NCj4g PiA+ID4gQWNjb3JkaW5nIHRvIHRoZSBndWlkZSBhYm92ZSwgdGhpcyBpcyB0aGUgYWRkcmVzcyBp bnRvIHdoaWNoIHRoZQ0KPiA+ID4gcGNpX2FkZHJlc3MNCj4gPiA+ID4gZ2V0IHRyYW5zbGF0ZWQg dG8gYW5kIHRoYXQgaXMgcGFzc2VkIHRvIHRoZSByb290IGNvbXBsZXguIEJldHdlZW4NCj4gdGhl DQo+ID4gPiByb290DQo+ID4gPiA+IGNvbXBsZXggYW5kIHRoZSBDUFUgdGhlcmUgY2FuIGJlIGlu dGVybWVkaWF0ZSB0cmFuc2xhdGlvbiBsYXllcnM6DQo+IHNlZQ0KPiA+ID4gdGhhdCB0bw0KPiA+ ID4gPiBnZXQgcGNpX2FkZHJlc3Mgd2UgY2FsbCAib2ZfdHJhbnNsYXRlX2FkZHJlc3MiOyB0aGlz IHdpbGwgYXBwbHkNCj4gYWxsDQo+ID4gPiB0aGUNCj4gPiA+ID4gdHJhbnNsYXRpb24gbGF5ZXJz IChyYW5nZXMgaW4gdGhlIERUKSB0aGF0IGl0IGZpbmRzIHRpbGwgaXQgY29tZXMNCj4gdG8NCj4g PiA+IHRoZSByb290DQo+ID4gPiA+IG5vZGUgb2YgdGhlIERUICh0aHVzIHJldHJpZXZpbmcgdGhl IENQVSBhZGRyZXNzKS4NCj4gPiA+ID4gTm93IHNhaWQgdGhhdCwgZm9yIGRlc2lnbndhcmUgd2Ug bmVlZCB0aGUgZmlyc3QgdHJhbnNsYXRlZCBQQ0kNCj4gPiA+IGFkZHJlc3MsIHRoYXQgd2UgY2Fs bA0KPiA+ID4NCj4gPiA+IEkgdGhpbmsgeW91IG1lYW4gInRyYW5zbGF0ZWQgQ1BVIGFkZHJlc3Mu IiBUaGUgZmxvdyBvZiBhZGRyZXNzZXMNCj4gbG9va3MNCj4gPiA+IGxpa2UgdGhpczoNCj4gPiA+ DQo+ID4gPiBDUFUgLT4gQ1BVIGJ1cyBhZGRyZXNzIC0+IGJ1cyBmYWJyaWMgYWRkcmVzcyBkZWNv ZGluZyAtPiBidXMNCj4gYWRkcmVzcw0KPiA+ID4gLT4gRFcgUENJIC0+IFBDSSBhZGRyZXNzDQo+ ID4gPg0KPiA+ID4gVGhpcyBpcyBxdWl0ZSBjb21tb24gdGhhdCBhbiBJUCBibG9jayBkb2VzIG5v dCBzZWUgdGhlIGZ1bGwgYWRkcmVzcy4NCj4gPiA+IEl0IGlzIHVudXN1YWwgdGhhdCB0aGUgSVAg YmxvY2sgbmVlZHMgdG8ga25vdyBpdHMgZnVsbCBhZGRyZXNzIG9uDQo+IHRoZQ0KPiA+ID4gc2xh dmUgc2lkZS4gSXQgaXMgcXVpdGUgY29tbW9uIGZvciB0aGUgbWFzdGVyIHNpZGUgYW5kIHRoZSBr ZXJuZWwNCj4gPiA+IGRlYWxzIHdpdGggdGhhdCBhbGwgdGhlIHRpbWUgd2l0aCBETUEgbWFwcGlu Zw0KPiA+ID4NCj4gPiA+ID4gaGVyZSBidXNfYWRkciBhZnRlciBSb2IgSGVycmluZyBzdWdnZXN0 ZWQgdGhlIG5hbWUuLi5ob25lc3RseSBJDQo+ID4gPiBjYW5ub3QgdGhpbmsgb2YgYQ0KPiA+ID4g PiBkaWZmZXJlbnQgbmFtZQ0KPiA+ID4NCj4gPiA+IFRoaW5raW5nIGFib3V0IHRoaXMgc29tZSBt b3JlLCBpcyB0aGlzIHJlYWxseSBhIHRyYW5zbGF0aW9uIHZlcnN1cw0KPiA+ID4ganVzdCBhIHN0 cmlwcGluZyBvZiBoaWdoIGJpdHM/IERvZXMgdGhlIERXIElQIGhhdmUgbGVzcyB0aGFuIDMyLQ0K PiBiaXRzDQo+ID4gPiBhZGRyZXNzPyBJZiBzbywgd2UgY291bGQgZXhwcmVzcyBkaWZmZXJlbnRs eSBhbmQganVzdCBtYXNrIHRoZSBDUFUNCj4gPiA+IGFkZHJlc3Mgd2l0aGluIHRoZSBkcml2ZXIg aW5zdGVhZC4NCj4gPg0KPiA+IEkgZG9u4oCZdCB0aGluayB3ZSBzaG91bGQgcmVseSBvbiBbQ1BV XSBhZGRyZXNzZXMuLi53aGF0IGlmIHRoZQ0KPiBpbnRlcm1lZGlhdGUNCj4gPiB0cmFuc2xhdGlv biBsYXllciBjaGFuZ2VzIHRoZSBsb3dlciBzaWduaWZpY2FudCBiaXRzIG9mIHRoZSAiYnVzDQo+ IGFkZHJlc3MiDQo+ID4gdG8gdHJhbnNsYXRlIGludG8gYSBjcHUgYWRkcmVzcz8NCj4gDQo+IElz IGl0IHJlYWxseSBhIHBvc3NpYmxpdHkgdGhhdCB0aGUgbG93ZXIgYml0cyBjb3VsZCBiZSBjaGFu Z2VkPw0KDQpJJ3ZlIGNoZWNrZWQgYWxsIHRoZSBjdXJyZW50IGRlaWdud2FyZSB1c2VycyBEVHMg ZXhjZXB0ICJwY2ktbGF5ZXJzY2FwZSIgDQp0aGF0IEkgY291bGQgbm90IGZpbmQ6DQpzcGVhcjEz MTAuZHRzaQ0Kc3BlYXIxMzQwLmR0c2kNCmRyYTcuZHRzaQ0KaW14NnFkbC5kdHNpDQppbXg2c3gu ZHRzaQ0Ka2V5c3RvbmUuZHRzaQ0KZXh5bm9zNTQ0MC5kdHNpDQoNCk5vbmUgb2YgdGhlbSBtb2Rp ZmllcyB0aGUgbG93ZXIgYml0cy4gVG8gYmUgbW9yZSBwcmVjaXNlIHRoZSBvbmx5IGd1eQ0KdGhh dCBwcm92aWRlcyBhbm90aGVyIHRyYW5zbGF0aW9uIGxheWVyIGlzICJkcmE3LmR0c2kiOg0KYXhp MA0KaHR0cDovL2x4ci5mcmVlLWVsZWN0cm9ucy5jb20vc291cmNlL2FyY2gvYXJtL2Jvb3QvZHRz L2RyYTcuZHRzaSNMMjA3DQoNCmF4aTENCmh0dHA6Ly9seHIuZnJlZS1lbGVjdHJvbnMuY29tL3Nv dXJjZS9hcmNoL2FybS9ib290L2R0cy9kcmE3LmR0c2kjTDI0MQ0KDQpGb3IgdGhpcyBjYXNlIG1h c2tpbmcgdGhlIHRvcCA0Yml0cyAoYml0czI4IHRvIDMxKSBzaG91bGQgbWFrZSB0aGUgam9iLg0K DQpTcGVha2luZyBpbiBnZW5lcmFsIHRlcm1zIHNvIGZhciBJJ3ZlIGFsd2F5cyBzZWVuIGxpbmVh ciBtYXBwaW5ncw0KdGhhdCBkaWZmZXIgYnkgYml0bWFzayBvZmZzZXQsIGhvd2V2ZXIgbGluZWFy IGRvZXMgbm90IG1lYW4gdGhhdCB5b3UgDQpjYW5ub3QgYWZmZWN0IHRoZSBsb3dlciBiaXRzOiBl LmcuIDwweDA+IHRvIDwweDAgKyBzaXplPiBjYW4gbWFwIHRvDQo8MHgwMDAwY2FmZSB0byAweDAw MDBjYWZlICsgc2l6ZT4sIGJ1dCBJIGd1ZXNzIHRoYXQgZm9yIEhXIGRlc2lnbiByZWFzb25zDQpp dCBpcyBtdWNoIGVhc2llciB0byByZW1hcCBkaXJlY3RseSB1c2luZyBhIGJpdG1hc2suLi5idXQg SSB3YXMgbm90IHN1cmUNCmFuZCBJIGRpZG4ndCB0aGluayBhYm91dCB0aGUgcHJvYmxlbXMgdGhh dCBjYW4gYXJpc2Ugd2l0aCBBQ1BJLg0KDQpJZiB5b3UgdGhpbmsgdGhlIGJpdG1hc2sgaXMgT2sg dGhlbiBJIGNhbiBkaXJlY3RseSBkZWZpbmUgaXQgaW4NCmRlc2lnbndhcmUgYW5kIHdlIGNhbiBk cm9wIHRoaXMgcGF0Y2guDQoNClRoYW5rcw0KR2FiDQogDQo+IA0KPiBJIHRoaW5rIHdlJ3JlIHRh bGtpbmcgYWJvdXQgTU1JTyBoZXJlLCBhbmQgYSBicmlkZ2UgaGFzIGFuIE1NSU8NCj4gd2luZG93 LiAgQSB3aW5kb3cgaXMgYSByYW5nZSBvZiBDUFUgcGh5c2ljYWwgYWRkcmVzc2VzIHRoYXQgdGhl DQo+IGJyaWRnZSBmb3J3YXJkcyB0byBQQ0kuICBUaGUgUENJIGNvcmUgYXNzdW1lcyB0aGF0IGEg Q1BVIHBoeXNpY2FsDQo+IGFkZHJlc3MgcmFuZ2UgaXMgbGluZWFybHkgbWFwcGVkIHRvIFBDSSBi dXMgYWRkcmVzc2VzLCBpLmUuLCBpZiB0aGUNCj4gd2luZG93IGJhc2UgaXMgWCBhbmQgbWFwcyB0 byBQQ0kgYWRkcmVzcyBZLCB0aGVuIGFzIGxvbmcgYXMgWCtuIGlzDQo+IGluc2lkZSB0aGUgd2lu ZG93LCBpdCBtdXN0IG1hcCB0byBZK24uDQo+IA0KPiBUaGF0IG1lYW5zIHRoZSBsb3ctb3JkZXIg Yml0cyAodGhlIG9uZXMgdGhhdCBhcmUgdGhlIG9mZnNldCBpbnRvIHRoZQ0KPiB3aW5kb3cpIGNh bm5vdCBjaGFuZ2UuDQo+IC0tDQo+IFRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5k IHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1wY2kiIGluDQo+IHRoZSBib2R5IG9mIGEgbWVz c2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnDQo+IE1vcmUgbWFqb3Jkb21vIGluZm8g YXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0K -- 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 -- 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 -- 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 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 -- 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: 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 -- 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 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 -- 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
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; };