-----Original Message-----
From: Jingoo Han [mailto:jingoohan1 at gmail.com]
Sent: Tuesday, August 04, 2015 5:20 AM
To: Gabriele Paoloni
Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd at arndb.de;
lorenzo.pieralisi at arm.com; Wangzhou (B); robh+dt at kernel.org;
james.morse at arm.com; Liviu.Dudau at arm.com; linux-pci at vger.kernel.org;
linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org;
Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
Pratyush Anand; Arnd Bergmann; jingoohan1 at gmail.com
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
of_pci_range

On 2015. 8. 3., at PM 8:18, Gabriele Paoloni
[off-list ref] wrote:

quoted

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

quoted

2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-

dra7xx.c

quoted

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)

quoted

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

quoted

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

quoted

-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
owner at 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 at arndb.de;
lorenzo.pieralisi at arm.com; Wangzhou (B); robh+dt at kernel.org;
james.morse at arm.com; Liviu.Dudau at arm.com; linux-pci at vger.kernel.org;
linux-arm-kernel at lists.infradead.org; devicetree at 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

[off-list ref]

quoted

quoted

wrote:

quoted

+Arnd

Hi,

quoted

On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
[+cc Kishon]

quoted

-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
owner at vger.kernel.org] On Behalf Of Rob Herring
Sent: Thursday, July 30, 2015 9:42 PM
To: Gabriele Paoloni
Cc: Bjorn Helgaas; arnd at arndb.de; lorenzo.pieralisi at arm.com;

Wangzhou

quoted

quoted

quoted

(B); robh+dt at kernel.org; james.morse at arm.com; Liviu.Dudau at arm.com;
linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
devicetree at 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
[off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
owner at 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:

quoted

quoted

quoted

[...]

quoted

quoted

quoted

quoted

quoted

I don?t think we should rely on [CPU] addresses...what if

the

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

intermediate

quoted

translation layer changes the lower significant bits of the

"bus

quoted

quoted

quoted

quoted

address"

quoted

to translate into a cpu address?

Is it really a possiblity that the lower bits could be

changed?

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I've checked all the current deignware users DTs except "pci-

layerscape"

quoted

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

quoted

quoted

quoted

guy

quoted

quoted

quoted

that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-

electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207

quoted

quoted

quoted

axi1
http://lxr.free-

electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241

quoted

quoted

quoted

For this case masking the top 4bits (bits28 to 31) should make

the

quoted

quoted

quoted

job.

IMO, we should just fix this case. After further study, I don't

think

quoted

quoted

quoted

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

quoted

hook.

quoted

quoted

Yes I guess that I could just assign pp->(*)_mod_base to the CPU

address

quoted

quoted

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

quoted

this discussion [1] before we decided the current approach. It'll

be

quoted

quoted

good to

quoted

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

quoted

quoted

fewer address bits are available. It can be used just for

convenience

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html