Thread (18 messages) 18 messages, 7 authors, 2015-08-07

Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range

From: Jingoo Han <jingoohan1@gmail.com>
Date: 2015-08-04 04:19:50
Also in: linux-arm-kernel, linux-pci

Possibly related (same subject, not in this thread)

On 2015. 8. 3., at PM 8:18, Gabriele Paoloni [off-list ref] 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
quoted hunk ↗ jump to hunk
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help