-----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
To: Gabriele Paoloni
Cc: Rob Herring; 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
of_pci_range

On Thu, Jul 30, 2015 at 01:52:13PM +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 Rob Herring
Sent: Thursday, July 30, 2015 2:43 PM
To: Gabriele Paoloni
Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;

Wangzhou

quoted

quoted

(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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
of_pci_range

On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni
[off-list ref] wrote:

quoted

Hi Bjorn

Many Thanks for your reply

I have commented back inline with resolutions from my side.

If you're ok with them I'll send it out a new version in the

appropriate patchset

quoted

Cheers

Gab

quoted

-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas@google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: arnd@arndb.de; lorenzo.pieralisi@arm.com; Wangzhou (B);
robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com;

linux-

quoted

quoted

quoted

quoted

pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
qiuzhenfa; Liguozhu (Kenneth)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
of_pci_range

Hi Gabriele,

As far as I can tell, this is not specific to PCIe, so please

use

quoted

quoted

"PCI"

quoted

quoted

in
the subject as a generic term that includes both PCI and PCIe.

sure agreed

quoted

On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:

quoted

From: gabriele paoloni <redacted>

    This patch is needed port PCIe designware to new DT

parsing

quoted

quoted

API

quoted

quoted

quoted

    As discussed in
    http://lists.infradead.org/pipermail/linux-arm-

kernel/2015-

quoted

quoted

quoted

quoted

January/317743.html

quoted

    in designware we have a problem as the PCI addresses in

the

quoted

quoted

PCIe

quoted

quoted

controller

quoted

    address space are required in order to perform correct HW

operation.

quoted

    In order to solve this problem commit f4c55c5a3 "PCI:

designware:

quoted

quoted

quoted

    Program ATU with untranslated address" added code to read

the

quoted

quoted

quoted

quoted

PCIe

Conventional reference is 12-char SHA1, like this:

  f4c55c5a3f7f ("PCI: designware: Program ATU with untranslated
address")

Agreed, will change this

quoted

quoted

    controller start address directly from the DT ranges.

    In the new DT parsing API

of_pci_get_host_bridge_resources()

quoted

quoted

quoted

quoted

hides the

quoted

    DT parser from the host controller drivers, so it is not

possible

quoted

quoted

quoted

    for drivers to parse values directly from the DT.

    In http://www.spinics.net/lists/linux-pci/msg42540.html we

already tried

quoted

    to use the new DT parsing API but there is a bug

(obviously)

quoted

quoted

in

quoted

quoted

setting

quoted

    the <*>_mod_base addresses
    Applying this patch we can easily set "<*>_mod_base = win-
__res.start"

By itself, this patch adds something.  It would help me

understand

quoted

quoted

it

quoted

quoted

if
the *user* of this new something were in the same patch series.

the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
I will ask Zhou Wang to include this patch in his patchset

quoted

quoted

    This patch adds a new field in "struct of_pci_range" to

store

quoted

quoted

the

quoted

quoted

quoted

    pci bus start address; it fills the field in

of_pci_range_parser_one();

quoted

    in of_pci_get_host_bridge_resources() it retrieves the

resource

quoted

quoted

entry

quoted

    after it is created and added to the resource list and

uses

quoted

quoted

quoted

quoted

quoted

    entry->__res.start to store the pci controller address

struct of_pci_range is starting to get confusing to non-OF folks

like

quoted

quoted

me.
It now contains:

  u32 pci_space;
  u64 pci_addr;
  u64 cpu_addr;
  u64 bus_addr;

Can you explain what all these things mean, and maybe even add

one-

quoted

quoted

line

quoted

quoted

comments to the structure?

sure I can add comments inline in the code

quoted

pci_space: The only uses I see are to determine whether to print
"Prefetch".  I don't see any real functionality that uses this.

Looking at the code I agree. it's seems to be used only in

powerpc

quoted

quoted

quoted

and microblaze to print out.
However from my understanding pci_space is the phys.hi field of

the

quoted

quoted

quoted

ranges property: it defines the properties of the address space

associated

quoted

to the PCI address. if you're curious you can find a nice and

quick

quoted

quoted

to read

quoted

"guide" in http://devicetree.org/MPC5200:PCI

quoted

pci_addr: I assume this is a PCI bus address, like what you

would

quoted

quoted

see

quoted

quoted

if
you put an analyzer on the bus/link.  This address could go in a

BAR.

quoted

quoted

quoted

Yes, this is the PCI start address of the range: phys.mid +

phys.low

quoted

quoted

in the

quoted

guide mentioned above

quoted

cpu_addr: I assume this is a CPU physical address, like what you

would

quoted

quoted

see
in /proc/iomem and what you would pass to ioremap().

Yes correct

quoted

bus_addr: ?

According to the guide above, this is the address into which the

pci_address

quoted

get translated to and that is passed to the root complex. Between

the

quoted

quoted

root

quoted

complex and the CPU there can be intermediate translation layers:

see

quoted

quoted

that to

quoted

get pci_address we call "of_translate_address"; this will apply

all

quoted

quoted

the

quoted

translation layers (ranges in the DT) that it finds till it comes

to

quoted

quoted

the root

quoted

node of the DT (thus retrieving the CPU address).
Now said that, for designware we need the first translated PCI

address, that we call

I think you mean "translated CPU address." The flow of addresses

looks

quoted

quoted

like this:

CPU -> CPU bus address -> bus fabric address decoding -> bus

address

quoted

quoted

-> DW PCI -> PCI address

This is quite common that an IP block does not see the full address.
It is unusual that the IP block needs to know its full address on

the

quoted

quoted

slave side. It is quite common for the master side and the kernel
deals with that all the time with DMA mapping

quoted

here bus_addr after Rob Herring suggested the name...honestly I

cannot think of a

quoted

different name

Thinking about this some more, is this really a translation versus
just a stripping of high bits? Does the DW IP have less than 32-

bits

quoted

quoted

address? If so, we could express differently and just mask the CPU
address within the driver instead.

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

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

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.

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

Thanks
Gab
 

I think we're talking about MMIO here, and a bridge has an MMIO
window.  A window is a range of CPU physical addresses that the
bridge forwards to PCI.  The PCI core assumes that a CPU physical
address range is linearly mapped to PCI bus addresses, i.e., if the
window base is X and maps to PCI address Y, then as long as X+n is
inside the window, it must map to Y+n.

That means the low-order bits (the ones that are the offset into the
window) cannot change.
--
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