| Message ID | EE11001F9E5DDD47B7634E2F8A612F2E01D712E7@lhreml503-mbs (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 2015. 8. 3., at PM 8:18, Gabriele Paoloni <gabriele.paoloni@huawei.com> wrote: > > Rob, Kishon what about the following solution?.... > > --- > drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++ > drivers/pci/host/pcie-designware.c | 9 +++------ Hi Paoloni, OK, it looks good. I want you to revert the following patch. commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" Would you remove all '*_mode_base's? Best regards, Jingoo Han > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c > index 80db09e..bb2635f 100644 > --- a/drivers/pci/host/pci-dra7xx.c > +++ b/drivers/pci/host/pci-dra7xx.c > @@ -61,6 +61,7 @@ > > #define PCIECTRL_DRA7XX_CONF_PHY_CS 0x010C > #define LINK_UP BIT(16) > +#define CPU_TO_BUS_ADDR 0x0FFFFFFF > > struct dra7xx_pcie { > void __iomem *base; > @@ -138,6 +139,17 @@ static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp) > > static void dra7xx_pcie_host_init(struct pcie_port *pp) > { > + if (pp->io_mod_base) > + pp->io_mod_base &= CPU_TO_BUS_ADDR; > + > + if (pp->mem_mod_base) > + pp->mem_mod_base &= CPU_TO_BUS_ADDR; > + > + if (pp->cfg0_mod_base) { > + pp->cfg0_mod_base &= CPU_TO_BUS_ADDR; > + pp->cfg1_mod_base &= CPU_TO_BUS_ADDR; > + } > + > dw_pcie_setup_rc(pp); > dra7xx_pcie_establish_link(pp); > if (IS_ENABLED(CONFIG_PCI_MSI)) > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 69486be..06c682b 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > pp->io_base = range.cpu_addr; > > /* Find the untranslated IO space address */ > - pp->io_mod_base = of_read_number(parser.range - > - parser.np + na, ns); > + pp->io_mod_base = range.cpu_addr;; > } > if (restype == IORESOURCE_MEM) { > of_pci_range_to_resource(&range, np, &pp->mem); > @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > pp->mem_bus_addr = range.pci_addr; > > /* Find the untranslated MEM space address */ > - pp->mem_mod_base = of_read_number(parser.range - > - parser.np + na, ns); > + pp->mem_mod_base = range.cpu_addr; > } > if (restype == 0) { > of_pci_range_to_resource(&range, np, &pp->cfg); > @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > pp->cfg1_base = pp->cfg.start + pp->cfg0_size; > > /* Find the untranslated configuration space address */ > - pp->cfg0_mod_base = of_read_number(parser.range - > - parser.np + na, ns); > + pp->cfg0_mod_base = range.cpu_addr; > pp->cfg1_mod_base = pp->cfg0_mod_base + > pp->cfg0_size; > } > -- > 1.9.1 > > >> -----Original Message----- >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >> owner@vger.kernel.org] On Behalf Of Rob Herring >> Sent: Friday, July 31, 2015 5:53 PM >> To: Kishon Vijay Abraham I >> Cc: Gabriele Paoloni; 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 >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >> of_pci_range >> >> 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 -- 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: Jingoo Han [mailto:jingoohan1@gmail.com] > Sent: Tuesday, August 04, 2015 5:20 AM > To: Gabriele Paoloni > Cc: Rob Herring; Kishon Vijay Abraham I; 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); > Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > On 2015. 8. 3., at PM 8:18, Gabriele Paoloni > <gabriele.paoloni@huawei.com> wrote: > > > > Rob, Kishon what about the following solution?.... > > > > --- > > drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++ > > drivers/pci/host/pcie-designware.c | 9 +++------ > > Hi Paoloni, > > OK, it looks good. > I want you to revert the following patch. > > commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated > address)" > > Would you remove all '*_mode_base's? Hi Jingoo Han, If I reverted the commit, in order to get designware working, dra7 should mask directly the CPU addresses stored in pp->cfg0_base, pp->cfg1_base, pp->mem_base and pp->io_base. The masking would happen at this point: http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L493 Now: - I see that pp->cfg<0/1>_base are used in devm_ioremap before that point and nowhere else, so they should be ok. - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is called in dra7xx_pcie_host_init()...so here I should move the masking after dw_pcie_setup() retuned, but I think it should be ok - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now currently in designware this is called by pci_bios_init_hw(); this is after the masking....so here we would have a the wrong value passed Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64 support", that is the patchset where this patch is needed, you can see that dw_pcie_setup() is removed and pp->io_base is used in pci_remap_iospace() before the masking would happen in dra7xx_pcie_host_init()...so for this patchset we should be good to revert the commit. I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05" as the one I just posted in this thread (this would not revert the commit but just moves the masking to dra7xx) I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64 support" together with the rest of designware rework. So we would have always a working version of designware... Are you ok with this? > > Best regards, > Jingoo Han > > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- > dra7xx.c > > index 80db09e..bb2635f 100644 > > --- a/drivers/pci/host/pci-dra7xx.c > > +++ b/drivers/pci/host/pci-dra7xx.c > > @@ -61,6 +61,7 @@ > > > > #define PCIECTRL_DRA7XX_CONF_PHY_CS 0x010C > > #define LINK_UP BIT(16) > > +#define CPU_TO_BUS_ADDR 0x0FFFFFFF > > > > struct dra7xx_pcie { > > void __iomem *base; > > @@ -138,6 +139,17 @@ static void dra7xx_pcie_enable_interrupts(struct > pcie_port *pp) > > > > static void dra7xx_pcie_host_init(struct pcie_port *pp) > > { > > + if (pp->io_mod_base) > > + pp->io_mod_base &= CPU_TO_BUS_ADDR; > > + > > + if (pp->mem_mod_base) > > + pp->mem_mod_base &= CPU_TO_BUS_ADDR; > > + > > + if (pp->cfg0_mod_base) { > > + pp->cfg0_mod_base &= CPU_TO_BUS_ADDR; > > + pp->cfg1_mod_base &= CPU_TO_BUS_ADDR; > > + } > > + > > dw_pcie_setup_rc(pp); > > dra7xx_pcie_establish_link(pp); > > if (IS_ENABLED(CONFIG_PCI_MSI)) > > diff --git a/drivers/pci/host/pcie-designware.c > b/drivers/pci/host/pcie-designware.c > > index 69486be..06c682b 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > pp->io_base = range.cpu_addr; > > > > /* Find the untranslated IO space address */ > > - pp->io_mod_base = of_read_number(parser.range - > > - parser.np + na, ns); > > + pp->io_mod_base = range.cpu_addr;; > > } > > if (restype == IORESOURCE_MEM) { > > of_pci_range_to_resource(&range, np, &pp->mem); > > @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > pp->mem_bus_addr = range.pci_addr; > > > > /* Find the untranslated MEM space address */ > > - pp->mem_mod_base = of_read_number(parser.range - > > - parser.np + na, ns); > > + pp->mem_mod_base = range.cpu_addr; > > } > > if (restype == 0) { > > of_pci_range_to_resource(&range, np, &pp->cfg); > > @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > pp->cfg1_base = pp->cfg.start + pp->cfg0_size; > > > > /* Find the untranslated configuration space address */ > > - pp->cfg0_mod_base = of_read_number(parser.range - > > - parser.np + na, ns); > > + pp->cfg0_mod_base = range.cpu_addr; > > pp->cfg1_mod_base = pp->cfg0_mod_base + > > pp->cfg0_size; > > } > > -- > > 1.9.1 > > > > > >> -----Original Message----- > >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > >> owner@vger.kernel.org] On Behalf Of Rob Herring > >> Sent: Friday, July 31, 2015 5:53 PM > >> To: Kishon Vijay Abraham I > >> Cc: Gabriele Paoloni; 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 > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > >> of_pci_range > >> > >> 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
Hi all This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to DRA7xx" commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in "[PATCH v6 3/6] PCI: designware: Add ARM64 support" Gab > -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Gabriele Paoloni > Sent: Tuesday, August 04, 2015 11:12 AM > To: Jingoo Han > Cc: Rob Herring; Kishon Vijay Abraham I; 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); > Pratyush Anand; Arnd Bergmann > Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct > of_pci_range > > > > > -----Original Message----- > > From: Jingoo Han [mailto:jingoohan1@gmail.com] > > Sent: Tuesday, August 04, 2015 5:20 AM > > To: Gabriele Paoloni > > Cc: Rob Herring; Kishon Vijay Abraham I; 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); > > Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > of_pci_range > > > > On 2015. 8. 3., at PM 8:18, Gabriele Paoloni > > <gabriele.paoloni@huawei.com> wrote: > > > > > > Rob, Kishon what about the following solution?.... > > > > > > --- > > > drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++ > > > drivers/pci/host/pcie-designware.c | 9 +++------ > > > > Hi Paoloni, > > > > OK, it looks good. > > I want you to revert the following patch. > > > > commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated > > address)" > > > > Would you remove all '*_mode_base's? > > Hi Jingoo Han, > > If I reverted the commit, in order to get designware working, dra7 > should mask directly the CPU addresses stored in pp->cfg0_base, > pp->cfg1_base, pp->mem_base and pp->io_base. > > The masking would happen at this point: > http://lxr.free-electrons.com/source/drivers/pci/host/pcie- > designware.c#L493 > > Now: > - I see that pp->cfg<0/1>_base are used in devm_ioremap before that > point and nowhere else, so they should be ok. > - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is > called > in dra7xx_pcie_host_init()...so here I should move the masking after > dw_pcie_setup() retuned, but I think it should be ok > - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now > currently > in designware this is called by pci_bios_init_hw(); this is after the > masking....so here we would have a the wrong value passed > > Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64 > support", > that is the patchset where this patch is needed, you can see that > dw_pcie_setup() is removed and pp->io_base is used in > pci_remap_iospace() before > the masking would happen in dra7xx_pcie_host_init()...so for this > patchset we > should be good to revert the commit. > > I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi: > Add PCIe > host support for HiSilicon SoC Hip05" as the one I just posted in this > thread (this would not revert the commit but just moves the masking to > dra7xx) > > I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64 > support" > together with the rest of designware rework. > > So we would have always a working version of designware... > > Are you ok with this? > > > > > Best regards, > > Jingoo Han > > > > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- > > dra7xx.c > > > index 80db09e..bb2635f 100644 > > > --- a/drivers/pci/host/pci-dra7xx.c > > > +++ b/drivers/pci/host/pci-dra7xx.c > > > @@ -61,6 +61,7 @@ > > > > > > #define PCIECTRL_DRA7XX_CONF_PHY_CS 0x010C > > > #define LINK_UP BIT(16) > > > +#define CPU_TO_BUS_ADDR 0x0FFFFFFF > > > > > > struct dra7xx_pcie { > > > void __iomem *base; > > > @@ -138,6 +139,17 @@ static void > dra7xx_pcie_enable_interrupts(struct > > pcie_port *pp) > > > > > > static void dra7xx_pcie_host_init(struct pcie_port *pp) > > > { > > > + if (pp->io_mod_base) > > > + pp->io_mod_base &= CPU_TO_BUS_ADDR; > > > + > > > + if (pp->mem_mod_base) > > > + pp->mem_mod_base &= CPU_TO_BUS_ADDR; > > > + > > > + if (pp->cfg0_mod_base) { > > > + pp->cfg0_mod_base &= CPU_TO_BUS_ADDR; > > > + pp->cfg1_mod_base &= CPU_TO_BUS_ADDR; > > > + } > > > + > > > dw_pcie_setup_rc(pp); > > > dra7xx_pcie_establish_link(pp); > > > if (IS_ENABLED(CONFIG_PCI_MSI)) > > > diff --git a/drivers/pci/host/pcie-designware.c > > b/drivers/pci/host/pcie-designware.c > > > index 69486be..06c682b 100644 > > > --- a/drivers/pci/host/pcie-designware.c > > > +++ b/drivers/pci/host/pcie-designware.c > > > @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > pp->io_base = range.cpu_addr; > > > > > > /* Find the untranslated IO space address */ > > > - pp->io_mod_base = of_read_number(parser.range - > > > - parser.np + na, ns); > > > + pp->io_mod_base = range.cpu_addr;; > > > } > > > if (restype == IORESOURCE_MEM) { > > > of_pci_range_to_resource(&range, np, &pp->mem); > > > @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > pp->mem_bus_addr = range.pci_addr; > > > > > > /* Find the untranslated MEM space address */ > > > - pp->mem_mod_base = of_read_number(parser.range - > > > - parser.np + na, ns); > > > + pp->mem_mod_base = range.cpu_addr; > > > } > > > if (restype == 0) { > > > of_pci_range_to_resource(&range, np, &pp->cfg); > > > @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > pp->cfg1_base = pp->cfg.start + pp->cfg0_size; > > > > > > /* Find the untranslated configuration space address */ > > > - pp->cfg0_mod_base = of_read_number(parser.range - > > > - parser.np + na, ns); > > > + pp->cfg0_mod_base = range.cpu_addr; > > > pp->cfg1_mod_base = pp->cfg0_mod_base + > > > pp->cfg0_size; > > > } > > > -- > > > 1.9.1 > > > > > > > > >> -----Original Message----- > > >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > >> owner@vger.kernel.org] On Behalf Of Rob Herring > > >> Sent: Friday, July 31, 2015 5:53 PM > > >> To: Kishon Vijay Abraham I > > >> Cc: Gabriele Paoloni; 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 > > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > >> of_pci_range > > >> > > >> 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 > ????????+-???w??????ir(???}????j:+v????zZ+??zf? > ?????i??z?????????f
On Thursday, August 06, 2015 10:53 PM, Gabriele Paoloni wrote: > > Hi all > > This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to > DRA7xx" To Zhou Wang, Please send your patches to 'jingoohan1@gmail.com', instead of 'jg1.han@samsung.com'. Even though, I have done mainline works for Samsung SoCs, Samsung does not support me as a maintainer. So, please don't send your patches to my Samsung e-mail. Best regards, Jingoo Han > > commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in > "[PATCH v6 3/6] PCI: designware: Add ARM64 support" > > Gab > > > -----Original Message----- > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > owner@vger.kernel.org] On Behalf Of Gabriele Paoloni > > Sent: Tuesday, August 04, 2015 11:12 AM > > To: Jingoo Han > > Cc: Rob Herring; Kishon Vijay Abraham I; 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); > > Pratyush Anand; Arnd Bergmann > > Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct > > of_pci_range > > > > > > > > > -----Original Message----- > > > From: Jingoo Han [mailto:jingoohan1@gmail.com] > > > Sent: Tuesday, August 04, 2015 5:20 AM > > > To: Gabriele Paoloni > > > Cc: Rob Herring; Kishon Vijay Abraham I; 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); > > > Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com > > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > > of_pci_range > > > > > > On 2015. 8. 3., at PM 8:18, Gabriele Paoloni > > > <gabriele.paoloni@huawei.com> wrote: > > > > > > > > Rob, Kishon what about the following solution?.... > > > > > > > > --- > > > > drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++ > > > > drivers/pci/host/pcie-designware.c | 9 +++------ > > > > > > Hi Paoloni, > > > > > > OK, it looks good. > > > I want you to revert the following patch. > > > > > > commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated > > > address)" > > > > > > Would you remove all '*_mode_base's? > > > > Hi Jingoo Han, > > > > If I reverted the commit, in order to get designware working, dra7 > > should mask directly the CPU addresses stored in pp->cfg0_base, > > pp->cfg1_base, pp->mem_base and pp->io_base. > > > > The masking would happen at this point: > > http://lxr.free-electrons.com/source/drivers/pci/host/pcie- > > designware.c#L493 > > > > Now: > > - I see that pp->cfg<0/1>_base are used in devm_ioremap before that > > point and nowhere else, so they should be ok. > > - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is > > called > > in dra7xx_pcie_host_init()...so here I should move the masking after > > dw_pcie_setup() retuned, but I think it should be ok > > - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now > > currently > > in designware this is called by pci_bios_init_hw(); this is after the > > masking....so here we would have a the wrong value passed > > > > Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64 > > support", > > that is the patchset where this patch is needed, you can see that > > dw_pcie_setup() is removed and pp->io_base is used in > > pci_remap_iospace() before > > the masking would happen in dra7xx_pcie_host_init()...so for this > > patchset we > > should be good to revert the commit. > > > > I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi: > > Add PCIe > > host support for HiSilicon SoC Hip05" as the one I just posted in this > > thread (this would not revert the commit but just moves the masking to > > dra7xx) > > > > I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64 > > support" > > together with the rest of designware rework. > > > > So we would have always a working version of designware... > > > > Are you ok with this? > > > > > > > > Best regards, > > > Jingoo Han > > > > > > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- > > > dra7xx.c > > > > index 80db09e..bb2635f 100644 > > > > --- a/drivers/pci/host/pci-dra7xx.c > > > > +++ b/drivers/pci/host/pci-dra7xx.c > > > > @@ -61,6 +61,7 @@ > > > > > > > > #define PCIECTRL_DRA7XX_CONF_PHY_CS 0x010C > > > > #define LINK_UP BIT(16) > > > > +#define CPU_TO_BUS_ADDR 0x0FFFFFFF > > > > > > > > struct dra7xx_pcie { > > > > void __iomem *base; > > > > @@ -138,6 +139,17 @@ static void > > dra7xx_pcie_enable_interrupts(struct > > > pcie_port *pp) > > > > > > > > static void dra7xx_pcie_host_init(struct pcie_port *pp) > > > > { > > > > + if (pp->io_mod_base) > > > > + pp->io_mod_base &= CPU_TO_BUS_ADDR; > > > > + > > > > + if (pp->mem_mod_base) > > > > + pp->mem_mod_base &= CPU_TO_BUS_ADDR; > > > > + > > > > + if (pp->cfg0_mod_base) { > > > > + pp->cfg0_mod_base &= CPU_TO_BUS_ADDR; > > > > + pp->cfg1_mod_base &= CPU_TO_BUS_ADDR; > > > > + } > > > > + > > > > dw_pcie_setup_rc(pp); > > > > dra7xx_pcie_establish_link(pp); > > > > if (IS_ENABLED(CONFIG_PCI_MSI)) > > > > diff --git a/drivers/pci/host/pcie-designware.c > > > b/drivers/pci/host/pcie-designware.c > > > > index 69486be..06c682b 100644 > > > > --- a/drivers/pci/host/pcie-designware.c > > > > +++ b/drivers/pci/host/pcie-designware.c > > > > @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > pp->io_base = range.cpu_addr; > > > > > > > > /* Find the untranslated IO space address */ > > > > - pp->io_mod_base = of_read_number(parser.range - > > > > - parser.np + na, ns); > > > > + pp->io_mod_base = range.cpu_addr;; > > > > } > > > > if (restype == IORESOURCE_MEM) { > > > > of_pci_range_to_resource(&range, np, &pp->mem); > > > > @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > pp->mem_bus_addr = range.pci_addr; > > > > > > > > /* Find the untranslated MEM space address */ > > > > - pp->mem_mod_base = of_read_number(parser.range - > > > > - parser.np + na, ns); > > > > + pp->mem_mod_base = range.cpu_addr; > > > > } > > > > if (restype == 0) { > > > > of_pci_range_to_resource(&range, np, &pp->cfg); > > > > @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > pp->cfg1_base = pp->cfg.start + pp->cfg0_size; > > > > > > > > /* Find the untranslated configuration space address */ > > > > - pp->cfg0_mod_base = of_read_number(parser.range - > > > > - parser.np + na, ns); > > > > + pp->cfg0_mod_base = range.cpu_addr; > > > > pp->cfg1_mod_base = pp->cfg0_mod_base + > > > > pp->cfg0_size; > > > > } > > > > -- > > > > 1.9.1 > > > > > > > > > > > >> -----Original Message----- > > > >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > > >> owner@vger.kernel.org] On Behalf Of Rob Herring > > > >> Sent: Friday, July 31, 2015 5:53 PM > > > >> To: Kishon Vijay Abraham I > > > >> Cc: Gabriele Paoloni; 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 > > > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct > > > >> of_pci_range > > > >> > > > >> 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 -- 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/6 23:06, Jingoo Han wrote: > On Thursday, August 06, 2015 10:53 PM, Gabriele Paoloni wrote: >> >> Hi all >> >> This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to >> DRA7xx" > > To Zhou Wang, > > Please send your patches to 'jingoohan1@gmail.com', instead of 'jg1.han@samsung.com'. > Even though, I have done mainline works for Samsung SoCs, Samsung does not support me > as a maintainer. So, please don't send your patches to my Samsung e-mail. > > Best regards, > Jingoo Han > Hi Jingoo, Sorry for that, I will add jingoohan1@gmail.com to v6 thread. Thanks for reminding, Zhou >> >> commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in >> "[PATCH v6 3/6] PCI: designware: Add ARM64 support" >> >> Gab >> >>> -----Original Message----- >>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>> owner@vger.kernel.org] On Behalf Of Gabriele Paoloni >>> Sent: Tuesday, August 04, 2015 11:12 AM >>> To: Jingoo Han >>> Cc: Rob Herring; Kishon Vijay Abraham I; 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); >>> Pratyush Anand; Arnd Bergmann >>> Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct >>> of_pci_range >>> >>> >>> >>>> -----Original Message----- >>>> From: Jingoo Han [mailto:jingoohan1@gmail.com] >>>> Sent: Tuesday, August 04, 2015 5:20 AM >>>> To: Gabriele Paoloni >>>> Cc: Rob Herring; Kishon Vijay Abraham I; 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); >>>> Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com >>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >>>> of_pci_range >>>> >>>> On 2015. 8. 3., at PM 8:18, Gabriele Paoloni >>>> <gabriele.paoloni@huawei.com> wrote: >>>>> >>>>> Rob, Kishon what about the following solution?.... >>>>> >>>>> --- >>>>> drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++ >>>>> drivers/pci/host/pcie-designware.c | 9 +++------ >>>> >>>> Hi Paoloni, >>>> >>>> OK, it looks good. >>>> I want you to revert the following patch. >>>> >>>> commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated >>>> address)" >>>> >>>> Would you remove all '*_mode_base's? >>> >>> Hi Jingoo Han, >>> >>> If I reverted the commit, in order to get designware working, dra7 >>> should mask directly the CPU addresses stored in pp->cfg0_base, >>> pp->cfg1_base, pp->mem_base and pp->io_base. >>> >>> The masking would happen at this point: >>> http://lxr.free-electrons.com/source/drivers/pci/host/pcie- >>> designware.c#L493 >>> >>> Now: >>> - I see that pp->cfg<0/1>_base are used in devm_ioremap before that >>> point and nowhere else, so they should be ok. >>> - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is >>> called >>> in dra7xx_pcie_host_init()...so here I should move the masking after >>> dw_pcie_setup() retuned, but I think it should be ok >>> - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now >>> currently >>> in designware this is called by pci_bios_init_hw(); this is after the >>> masking....so here we would have a the wrong value passed >>> >>> Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64 >>> support", >>> that is the patchset where this patch is needed, you can see that >>> dw_pcie_setup() is removed and pp->io_base is used in >>> pci_remap_iospace() before >>> the masking would happen in dra7xx_pcie_host_init()...so for this >>> patchset we >>> should be good to revert the commit. >>> >>> I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi: >>> Add PCIe >>> host support for HiSilicon SoC Hip05" as the one I just posted in this >>> thread (this would not revert the commit but just moves the masking to >>> dra7xx) >>> >>> I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64 >>> support" >>> together with the rest of designware rework. >>> >>> So we would have always a working version of designware... >>> >>> Are you ok with this? >>> >>>> >>>> Best regards, >>>> Jingoo Han >>>> >>>>> 2 files changed, 15 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- >>>> dra7xx.c >>>>> index 80db09e..bb2635f 100644 >>>>> --- a/drivers/pci/host/pci-dra7xx.c >>>>> +++ b/drivers/pci/host/pci-dra7xx.c >>>>> @@ -61,6 +61,7 @@ >>>>> >>>>> #define PCIECTRL_DRA7XX_CONF_PHY_CS 0x010C >>>>> #define LINK_UP BIT(16) >>>>> +#define CPU_TO_BUS_ADDR 0x0FFFFFFF >>>>> >>>>> struct dra7xx_pcie { >>>>> void __iomem *base; >>>>> @@ -138,6 +139,17 @@ static void >>> dra7xx_pcie_enable_interrupts(struct >>>> pcie_port *pp) >>>>> >>>>> static void dra7xx_pcie_host_init(struct pcie_port *pp) >>>>> { >>>>> + if (pp->io_mod_base) >>>>> + pp->io_mod_base &= CPU_TO_BUS_ADDR; >>>>> + >>>>> + if (pp->mem_mod_base) >>>>> + pp->mem_mod_base &= CPU_TO_BUS_ADDR; >>>>> + >>>>> + if (pp->cfg0_mod_base) { >>>>> + pp->cfg0_mod_base &= CPU_TO_BUS_ADDR; >>>>> + pp->cfg1_mod_base &= CPU_TO_BUS_ADDR; >>>>> + } >>>>> + >>>>> dw_pcie_setup_rc(pp); >>>>> dra7xx_pcie_establish_link(pp); >>>>> if (IS_ENABLED(CONFIG_PCI_MSI)) >>>>> diff --git a/drivers/pci/host/pcie-designware.c >>>> b/drivers/pci/host/pcie-designware.c >>>>> index 69486be..06c682b 100644 >>>>> --- a/drivers/pci/host/pcie-designware.c >>>>> +++ b/drivers/pci/host/pcie-designware.c >>>>> @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) >>>>> pp->io_base = range.cpu_addr; >>>>> >>>>> /* Find the untranslated IO space address */ >>>>> - pp->io_mod_base = of_read_number(parser.range - >>>>> - parser.np + na, ns); >>>>> + pp->io_mod_base = range.cpu_addr;; >>>>> } >>>>> if (restype == IORESOURCE_MEM) { >>>>> of_pci_range_to_resource(&range, np, &pp->mem); >>>>> @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp) >>>>> pp->mem_bus_addr = range.pci_addr; >>>>> >>>>> /* Find the untranslated MEM space address */ >>>>> - pp->mem_mod_base = of_read_number(parser.range - >>>>> - parser.np + na, ns); >>>>> + pp->mem_mod_base = range.cpu_addr; >>>>> } >>>>> if (restype == 0) { >>>>> of_pci_range_to_resource(&range, np, &pp->cfg); >>>>> @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp) >>>>> pp->cfg1_base = pp->cfg.start + pp->cfg0_size; >>>>> >>>>> /* Find the untranslated configuration space address */ >>>>> - pp->cfg0_mod_base = of_read_number(parser.range - >>>>> - parser.np + na, ns); >>>>> + pp->cfg0_mod_base = range.cpu_addr; >>>>> pp->cfg1_mod_base = pp->cfg0_mod_base + >>>>> pp->cfg0_size; >>>>> } >>>>> -- >>>>> 1.9.1 >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>>>>> owner@vger.kernel.org] On Behalf Of Rob Herring >>>>>> Sent: Friday, July 31, 2015 5:53 PM >>>>>> To: Kishon Vijay Abraham I >>>>>> Cc: Gabriele Paoloni; 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 >>>>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct >>>>>> of_pci_range >>>>>> >>>>>> 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 > > > . > -- 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/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c index 80db09e..bb2635f 100644 --- a/drivers/pci/host/pci-dra7xx.c +++ b/drivers/pci/host/pci-dra7xx.c @@ -61,6 +61,7 @@ #define PCIECTRL_DRA7XX_CONF_PHY_CS 0x010C #define LINK_UP BIT(16) +#define CPU_TO_BUS_ADDR 0x0FFFFFFF struct dra7xx_pcie { void __iomem *base; @@ -138,6 +139,17 @@ static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp) static void dra7xx_pcie_host_init(struct pcie_port *pp) { + if (pp->io_mod_base) + pp->io_mod_base &= CPU_TO_BUS_ADDR; + + if (pp->mem_mod_base) + pp->mem_mod_base &= CPU_TO_BUS_ADDR; + + if (pp->cfg0_mod_base) { + pp->cfg0_mod_base &= CPU_TO_BUS_ADDR; + pp->cfg1_mod_base &= CPU_TO_BUS_ADDR; + } + dw_pcie_setup_rc(pp); dra7xx_pcie_establish_link(pp); if (IS_ENABLED(CONFIG_PCI_MSI)) diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 69486be..06c682b 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->io_base = range.cpu_addr; /* Find the untranslated IO space address */ - pp->io_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->io_mod_base = range.cpu_addr;; } if (restype == IORESOURCE_MEM) { of_pci_range_to_resource(&range, np, &pp->mem); @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->mem_bus_addr = range.pci_addr; /* Find the untranslated MEM space address */ - pp->mem_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->mem_mod_base = range.cpu_addr; } if (restype == 0) { of_pci_range_to_resource(&range, np, &pp->cfg); @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg1_base = pp->cfg.start + pp->cfg0_size; /* Find the untranslated configuration space address */ - pp->cfg0_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->cfg0_mod_base = range.cpu_addr; pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; }