PCI: Store PCIe bus address in struct of_pci_range

PATCH Superseded Gabriele Paoloni 7 participants 28 messages Jul 27, 2015 – Aug 7, 2015

Subsystems (2)

PCI Device tree

lore

Share & tools

lore

Gabriele Paoloni

author

2015-07-27 15:10 UTC #0 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

From: gabriele paoloni <[email protected]>

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
in designware we have a problem as the PCI addresses in the PCIe controller address space are required in order to perform correct HW operation.

In order to solve this problem commit f4c55c5a3 "PCI: designware:
Program ATU with untranslated address" added code to read the PCIe controller start address directly from the DT ranges.

In the new DT parsing API of_pci_get_host_bridge_resources() hides the 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 to use the new DT parsing API but there is a bug (obviously) in setting the <*>_mod_base addresses
Applying this patch we can easily set "<*>_mod_base = win->__res.start"

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(); in of_pci_get_host_bridge_resources() it retrieves the resource entry after it is created and added to the resource list and uses entry->__res.start to store the pci controller address

the patch is based on 4.2-rc1

Signed-off-by Gabriele Paoloni <[email protected]>

Acked-by Liviu Dudau <[email protected]>

Acked-by Rob Herring <[email protected]>

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

drivers/of/address.c +2 -0

index 8bfda6a..23a5793 100644
@@ -253,6 +253,7 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 						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,
 	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);
 
 	parser->range += parser->np;

drivers/of/of_pci.c +4 -0

index 5751dc5..fe57030 100644
@@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 
 	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,
 		}
 
 		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
+		entry = list_last_entry(resources, struct resource_entry, node);
+		/* we are using __res for storing the PCI controller address */
+		entry->__res.start = range.bus_addr;
 	}
 
 	return 0;

include/linux/of_address.h +1 -0

index d88e81b..865f96e 100644
@@ -16,6 +16,7 @@ struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };

Signature · 1 line

1.9.1

Gabriele Paoloni

author

2015-07-29 16:04 UTC #1 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Hi Bjorn

Liviu and Rob already acked, do you think it is ok to merge this?

Cheers

Gab

Quoted 121 lines

-----Original Message-----
From: Gabriele Paoloni
Sent: Monday, July 27, 2015 4:17 PM
To: Gabriele Paoloni; arnd at arndb.de; lorenzo.pieralisi at arm.com; Wangzhou (B); bhelgaas at google.com; robh+dt at kernel.org; james.morse at arm.com; Liviu.Dudau at arm.com
Cc: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth)
Subject: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
From: gabriele paoloni <[email protected]>
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
in designware we have a problem as the PCI addresses in the PCIe
controller
address space are required in order to perform correct HW operation.
In order to solve this problem commit f4c55c5a3 "PCI: designware:
Program ATU with untranslated address" added code to read the PCIe controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources() hides
the
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
to use the new DT parsing API but there is a bug (obviously) in
setting
the <*>_mod_base addresses
Applying this patch we can easily set "<*>_mod_base = win-
Quoted 1 line
__res.start"
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();
in of_pci_get_host_bridge_resources() it retrieves the resource
entry
after it is created and added to the resource list and uses entry->__res.start to store the pci controller address
the patch is based on 4.2-rc1
Signed-off-by Gabriele Paoloni <[email protected]>
Acked-by Liviu Dudau <[email protected]>
Acked-by Rob Herring <[email protected]>
---
 drivers/of/address.c       | 2 ++
 drivers/of/of_pci.c        | 4 ++++
 include/linux/of_address.h | 1 +
 3 files changed, 7 insertions(+)
drivers/of/address.c +1 -0
index 8bfda6a..23a5793 100644
@@ -253,6 +253,7 @@ struct of_pci_range *of_pci_range_parser_one(struct
 of_pci_range_parser *parser,
 						struct of_pci_range *range)
 {
 	const int na = 3, ns = 2;
+	const int p_ns = of_n_size_cells(parser->node);
 
 	if (!range)
drivers/of/address.c +1 -0
 		return NULL;
@@ -265,6 +266,7 @@ struct of_pci_range *of_pci_range_parser_one(struct
 of_pci_range_parser *parser,
 	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);
drivers/of/address.c +0 -0
 	parser->range += parser->np;
drivers/of/of_pci.c +1 -0
index 5751dc5..fe57030 100644
@@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
 device_node *dev,
 
 	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)
drivers/of/of_pci.c +2 -0
 			snprintf(range_type, 4, " IO");
@@ -240,6 +241,9 @@ int of_pci_get_host_bridge_resources(struct
 device_node *dev,
 		}
 
 		pci_add_resource_offset(resources, res,	res->start -
 range.pci_addr);
+		entry = list_last_entry(resources, struct resource_entry,
 node);
+		/* we are using __res for storing the PCI controller
address */
drivers/of/of_pci.c +1 -0
+		entry->__res.start = range.bus_addr;
 	}
 
 	return 0;
include/linux/of_address.h +1 -1
index d88e81b..865f96e 100644
@@ -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

Bjorn Helgaas

maintainer

2015-07-29 17:20 UTC #2 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

In reply to Gabriele Paoloni

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.

Quoted 10 lines · Gabriele Paoloni

From: gabriele paoloni <[email protected]>
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
in designware we have a problem as the PCI addresses in the PCIe controller address space are required in order to perform correct HW operation.
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")

Quoted 10 lines

controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources() hides the 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 to use the new DT parsing API but there is a bug (obviously) in setting 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.

Quoted 5 lines

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(); in of_pci_get_host_bridge_resources() it retrieves the resource entry 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?

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

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.

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

bus_addr: ?

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 1 line

the patch is based on 4.2-rc1

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

Quoted 3 lines

Signed-off-by Gabriele Paoloni <[email protected]>
Acked-by Liviu Dudau <[email protected]>
Acked-by Rob Herring <[email protected]>

Please un-indent your changelog.

Quoted 67 lines

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

drivers/of/address.c +2 -0

index 8bfda6a..23a5793 100644
@@ -253,6 +253,7 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 						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,
 	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);
 
 	parser->range += parser->np;

drivers/of/of_pci.c +4 -0

index 5751dc5..fe57030 100644
@@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 
 	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,
 		}
 
 		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
+		entry = list_last_entry(resources, struct resource_entry, node);
+		/* we are using __res for storing the PCI controller address */
+		entry->__res.start = range.bus_addr;
 	}
 
 	return 0;

include/linux/of_address.h +1 -0

index d88e81b..865f96e 100644
@@ -16,6 +16,7 @@ struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };

Signature · 6 lines

1.9.1

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

Gabriele Paoloni

author

2015-07-29 19:44 UTC #3 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

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

Quoted 12 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range

Quoted 5 lines

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

Quoted 13 lines · Gabriele Paoloni

Quoted 5 lines
From: gabriele paoloni <[email protected]>
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 1 line
in designware we have a problem as the PCI addresses in the PCIe
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 1 line

Quoted 8 lines

Quoted 2 lines
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 11 lines

Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 4 lines
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 1 line
to use the new DT parsing API but there is a bug (obviously) in

Quoted 8 lines

setting
Quoted 3 lines
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 13 lines

Quoted 3 lines
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 1 line
in of_pci_get_host_bridge_resources() it retrieves the resource
entry
Quoted 2 lines
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:

Quoted 7 lines

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

Quoted 3 lines

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

Quoted 4 lines

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

Quoted 4 lines

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

Quoted 3 lines

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

Quoted 10 lines

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 4 lines

Quoted 1 line
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 6 lines

Quoted 4 lines
Signed-off-by Gabriele Paoloni <[email protected]>
Acked-by Liviu Dudau <[email protected]>
Acked-by Rob Herring <[email protected]>
Please un-indent your changelog.

Ok agreed

Quoted 77 lines

drivers/of/address.c struct of_pci_range
Quoted 12 lines
---
 drivers/of/address.c       | 2 ++
 drivers/of/of_pci.c        | 4 ++++
 include/linux/of_address.h | 1 +
 3 files changed, 7 insertions(+)
drivers/of/address.c +0 -0
index 8bfda6a..23a5793 100644
@@ -253,6 +253,7 @@ struct of_pci_range
*of_pci_range_parser_one(struct of_pci_range_parser *parser,
drivers/of/address.c struct of_pci_range
Quoted 8 lines
drivers/of/address.c +1 -0
 						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,
drivers/of/address.c struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
Quoted 5 lines
drivers/of/address.c +1 -0
 	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);
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct
Quoted 7 lines
drivers/of/address.c +0 -0
 	parser->range += parser->np;
drivers/of/of_pci.c +0 -0
index 5751dc5..fe57030 100644
@@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
device_node *dev,
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct
Quoted 8 lines
drivers/of/of_pci.c +1 -0
 	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,
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct · line 243
Quoted 3 lines
drivers/of/of_pci.c +0 -0
 		}
 
 		pci_add_resource_offset(resources, res,	res->start -
range.pci_addr);
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct · line 244
Quoted 1 line
drivers/of/of_pci.c +1 -0
+		entry = list_last_entry(resources, struct resource_entry,
node);
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct · line 245
Quoted 1 line
drivers/of/of_pci.c +1 -0
+		/* we are using __res for storing the PCI controller
address */
Quoted 19 lines
drivers/of/of_pci.c +1 -0
+		entry->__res.start = range.bus_addr;
 	}
 
 	return 0;
include/linux/of_address.h +1 -1
index d88e81b..865f96e 100644
@@ -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
Quoted 2 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-pci"
in
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas

maintainer

2015-07-29 21:47 UTC #4 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

[+cc Andrew]

Quoted 31 lines · Gabriele Paoloni

Quoted 25 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Quoted 4 lines · Gabriele Paoloni
From: gabriele paoloni <[email protected]>
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 1 line
in of_pci_get_host_bridge_resources() it retrieves the resource
entry
Quoted 2 lines
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;
Quoted 5 lines
Can you explain what all these things mean, and maybe even add one-line comments to the structure?
pci_space: The only uses I see are to determine whether to print "Prefetch". I don't see any real functionality that uses this.

Quoted 6 lines

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

I think pci_space should be removed and the users should test "range.flags & IORESOURCE_PREFETCH" instead. That's already set by of_bus_pci_get_flags(). This is separate from your current patch, of course.

29b635c00f3e ("of/pci: Provide support for parsing PCI DT ranges property") added struct of_pci_range, and even at the time, of_bus_pci_get_flags() set IORESOURCE_PREFETCH in of_pci_range.flags.

654837e8fe8d ("powerpc/pci: Use of_pci_range_parser helper in pci_process_bridge_OF_ranges") converted powerpc to use of_pci_range_parser() instead of parsing manually. It converted other references to look at struct of_pci_range.flags; I'm not sure why it didn't do that for the prefetch bit.

I copied Andrew in case there's some subtlety here.

Quoted 11 lines

Quoted 3 lines
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
Quoted 3 lines
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().

Quoted 7 lines

Yes correct
Quoted 1 line
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:

I can't quite parse this, but I do understand how a host bridge can translate CPU physical addresses to a different range of PCI bus addresses. What I don't understand is the difference between "pci_addr" and the "bus_addr" you're adding.

Quoted 32 lines

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
Quoted 22 lines
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 12 lines
include/linux/of_address.h +1 -0
index d88e81b..865f96e 100644
@@ -16,6 +16,7 @@ struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };

Gabriele Paoloni

author

2015-07-30 08:30 UTC #5 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 55 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 10:47 PM
To: Gabriele Paoloni
Cc: 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); Andrew Murray
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
[+cc Andrew]
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Quoted 3 lines · Gabriele Paoloni
From: gabriele paoloni <[email protected]>
This patch adds a new field in "struct of_pci_range" to store
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 5 lines
Quoted 5 lines
entry
Quoted 2 lines
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
Quoted 9 lines
Quoted 9 lines
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
Quoted 9 lines
Quoted 4 lines
comments to the structure?
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

Quoted 8 lines

Quoted 1 line
to the PCI address. if you're curious you can find a nice and quick
to read
Quoted 1 line
"guide" in http://devicetree.org/MPC5200:PCI
I think pci_space should be removed and the users should test "range.flags & IORESOURCE_PREFETCH" instead. That's already set by of_bus_pci_get_flags(). This is separate from your current patch, of course.

Ok so I'll do nothing in my patch about this; maybe I can propose a separate patch for this, but I cannot test it (I've got no microblaze and powerpc neither....)

Quoted 35 lines

29b635c00f3e ("of/pci: Provide support for parsing PCI DT ranges property") added struct of_pci_range, and even at the time, of_bus_pci_get_flags() set IORESOURCE_PREFETCH in of_pci_range.flags.
654837e8fe8d ("powerpc/pci: Use of_pci_range_parser helper in pci_process_bridge_OF_ranges") converted powerpc to use of_pci_range_parser() instead of parsing manually. It converted other references to look at struct of_pci_range.flags; I'm not sure why it didn't do that for the prefetch bit.
I copied Andrew in case there's some subtlety here.
Quoted 1 line
Quoted 1 line
pci_addr: I assume this is a PCI bus address, like what you would
see
Quoted 2 lines
Quoted 2 lines
if
you put an analyzer on the bus/link. This address could go in a
BAR.
Quoted 2 lines
Yes, this is the PCI start address of the range: phys.mid + phys.low
in the
Quoted 3 lines
guide mentioned above
Quoted 1 line
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 8 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 1 line
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between the

Quoted 8 lines

root
Quoted 1 line
complex and the CPU there can be intermediate translation layers:
I can't quite parse this, but I do understand how a host bridge can translate CPU physical addresses to a different range of PCI bus addresses. What I don't understand is the difference between "pci_addr"
and the "bus_addr" you're adding.

For example see:
http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6qdl.dtsi#L148

ranges = <0x00000800 0 0x01f00000 0x01f00000 0 0x00080000 /* configuration space */

          0x81000000 0 0          0x01f80000 0 0x00010000   /* downstream I/O */
          0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable memory *
               /\          /\          /\
           pci_space    pci_addr     bus_addr

The host bridge performs the first translation from pci_addr to bus_addr. If there are other ranges in the parents nodes in the DT bus_addr gets translated further till you get the cpu_addr.

Hope it is a bit clearer now....

Quoted 40 lines

Quoted 3 lines
see that to
get pci_address we call "of_translate_address"; this will apply all
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes to
the root
Quoted 2 lines
node of the DT (thus retrieving the CPU address). Now said that, for designware we need the first translated PCI
address, that we call
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 9 lines
different name
Quoted 5 lines
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
Quoted 2 lines
Quoted 2 lines
code
is OF, not ACPI, but I assume that it should be possible to
describe
Quoted 4 lines
Quoted 4 lines
your
hardware via ACPI as well as by OF.
Review
Quoted 1 line
(quoted diff) +0 -0
b/include/linux/of_address.h
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
index d88e81b..865f96e 100644
include/linux/of_address.h +1 -0
@@ -16,6 +16,7 @@ struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };

Liviu Dudau

2015-07-30 11:20 UTC #6 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Hello,

Quoted 38 lines · Gabriele Paoloni

Quoted 38 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 10:47 PM
To: Gabriele Paoloni
Cc: 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); Andrew Murray
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
[+cc Andrew]
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Quoted 3 lines · Gabriele Paoloni
From: gabriele paoloni <[email protected]>
This patch adds a new field in "struct of_pci_range" to store
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 5 lines
Quoted 5 lines
entry
Quoted 2 lines
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
Quoted 3 lines
Quoted 3 lines
me.
It now contains:

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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?

I can try to do that, as I worked with Andrew Murray when he did the patch.

- pci_space is indeed the range.flags variable and it was trying to keep the original value from the DT mainly to try to identify the config space in the ranges described. It has now become clear that declaring config space in the ranges area is wrong even if one supports ECAM so pci_space could be removed as suggested by Bjorn.
- pci_addr is the address that goes on the PCI(e) bus.
- cpu_addr is the translated address that the CPU uses. It does not necessarily means it is the same address that the Root Complex sees when requested to do Address Translation between host side and bus side. Also, what gets stored in the cpu_addr is not equal to what gets declared in the ranges property if there are other address translation parents between the Root Complex and the CPU.
- bus_addr is meant to be the un-translated cpu_addr that DesignWare needs in order to setup its ATS service. The reason for putting it in the of_pci_range is because the struct resource does not have the concept of an untranslated address.

Best regards,
Liviu

Quoted 123 lines

Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 1 line
to the PCI address. if you're curious you can find a nice and quick
to read
Quoted 1 line
"guide" in http://devicetree.org/MPC5200:PCI
I think pci_space should be removed and the users should test "range.flags & IORESOURCE_PREFETCH" instead. That's already set by of_bus_pci_get_flags(). This is separate from your current patch, of course.
Ok so I'll do nothing in my patch about this; maybe I can propose a separate patch for this, but I cannot test it (I've got no microblaze and powerpc neither....)
Quoted 35 lines
29b635c00f3e ("of/pci: Provide support for parsing PCI DT ranges property") added struct of_pci_range, and even at the time, of_bus_pci_get_flags() set IORESOURCE_PREFETCH in of_pci_range.flags.
654837e8fe8d ("powerpc/pci: Use of_pci_range_parser helper in pci_process_bridge_OF_ranges") converted powerpc to use of_pci_range_parser() instead of parsing manually. It converted other references to look at struct of_pci_range.flags; I'm not sure why it didn't do that for the prefetch bit.
I copied Andrew in case there's some subtlety here.
Quoted 1 line
Quoted 1 line
pci_addr: I assume this is a PCI bus address, like what you would
see
Quoted 2 lines
Quoted 2 lines
if
you put an analyzer on the bus/link. This address could go in a
BAR.
Quoted 2 lines
Yes, this is the PCI start address of the range: phys.mid + phys.low
in the
Quoted 3 lines
guide mentioned above
Quoted 1 line
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 8 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 1 line
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between the
Quoted 8 lines
root
Quoted 1 line
complex and the CPU there can be intermediate translation layers:
I can't quite parse this, but I do understand how a host bridge can translate CPU physical addresses to a different range of PCI bus addresses. What I don't understand is the difference between "pci_addr"
and the "bus_addr" you're adding.
For example see:
http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6qdl.dtsi#L148
ranges = <0x00000800 0 0x01f00000 0x01f00000 0 0x00080000 /* configuration space */
          0x81000000 0 0          0x01f80000 0 0x00010000   /* downstream I/O */
          0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable memory *
               /\          /\          /\
           pci_space    pci_addr     bus_addr            
The host bridge performs the first translation from pci_addr to bus_addr. If there are other ranges in the parents nodes in the DT bus_addr gets translated further till you get the cpu_addr.
Hope it is a bit clearer now....
Quoted 40 lines
Quoted 3 lines
see that to
get pci_address we call "of_translate_address"; this will apply all
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes to
the root
Quoted 2 lines
node of the DT (thus retrieving the CPU address). Now said that, for designware we need the first translated PCI
address, that we call
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 9 lines
different name
Quoted 5 lines
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
Quoted 2 lines
Quoted 2 lines
code
is OF, not ACPI, but I assume that it should be possible to
describe
Quoted 4 lines
Quoted 4 lines
your
hardware via ACPI as well as by OF.
Review
Quoted 1 line
(quoted diff) +0 -0
b/include/linux/of_address.h
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
index d88e81b..865f96e 100644
include/linux/of_address.h +1 -0
@@ -16,6 +16,7 @@ struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };

Signature · 8 lines

====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

Zhou Wang

2015-07-30 07:16 UTC #7 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

In reply to Gabriele Paoloni

Quoted 72 lines · Gabriele Paoloni

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
Quoted 12 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 5 lines
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
Quoted 13 lines · Gabriele Paoloni
Quoted 5 lines
From: gabriele paoloni <[email protected]>
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 1 line
in designware we have a problem as the PCI addresses in the PCIe
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
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 16 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 4 lines
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 1 line
to use the new DT parsing API but there is a bug (obviously) in
setting
Quoted 3 lines
the <*>_mod_base addresses
Applying this patch we can easily set "<*>_mod_base = win-
__res.start"

Quoted 7 lines

Quoted 3 lines
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

Hi Gab,

I can merge this patch in my series if this make it clearer to understand.

Thanks,
Zhou

Quoted 172 lines

Quoted 14 lines
Quoted 4 lines
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 1 line
in of_pci_get_host_bridge_resources() it retrieves the resource
entry
Quoted 2 lines
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:
Quoted 7 lines
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
Quoted 3 lines
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
Quoted 4 lines
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
Quoted 4 lines
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
Quoted 3 lines
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
Quoted 10 lines
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 4 lines
Quoted 1 line
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 6 lines
Quoted 4 lines
Signed-off-by Gabriele Paoloni <[email protected]>
Acked-by Liviu Dudau <[email protected]>
Acked-by Rob Herring <[email protected]>
Please un-indent your changelog.
Ok agreed
Quoted 72 lines
drivers/of/address.c struct of_pci_range
Quoted 12 lines
---
 drivers/of/address.c       | 2 ++
 drivers/of/of_pci.c        | 4 ++++
 include/linux/of_address.h | 1 +
 3 files changed, 7 insertions(+)
drivers/of/address.c +0 -0
index 8bfda6a..23a5793 100644
@@ -253,6 +253,7 @@ struct of_pci_range
*of_pci_range_parser_one(struct of_pci_range_parser *parser,
drivers/of/address.c struct of_pci_range
Quoted 8 lines
drivers/of/address.c +1 -0
 						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,
drivers/of/address.c struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
Quoted 5 lines
drivers/of/address.c +1 -0
 	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);
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct
Quoted 7 lines
drivers/of/address.c +0 -0
 	parser->range += parser->np;
drivers/of/of_pci.c +0 -0
index 5751dc5..fe57030 100644
@@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
device_node *dev,
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct
Quoted 8 lines
drivers/of/of_pci.c +1 -0
 	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,
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct · line 243
Quoted 3 lines
drivers/of/of_pci.c +0 -0
 		}
 
 		pci_add_resource_offset(resources, res,	res->start -
range.pci_addr);
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct · line 244
Quoted 1 line
drivers/of/of_pci.c +1 -0
+		entry = list_last_entry(resources, struct resource_entry,
node);
drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct · line 245
Quoted 1 line
drivers/of/of_pci.c +1 -0
+		/* we are using __res for storing the PCI controller
address */
Quoted 19 lines
drivers/of/of_pci.c +1 -0
+		entry->__res.start = range.bus_addr;
 	}
 
 	return 0;
include/linux/of_address.h +1 -1
index d88e81b..865f96e 100644
@@ -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
Quoted 5 lines
Quoted 2 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-pci"
in
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
.

Rob Herring

2015-07-30 13:42 UTC #8 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

In reply to Gabriele Paoloni

Quoted 133 lines · Gabriele Paoloni

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
Quoted 12 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 5 lines
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
Quoted 13 lines · Gabriele Paoloni
Quoted 5 lines
From: gabriele paoloni <[email protected]>
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 1 line
in designware we have a problem as the PCI addresses in the PCIe
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
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 11 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 4 lines
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 1 line
to use the new DT parsing API but there is a bug (obviously) in
Quoted 8 lines
setting
Quoted 3 lines
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 13 lines
Quoted 3 lines
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 1 line
in of_pci_get_host_bridge_resources() it retrieves the resource
entry
Quoted 2 lines
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:
Quoted 7 lines
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
Quoted 3 lines
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
Quoted 4 lines
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
Quoted 4 lines
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
Quoted 3 lines
bus_addr: ?

Quoted 7 lines

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

I think you mean "translated CPU address." The flow of addresses looks like this:

CPU -> CPU bus address -> bus fabric address decoding -> bus address -> 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 slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping

Quoted 2 lines

here bus_addr after Rob Herring suggested the name...honestly I cannot think of a 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 address? If so, we could express differently and just mask the CPU address within the driver instead.

Rob

Gabriele Paoloni

author

2015-07-30 13:52 UTC #9 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 192 lines

-----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 2:43 PM
To: Gabriele Paoloni
Cc: 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 7 lines · Gabriele Paoloni
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 20 lines
Cheers
Gab
Quoted 15 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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)
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"
Quoted 10 lines
Quoted 2 lines
in
the subject as a generic term that includes both PCI and PCIe.
sure agreed
Quoted 5 lines · Gabriele Paoloni
Quoted 3 lines
From: gabriele paoloni <[email protected]>
This patch is needed port PCIe designware to new DT parsing
API
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
As discussed in
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-
January/317743.html
Quoted 1 line
in designware we have a problem as the PCI addresses in the
PCIe
Quoted 5 lines
Quoted 5 lines
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 2 lines
In order to solve this problem commit f4c55c5a3 "PCI:
designware:
Quoted 16 lines
Quoted 7 lines
Quoted 1 line
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 6 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 1 line
DT parser from the host controller drivers, so it is not
possible
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
for drivers to parse values directly from the DT.
In http://www.spinics.net/lists/linux-pci/msg42540.html we
already tried
Quoted 1 line
to use the new DT parsing API but there is a bug (obviously)
in
Quoted 6 lines
Quoted 6 lines
setting
Quoted 3 lines
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
Quoted 9 lines
Quoted 2 lines
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 2 lines
Quoted 2 lines
This patch adds a new field in "struct of_pci_range" to store
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 5 lines
Quoted 5 lines
entry
Quoted 2 lines
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
Quoted 9 lines
Quoted 9 lines
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
Quoted 5 lines
Quoted 1 line
comments to the structure?
sure I can add comments inline in the code
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
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
Quoted 1 line
to the PCI address. if you're curious you can find a nice and quick
to read
Quoted 4 lines
"guide" in http://devicetree.org/MPC5200:PCI
Quoted 2 lines
pci_addr: I assume this is a PCI bus address, like what you would
see
Quoted 4 lines
Quoted 2 lines
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
Quoted 4 lines
guide mentioned above
Quoted 2 lines
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 10 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 3 lines
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between the
root
Quoted 1 line
complex and the CPU there can be intermediate translation layers: see
that to
Quoted 1 line
get pci_address we call "of_translate_address"; this will apply all
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes to
the root
Quoted 2 lines
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 like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus address -> 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 slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping

Quoted 8 lines

Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 1 line
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 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 PCI addresses...what if the intermediate translation layer changes the lower significant bits of the "bus address" to translate into a cpu address?

In that case we're going to program the DW with a wrong address

What I am saying is that the DW driver should not rely on the behavior of external HW....what do you think?

Quoted 6 lines

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni

author

2015-07-30 14:15 UTC #10 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 226 lines

-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci- owner at vger.kernel.org] On Behalf Of Gabriele Paoloni
Sent: Thursday, July 30, 2015 2:52 PM
To: Rob Herring
Cc: 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)
Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 15 lines
-----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 2:43 PM
To: Gabriele Paoloni
Cc: 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)
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
<[email protected]> wrote:
Quoted 19 lines
Quoted 7 lines
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 11 lines
Cheers
Gab
Quoted 6 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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-
Quoted 24 lines
Quoted 9 lines
Quoted 9 lines
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; devicetree at 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
"PCI"
Quoted 10 lines
Quoted 2 lines
in
the subject as a generic term that includes both PCI and PCIe.
sure agreed
Quoted 5 lines · Gabriele Paoloni
Quoted 3 lines
From: gabriele paoloni <[email protected]>
This patch is needed port PCIe designware to new DT parsing
API
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
As discussed in
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-
January/317743.html
Quoted 9 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
in designware we have a problem as the PCI addresses in the
PCIe
Quoted 5 lines
Quoted 5 lines
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 2 lines
In order to solve this problem commit f4c55c5a3 "PCI:
designware:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Program ATU with untranslated address" added code to read
the
Quoted 21 lines
Quoted 15 lines
Quoted 6 lines
PCIe
Conventional reference is 12-char SHA1, like this:
f4c55c5a3f7f ("PCI: designware: Program ATU with untranslated
address")
Agreed, will change this
Quoted 6 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 1 line
DT parser from the host controller drivers, so it is not
possible
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
for drivers to parse values directly from the DT.
In http://www.spinics.net/lists/linux-pci/msg42540.html we
already tried
Quoted 1 line
to use the new DT parsing API but there is a bug (obviously)
Quoted 17 lines
in
Quoted 6 lines
Quoted 6 lines
setting
Quoted 3 lines
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
Quoted 9 lines
Quoted 2 lines
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 2 lines
Quoted 2 lines
This patch adds a new field in "struct of_pci_range" to
store
Quoted 20 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 5 lines
Quoted 5 lines
entry
Quoted 2 lines
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
Quoted 9 lines
Quoted 9 lines
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 13 lines
line
Quoted 5 lines
Quoted 1 line
comments to the structure?
sure I can add comments inline in the code
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
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
Quoted 10 lines
associated
Quoted 1 line
to the PCI address. if you're curious you can find a nice and quick
to read
Quoted 4 lines
"guide" in http://devicetree.org/MPC5200:PCI
Quoted 2 lines
pci_addr: I assume this is a PCI bus address, like what you would
see
Quoted 2 lines
Quoted 2 lines
if
you put an analyzer on the bus/link. This address could go in a
BAR.
Quoted 2 lines
Quoted 2 lines
Yes, this is the PCI start address of the range: phys.mid +
phys.low
Quoted 9 lines
in the
Quoted 4 lines
guide mentioned above
Quoted 2 lines
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 3 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Quoted 9 lines
Quoted 7 lines
Yes correct
Quoted 3 lines
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between
the
Quoted 2 lines
root
Quoted 1 line
complex and the CPU there can be intermediate translation layers:
see
Quoted 4 lines
that to
Quoted 1 line
get pci_address we call "of_translate_address"; this will apply all
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes
to
Quoted 6 lines
the root
Quoted 2 lines
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 10 lines
like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus address -> 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 slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping
Quoted 8 lines
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 1 line
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 address? If so, we could express differently and just mask the CPU address within the driver instead.

Quoted 5 lines

I don?t think we should rely on PCI addresses...what if the intermediate
translation layer changes the lower significant bits of the "bus address"
to translate into a cpu address?

Sorry above I meant "I don?t think we should rely on CPU addresses"

Quoted 7 lines

In that case we're going to program the DW with a wrong address
What I am saying is that the DW driver should not rely on the behavior of external HW....what do you think?
Quoted 1 line

Quoted 7 lines

Quoted 3 lines
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-pci"
in
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
???

?&?~?&???+-

Quoted 1 line

?????w??????m?b??ir(?????}????z?&j:+v???

????zZ+??+zf???h???~????i???

Quoted 1 line

z?

?w????????&?)?f

Bjorn Helgaas

maintainer

2015-07-30 16:14 UTC #11 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

In reply to Gabriele Paoloni

Quoted 196 lines · Gabriele Paoloni

Quoted 196 lines
-----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 2:43 PM
To: Gabriele Paoloni
Cc: 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 7 lines · Gabriele Paoloni
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 20 lines
Cheers
Gab
Quoted 15 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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)
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"
Quoted 10 lines
Quoted 2 lines
in
the subject as a generic term that includes both PCI and PCIe.
sure agreed
Quoted 5 lines · Gabriele Paoloni
Quoted 3 lines
From: gabriele paoloni <[email protected]>
This patch is needed port PCIe designware to new DT parsing
API
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
As discussed in
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-
January/317743.html
Quoted 1 line
in designware we have a problem as the PCI addresses in the
PCIe
Quoted 5 lines
Quoted 5 lines
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 2 lines
In order to solve this problem commit f4c55c5a3 "PCI:
designware:
Quoted 16 lines
Quoted 7 lines
Quoted 1 line
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 6 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 1 line
DT parser from the host controller drivers, so it is not
possible
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
for drivers to parse values directly from the DT.
In http://www.spinics.net/lists/linux-pci/msg42540.html we
already tried
Quoted 1 line
to use the new DT parsing API but there is a bug (obviously)
in
Quoted 6 lines
Quoted 6 lines
setting
Quoted 3 lines
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
Quoted 9 lines
Quoted 2 lines
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 2 lines
Quoted 2 lines
This patch adds a new field in "struct of_pci_range" to store
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 5 lines
Quoted 5 lines
entry
Quoted 2 lines
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
Quoted 9 lines
Quoted 9 lines
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
Quoted 5 lines
Quoted 1 line
comments to the structure?
sure I can add comments inline in the code
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
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
Quoted 1 line
to the PCI address. if you're curious you can find a nice and quick
to read
Quoted 4 lines
"guide" in http://devicetree.org/MPC5200:PCI
Quoted 2 lines
pci_addr: I assume this is a PCI bus address, like what you would
see
Quoted 4 lines
Quoted 2 lines
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
Quoted 4 lines
guide mentioned above
Quoted 2 lines
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 10 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 3 lines
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between the
root
Quoted 1 line
complex and the CPU there can be intermediate translation layers: see
that to
Quoted 1 line
get pci_address we call "of_translate_address"; this will apply all
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes to
the root
Quoted 2 lines
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 like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus address -> 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 slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 1 line
different name

Quoted 8 lines

Quoted 4 lines
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 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 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 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.

Gabriele Paoloni

author

2015-07-30 16:50 UTC #12 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 244 lines

-----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
To: Gabriele Paoloni
Cc: Rob Herring; 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 8 lines · Gabriele Paoloni
Quoted 8 lines
-----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 2:43 PM
To: Gabriele Paoloni
Cc: Bjorn Helgaas; arnd at arndb.de; lorenzo.pieralisi at arm.com;
Wangzhou
Quoted 20 lines
Quoted 20 lines
(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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 7 lines · Gabriele Paoloni
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 3 lines
Cheers
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Gab
Quoted 6 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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-
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; devicetree at 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 11 lines
Quoted 11 lines
"PCI"
Quoted 10 lines
Quoted 2 lines
in
the subject as a generic term that includes both PCI and PCIe.
sure agreed
Quoted 5 lines · Gabriele Paoloni
Quoted 3 lines
From: gabriele paoloni <[email protected]>
This patch is needed port PCIe designware to new DT
parsing
Quoted 3 lines
Quoted 3 lines
API
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
As discussed in
http://lists.infradead.org/pipermail/linux-arm-
kernel/2015-
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
January/317743.html
Quoted 1 line
in designware we have a problem as the PCI addresses in
the
Quoted 8 lines
Quoted 8 lines
PCIe
Quoted 5 lines
Quoted 5 lines
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 2 lines
In order to solve this problem commit f4c55c5a3 "PCI:
designware:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Program ATU with untranslated address" added code to read
the
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
PCIe
Conventional reference is 12-char SHA1, like this:
f4c55c5a3f7f ("PCI: designware: Program ATU with untranslated
address")
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Agreed, will change this
Quoted 4 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API
of_pci_get_host_bridge_resources()
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
hides the
Quoted 1 line
DT parser from the host controller drivers, so it is not
possible
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
for drivers to parse values directly from the DT.
In http://www.spinics.net/lists/linux-pci/msg42540.html we
already tried
Quoted 1 line
to use the new DT parsing API but there is a bug
(obviously)
Quoted 7 lines
Quoted 7 lines
in
Quoted 6 lines
Quoted 6 lines
setting
Quoted 3 lines
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 10 lines
Quoted 10 lines
it
Quoted 9 lines
Quoted 2 lines
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 2 lines
Quoted 2 lines
This patch adds a new field in "struct of_pci_range" to
store
Quoted 7 lines
Quoted 7 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 2 lines
Quoted 2 lines
entry
Quoted 1 line
after it is created and added to the resource list and
uses
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
    entry->__res.start to store the pci controller address
struct of_pci_range is starting to get confusing to non-OF folks
like
Quoted 3 lines
Quoted 3 lines
me.
It now contains:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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 10 lines
Quoted 10 lines
line
Quoted 9 lines
Quoted 1 line
comments to the structure?
sure I can add comments inline in the code
Quoted 3 lines
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 2 lines
Quoted 2 lines
Quoted 2 lines
and microblaze to print out.
However from my understanding pci_space is the phys.hi field of
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
ranges property: it defines the properties of the address space
associated
Quoted 1 line
to the PCI address. if you're curious you can find a nice and
quick
Quoted 5 lines
Quoted 5 lines
to read
Quoted 4 lines
"guide" in http://devicetree.org/MPC5200:PCI
Quoted 2 lines
pci_addr: I assume this is a PCI bus address, like what you
would
Quoted 3 lines
Quoted 3 lines
see
Quoted 2 lines
Quoted 2 lines
if
you put an analyzer on the bus/link. This address could go in a
BAR.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Yes, this is the PCI start address of the range: phys.mid +
phys.low
Quoted 12 lines
Quoted 12 lines
in the
Quoted 4 lines
guide mentioned above
Quoted 2 lines
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 6 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between
the
Quoted 2 lines
Quoted 2 lines
root
Quoted 1 line
complex and the CPU there can be intermediate translation layers:
see
Quoted 2 lines
Quoted 2 lines
that to
Quoted 1 line
get pci_address we call "of_translate_address"; this will apply
all
Quoted 2 lines
Quoted 2 lines
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes
to
Quoted 6 lines
Quoted 6 lines
the root
Quoted 2 lines
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 3 lines
Quoted 3 lines
like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus
address
Quoted 4 lines
Quoted 4 lines
-> 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 9 lines
Quoted 9 lines
slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 1 line
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 3 lines
Quoted 2 lines
address? If so, we could express differently and just mask the CPU address within the driver instead.

Quoted 7 lines

Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the "bus
address"
Quoted 1 line
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

Quoted 14 lines

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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas

maintainer

2015-07-30 17:14 UTC #13 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

[+cc Jingoo, Pratyush]

Quoted 268 lines · Gabriele Paoloni

Quoted 242 lines
-----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
To: Gabriele Paoloni
Cc: Rob Herring; 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 8 lines · Gabriele Paoloni
Quoted 8 lines
-----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 2:43 PM
To: Gabriele Paoloni
Cc: Bjorn Helgaas; arnd at arndb.de; lorenzo.pieralisi at arm.com;
Wangzhou
Quoted 20 lines
Quoted 20 lines
(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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 7 lines · Gabriele Paoloni
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 3 lines
Cheers
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Gab
Quoted 6 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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-
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; devicetree at 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 11 lines
Quoted 11 lines
"PCI"
Quoted 10 lines
Quoted 2 lines
in
the subject as a generic term that includes both PCI and PCIe.
sure agreed
Quoted 5 lines · Gabriele Paoloni
Quoted 3 lines
From: gabriele paoloni <[email protected]>
This patch is needed port PCIe designware to new DT
parsing
Quoted 3 lines
Quoted 3 lines
API
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
As discussed in
http://lists.infradead.org/pipermail/linux-arm-
kernel/2015-
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
January/317743.html
Quoted 1 line
in designware we have a problem as the PCI addresses in
the
Quoted 8 lines
Quoted 8 lines
PCIe
Quoted 5 lines
Quoted 5 lines
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 2 lines
In order to solve this problem commit f4c55c5a3 "PCI:
designware:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Program ATU with untranslated address" added code to read
the
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
PCIe
Conventional reference is 12-char SHA1, like this:
f4c55c5a3f7f ("PCI: designware: Program ATU with untranslated
address")
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Agreed, will change this
Quoted 4 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API
of_pci_get_host_bridge_resources()
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
hides the
Quoted 1 line
DT parser from the host controller drivers, so it is not
possible
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
for drivers to parse values directly from the DT.
In http://www.spinics.net/lists/linux-pci/msg42540.html we
already tried
Quoted 1 line
to use the new DT parsing API but there is a bug
(obviously)
Quoted 7 lines
Quoted 7 lines
in
Quoted 6 lines
Quoted 6 lines
setting
Quoted 3 lines
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 10 lines
Quoted 10 lines
it
Quoted 9 lines
Quoted 2 lines
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 2 lines
Quoted 2 lines
This patch adds a new field in "struct of_pci_range" to
store
Quoted 7 lines
Quoted 7 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 2 lines
Quoted 2 lines
entry
Quoted 1 line
after it is created and added to the resource list and
uses
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
    entry->__res.start to store the pci controller address
struct of_pci_range is starting to get confusing to non-OF folks
like
Quoted 3 lines
Quoted 3 lines
me.
It now contains:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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 10 lines
Quoted 10 lines
line
Quoted 9 lines
Quoted 1 line
comments to the structure?
sure I can add comments inline in the code
Quoted 3 lines
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 2 lines
Quoted 2 lines
Quoted 2 lines
and microblaze to print out.
However from my understanding pci_space is the phys.hi field of
the
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
ranges property: it defines the properties of the address space
associated
Quoted 1 line
to the PCI address. if you're curious you can find a nice and
quick
Quoted 5 lines
Quoted 5 lines
to read
Quoted 4 lines
"guide" in http://devicetree.org/MPC5200:PCI
Quoted 2 lines
pci_addr: I assume this is a PCI bus address, like what you
would
Quoted 3 lines
Quoted 3 lines
see
Quoted 2 lines
Quoted 2 lines
if
you put an analyzer on the bus/link. This address could go in a
BAR.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Yes, this is the PCI start address of the range: phys.mid +
phys.low
Quoted 12 lines
Quoted 12 lines
in the
Quoted 4 lines
guide mentioned above
Quoted 2 lines
cpu_addr: I assume this is a CPU physical address, like what you
would
Quoted 6 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
bus_addr: ?
According to the guide above, this is the address into which the
pci_address
Quoted 1 line
get translated to and that is passed to the root complex. Between
the
Quoted 2 lines
Quoted 2 lines
root
Quoted 1 line
complex and the CPU there can be intermediate translation layers:
see
Quoted 2 lines
Quoted 2 lines
that to
Quoted 1 line
get pci_address we call "of_translate_address"; this will apply
all
Quoted 2 lines
Quoted 2 lines
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it comes
to
Quoted 6 lines
Quoted 6 lines
the root
Quoted 2 lines
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 3 lines
Quoted 3 lines
like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus
address
Quoted 4 lines
Quoted 4 lines
-> 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 9 lines
Quoted 9 lines
slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly I
cannot think of a
Quoted 1 line
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 3 lines
Quoted 2 lines
address? If so, we could express differently and just mask the CPU address within the driver instead.
Quoted 7 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the "bus
address"
Quoted 1 line
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

Quoted 8 lines

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.

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.

Quoted 2 lines

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

Quoted 13 lines

Quoted 13 lines
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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni

author

2015-07-30 17:34 UTC #14 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 335 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
To: Gabriele Paoloni
Cc: Rob Herring; 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
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
[+cc Jingoo, Pratyush]
Quoted 6 lines · Gabriele Paoloni
Quoted 6 lines
-----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
To: Gabriele Paoloni
Cc: Rob Herring; arnd at arndb.de; lorenzo.pieralisi at arm.com; Wangzhou
(B);
Quoted 10 lines
Quoted 10 lines
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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 2 lines · Gabriele Paoloni
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
-----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 2:43 PM
To: Gabriele Paoloni
Cc: Bjorn Helgaas; arnd at arndb.de; lorenzo.pieralisi at arm.com;
Wangzhou
Quoted 1 line
Quoted 1 line
(B); robh+dt at kernel.org; james.morse at arm.com;
Liviu.Dudau at arm.com;
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 7 lines · Gabriele Paoloni
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 11 lines
Cheers
Gab
Quoted 6 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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;
Quoted 3 lines
Quoted 3 lines
linux-
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org; Yuanzhichang; Zhudacai;
zhangjukuo;
Quoted 16 lines
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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 8 lines
Quoted 8 lines
"PCI"
Quoted 7 lines
Quoted 2 lines
in
the subject as a generic term that includes both PCI and PCIe.
sure agreed
Quoted 2 lines
On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni
Quoted 7 lines · wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
From: gabriele paoloni <[email protected]>
This patch is needed port PCIe designware to new DT
parsing
Quoted 3 lines
Quoted 3 lines
API
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
As discussed in
http://lists.infradead.org/pipermail/linux-arm-
Quoted 7 lines
Quoted 7 lines
kernel/2015-
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
January/317743.html
Quoted 1 line
in designware we have a problem as the PCI addresses in
the
Quoted 3 lines
Quoted 3 lines
PCIe
Quoted 2 lines
Quoted 2 lines
controller
Quoted 1 line
address space are required in order to perform correct
HW
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
operation.
Quoted 2 lines
In order to solve this problem commit f4c55c5a3 "PCI:
designware:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Program ATU with untranslated address" added code to
read
Quoted 6 lines
Quoted 6 lines
the
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
PCIe
Conventional reference is 12-char SHA1, like this:
f4c55c5a3f7f ("PCI: designware: Program ATU with
untranslated
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
address")
Agreed, will change this
Quoted 4 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API
of_pci_get_host_bridge_resources()
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
hides the
Quoted 1 line
DT parser from the host controller drivers, so it is
not
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
possible
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
for drivers to parse values directly from the DT.
In http://www.spinics.net/lists/linux-pci/msg42540.html
we
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
already tried
Quoted 1 line
to use the new DT parsing API but there is a bug
(obviously)
Quoted 4 lines
Quoted 4 lines
in
Quoted 3 lines
Quoted 3 lines
setting
Quoted 2 lines
the <*>_mod_base addresses
Applying this patch we can easily set "<*>_mod_base =
win-
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
__res.start"
By itself, this patch adds something. It would help me
understand
Quoted 3 lines
Quoted 3 lines
it
Quoted 2 lines
Quoted 2 lines
if
the *user* of this new something were in the same patch
series.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64
support"
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
I will ask Zhou Wang to include this patch in his patchset
Quoted 2 lines
Quoted 2 lines
This patch adds a new field in "struct of_pci_range" to
store
Quoted 3 lines
Quoted 3 lines
the
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
pci bus start address; it fills the field in
of_pci_range_parser_one();
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
in of_pci_get_host_bridge_resources() it retrieves the
resource
Quoted 2 lines
Quoted 2 lines
entry
Quoted 1 line
after it is created and added to the resource list and
uses
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
    entry->__res.start to store the pci controller address
struct of_pci_range is starting to get confusing to non-OF
folks
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
like
Quoted 9 lines
Quoted 9 lines
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
Quoted 8 lines
Quoted 8 lines
one-
Quoted 7 lines
Quoted 7 lines
line
Quoted 6 lines
Quoted 1 line
comments to the structure?
sure I can add comments inline in the code
Quoted 2 lines
pci_space: The only uses I see are to determine whether to
print
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Prefetch". I don't see any real functionality that uses
this.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Looking at the code I agree. it's seems to be used only in
powerpc
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
and microblaze to print out.
However from my understanding pci_space is the phys.hi field
of
Quoted 2 lines
Quoted 2 lines
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
ranges property: it defines the properties of the address
space
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
associated
Quoted 1 line
to the PCI address. if you're curious you can find a nice and
quick
Quoted 1 line
Quoted 1 line
to read
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
"guide" in http://devicetree.org/MPC5200:PCI
Quoted 2 lines
pci_addr: I assume this is a PCI bus address, like what you
would
Quoted 3 lines
Quoted 3 lines
see
Quoted 2 lines
Quoted 2 lines
if
you put an analyzer on the bus/link. This address could go
in a
Quoted 9 lines
Quoted 9 lines
BAR.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Yes, this is the PCI start address of the range: phys.mid +
phys.low
Quoted 5 lines
Quoted 5 lines
in the
Quoted 4 lines
guide mentioned above
Quoted 2 lines
cpu_addr: I assume this is a CPU physical address, like what
you
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
would
Quoted 10 lines
Quoted 2 lines
see
in /proc/iomem and what you would pass to ioremap().
Yes correct
Quoted 3 lines
bus_addr: ?
According to the guide above, this is the address into which
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
pci_address
Quoted 1 line
get translated to and that is passed to the root complex.
Between
Quoted 3 lines
Quoted 3 lines
the
Quoted 2 lines
Quoted 2 lines
root
Quoted 1 line
complex and the CPU there can be intermediate translation
layers:
Quoted 3 lines
Quoted 3 lines
see
Quoted 2 lines
Quoted 2 lines
that to
Quoted 1 line
get pci_address we call "of_translate_address"; this will
apply
Quoted 3 lines
Quoted 3 lines
all
Quoted 2 lines
Quoted 2 lines
the
Quoted 1 line
translation layers (ranges in the DT) that it finds till it
comes
Quoted 7 lines
Quoted 7 lines
to
Quoted 6 lines
Quoted 6 lines
the root
Quoted 2 lines
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
Quoted 8 lines
Quoted 8 lines
looks
Quoted 3 lines
Quoted 3 lines
like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus
address
Quoted 3 lines
Quoted 3 lines
-> DW PCI -> PCI address
This is quite common that an IP block does not see the full
address.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
It is unusual that the IP block needs to know its full address
on
Quoted 2 lines
Quoted 2 lines
the
Quoted 1 line
Quoted 1 line
slave side. It is quite common for the master side and the
kernel
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
deals with that all the time with DMA mapping
Quoted 1 line
here bus_addr after Rob Herring suggested the name...honestly
I
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
cannot think of a
Quoted 1 line
different name
Thinking about this some more, is this really a translation
versus
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
just a stripping of high bits? Does the DW IP have less than 32-
bits
Quoted 1 line
Quoted 1 line
address? If so, we could express differently and just mask the
CPU
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
Quoted 1 line
address within the driver instead.
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the "bus
address"
Quoted 1 line
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 17 lines
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
Quoted 6 lines
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
Quoted 1 line
it is much easier to remap directly using a bitmask...but I was not
sure
Quoted 1 line
and I didn't think about the problems that can arise with ACPI.
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.

Quoted 6 lines

Quoted 2 lines
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....

Jingoo, Pratyush what do you think?

Quoted 15 lines

Quoted 8 lines
Quoted 8 lines
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.
Quoted 4 lines
Quoted 4 lines
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
Quoted 2 lines
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Rob Herring

2015-07-30 20:41 UTC #15 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 9 lines · Gabriele Paoloni

Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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 24 lines

Quoted 24 lines
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the "bus
address"
Quoted 1 line
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 14 lines
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

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
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.

Quoted 9 lines

Quoted 9 lines
Quoted 5 lines
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
Quoted 1 line
it is much easier to remap directly using a bitmask...but I was not
sure
Quoted 1 line
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 14 lines

Quoted 7 lines
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.
Quoted 6 lines
Quoted 2 lines
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).

Quoted 6 lines

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

Gabriele Paoloni

author

2015-07-31 14:25 UTC #16 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

[+cc Kishon]

Quoted 58 lines

-----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 (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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the only
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 1 line
Quoted 1 line
Quoted 1 line

Quoted 8 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
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?

Quoted 66 lines

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 3 lines
Quoted 3 lines
Quoted 3 lines
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 1 line
Quoted 1 line
Quoted 1 line
cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map
to
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
<0x0000cafe to 0x0000cafe + size>, but I guess that for HW design
reasons
Quoted 1 line
it is much easier to remap directly using a bitmask...but I was
not
Quoted 2 lines
Quoted 2 lines
sure
Quoted 1 line
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 2 lines
Quoted 2 lines
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 8 lines
Quoted 8 lines
inside the window.
For the bits outside the window offset, I don't know enough to say whether masking is sufficient or safe.
Quoted 2 lines
If you think the bitmask is Ok then I can directly define it in designware and we can drop this patch.
Quoted 6 lines
Quoted 3 lines
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 1 line
I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so
clearing
Quoted 3 lines
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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kishon Vijay Abraham I

2015-07-31 14:57 UTC #17 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

+Arnd

Hi,

Quoted 66 lines · Gabriele Paoloni

[+cc Kishon]
Quoted 64 lines
-----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 (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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the only
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 2 lines
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?

Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) had this discussion [1] before we decided the current approach. It'll be good to check with Arnd too.

[1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/253528.html

Thanks
Kishon

Gabriele Paoloni

author

2015-07-31 15:09 UTC #18 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 99 lines

-----Original Message-----
From: Kishon Vijay Abraham I [mailto:kishon at ti.com]
Sent: Friday, July 31, 2015 3:57 PM
To: Gabriele Paoloni; Rob Herring
Cc: 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; Arnd Bergmann
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
+Arnd
Hi,
Quoted 8 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 46 lines
Quoted 46 lines
(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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the only
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
Quoted 7 lines
Quoted 7 lines
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should make
the
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
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.
Quoted 3 lines
Quoted 1 line
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 3 lines
in DW and mask it out in dra7xx_pcie_host_init()...
Kishon, would you be ok with that?

Quoted 8 lines

Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) had
this discussion [1] before we decided the current approach. It'll be good to
check with Arnd too.
[1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-
May/253528.html

In this patch you use the mask into designware....instead the approach proposed by Rob means to have the mask declared in the dra7 driver and you modified the pp members in dra7xx_pcie_host_init by masking them...

BTW good to have Arnd opinion too..

Quoted 3 lines

Thanks
Kishon

Jingoo Han

2015-08-03 14:41 UTC #19 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 82 lines · Gabriele Paoloni

Quoted 82 lines
-----Original Message-----
From: Kishon Vijay Abraham I [mailto:kishon at ti.com]
Sent: Friday, July 31, 2015 3:57 PM
To: Gabriele Paoloni; Rob Herring
Cc: 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; Arnd Bergmann
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
+Arnd
Hi,
Quoted 9 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 46 lines
Quoted 46 lines
(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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the only
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
Quoted 7 lines
Quoted 7 lines
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should make
the

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
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.

Yep, it is SOC specific code for dra7.
This is NOT a DW issue.

Quoted 13 lines

Quoted 13 lines
Quoted 2 lines
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 3 lines
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
this discussion [1] before we decided the current approach. It'll be good to
check with Arnd too.

Quoted 7 lines

Quoted 2 lines
[1] -> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-
May/253528.html
In this patch you use the mask into designware....instead the approach proposed by Rob means to have the mask declared in the dra7 driver and you modified the pp members in dra7xx_pcie_host_init by masking them...

I want to move that code to dra7 driver,
because that code is dra7-specific.

Best regards,
Jingoo Han

Quoted 4 lines

BTW good to have Arnd opinion too..
Quoted 3 lines
Thanks
Kishon

Rob Herring

2015-07-31 16:53 UTC #20 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

In reply to Kishon Vijay Abraham I

Quoted 77 lines · Kishon Vijay Abraham I

+Arnd
Hi,
Quoted 72 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 58 lines
-----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 (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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the only
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
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()...

Quoted 7 lines

Quoted 1 line
Kishon, would you be ok with that?
Initially I was using *base-mask* property from dt. Me and Arnd (cc'ed) had this discussion [1] before we decided the current approach. It'll be good to 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 fewer address bits are available. It can be used just for convenience 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 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 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

Gabriele Paoloni

author

2015-08-03 11:18 UTC #21 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Rob, Kishon what about the following solution?....

---
 drivers/pci/host/pci-dra7xx.c      | 12 ++++++++++++
 drivers/pci/host/pcie-designware.c |  9 +++------
 2 files changed, 15 insertions(+), 6 deletions(-)

drivers/pci/host/pci-dra7xx.c +12 -0

index 80db09e..bb2635f 100644
@@ -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)
 
 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))

drivers/pci/host/pcie-designware.c +3 -6

index 69486be..06c682b 100644
@@ -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;
 		}

Signature · 1 line

1.9.1

Quoted 127 lines

-----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
Quoted 13 lines · Kishon Vijay Abraham I
+Arnd
Hi,
Quoted 8 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 18 lines
Quoted 18 lines
Quoted 18 lines
(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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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
Quoted 23 lines · wrote:
Quoted 23 lines
Quoted 23 lines
[...]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the
only
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should make
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 5 lines
Quoted 3 lines
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 1 line
this discussion [1] before we decided the current approach. It'll be
good to
Quoted 3 lines
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 fewer address bits are available. It can be used just for convenience 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 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 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 the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jingoo Han

2015-08-04 04:19 UTC #22 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 6 lines · Gabriele Paoloni

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?

Best regards,
Jingoo Han

Quoted 196 lines

2 files changed, 15 insertions(+), 6 deletions(-)
drivers/pci/host/pci-dra7xx.c +12 -0
index 80db09e..bb2635f 100644
@@ -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)
 
 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))
drivers/pci/host/pcie-designware.c +3 -6
index 69486be..06c682b 100644
@@ -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;
       }
Signature · 1 line
1.9.1
Quoted 126 lines
-----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
Quoted 13 lines · Kishon Vijay Abraham I
+Arnd
Hi,
Quoted 9 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 18 lines
Quoted 18 lines
Quoted 18 lines
(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
Quoted 9 lines · Gabriele Paoloni
Quoted 9 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: 30 July 2015 18:15
Quoted 5 lines · Gabriele Paoloni
Quoted 5 lines
-----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
Quoted 23 lines · wrote:
Quoted 23 lines
Quoted 23 lines
[...]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if the
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 4 lines
address"
Quoted 1 line
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 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the
only
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should make
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 2 lines
Quoted 2 lines
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 5 lines
Quoted 3 lines
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 1 line
this discussion [1] before we decided the current approach. It'll be
good to
Quoted 3 lines
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 fewer address bits are available. It can be used just for convenience 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 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 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 the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni

author

2015-08-04 10:12 UTC #23 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 26 lines

-----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
Quoted 6 lines · Gabriele Paoloni
Rob, Kishon what about the following solution?....
---
drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++
drivers/pci/host/pcie-designware.c | 9 +++------
Hi Paoloni,

Quoted 7 lines

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?

Quoted 214 lines

Best regards,
Jingoo Han
drivers/pci/host/pci-
Quoted 3 lines
2 files changed, 15 insertions(+), 6 deletions(-)
drivers/pci/host/pci- renamed from drivers/pci/host/pci-dra7xx.c +0 -0
dra7xx.c
drivers/pci/host/pci-dra7xx.c static void dra7xx_pcie_enable_interrupts(struct
Quoted 12 lines
index 80db09e..bb2635f 100644
drivers/pci/host/pci-dra7xx.c +1 -0
@@ -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)
Review
Quoted 18 lines
drivers/pci/host/pci-dra7xx.c +11 -0
 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))
(quoted diff) +0 -0
b/drivers/pci/host/pcie-designware.c
Quoted 34 lines
index 69486be..06c682b 100644
drivers/pci/host/pcie-designware.c +3 -7
@@ -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;
       }
--
Quoted 18 lines
1.9.1
Quoted 15 lines
-----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
<[email protected]>
Quoted 28 lines
Quoted 28 lines
Quoted 13 lines · wrote:
+Arnd
Hi,
Quoted 9 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 13 lines
Quoted 13 lines
Quoted 13 lines
(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
Quoted 4 lines · Gabriele Paoloni
Quoted 4 lines
-----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
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
-----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
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
[...]
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if
the
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
intermediate
Quoted 1 line
translation layer changes the lower significant bits of the
"bus
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
address"
Quoted 1 line
to translate into a cpu address?
Is it really a possiblity that the lower bits could be
changed?
Quoted 26 lines
Quoted 26 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 2 lines
I've checked all the current deignware users DTs except "pci-
layerscape"
Quoted 9 lines
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
None of them modifies the lower bits. To be more precise the
only
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should make
the
Quoted 19 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 11 lines
hook.
Quoted 2 lines
Quoted 2 lines
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 5 lines
Quoted 3 lines
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 1 line
this discussion [1] before we decided the current approach. It'll
be
Quoted 7 lines
Quoted 7 lines
good to
Quoted 3 lines
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 1 line
Quoted 1 line
fewer address bits are available. It can be used just for
convenience
Quoted 2 lines
Quoted 2 lines
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 3 lines
Quoted 3 lines
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 7 lines
Quoted 7 lines
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 2 lines
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni

author

2015-08-06 13:52 UTC #24 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Hi all

This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to DRA7xx"

commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in "[PATCH v6 3/6] PCI: designware: Add ARM64 support"

Gab

Quoted 319 lines

-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
owner at vger.kernel.org] On Behalf Of Gabriele Paoloni
Sent: Tuesday, August 04, 2015 11:12 AM
To: Jingoo Han
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
Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 24 lines
-----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
Quoted 6 lines · Gabriele Paoloni
Rob, Kishon what about the following solution?....
---
drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++
drivers/pci/host/pcie-designware.c | 9 +++------
Hi Paoloni,
Quoted 7 lines
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?
Quoted 13 lines
Best regards,
Jingoo Han
drivers/pci/host/pci-
Quoted 3 lines
2 files changed, 15 insertions(+), 6 deletions(-)
drivers/pci/host/pci- renamed from drivers/pci/host/pci-dra7xx.c +0 -0
dra7xx.c
Quoted 5 lines
index 80db09e..bb2635f 100644
drivers/pci/host/pci-dra7xx.c +0 -0
@@ -61,6 +61,7 @@
 
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c static void
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c +1 -0
 #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
Quoted 12 lines
pcie_port *pp)
Quoted 11 lines
drivers/pci/host/pci-dra7xx.c +8 -0
 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;
Quoted 53 lines
Review
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c +3 -0
+        pp->cfg1_mod_base &= CPU_TO_BUS_ADDR;
+    }
+
   dw_pcie_setup_rc(pp);
   dra7xx_pcie_establish_link(pp);
   if (IS_ENABLED(CONFIG_PCI_MSI))
(quoted diff) +0 -0
b/drivers/pci/host/pcie-designware.c
Quoted 45 lines
index 69486be..06c682b 100644
drivers/pci/host/pcie-designware.c +3 -7
@@ -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 8 lines
-----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;
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 18 lines
Quoted 1 line
Quoted 1 line
On Fri, Jul 31, 2015 at 9:57 AM, Kishon Vijay Abraham I
<[email protected]>
Quoted 16 lines
Quoted 16 lines
Quoted 13 lines · wrote:
+Arnd
Hi,
Quoted 9 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 1 line
Quoted 1 line
Quoted 1 line
(B); robh+dt at kernel.org; james.morse at arm.com;
Liviu.Dudau at arm.com;
Quoted 18 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
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
Quoted 4 lines · Gabriele Paoloni
Quoted 4 lines
-----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
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
-----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
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
[...]
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
intermediate
Quoted 1 line
translation layer changes the lower significant bits of
the
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
"bus
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
address"
Quoted 1 line
to translate into a cpu address?
Is it really a possiblity that the lower bits could be
changed?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
I've checked all the current deignware users DTs except
"pci-
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
layerscape"
Quoted 10 lines
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 4 lines
Quoted 4 lines
Quoted 4 lines
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should
make
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 4 lines
hook.
Quoted 2 lines
Quoted 2 lines
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 12 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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 1 line
this discussion [1] before we decided the current approach. It'll
be
Quoted 4 lines
Quoted 4 lines
good to
Quoted 3 lines
check with Arnd too.
[1] -> http://lists.infradead.org/pipermail/linux-arm-
kernel/2014-
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
May/253528.html
The problem I have here is the use of ranges does not necessarily
mean
Quoted 1 line
Quoted 1 line
fewer address bits are available. It can be used just for
convenience
Quoted 1 line
Quoted 1 line
of not putting the full address into every node's reg property.
And
Quoted 8 lines
Quoted 1 line
Quoted 1 line
vice versa, there are probably plenty of cases where we have the
full
Quoted 3 lines
Quoted 3 lines
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 2 lines
Quoted 2 lines
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
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
the kernel later if this becomes a common need.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-
pci"
Quoted 3 lines
in
Quoted 2 lines
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
????????+-???w??????ir(???}????j:+v????zZ+??zf? ?????i??z?????????f

Jingoo Han

2015-08-06 15:06 UTC #25 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 5 lines · Gabriele Paoloni

Hi all
This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to DRA7xx"

To Zhou Wang,

Please send your patches to 'jingoohan1 at gmail.com', instead of 'jg1.han at samsung.com'. Even though, I have done mainline works for Samsung SoCs, Samsung does not support me as a maintainer. So, please don't send your patches to my Samsung e-mail.

Best regards,
Jingoo Han

Quoted 323 lines

commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in "[PATCH v6 3/6] PCI: designware: Add ARM64 support"
Gab
Quoted 317 lines
-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
owner at vger.kernel.org] On Behalf Of Gabriele Paoloni
Sent: Tuesday, August 04, 2015 11:12 AM
To: Jingoo Han
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
Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 24 lines
-----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
Quoted 6 lines · Gabriele Paoloni
Rob, Kishon what about the following solution?....
---
drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++
drivers/pci/host/pcie-designware.c | 9 +++------
Hi Paoloni,
Quoted 7 lines
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?
Quoted 13 lines
Best regards,
Jingoo Han
drivers/pci/host/pci-
Quoted 3 lines
2 files changed, 15 insertions(+), 6 deletions(-)
drivers/pci/host/pci- renamed from drivers/pci/host/pci-dra7xx.c +0 -0
dra7xx.c
Quoted 5 lines
index 80db09e..bb2635f 100644
drivers/pci/host/pci-dra7xx.c +0 -0
@@ -61,6 +61,7 @@
 
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c static void
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c +1 -0
 #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
Quoted 12 lines
pcie_port *pp)
Quoted 11 lines
drivers/pci/host/pci-dra7xx.c +8 -0
 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;
Quoted 53 lines
Review
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c +3 -0
+        pp->cfg1_mod_base &= CPU_TO_BUS_ADDR;
+    }
+
   dw_pcie_setup_rc(pp);
   dra7xx_pcie_establish_link(pp);
   if (IS_ENABLED(CONFIG_PCI_MSI))
(quoted diff) +0 -0
b/drivers/pci/host/pcie-designware.c
Quoted 45 lines
index 69486be..06c682b 100644
drivers/pci/host/pcie-designware.c +3 -7
@@ -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 8 lines
-----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;
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 18 lines
Quoted 1 line
Quoted 1 line
On Fri, Jul 31, 2015 at 9:57 AM, Kishon Vijay Abraham I
<[email protected]>
Quoted 16 lines
Quoted 16 lines
Quoted 13 lines · wrote:
+Arnd
Hi,
Quoted 9 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 1 line
Quoted 1 line
Quoted 1 line
(B); robh+dt at kernel.org; james.morse at arm.com;
Liviu.Dudau at arm.com;
Quoted 18 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
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
Quoted 4 lines · Gabriele Paoloni
Quoted 4 lines
-----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
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
-----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
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
[...]
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
intermediate
Quoted 1 line
translation layer changes the lower significant bits of
the
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
"bus
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
address"
Quoted 1 line
to translate into a cpu address?
Is it really a possiblity that the lower bits could be
changed?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
I've checked all the current deignware users DTs except
"pci-
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
layerscape"
Quoted 10 lines
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 4 lines
Quoted 4 lines
Quoted 4 lines
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should
make
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 4 lines
hook.
Quoted 2 lines
Quoted 2 lines
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 12 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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 1 line
this discussion [1] before we decided the current approach. It'll
be
Quoted 4 lines
Quoted 4 lines
good to
Quoted 3 lines
check with Arnd too.
[1] -> http://lists.infradead.org/pipermail/linux-arm-
kernel/2014-
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
May/253528.html
The problem I have here is the use of ranges does not necessarily
mean
Quoted 1 line
Quoted 1 line
fewer address bits are available. It can be used just for
convenience
Quoted 1 line
Quoted 1 line
of not putting the full address into every node's reg property.
And
Quoted 8 lines
Quoted 1 line
Quoted 1 line
vice versa, there are probably plenty of cases where we have the
full
Quoted 3 lines
Quoted 3 lines
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 2 lines
Quoted 2 lines
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
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
the kernel later if this becomes a common need.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-
pci"
Quoted 3 lines
in
Quoted 2 lines
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Zhou Wang

2015-08-07 05:46 UTC #26 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

Quoted 7 lines · Jingoo Han

Quoted 5 lines · Gabriele Paoloni
Hi all
This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to DRA7xx"

Quoted 9 lines

To Zhou Wang,
Please send your patches to 'jingoohan1 at gmail.com', instead of 'jg1.han at samsung.com'. Even though, I have done mainline works for Samsung SoCs, Samsung does not support me as a maintainer. So, please don't send your patches to my Samsung e-mail.
Best regards,
Jingoo Han

Hi Jingoo,

Sorry for that, I will add jingoohan1 at gmail.com to v6 thread.

Thanks for reminding,
Zhou

Quoted 327 lines

Quoted 323 lines
commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in "[PATCH v6 3/6] PCI: designware: Add ARM64 support"
Gab
Quoted 317 lines
-----Original Message-----
From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
owner at vger.kernel.org] On Behalf Of Gabriele Paoloni
Sent: Tuesday, August 04, 2015 11:12 AM
To: Jingoo Han
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
Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 24 lines
-----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
Quoted 6 lines · Gabriele Paoloni
Rob, Kishon what about the following solution?....
---
drivers/pci/host/pci-dra7xx.c | 12 ++++++++++++
drivers/pci/host/pcie-designware.c | 9 +++------
Hi Paoloni,
Quoted 7 lines
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?
Quoted 13 lines
Best regards,
Jingoo Han
drivers/pci/host/pci-
Quoted 3 lines
2 files changed, 15 insertions(+), 6 deletions(-)
drivers/pci/host/pci- renamed from drivers/pci/host/pci-dra7xx.c +0 -0
dra7xx.c
Quoted 5 lines
index 80db09e..bb2635f 100644
drivers/pci/host/pci-dra7xx.c +0 -0
@@ -61,6 +61,7 @@
 
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c static void
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c +1 -0
 #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
Quoted 12 lines
pcie_port *pp)
Quoted 11 lines
drivers/pci/host/pci-dra7xx.c +8 -0
 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;
Quoted 53 lines
Review
Quoted 7 lines
drivers/pci/host/pci-dra7xx.c +3 -0
+        pp->cfg1_mod_base &= CPU_TO_BUS_ADDR;
+    }
+
   dw_pcie_setup_rc(pp);
   dra7xx_pcie_establish_link(pp);
   if (IS_ENABLED(CONFIG_PCI_MSI))
(quoted diff) +0 -0
b/drivers/pci/host/pcie-designware.c
Quoted 45 lines
index 69486be..06c682b 100644
drivers/pci/host/pcie-designware.c +3 -7
@@ -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 8 lines
-----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;
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 18 lines
Quoted 1 line
Quoted 1 line
On Fri, Jul 31, 2015 at 9:57 AM, Kishon Vijay Abraham I
<[email protected]>
Quoted 16 lines
Quoted 16 lines
Quoted 13 lines · wrote:
+Arnd
Hi,
Quoted 9 lines · Gabriele Paoloni
[+cc Kishon]
Quoted 6 lines
-----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 1 line
Quoted 1 line
Quoted 1 line
(B); robh+dt at kernel.org; james.morse at arm.com;
Liviu.Dudau at arm.com;
Quoted 18 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
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
Quoted 4 lines · Gabriele Paoloni
Quoted 4 lines
-----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
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
-----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
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
[...]
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
I don?t think we should rely on [CPU] addresses...what if
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
intermediate
translation layer changes the lower significant bits of
the
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
"bus
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
address"
to translate into a cpu address?
Is it really a possiblity that the lower bits could be
changed?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
I've checked all the current deignware users DTs except
"pci-
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
layerscape"
Quoted 10 lines
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 4 lines
Quoted 4 lines
Quoted 4 lines
guy
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
axi1
http://lxr.free-
electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For this case masking the top 4bits (bits28 to 31) should
make
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
job.
IMO, we should just fix this case. After further study, I don't
think
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 4 lines
hook.
Quoted 2 lines
Quoted 2 lines
Yes I guess that I could just assign pp->(*)_mod_base to the CPU
address
Quoted 12 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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 1 line
this discussion [1] before we decided the current approach. It'll
be
Quoted 4 lines
Quoted 4 lines
good to
Quoted 3 lines
check with Arnd too.
[1] -> http://lists.infradead.org/pipermail/linux-arm-
kernel/2014-
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
May/253528.html
The problem I have here is the use of ranges does not necessarily
mean
Quoted 1 line
Quoted 1 line
fewer address bits are available. It can be used just for
convenience
Quoted 1 line
Quoted 1 line
of not putting the full address into every node's reg property.
And
Quoted 8 lines
Quoted 1 line
Quoted 1 line
vice versa, there are probably plenty of cases where we have the
full
Quoted 3 lines
Quoted 3 lines
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 2 lines
Quoted 2 lines
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
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
the kernel later if this becomes a common need.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-
pci"
Quoted 3 lines
in
Quoted 2 lines
Quoted 2 lines
the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
.

Bjorn Helgaas

maintainer

2015-07-30 16:06 UTC #27 lore raw

PATCH v6 1/1 PCI: Store PCIe bus address in struct of_pci_range

In reply to Rob Herring

Quoted 154 lines · Rob Herring

Quoted 133 lines · Gabriele Paoloni
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
Quoted 12 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:bhelgaas at google.com]
Sent: Wednesday, July 29, 2015 6:21 PM
To: Gabriele Paoloni
Cc: 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)
Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Quoted 5 lines
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
Quoted 13 lines · Gabriele Paoloni
Quoted 5 lines
From: gabriele paoloni <[email protected]>
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 1 line
in designware we have a problem as the PCI addresses in the PCIe
controller
Quoted 1 line
address space are required in order to perform correct HW
operation.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
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 11 lines
Quoted 4 lines
controller start address directly from the DT ranges.
In the new DT parsing API of_pci_get_host_bridge_resources()
hides the
Quoted 4 lines
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 1 line
to use the new DT parsing API but there is a bug (obviously) in
Quoted 8 lines
setting
Quoted 3 lines
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 13 lines
Quoted 3 lines
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 1 line
in of_pci_get_host_bridge_resources() it retrieves the resource
entry
Quoted 2 lines
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:
Quoted 7 lines
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
Quoted 3 lines
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
Quoted 4 lines
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
Quoted 4 lines
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
Quoted 3 lines
bus_addr: ?
Quoted 7 lines
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
I think you mean "translated CPU address." The flow of addresses looks like this:
CPU -> CPU bus address -> bus fabric address decoding -> bus address -> 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 slave side. It is quite common for the master side and the kernel deals with that all the time with DMA mapping

Quoted 7 lines

Quoted 2 lines
here bus_addr after Rob Herring suggested the name...honestly I cannot think of a 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 address? If so, we could express differently and just mask the CPU address within the driver instead.

I would like this much better if it would be sufficient.

If I understand this correctly, this is all for the MMIO path, i.e., it has nothing to do with DMA addresses generated by the device being translated to main memory addresses.

ACPI has a fairly good abstract model for describing the address translations the kernel and drivers need to know about. We should assume we'll need to describe this hardware via ACPI in addition to DT eventually, so I'm trying to figure out how we would map this into ACPI terms.

If the driver really needs this intermediate address (the "bus address" above), I think that would mean adding a second ACPI bridge device. The first bridge would have a _TRA showing the offset from CPU physical address to bus address, and the second bridge would have a _TRA showing the offset from bus address to PCI address.

If you only had a single ACPI bridge device, you'd only have one _TRA (from CPU physical to PCI bus address), and there'd be no way for the driver to learn about the intermediate bus address.