> -----Original Message----- > From: Jingoo Han [mailto:jingoohan1@xxxxxxxxx] > Sent: Tuesday, August 04, 2015 5:20 AM > To: Gabriele Paoloni > Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@xxxxxxxx; > lorenzo.pieralisi@xxxxxxx; Wangzhou (B); robh+dt@xxxxxxxxxx; > james.morse@xxxxxxx; Liviu.Dudau@xxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth); > Pratyush Anand; Arnd Bergmann; jingoohan1@xxxxxxxxx > 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx [mailto:linux-pci- > >> owner@xxxxxxxxxxxxxxx] On Behalf Of Rob Herring > >> Sent: Friday, July 31, 2015 5:53 PM > >> To: Kishon Vijay Abraham I > >> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@xxxxxxxx; > >> lorenzo.pieralisi@xxxxxxx; Wangzhou (B); robh+dt@xxxxxxxxxx; > >> james.morse@xxxxxxx; Liviu.Dudau@xxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > >> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth); > >> 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@xxxxxx> > >> wrote: > >>> +Arnd > >>> > >>> Hi, > >>> > >>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote: > >>>> [+cc Kishon] > >>>> > >>>>> -----Original Message----- > >>>>> From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci- > >>>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Rob Herring > >>>>> Sent: Thursday, July 30, 2015 9:42 PM > >>>>> To: Gabriele Paoloni > >>>>> Cc: Bjorn Helgaas; arnd@xxxxxxxx; lorenzo.pieralisi@xxxxxxx; > >> Wangzhou > >>>>> (B); robh+dt@xxxxxxxxxx; james.morse@xxxxxxx; Liviu.Dudau@xxxxxxx; > >>>>> linux-pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > >>>>> devicetree@xxxxxxxxxxxxxxx; Yuanzhichang; Zhudacai; zhangjukuo; > >>>>> qiuzhenfa; Liguozhu (Kenneth); 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@xxxxxxxxxx> wrote: > >>>>>>> -----Original Message----- > >>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > >>>>>>> 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@xxxxxxxxxxxxxxx [mailto:linux-pci- > >>>>>>>>> owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html ÿ淸º{.nÇ+돴윯돪†+%듚ÿ깁負¥Šwÿº{.nÇ+돴¥Š{깸—"þ)í끾èw* jgП¨ ¶‰šŽ듶¢jÿ¾?G«앶ÿ◀ ◁¦j:+v돣ŠwèjØm¶Ÿÿ ? ®w?듺þf＂·hš뤴얎ÿ녪¥
