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@xxxxxxxxx', instead of 'jg1.han@xxxxxxxxxxx'. 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@xxxxxxxxxxxxxxx [mailto:linux-pci- > > owner@xxxxxxxxxxxxxxx] 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@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 > > Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct > > of_pci_range > > > > > > > > > -----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 -- 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
