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

Cheers

Gab

-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: arnd-r2nGTMty4D4@public.gmane.org; lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org; Wangzhou (B);
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; james.morse-5wv7dgnIgG8@public.gmane.org; Liviu.Dudau-5wv7dgnIgG8@public.gmane.org; linux-
pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org;
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 "PCI"
in
the subject as a generic term that includes both PCI and PCIe.

sure agreed

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 API
    As discussed in
    http://lists.infradead.org/pipermail/linux-arm-kernel/2015-

January/317743.html

quoted

    in designware we have a problem as the PCI addresses in the PCIe

controller

quoted

    address space are required in order to perform correct HW

operation.

quoted

    In order to solve this problem commit f4c55c5a3 "PCI: designware:
    Program ATU with untranslated address" added code to read the

PCIe

Conventional reference is 12-char SHA1, like this:

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

Agreed, will change this

quoted

    controller start address directly from the DT ranges.

    In the new DT parsing API of_pci_get_host_bridge_resources()

hides the

quoted

    DT parser from the host controller drivers, so it is not possible
    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) in

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

    This patch adds a new field in "struct of_pci_range" to store the
    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

entry

quoted

    after it is created and added to the resource list and uses
    entry->__res.start to store the pci controller address

struct of_pci_range is starting to get confusing to non-OF folks like
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-line
comments to the structure?

sure I can add comments inline in the code

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 
and microblaze to print out.
However from my understanding pci_space is the phys.hi field of the 
ranges property: it defines the properties of the address space associated
to the PCI address. if you're curious you can find a nice and quick to read
"guide" in http://devicetree.org/MPC5200:PCI

pci_addr: I assume this is a PCI bus address, like what you would see
if
you put an analyzer on the bus/link.  This address could go in a BAR.

Yes, this is the PCI start address of the range: phys.mid + phys.low in the
guide mentioned above

cpu_addr: I assume this is a CPU physical address, like what you would
see
in /proc/iomem and what you would pass to ioremap().

Yes correct

bus_addr: ?

According to the guide above, this is the address into which the pci_address 
get translated to and that is passed to the root complex. Between the root 
complex and the CPU there can be intermediate translation layers: see that to 
get pci_address we call "of_translate_address"; this will apply all the 
translation layers (ranges in the DT) that it finds till it comes to the root 
node of the DT (thus retrieving the CPU address).
Now said that, for designware we need the first translated PCI address, that we call
here bus_addr after Rob Herring suggested the name...honestly I cannot think of a 
different name

   

I'm trying to imagine how this might be expressed in ACPI.  A host
bridge
ACPI _CRS contains a CPU physical address and applying a _TRA
(translation
offset) to the CPU address gives you a PCI bus address.  I know this
code
is OF, not ACPI, but I assume that it should be possible to describe
your
hardware via ACPI as well as by OF.

quoted

    the patch is based on 4.2-rc1

You can put this after the "---" line because it's not relevant in the
permanent changelog.

Agreed

quoted

    Signed-off-by: Gabriele Paoloni [off-list ref]
    Acked-by: Liviu Dudau [off-list ref]
    Acked-by: Rob Herring [off-list ref]

Please un-indent your changelog.

Ok agreed

quoted

---
 drivers/of/address.c       | 2 ++
 drivers/of/of_pci.c        | 4 ++++
 include/linux/of_address.h | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 8bfda6a..23a5793 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c

@@ -253,6 +253,7 @@ struct of_pci_range

*of_pci_range_parser_one(struct of_pci_range_parser *parser,

quoted

 						struct of_pci_range *range)
 {
 	const int na = 3, ns = 2;
+	const int p_ns = of_n_size_cells(parser->node);

 	if (!range)
 		return NULL;

@@ -265,6 +266,7 @@ struct of_pci_range

*of_pci_range_parser_one(struct of_pci_range_parser *parser,

quoted

 	range->pci_addr = of_read_number(parser->range + 1, ns);
 	range->cpu_addr = of_translate_address(parser->node,
 				parser->range + na);
+	range->bus_addr = of_read_number(parser->range + na, p_ns);
 	range->size = of_read_number(parser->range + parser->pna + na,

ns);

quoted

 	parser->range += parser->np;

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 5751dc5..fe57030 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c

@@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct

device_node *dev,

quoted

 	pr_debug("Parsing ranges property...\n");
 	for_each_of_pci_range(&parser, &range) {
+		struct resource_entry *entry;
 		/* Read next ranges element */
 		if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
 			snprintf(range_type, 4, " IO");

@@ -240,6 +241,9 @@ int of_pci_get_host_bridge_resources(struct

device_node *dev,

quoted

 		}

 		pci_add_resource_offset(resources, res,	res->start -

range.pci_addr);

quoted

+		entry = list_last_entry(resources, struct resource_entry,

node);

quoted

+		/* we are using __res for storing the PCI controller

address */

quoted

+		entry->__res.start = range.bus_addr;
 	}

 	return 0;

diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index d88e81b..865f96e 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h

@@ -16,6 +16,7 @@ struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci"

in

quoted

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html