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

[Rob Herring]

On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni
<gabriele.paoloni at huawei.com> wrote:
>> -----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:
>> > > -----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:

[...]

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

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.

>> >
>> > Speaking in general terms so far I've always seen linear mappings
>> > that differ by bitmask offset, however linear does not mean that you
>> > cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to
>> > <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design
>> reasons
>> > it is much easier to remap directly using a bitmask...but I was not
>> sure
>> > 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.

>> 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
>> inside the window.
>>
>> For the bits outside the window offset, I don't know enough to say
>> whether masking is sufficient or safe.
>>
>> > 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
> I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so clearing
> 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