[+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
[off-list ref] wrote:

quoted

quoted

-----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:

quoted

quoted

-----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:

[...]

quoted

quoted

quoted

quoted

quoted

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

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?

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

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

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? 

In looking at this, I'm curious why only 2 ATU viewports are used by
default as this causes switching on config accesses. Probably some
configuration only has 2 viewports. This would not even work on SMP
systems! Memory and i/o accesses do not have any lock. A config access
on one core will temporarily disable the i/o or memory viewport which
will cause an abort, dropped, or garbage data on an i/o or memory
access from another core. You would have to be accessing cfg space on
a bus other than the root bus, so maybe no one is doing that.

quoted

quoted

quoted

Speaking in general terms so far I've always seen linear mappings
that differ by bitmask offset, however linear does not mean that

you

quoted

quoted

quoted

cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map

to

quoted

quoted

quoted

<0x0000cafe to 0x0000cafe + size>, but I guess that for HW design

reasons

quoted

it is much easier to remap directly using a bitmask...but I was

not

quoted

quoted

sure

quoted

and I didn't think about the problems that can arise with ACPI.

For higher speed buses, the h/w designs are not going to be doing
complicated translations in order to meet timing requirements. At the
top level only the highest order bits are going to be looked at. For
downstream segments, the high order bits may get dropped to simplify
routing. If an IP block has full address bits in this case, they could
either be tied to 0 or tied off to the correct address. Either is
reasonable, so I'm sure there is h/w doing both. That doesn't mean h/w
designers haven't done something crazy, too.

quoted

quoted

As I said, it wouldn't make sense for the bits that comprise the
offset into the window to change, so the mapping only has to be

linear

quoted

quoted

inside the window.

For the bits outside the window offset, I don't know enough to say
whether masking is sufficient or safe.

quoted

If you think the bitmask is Ok then I can directly define it in
designware and we can drop this patch.

It's OK by me, but I know nothing about the actual hardware involved.
For the DesignWare question, I think you would just have to convince
Jingoo and Pratyush (cc'd).

Yes actually looking at
http://lxr.free-

electrons.com/source/arch/arm/boot/dts/spear1310.dtsi#L99

quoted

I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so

clearing

quoted

the top 4 bits would alter the behaviour of the current designware
SW framework...now I don't know if DW needs only the low 28b of that
value or not....

Given that most cases have same cpu and bus address, masking is not
the right solution in general.

There is also a bug here BTW. pcie0 and pcie2 have the same CPU
address for the memory range.

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