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@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 > ?移?????쵔+-깁負쪐w츩?궝??욎ir(㎍썳變}찠꼿쟺?j:+v돣?쳭喩zZ+€?zf＂ > 톒쉱?넮녬i鎬z?췿ⅱ?솳鈺??刪f ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
