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@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 > Hi Jingoo, Sorry for that, I will add jingoohan1@xxxxxxxxx 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@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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
