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

Commit Message

Gabriele Paoloni July 27, 2015, 3:17 p.m. UTC

From: gabriele paoloni <[e-mail]>

    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 <[e-mail]>
    Acked-by: Liviu Dudau <[e-mail]>
    Acked-by: Rob Herring <[e-mail]>
---
 drivers/of/address.c       | 2 ++
 drivers/of/of_pci.c        | 4 ++++
 include/linux/of_address.h | 1 +
 3 files changed, 7 insertions(+)

Comments

Gabriele Paoloni July 29, 2015, 4:04 p.m. UTC | #1

Hi Bjorn

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

Cheers

Gab

> -----Original Message-----
> From: Gabriele Paoloni
> Sent: Monday, July 27, 2015 4:17 PM
> To: Gabriele Paoloni; [e-mail]; [e-mail];
> Wangzhou (B); [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth)
> Subject: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
> 
> From: gabriele paoloni <[e-mail]>
> 
>     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 <[e-mail]>
>     Acked-by: Liviu Dudau <[e-mail]>
>     Acked-by: Rob Herring <[e-mail]>
> ---
>  drivers/of/address.c       | 2 ++
>  drivers/of/of_pci.c        | 4 ++++
>  include/linux/of_address.h | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 8bfda6a..23a5793 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -253,6 +253,7 @@ struct of_pci_range *of_pci_range_parser_one(struct
> of_pci_range_parser *parser,
>  						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;
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 5751dc5..fe57030 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
> device_node *dev,
> 
>  	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;
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index d88e81b..865f96e 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -16,6 +16,7 @@ struct of_pci_range {
>  	u32 pci_space;
>  	u64 pci_addr;
>  	u64 cpu_addr;
> +	u64 bus_addr;
>  	u64 size;
>  	u32 flags;
>  };
> --
> 1.9.1

Bjorn Helgaas July 29, 2015, 5:20 p.m. UTC | #2

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.

On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <[e-mail]>
> 
>     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")

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

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

>     the patch is based on 4.2-rc1

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

>     Signed-off-by: Gabriele Paoloni <[e-mail]>
>     Acked-by: Liviu Dudau <[e-mail]>
>     Acked-by: Rob Herring <[e-mail]>

Please un-indent your changelog.

> ---
>  drivers/of/address.c       | 2 ++
>  drivers/of/of_pci.c        | 4 ++++
>  include/linux/of_address.h | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 8bfda6a..23a5793 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -253,6 +253,7 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>  						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;
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 5751dc5..fe57030 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
>  
>  	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;
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index d88e81b..865f96e 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -16,6 +16,7 @@ struct of_pci_range {
>  	u32 pci_space;
>  	u64 pci_addr;
>  	u64 cpu_addr;
> +	u64 bus_addr;
>  	u64 size;
>  	u32 flags;
>  };
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni July 29, 2015, 7:44 p.m. UTC | #3

Hi Bjorn

Many Thanks for your reply

I have commented back inline with resolutions from my side.

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

Cheers

Gab

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[e-mail]]
> Sent: Wednesday, July 29, 2015 6:21 PM
> To: Gabriele Paoloni
> Cc: [e-mail]; [e-mail]; Wangzhou (B);
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth)
> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> of_pci_range
> 
> Hi Gabriele,
> 
> As far as I can tell, this is not specific to PCIe, so please use "PCI"
> in
> the subject as a generic term that includes both PCI and PCIe.

sure agreed

> 
> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > From: gabriele paoloni <[e-mail]>
> >
> >     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")

Agreed, will change this

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

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


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

sure I can add comments inline in the code

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

Looking at the code I agree. it's seems to be used only in powerpc 
and microblaze to print out.
However from my understanding pci_space is the phys.hi field of the 
ranges property: it defines the properties of the address space associated
to the PCI address. if you're curious you can find a nice and quick to read
"guide" in http://devicetree.org/MPC5200:PCI

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

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

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

Yes correct

> 
> bus_addr: ?
> 

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

   

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

> 
> >     Signed-off-by: Gabriele Paoloni <[e-mail]>
> >     Acked-by: Liviu Dudau <[e-mail]>
> >     Acked-by: Rob Herring <[e-mail]>
> 
> Please un-indent your changelog.

Ok agreed

> 
> > ---
> >  drivers/of/address.c       | 2 ++
> >  drivers/of/of_pci.c        | 4 ++++
> >  include/linux/of_address.h | 1 +
> >  3 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 8bfda6a..23a5793 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -253,6 +253,7 @@ struct of_pci_range
> *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> >  						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;
> > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> > index 5751dc5..fe57030 100644
> > --- a/drivers/of/of_pci.c
> > +++ b/drivers/of/of_pci.c
> > @@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
> device_node *dev,
> >
> >  	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;
> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > index d88e81b..865f96e 100644
> > --- a/include/linux/of_address.h
> > +++ b/include/linux/of_address.h
> > @@ -16,6 +16,7 @@ struct of_pci_range {
> >  	u32 pci_space;
> >  	u64 pci_addr;
> >  	u64 cpu_addr;
> > +	u64 bus_addr;
> >  	u64 size;
> >  	u32 flags;
> >  };
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas July 29, 2015, 9:47 p.m. UTC | #4

[+cc Andrew]

On Wed, Jul 29, 2015 at 07:44:18PM +0000, Gabriele Paoloni wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[e-mail]]
> > Sent: Wednesday, July 29, 2015 6:21 PM
> > To: Gabriele Paoloni

> > On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > > From: gabriele paoloni <[e-mail]>

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

> > pci_addr: I assume this is a PCI bus address, like what you would see
> > if
> > you put an analyzer on the bus/link.  This address could go in a BAR.
> 
> Yes, this is the PCI start address of the range: phys.mid + phys.low in the
> guide mentioned above
> 
> > cpu_addr: I assume this is a CPU physical address, like what you would
> > see
> > in /proc/iomem and what you would pass to ioremap().
> 
> Yes correct
> 
> > bus_addr: ?
> 
> According to the guide above, this is the address into which the pci_address 
> get translated to and that is passed to the root complex. Between the root 
> complex and the CPU there can be intermediate translation layers: 

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.

> see that to 
> get pci_address we call "of_translate_address"; this will apply all the 
> translation layers (ranges in the DT) that it finds till it comes to the root 
> node of the DT (thus retrieving the CPU address).
> Now said that, for designware we need the first translated PCI address, that we call
> here bus_addr after Rob Herring suggested the name...honestly I cannot think of a 
> different name
> 
>    
> 
> > I'm trying to imagine how this might be expressed in ACPI.  A host
> > bridge
> > ACPI _CRS contains a CPU physical address and applying a _TRA
> > (translation
> > offset) to the CPU address gives you a PCI bus address.  I know this
> > code
> > is OF, not ACPI, but I assume that it should be possible to describe
> > your
> > hardware via ACPI as well as by OF.

> > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > > index d88e81b..865f96e 100644
> > > --- a/include/linux/of_address.h
> > > +++ b/include/linux/of_address.h
> > > @@ -16,6 +16,7 @@ struct of_pci_range {
> > >  	u32 pci_space;
> > >  	u64 pci_addr;
> > >  	u64 cpu_addr;
> > > +	u64 bus_addr;
> > >  	u64 size;
> > >  	u32 flags;
> > >  };

Zhou Wang July 30, 2015, 7:16 a.m. UTC | #5

On 2015/7/30 3:44, Gabriele Paoloni wrote:
> Hi Bjorn
> 
> Many Thanks for your reply
> 
> I have commented back inline with resolutions from my side.
> 
> If you're ok with them I'll send it out a new version in the appropriate patchset
> 
> Cheers
> 
> Gab
> 
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:[e-mail]]
>> Sent: Wednesday, July 29, 2015 6:21 PM
>> To: Gabriele Paoloni
>> Cc: [e-mail]; [e-mail]; Wangzhou (B);
>> [e-mail]; [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
>> qiuzhenfa; Liguozhu (Kenneth)
>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>> of_pci_range
>>
>> Hi Gabriele,
>>
>> As far as I can tell, this is not specific to PCIe, so please use "PCI"
>> in
>> the subject as a generic term that includes both PCI and PCIe.
> 
> sure agreed
> 
>>
>> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
>>> From: gabriele paoloni <[e-mail]>
>>>
>>>     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")
> 
> Agreed, will change this
> 
>>
>>>     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.
> 
> 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

> 
>>
>>>     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?
> 
> sure I can add comments inline in the code
> 
>>
>> pci_space: The only uses I see are to determine whether to print
>> "Prefetch".  I don't see any real functionality that uses this.
> 
> Looking at the code I agree. it's seems to be used only in powerpc 
> and microblaze to print out.
> However from my understanding pci_space is the phys.hi field of the 
> ranges property: it defines the properties of the address space associated
> to the PCI address. if you're curious you can find a nice and quick to read
> "guide" in http://devicetree.org/MPC5200:PCI
> 
>>
>> pci_addr: I assume this is a PCI bus address, like what you would see
>> if
>> you put an analyzer on the bus/link.  This address could go in a BAR.
> 
> Yes, this is the PCI start address of the range: phys.mid + phys.low in the
> guide mentioned above
> 
>>
>> cpu_addr: I assume this is a CPU physical address, like what you would
>> see
>> in /proc/iomem and what you would pass to ioremap().
> 
> Yes correct
> 
>>
>> bus_addr: ?
>>
> 
> According to the guide above, this is the address into which the pci_address 
> get translated to and that is passed to the root complex. Between the root 
> complex and the CPU there can be intermediate translation layers: see that to 
> get pci_address we call "of_translate_address"; this will apply all the 
> translation layers (ranges in the DT) that it finds till it comes to the root 
> node of the DT (thus retrieving the CPU address).
> Now said that, for designware we need the first translated PCI address, that we call
> here bus_addr after Rob Herring suggested the name...honestly I cannot think of a 
> different name
> 
>    
> 
>> I'm trying to imagine how this might be expressed in ACPI.  A host
>> bridge
>> ACPI _CRS contains a CPU physical address and applying a _TRA
>> (translation
>> offset) to the CPU address gives you a PCI bus address.  I know this
>> code
>> is OF, not ACPI, but I assume that it should be possible to describe
>> your
>> hardware via ACPI as well as by OF.
>>
>>>     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
> 
>>
>>>     Signed-off-by: Gabriele Paoloni <[e-mail]>
>>>     Acked-by: Liviu Dudau <[e-mail]>
>>>     Acked-by: Rob Herring <[e-mail]>
>>
>> Please un-indent your changelog.
> 
> Ok agreed
> 
>>
>>> ---
>>>  drivers/of/address.c       | 2 ++
>>>  drivers/of/of_pci.c        | 4 ++++
>>>  include/linux/of_address.h | 1 +
>>>  3 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>> index 8bfda6a..23a5793 100644
>>> --- a/drivers/of/address.c
>>> +++ b/drivers/of/address.c
>>> @@ -253,6 +253,7 @@ struct of_pci_range
>> *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>>>  						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;
>>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>>> index 5751dc5..fe57030 100644
>>> --- a/drivers/of/of_pci.c
>>> +++ b/drivers/of/of_pci.c
>>> @@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
>> device_node *dev,
>>>
>>>  	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;
>>> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
>>> index d88e81b..865f96e 100644
>>> --- a/include/linux/of_address.h
>>> +++ b/include/linux/of_address.h
>>> @@ -16,6 +16,7 @@ struct of_pci_range {
>>>  	u32 pci_space;
>>>  	u64 pci_addr;
>>>  	u64 cpu_addr;
>>> +	u64 bus_addr;
>>>  	u64 size;
>>>  	u32 flags;
>>>  };
>>> --
>>> 1.9.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci"
>> in
>>> the body of a message to [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
>

Gabriele Paoloni July 30, 2015, 8:30 a.m. UTC | #6

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[e-mail]]
> Sent: Wednesday, July 29, 2015 10:47 PM
> To: Gabriele Paoloni
> Cc: [e-mail]; [e-mail]; Wangzhou (B);
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth); Andrew Murray
> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> of_pci_range
> 
> [+cc Andrew]
> 
> On Wed, Jul 29, 2015 at 07:44:18PM +0000, Gabriele Paoloni wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:[e-mail]]
> > > Sent: Wednesday, July 29, 2015 6:21 PM
> > > To: Gabriele Paoloni
> 
> > > On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > > > From: gabriele paoloni <[e-mail]>
> 
> > > >     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.
> >
> > 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.

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

> 
> 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.
> 
> > > pci_addr: I assume this is a PCI bus address, like what you would
> see
> > > if
> > > you put an analyzer on the bus/link.  This address could go in a
> BAR.
> >
> > Yes, this is the PCI start address of the range: phys.mid + phys.low
> in the
> > guide mentioned above
> >
> > > cpu_addr: I assume this is a CPU physical address, like what you
> would
> > > see
> > > in /proc/iomem and what you would pass to ioremap().
> >
> > Yes correct
> >
> > > bus_addr: ?
> >
> > According to the guide above, this is the address into which the
> pci_address
> > get translated to and that is passed to the root complex. Between the
> root
> > complex and the CPU there can be intermediate translation layers:
> 
> 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....

> 
> > see that to
> > get pci_address we call "of_translate_address"; this will apply all
> the
> > translation layers (ranges in the DT) that it finds till it comes to
> the root
> > node of the DT (thus retrieving the CPU address).
> > Now said that, for designware we need the first translated PCI
> address, that we call
> > here bus_addr after Rob Herring suggested the name...honestly I
> cannot think of a
> > different name
> >
> >
> >
> > > I'm trying to imagine how this might be expressed in ACPI.  A host
> > > bridge
> > > ACPI _CRS contains a CPU physical address and applying a _TRA
> > > (translation
> > > offset) to the CPU address gives you a PCI bus address.  I know
> this
> > > code
> > > is OF, not ACPI, but I assume that it should be possible to
> describe
> > > your
> > > hardware via ACPI as well as by OF.
> 
> > > > diff --git a/include/linux/of_address.h
> b/include/linux/of_address.h
> > > > index d88e81b..865f96e 100644
> > > > --- a/include/linux/of_address.h
> > > > +++ b/include/linux/of_address.h
> > > > @@ -16,6 +16,7 @@ struct of_pci_range {
> > > >  	u32 pci_space;
> > > >  	u64 pci_addr;
> > > >  	u64 cpu_addr;
> > > > +	u64 bus_addr;
> > > >  	u64 size;
> > > >  	u32 flags;
> > > >  };

Liviu Dudau July 30, 2015, 11:20 a.m. UTC | #7

Hello,

On Thu, Jul 30, 2015 at 09:30:41AM +0100, Gabriele Paoloni wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[e-mail]]
> > Sent: Wednesday, July 29, 2015 10:47 PM
> > To: Gabriele Paoloni
> > Cc: [e-mail]; [e-mail]; Wangzhou (B);
> > [e-mail]; [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail];
> > [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> > qiuzhenfa; Liguozhu (Kenneth); Andrew Murray
> > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> > of_pci_range
> > 
> > [+cc Andrew]
> > 
> > On Wed, Jul 29, 2015 at 07:44:18PM +0000, Gabriele Paoloni wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas [mailto:[e-mail]]
> > > > Sent: Wednesday, July 29, 2015 6:21 PM
> > > > To: Gabriele Paoloni
> > 
> > > > On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > > > > From: gabriele paoloni <[e-mail]>
> > 
> > > > >     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?

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


> > 
> > > > 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
> > 
> > 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....)
> 
> > 
> > 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.
> > 
> > > > pci_addr: I assume this is a PCI bus address, like what you would
> > see
> > > > if
> > > > you put an analyzer on the bus/link.  This address could go in a
> > BAR.
> > >
> > > Yes, this is the PCI start address of the range: phys.mid + phys.low
> > in the
> > > guide mentioned above
> > >
> > > > cpu_addr: I assume this is a CPU physical address, like what you
> > would
> > > > see
> > > > in /proc/iomem and what you would pass to ioremap().
> > >
> > > Yes correct
> > >
> > > > bus_addr: ?
> > >
> > > According to the guide above, this is the address into which the
> > pci_address
> > > get translated to and that is passed to the root complex. Between the
> > root
> > > complex and the CPU there can be intermediate translation layers:
> > 
> > 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....
> 
> > 
> > > see that to
> > > get pci_address we call "of_translate_address"; this will apply all
> > the
> > > translation layers (ranges in the DT) that it finds till it comes to
> > the root
> > > node of the DT (thus retrieving the CPU address).
> > > Now said that, for designware we need the first translated PCI
> > address, that we call
> > > here bus_addr after Rob Herring suggested the name...honestly I
> > cannot think of a
> > > different name
> > >
> > >
> > >
> > > > I'm trying to imagine how this might be expressed in ACPI.  A host
> > > > bridge
> > > > ACPI _CRS contains a CPU physical address and applying a _TRA
> > > > (translation
> > > > offset) to the CPU address gives you a PCI bus address.  I know
> > this
> > > > code
> > > > is OF, not ACPI, but I assume that it should be possible to
> > describe
> > > > your
> > > > hardware via ACPI as well as by OF.
> > 
> > > > > diff --git a/include/linux/of_address.h
> > b/include/linux/of_address.h
> > > > > index d88e81b..865f96e 100644
> > > > > --- a/include/linux/of_address.h
> > > > > +++ b/include/linux/of_address.h
> > > > > @@ -16,6 +16,7 @@ struct of_pci_range {
> > > > >  	u32 pci_space;
> > > > >  	u64 pci_addr;
> > > > >  	u64 cpu_addr;
> > > > > +	u64 bus_addr;
> > > > >  	u64 size;
> > > > >  	u32 flags;
> > > > >  };
>

Rob Herring July 30, 2015, 1:42 p.m. UTC | #8

On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni
<[e-mail]> wrote:
> Hi Bjorn
>
> Many Thanks for your reply
>
> I have commented back inline with resolutions from my side.
>
> If you're ok with them I'll send it out a new version in the appropriate patchset
>
> Cheers
>
> Gab
>
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:[e-mail]]
>> Sent: Wednesday, July 29, 2015 6:21 PM
>> To: Gabriele Paoloni
>> Cc: [e-mail]; [e-mail]; Wangzhou (B);
>> [e-mail]; [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
>> qiuzhenfa; Liguozhu (Kenneth)
>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>> of_pci_range
>>
>> Hi Gabriele,
>>
>> As far as I can tell, this is not specific to PCIe, so please use "PCI"
>> in
>> the subject as a generic term that includes both PCI and PCIe.
>
> sure agreed
>
>>
>> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
>> > From: gabriele paoloni <[e-mail]>
>> >
>> >     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")
>
> Agreed, will change this
>
>>
>> >     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.
>
> the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
> I will ask Zhou Wang to include this patch in his patchset
>
>
>>
>> >     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?
>
> sure I can add comments inline in the code
>
>>
>> pci_space: The only uses I see are to determine whether to print
>> "Prefetch".  I don't see any real functionality that uses this.
>
> Looking at the code I agree. it's seems to be used only in powerpc
> and microblaze to print out.
> However from my understanding pci_space is the phys.hi field of the
> ranges property: it defines the properties of the address space associated
> to the PCI address. if you're curious you can find a nice and quick to read
> "guide" in http://devicetree.org/MPC5200:PCI
>
>>
>> pci_addr: I assume this is a PCI bus address, like what you would see
>> if
>> you put an analyzer on the bus/link.  This address could go in a BAR.
>
> Yes, this is the PCI start address of the range: phys.mid + phys.low in the
> guide mentioned above
>
>>
>> cpu_addr: I assume this is a CPU physical address, like what you would
>> see
>> in /proc/iomem and what you would pass to ioremap().
>
> Yes correct
>
>>
>> bus_addr: ?
>>
>
> According to the guide above, this is the address into which the pci_address
> get translated to and that is passed to the root complex. Between the root
> complex and the CPU there can be intermediate translation layers: see that to
> get pci_address we call "of_translate_address"; this will apply all the
> translation layers (ranges in the DT) that it finds till it comes to the root
> node of the DT (thus retrieving the CPU address).
> Now said that, for designware we need the first translated PCI address, that we call

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

> 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 July 30, 2015, 1:52 p.m. UTC | #9

> -----Original Message-----

> From: [e-mail] [mailto:linux-pci-

> [e-mail]] On Behalf Of Rob Herring

> Sent: Thursday, July 30, 2015 2:43 PM

> To: Gabriele Paoloni

> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou

> (B); [e-mail]; [e-mail]; [e-mail];

> [e-mail]; [e-mail];

> [e-mail]; 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

> <[e-mail]> wrote:

> > Hi Bjorn

> >

> > Many Thanks for your reply

> >

> > I have commented back inline with resolutions from my side.

> >

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

> appropriate patchset

> >

> > Cheers

> >

> > Gab

> >

> >> -----Original Message-----

> >> From: Bjorn Helgaas [mailto:[e-mail]]

> >> Sent: Wednesday, July 29, 2015 6:21 PM

> >> To: Gabriele Paoloni

> >> Cc: [e-mail]; [e-mail]; Wangzhou (B);

> >> [e-mail]; [e-mail]; [e-mail]; linux-

> >> [e-mail]; [e-mail];

> >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> >> qiuzhenfa; Liguozhu (Kenneth)

> >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> >> of_pci_range

> >>

> >> Hi Gabriele,

> >>

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

> "PCI"

> >> in

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

> >

> > sure agreed

> >

> >>

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

> >> > From: gabriele paoloni <[e-mail]>

> >> >

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

> >

> > Agreed, will change this

> >

> >>

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

> >

> > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"

> > I will ask Zhou Wang to include this patch in his patchset

> >

> >

> >>

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

> >

> > sure I can add comments inline in the code

> >

> >>

> >> pci_space: The only uses I see are to determine whether to print

> >> "Prefetch".  I don't see any real functionality that uses this.

> >

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

> > and microblaze to print out.

> > However from my understanding pci_space is the phys.hi field of the

> > ranges property: it defines the properties of the address space

> associated

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

> to read

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

> >

> >>

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

> see

> >> if

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

> >

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

> in the

> > guide mentioned above

> >

> >>

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

> would

> >> see

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

> >

> > Yes correct

> >

> >>

> >> bus_addr: ?

> >>

> >

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

> pci_address

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

> root

> > complex and the CPU there can be intermediate translation layers: see

> that to

> > get pci_address we call "of_translate_address"; this will apply all

> the

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

> the root

> > node of the DT (thus retrieving the CPU address).

> > Now said that, for designware we need the first translated PCI

> address, that we call

> 

> 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

> 

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

> 

> Rob

> --

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

> the body of a message to [e-mail]

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gabriele Paoloni July 30, 2015, 2:15 p.m. UTC | #10

> -----Original Message-----

> From: [e-mail] [mailto:linux-pci-

> [e-mail]] On Behalf Of Gabriele Paoloni

> Sent: Thursday, July 30, 2015 2:52 PM

> To: Rob Herring

> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou

> (B); [e-mail]; [e-mail]; [e-mail];

> [e-mail]; [e-mail];

> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> qiuzhenfa; Liguozhu (Kenneth)

> Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct

> of_pci_range

> 

> 

> 

> > -----Original Message-----

> > From: [e-mail] [mailto:linux-pci-

> > [e-mail]] On Behalf Of Rob Herring

> > Sent: Thursday, July 30, 2015 2:43 PM

> > To: Gabriele Paoloni

> > Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou

> > (B); [e-mail]; [e-mail]; [e-mail];

> > [e-mail]; [e-mail];

> > [e-mail]; 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

> > <[e-mail]> wrote:

> > > Hi Bjorn

> > >

> > > Many Thanks for your reply

> > >

> > > I have commented back inline with resolutions from my side.

> > >

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

> > appropriate patchset

> > >

> > > Cheers

> > >

> > > Gab

> > >

> > >> -----Original Message-----

> > >> From: Bjorn Helgaas [mailto:[e-mail]]

> > >> Sent: Wednesday, July 29, 2015 6:21 PM

> > >> To: Gabriele Paoloni

> > >> Cc: [e-mail]; [e-mail]; Wangzhou (B);

> > >> [e-mail]; [e-mail]; [e-mail];

> linux-

> > >> [e-mail]; [e-mail];

> > >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> > >> qiuzhenfa; Liguozhu (Kenneth)

> > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> > >> of_pci_range

> > >>

> > >> Hi Gabriele,

> > >>

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

> > "PCI"

> > >> in

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

> > >

> > > sure agreed

> > >

> > >>

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

> > >> > From: gabriele paoloni <[e-mail]>

> > >> >

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

> > >

> > > Agreed, will change this

> > >

> > >>

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

> > >

> > > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"

> > > I will ask Zhou Wang to include this patch in his patchset

> > >

> > >

> > >>

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

> > >

> > > sure I can add comments inline in the code

> > >

> > >>

> > >> pci_space: The only uses I see are to determine whether to print

> > >> "Prefetch".  I don't see any real functionality that uses this.

> > >

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

> > > and microblaze to print out.

> > > However from my understanding pci_space is the phys.hi field of the

> > > ranges property: it defines the properties of the address space

> > associated

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

> > to read

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

> > >

> > >>

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

> > see

> > >> if

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

> BAR.

> > >

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

> phys.low

> > in the

> > > guide mentioned above

> > >

> > >>

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

> > would

> > >> see

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

> > >

> > > Yes correct

> > >

> > >>

> > >> bus_addr: ?

> > >>

> > >

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

> > pci_address

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

> the

> > root

> > > complex and the CPU there can be intermediate translation layers:

> see

> > that to

> > > get pci_address we call "of_translate_address"; this will apply all

> > the

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

> to

> > the root

> > > node of the DT (thus retrieving the CPU address).

> > > Now said that, for designware we need the first translated PCI

> > address, that we call

> >

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

> >

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

> 

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

> 

> >

> > Rob

> > --

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

> in

> > the body of a message to [e-mail]

> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

> ????&?~?&???+-

> ?????w??????m?b??ir(?????}????z?&j:+v???
????zZ+??+zf???h???~????i???

> z??w????????&?)?f

Bjorn Helgaas July 30, 2015, 4:06 p.m. UTC | #11

On Thu, Jul 30, 2015 at 08:42:53AM -0500, Rob Herring wrote:
> On Wed, Jul 29, 2015 at 2:44 PM, Gabriele Paoloni
> <[e-mail]> wrote:
> > Hi Bjorn
> >
> > Many Thanks for your reply
> >
> > I have commented back inline with resolutions from my side.
> >
> > If you're ok with them I'll send it out a new version in the appropriate patchset
> >
> > Cheers
> >
> > Gab
> >
> >> -----Original Message-----
> >> From: Bjorn Helgaas [mailto:[e-mail]]
> >> Sent: Wednesday, July 29, 2015 6:21 PM
> >> To: Gabriele Paoloni
> >> Cc: [e-mail]; [e-mail]; Wangzhou (B);
> >> [e-mail]; [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail];
> >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> >> qiuzhenfa; Liguozhu (Kenneth)
> >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> >> of_pci_range
> >>
> >> Hi Gabriele,
> >>
> >> As far as I can tell, this is not specific to PCIe, so please use "PCI"
> >> in
> >> the subject as a generic term that includes both PCI and PCIe.
> >
> > sure agreed
> >
> >>
> >> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> >> > From: gabriele paoloni <[e-mail]>
> >> >
> >> >     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")
> >
> > Agreed, will change this
> >
> >>
> >> >     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.
> >
> > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
> > I will ask Zhou Wang to include this patch in his patchset
> >
> >
> >>
> >> >     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?
> >
> > sure I can add comments inline in the code
> >
> >>
> >> pci_space: The only uses I see are to determine whether to print
> >> "Prefetch".  I don't see any real functionality that uses this.
> >
> > Looking at the code I agree. it's seems to be used only in powerpc
> > and microblaze to print out.
> > However from my understanding pci_space is the phys.hi field of the
> > ranges property: it defines the properties of the address space associated
> > to the PCI address. if you're curious you can find a nice and quick to read
> > "guide" in http://devicetree.org/MPC5200:PCI
> >
> >>
> >> pci_addr: I assume this is a PCI bus address, like what you would see
> >> if
> >> you put an analyzer on the bus/link.  This address could go in a BAR.
> >
> > Yes, this is the PCI start address of the range: phys.mid + phys.low in the
> > guide mentioned above
> >
> >>
> >> cpu_addr: I assume this is a CPU physical address, like what you would
> >> see
> >> in /proc/iomem and what you would pass to ioremap().
> >
> > Yes correct
> >
> >>
> >> bus_addr: ?
> >>
> >
> > According to the guide above, this is the address into which the pci_address
> > get translated to and that is passed to the root complex. Between the root
> > complex and the CPU there can be intermediate translation layers: see that to
> > get pci_address we call "of_translate_address"; this will apply all the
> > translation layers (ranges in the DT) that it finds till it comes to the root
> > node of the DT (thus retrieving the CPU address).
> > Now said that, for designware we need the first translated PCI address, that we call
> 
> 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
> 
> > 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.

Bjorn Helgaas July 30, 2015, 4:14 p.m. UTC | #12

On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:
> 
> 
> > -----Original Message-----
> > From: [e-mail] [mailto:linux-pci-
> > [e-mail]] On Behalf Of Rob Herring
> > Sent: Thursday, July 30, 2015 2:43 PM
> > To: Gabriele Paoloni
> > Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou
> > (B); [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; 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
> > <[e-mail]> wrote:
> > > Hi Bjorn
> > >
> > > Many Thanks for your reply
> > >
> > > I have commented back inline with resolutions from my side.
> > >
> > > If you're ok with them I'll send it out a new version in the
> > appropriate patchset
> > >
> > > Cheers
> > >
> > > Gab
> > >
> > >> -----Original Message-----
> > >> From: Bjorn Helgaas [mailto:[e-mail]]
> > >> Sent: Wednesday, July 29, 2015 6:21 PM
> > >> To: Gabriele Paoloni
> > >> Cc: [e-mail]; [e-mail]; Wangzhou (B);
> > >> [e-mail]; [e-mail]; [e-mail]; linux-
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> > >> qiuzhenfa; Liguozhu (Kenneth)
> > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> > >> of_pci_range
> > >>
> > >> Hi Gabriele,
> > >>
> > >> As far as I can tell, this is not specific to PCIe, so please use
> > "PCI"
> > >> in
> > >> the subject as a generic term that includes both PCI and PCIe.
> > >
> > > sure agreed
> > >
> > >>
> > >> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > >> > From: gabriele paoloni <[e-mail]>
> > >> >
> > >> >     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")
> > >
> > > Agreed, will change this
> > >
> > >>
> > >> >     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.
> > >
> > > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
> > > I will ask Zhou Wang to include this patch in his patchset
> > >
> > >
> > >>
> > >> >     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?
> > >
> > > sure I can add comments inline in the code
> > >
> > >>
> > >> pci_space: The only uses I see are to determine whether to print
> > >> "Prefetch".  I don't see any real functionality that uses this.
> > >
> > > Looking at the code I agree. it's seems to be used only in powerpc
> > > and microblaze to print out.
> > > However from my understanding pci_space is the phys.hi field of the
> > > ranges property: it defines the properties of the address space
> > associated
> > > to the PCI address. if you're curious you can find a nice and quick
> > to read
> > > "guide" in http://devicetree.org/MPC5200:PCI
> > >
> > >>
> > >> pci_addr: I assume this is a PCI bus address, like what you would
> > see
> > >> if
> > >> you put an analyzer on the bus/link.  This address could go in a BAR.
> > >
> > > Yes, this is the PCI start address of the range: phys.mid + phys.low
> > in the
> > > guide mentioned above
> > >
> > >>
> > >> cpu_addr: I assume this is a CPU physical address, like what you
> > would
> > >> see
> > >> in /proc/iomem and what you would pass to ioremap().
> > >
> > > Yes correct
> > >
> > >>
> > >> bus_addr: ?
> > >>
> > >
> > > According to the guide above, this is the address into which the
> > pci_address
> > > get translated to and that is passed to the root complex. Between the
> > root
> > > complex and the CPU there can be intermediate translation layers: see
> > that to
> > > get pci_address we call "of_translate_address"; this will apply all
> > the
> > > translation layers (ranges in the DT) that it finds till it comes to
> > the root
> > > node of the DT (thus retrieving the CPU address).
> > > Now said that, for designware we need the first translated PCI
> > address, that we call
> > 
> > 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
> > 
> > > 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 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 July 30, 2015, 4:50 p.m. UTC | #13

> -----Original Message-----

> From: [e-mail] [mailto:linux-pci-

> [e-mail]] On Behalf Of Bjorn Helgaas

> Sent: Thursday, July 30, 2015 5:15 PM

> To: Gabriele Paoloni

> Cc: Rob Herring; [e-mail]; [e-mail]; Wangzhou (B);

> [e-mail]; [e-mail]; [e-mail]; linux-

> [e-mail]; [e-mail];

> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> qiuzhenfa; Liguozhu (Kenneth)

> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> of_pci_range

> 

> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:

> >

> >

> > > -----Original Message-----

> > > From: [e-mail] [mailto:linux-pci-

> > > [e-mail]] On Behalf Of Rob Herring

> > > Sent: Thursday, July 30, 2015 2:43 PM

> > > To: Gabriele Paoloni

> > > Cc: Bjorn Helgaas; [e-mail]; [e-mail];

> Wangzhou

> > > (B); [e-mail]; [e-mail]; [e-mail];

> > > [e-mail]; [e-mail];

> > > [e-mail]; 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

> > > <[e-mail]> wrote:

> > > > Hi Bjorn

> > > >

> > > > Many Thanks for your reply

> > > >

> > > > I have commented back inline with resolutions from my side.

> > > >

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

> > > appropriate patchset

> > > >

> > > > Cheers

> > > >

> > > > Gab

> > > >

> > > >> -----Original Message-----

> > > >> From: Bjorn Helgaas [mailto:[e-mail]]

> > > >> Sent: Wednesday, July 29, 2015 6:21 PM

> > > >> To: Gabriele Paoloni

> > > >> Cc: [e-mail]; [e-mail]; Wangzhou (B);

> > > >> [e-mail]; [e-mail]; [e-mail];

> linux-

> > > >> [e-mail]; [e-mail];

> > > >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> > > >> qiuzhenfa; Liguozhu (Kenneth)

> > > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> > > >> of_pci_range

> > > >>

> > > >> Hi Gabriele,

> > > >>

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

> use

> > > "PCI"

> > > >> in

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

> > > >

> > > > sure agreed

> > > >

> > > >>

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

> > > >> > From: gabriele paoloni <[e-mail]>

> > > >> >

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

> > > >

> > > > Agreed, will change this

> > > >

> > > >>

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

> > > >

> > > > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"

> > > > I will ask Zhou Wang to include this patch in his patchset

> > > >

> > > >

> > > >>

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

> > > >

> > > > sure I can add comments inline in the code

> > > >

> > > >>

> > > >> pci_space: The only uses I see are to determine whether to print

> > > >> "Prefetch".  I don't see any real functionality that uses this.

> > > >

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

> powerpc

> > > > and microblaze to print out.

> > > > However from my understanding pci_space is the phys.hi field of

> the

> > > > ranges property: it defines the properties of the address space

> > > associated

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

> quick

> > > to read

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

> > > >

> > > >>

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

> would

> > > see

> > > >> if

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

> BAR.

> > > >

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

> phys.low

> > > in the

> > > > guide mentioned above

> > > >

> > > >>

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

> > > would

> > > >> see

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

> > > >

> > > > Yes correct

> > > >

> > > >>

> > > >> bus_addr: ?

> > > >>

> > > >

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

> > > pci_address

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

> the

> > > root

> > > > complex and the CPU there can be intermediate translation layers:

> see

> > > that to

> > > > get pci_address we call "of_translate_address"; this will apply

> all

> > > the

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

> to

> > > the root

> > > > node of the DT (thus retrieving the CPU address).

> > > > Now said that, for designware we need the first translated PCI

> > > address, that we call

> > >

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

> > >

> > > > 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 don’t think we should rely on [CPU] addresses...what if the

> intermediate

> > translation layer changes the lower significant bits of the "bus

> address"

> > to translate into a cpu address?

> 

> Is it really a possiblity that the lower bits could be changed?


I've checked all the current deignware users DTs except "pci-layerscape" 
that I could not find:
spear1310.dtsi
spear1340.dtsi
dra7.dtsi
imx6qdl.dtsi
imx6sx.dtsi
keystone.dtsi
exynos5440.dtsi

None of them modifies the lower bits. To be more precise the only guy
that provides another translation layer is "dra7.dtsi":
axi0
http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207

axi1
http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241

For this case masking the top 4bits (bits28 to 31) should make the job.

Speaking in general terms so far I've always seen linear mappings
that differ by bitmask offset, however linear does not mean that you 
cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to
<0x0000cafe to 0x0000cafe + size>, but I guess that for HW design reasons
it is much easier to remap directly using a bitmask...but I was not sure
and I didn't think about the problems that can arise with ACPI.

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

Thanks
Gab
 
> 

> I think we're talking about MMIO here, and a bridge has an MMIO

> window.  A window is a range of CPU physical addresses that the

> bridge forwards to PCI.  The PCI core assumes that a CPU physical

> address range is linearly mapped to PCI bus addresses, i.e., if the

> window base is X and maps to PCI address Y, then as long as X+n is

> inside the window, it must map to Y+n.

> 

> That means the low-order bits (the ones that are the offset into the

> window) cannot change.

> --

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

> the body of a message to [e-mail]

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas July 30, 2015, 5:14 p.m. UTC | #14

[+cc Jingoo, Pratyush]

On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:
> > -----Original Message-----
> > From: [e-mail] [mailto:linux-pci-
> > [e-mail]] On Behalf Of Bjorn Helgaas
> > Sent: Thursday, July 30, 2015 5:15 PM
> > To: Gabriele Paoloni
> > Cc: Rob Herring; [e-mail]; [e-mail]; Wangzhou (B);
> > [e-mail]; [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail];
> > [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> > qiuzhenfa; Liguozhu (Kenneth)
> > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> > of_pci_range
> > 
> > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: [e-mail] [mailto:linux-pci-
> > > > [e-mail]] On Behalf Of Rob Herring
> > > > Sent: Thursday, July 30, 2015 2:43 PM
> > > > To: Gabriele Paoloni
> > > > Cc: Bjorn Helgaas; [e-mail]; [e-mail];
> > Wangzhou
> > > > (B); [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; 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
> > > > <[e-mail]> wrote:
> > > > > Hi Bjorn
> > > > >
> > > > > Many Thanks for your reply
> > > > >
> > > > > I have commented back inline with resolutions from my side.
> > > > >
> > > > > If you're ok with them I'll send it out a new version in the
> > > > appropriate patchset
> > > > >
> > > > > Cheers
> > > > >
> > > > > Gab
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Bjorn Helgaas [mailto:[e-mail]]
> > > > >> Sent: Wednesday, July 29, 2015 6:21 PM
> > > > >> To: Gabriele Paoloni
> > > > >> Cc: [e-mail]; [e-mail]; Wangzhou (B);
> > > > >> [e-mail]; [e-mail]; [e-mail];
> > linux-
> > > > >> [e-mail]; [e-mail];
> > > > >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
> > > > >> qiuzhenfa; Liguozhu (Kenneth)
> > > > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> > > > >> of_pci_range
> > > > >>
> > > > >> Hi Gabriele,
> > > > >>
> > > > >> As far as I can tell, this is not specific to PCIe, so please
> > use
> > > > "PCI"
> > > > >> in
> > > > >> the subject as a generic term that includes both PCI and PCIe.
> > > > >
> > > > > sure agreed
> > > > >
> > > > >>
> > > > >> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
> > > > >> > From: gabriele paoloni <[e-mail]>
> > > > >> >
> > > > >> >     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")
> > > > >
> > > > > Agreed, will change this
> > > > >
> > > > >>
> > > > >> >     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.
> > > > >
> > > > > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
> > > > > I will ask Zhou Wang to include this patch in his patchset
> > > > >
> > > > >
> > > > >>
> > > > >> >     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?
> > > > >
> > > > > sure I can add comments inline in the code
> > > > >
> > > > >>
> > > > >> pci_space: The only uses I see are to determine whether to print
> > > > >> "Prefetch".  I don't see any real functionality that uses this.
> > > > >
> > > > > Looking at the code I agree. it's seems to be used only in
> > powerpc
> > > > > and microblaze to print out.
> > > > > However from my understanding pci_space is the phys.hi field of
> > the
> > > > > ranges property: it defines the properties of the address space
> > > > associated
> > > > > to the PCI address. if you're curious you can find a nice and
> > quick
> > > > to read
> > > > > "guide" in http://devicetree.org/MPC5200:PCI
> > > > >
> > > > >>
> > > > >> pci_addr: I assume this is a PCI bus address, like what you
> > would
> > > > see
> > > > >> if
> > > > >> you put an analyzer on the bus/link.  This address could go in a
> > BAR.
> > > > >
> > > > > Yes, this is the PCI start address of the range: phys.mid +
> > phys.low
> > > > in the
> > > > > guide mentioned above
> > > > >
> > > > >>
> > > > >> cpu_addr: I assume this is a CPU physical address, like what you
> > > > would
> > > > >> see
> > > > >> in /proc/iomem and what you would pass to ioremap().
> > > > >
> > > > > Yes correct
> > > > >
> > > > >>
> > > > >> bus_addr: ?
> > > > >>
> > > > >
> > > > > According to the guide above, this is the address into which the
> > > > pci_address
> > > > > get translated to and that is passed to the root complex. Between
> > the
> > > > root
> > > > > complex and the CPU there can be intermediate translation layers:
> > see
> > > > that to
> > > > > get pci_address we call "of_translate_address"; this will apply
> > all
> > > > the
> > > > > translation layers (ranges in the DT) that it finds till it comes
> > to
> > > > the root
> > > > > node of the DT (thus retrieving the CPU address).
> > > > > Now said that, for designware we need the first translated PCI
> > > > address, that we call
> > > >
> > > > 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
> > > >
> > > > > 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 don’t think we should rely on [CPU] addresses...what if the
> > intermediate
> > > translation layer changes the lower significant bits of the "bus
> > address"
> > > to translate into a cpu address?
> > 
> > Is it really a possiblity that the lower bits could be changed?
> 
> I've checked all the current deignware users DTs except "pci-layerscape" 
> that I could not find:
> spear1310.dtsi
> spear1340.dtsi
> dra7.dtsi
> imx6qdl.dtsi
> imx6sx.dtsi
> keystone.dtsi
> exynos5440.dtsi
> 
> None of them modifies the lower bits. To be more precise the only guy
> that provides another translation layer is "dra7.dtsi":
> axi0
> http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
> 
> axi1
> http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
> 
> For this case masking the top 4bits (bits28 to 31) should make the job.
> 
> 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.

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

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

Gabriele Paoloni July 30, 2015, 5:34 p.m. UTC | #15

> -----Original Message-----

> From: Bjorn Helgaas [mailto:[e-mail]]

> Sent: 30 July 2015 18:15

> To: Gabriele Paoloni

> Cc: Rob Herring; [e-mail]; [e-mail]; Wangzhou (B);

> [e-mail]; [e-mail]; [e-mail]; linux-

> [e-mail]; [e-mail];

> [e-mail]; 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]

> 

> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:

> > > -----Original Message-----

> > > From: [e-mail] [mailto:linux-pci-

> > > [e-mail]] On Behalf Of Bjorn Helgaas

> > > Sent: Thursday, July 30, 2015 5:15 PM

> > > To: Gabriele Paoloni

> > > Cc: Rob Herring; [e-mail]; [e-mail]; Wangzhou

> (B);

> > > [e-mail]; [e-mail]; [e-mail]; linux-

> > > [e-mail]; [e-mail];

> > > [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> > > qiuzhenfa; Liguozhu (Kenneth)

> > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> > > of_pci_range

> > >

> > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:

> > > >

> > > >

> > > > > -----Original Message-----

> > > > > From: [e-mail] [mailto:linux-pci-

> > > > > [e-mail]] On Behalf Of Rob Herring

> > > > > Sent: Thursday, July 30, 2015 2:43 PM

> > > > > To: Gabriele Paoloni

> > > > > Cc: Bjorn Helgaas; [e-mail]; [e-mail];

> > > Wangzhou

> > > > > (B); [e-mail]; [e-mail];

> [e-mail];

> > > > > [e-mail]; [e-mail];

> > > > > [e-mail]; 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

> > > > > <[e-mail]> wrote:

> > > > > > Hi Bjorn

> > > > > >

> > > > > > Many Thanks for your reply

> > > > > >

> > > > > > I have commented back inline with resolutions from my side.

> > > > > >

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

> > > > > appropriate patchset

> > > > > >

> > > > > > Cheers

> > > > > >

> > > > > > Gab

> > > > > >

> > > > > >> -----Original Message-----

> > > > > >> From: Bjorn Helgaas [mailto:[e-mail]]

> > > > > >> Sent: Wednesday, July 29, 2015 6:21 PM

> > > > > >> To: Gabriele Paoloni

> > > > > >> Cc: [e-mail]; [e-mail]; Wangzhou (B);

> > > > > >> [e-mail]; [e-mail]; [e-mail];

> > > linux-

> > > > > >> [e-mail]; [e-mail];

> > > > > >> [e-mail]; Yuanzhichang; Zhudacai;

> zhangjukuo;

> > > > > >> qiuzhenfa; Liguozhu (Kenneth)

> > > > > >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> > > > > >> of_pci_range

> > > > > >>

> > > > > >> Hi Gabriele,

> > > > > >>

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

> > > use

> > > > > "PCI"

> > > > > >> in

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

> > > > > >

> > > > > > sure agreed

> > > > > >

> > > > > >>

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

> wrote:

> > > > > >> > From: gabriele paoloni <[e-mail]>

> > > > > >> >

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

> > > > > >

> > > > > > Agreed, will change this

> > > > > >

> > > > > >>

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

> > > > > >

> > > > > > the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64

> support"

> > > > > > I will ask Zhou Wang to include this patch in his patchset

> > > > > >

> > > > > >

> > > > > >>

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

> > > > > >

> > > > > > sure I can add comments inline in the code

> > > > > >

> > > > > >>

> > > > > >> pci_space: The only uses I see are to determine whether to

> print

> > > > > >> "Prefetch".  I don't see any real functionality that uses

> this.

> > > > > >

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

> > > powerpc

> > > > > > and microblaze to print out.

> > > > > > However from my understanding pci_space is the phys.hi field

> of

> > > the

> > > > > > ranges property: it defines the properties of the address

> space

> > > > > associated

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

> > > quick

> > > > > to read

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

> > > > > >

> > > > > >>

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

> > > would

> > > > > see

> > > > > >> if

> > > > > >> you put an analyzer on the bus/link.  This address could go

> in a

> > > BAR.

> > > > > >

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

> > > phys.low

> > > > > in the

> > > > > > guide mentioned above

> > > > > >

> > > > > >>

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

> you

> > > > > would

> > > > > >> see

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

> > > > > >

> > > > > > Yes correct

> > > > > >

> > > > > >>

> > > > > >> bus_addr: ?

> > > > > >>

> > > > > >

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

> the

> > > > > pci_address

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

> Between

> > > the

> > > > > root

> > > > > > complex and the CPU there can be intermediate translation

> layers:

> > > see

> > > > > that to

> > > > > > get pci_address we call "of_translate_address"; this will

> apply

> > > all

> > > > > the

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

> comes

> > > to

> > > > > the root

> > > > > > node of the DT (thus retrieving the CPU address).

> > > > > > Now said that, for designware we need the first translated PCI

> > > > > address, that we call

> > > > >

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

> > > > >

> > > > > > 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 don’t think we should rely on [CPU] addresses...what if the

> > > intermediate

> > > > translation layer changes the lower significant bits of the "bus

> > > address"

> > > > to translate into a cpu address?

> > >

> > > Is it really a possiblity that the lower bits could be changed?

> >

> > I've checked all the current deignware users DTs except "pci-

> layerscape"

> > that I could not find:

> > spear1310.dtsi

> > spear1340.dtsi

> > dra7.dtsi

> > imx6qdl.dtsi

> > imx6sx.dtsi

> > keystone.dtsi

> > exynos5440.dtsi

> >

> > None of them modifies the lower bits. To be more precise the only guy

> > that provides another translation layer is "dra7.dtsi":

> > axi0

> > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207

> >

> > axi1

> > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241

> >

> > For this case masking the top 4bits (bits28 to 31) should make the job.

> >

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

> 

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

> 

> > > 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 [e-mail]

> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring July 30, 2015, 8:41 p.m. UTC | #16

On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni
<[e-mail]> wrote:
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:[e-mail]]
>> Sent: 30 July 2015 18:15
>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:
>> > > -----Original Message-----
>> > > From: [e-mail] [mailto:linux-pci-
>> > > [e-mail]] On Behalf Of Bjorn Helgaas
>> > > Sent: Thursday, July 30, 2015 5:15 PM
>> > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:

[...]

>> > > > I don’t think we should rely on [CPU] addresses...what if the
>> > > intermediate
>> > > > translation layer changes the lower significant bits of the "bus
>> > > address"
>> > > > to translate into a cpu address?
>> > >
>> > > Is it really a possiblity that the lower bits could be changed?
>> >
>> > I've checked all the current deignware users DTs except "pci-
>> layerscape"
>> > that I could not find:
>> > spear1310.dtsi
>> > spear1340.dtsi
>> > dra7.dtsi
>> > imx6qdl.dtsi
>> > imx6sx.dtsi
>> > keystone.dtsi
>> > exynos5440.dtsi
>> >
>> > None of them modifies the lower bits. To be more precise the only guy
>> > that provides another translation layer is "dra7.dtsi":
>> > axi0
>> > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
>> >
>> > axi1
>> > http://lxr.free-electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
>> >
>> > For this case masking the top 4bits (bits28 to 31) should make the job.

IMO, we should just fix this case. After further study, I don't think
this is a DW issue, but rather an SOC integration issue.

I believe you can just fixup the address in the pp->ops->host_init hook.

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

>> >
>> > Speaking in general terms so far I've always seen linear mappings
>> > that differ by bitmask offset, however linear does not mean that you
>> > cannot affect the lower bits: e.g. <0x0> to <0x0 + size> can map to
>> > <0x0000cafe to 0x0000cafe + size>, but I guess that for HW design
>> reasons
>> > it is much easier to remap directly using a bitmask...but I was not
>> sure
>> > and I didn't think about the problems that can arise with ACPI.

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

>> As I said, it wouldn't make sense for the bits that comprise the
>> offset into the window to change, so the mapping only has to be linear
>> inside the window.
>>
>> For the bits outside the window offset, I don't know enough to say
>> whether masking is sufficient or safe.
>>
>> > If you think the bitmask is Ok then I can directly define it in
>> > designware and we can drop this patch.
>>
>> It's OK by me, but I know nothing about the actual hardware involved.
>> For the DesignWare question, I think you would just have to convince
>> Jingoo and Pratyush (cc'd).
>
> Yes actually looking at
> http://lxr.free-electrons.com/source/arch/arm/boot/dts/spear1310.dtsi#L99
> I can see that pci_addr=0 is mapped to bus_addr 0x80020000, so clearing
> the top 4 bits would alter the behaviour of the current designware
> SW framework...now I don't know if DW needs only the low 28b of that
> value or not....

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

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

Rob

Gabriele Paoloni July 31, 2015, 2:25 p.m. UTC | #17

[+cc Kishon]

> -----Original Message-----

> From: [e-mail] [mailto:linux-pci-

> [e-mail]] On Behalf Of Rob Herring

> Sent: Thursday, July 30, 2015 9:42 PM

> To: Gabriele Paoloni

> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou

> (B); [e-mail]; [e-mail]; [e-mail];

> [e-mail]; [e-mail];

> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand

> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> of_pci_range

> 

> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni

> <[e-mail]> wrote:

> >> -----Original Message-----

> >> From: Bjorn Helgaas [mailto:[e-mail]]

> >> Sent: 30 July 2015 18:15

> >> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:

> >> > > -----Original Message-----

> >> > > From: [e-mail] [mailto:linux-pci-

> >> > > [e-mail]] On Behalf Of Bjorn Helgaas

> >> > > Sent: Thursday, July 30, 2015 5:15 PM

> >> > > On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:

> 

> [...]

> 

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

> >> > > intermediate

> >> > > > translation layer changes the lower significant bits of the

> "bus

> >> > > address"

> >> > > > to translate into a cpu address?

> >> > >

> >> > > Is it really a possiblity that the lower bits could be changed?

> >> >

> >> > I've checked all the current deignware users DTs except "pci-

> >> layerscape"

> >> > that I could not find:

> >> > spear1310.dtsi

> >> > spear1340.dtsi

> >> > dra7.dtsi

> >> > imx6qdl.dtsi

> >> > imx6sx.dtsi

> >> > keystone.dtsi

> >> > exynos5440.dtsi

> >> >

> >> > None of them modifies the lower bits. To be more precise the only

> guy

> >> > that provides another translation layer is "dra7.dtsi":

> >> > axi0

> >> > http://lxr.free-

> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207

> >> >

> >> > axi1

> >> > http://lxr.free-

> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241

> >> >

> >> > For this case masking the top 4bits (bits28 to 31) should make the

> job.

> 

> IMO, we should just fix this case. After further study, I don't think

> this is a DW issue, but rather an SOC integration issue.

> 

> I believe you can just fixup the address in the pp->ops->host_init hook.

> 


Yes I guess that I could just assign pp->(*)_mod_base to the CPU address 
in DW and mask it out in dra7xx_pcie_host_init()...

Kishon, would you be ok with that? 


> In looking at this, I'm curious why only 2 ATU viewports are used by

> default as this causes switching on config accesses. Probably some

> configuration only has 2 viewports. This would not even work on SMP

> systems! Memory and i/o accesses do not have any lock. A config access

> on one core will temporarily disable the i/o or memory viewport which

> will cause an abort, dropped, or garbage data on an i/o or memory

> access from another core. You would have to be accessing cfg space on

> a bus other than the root bus, so maybe no one is doing that.

> 

> >> >

> >> > Speaking in general terms so far I've always seen linear mappings

> >> > that differ by bitmask offset, however linear does not mean that

> you

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

> to

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

> >> reasons

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

> not

> >> sure

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

> 

> For higher speed buses, the h/w designs are not going to be doing

> complicated translations in order to meet timing requirements. At the

> top level only the highest order bits are going to be looked at. For

> downstream segments, the high order bits may get dropped to simplify

> routing. If an IP block has full address bits in this case, they could

> either be tied to 0 or tied off to the correct address. Either is

> reasonable, so I'm sure there is h/w doing both. That doesn't mean h/w

> designers haven't done something crazy, too.

> 

> >> As I said, it wouldn't make sense for the bits that comprise the

> >> offset into the window to change, so the mapping only has to be

> linear

> >> inside the window.

> >>

> >> For the bits outside the window offset, I don't know enough to say

> >> whether masking is sufficient or safe.

> >>

> >> > If you think the bitmask is Ok then I can directly define it in

> >> > designware and we can drop this patch.

> >>

> >> It's OK by me, but I know nothing about the actual hardware involved.

> >> For the DesignWare question, I think you would just have to convince

> >> Jingoo and Pratyush (cc'd).

> >

> > Yes actually looking at

> > http://lxr.free-

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

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

> clearing

> > the top 4 bits would alter the behaviour of the current designware

> > SW framework...now I don't know if DW needs only the low 28b of that

> > value or not....

> 

> Given that most cases have same cpu and bus address, masking is not

> the right solution in general.

> 

> There is also a bug here BTW. pcie0 and pcie2 have the same CPU

> address for the memory range.

> 

> Rob

> --

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

> the body of a message to [e-mail]

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kishon Vijay Abraham I July 31, 2015, 2:57 p.m. UTC | #18

+Arnd

Hi,

On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
> [+cc Kishon]
> 
>> -----Original Message-----
>> From: [e-mail] [mailto:linux-pci-
>> [e-mail]] On Behalf Of Rob Herring
>> Sent: Thursday, July 30, 2015 9:42 PM
>> To: Gabriele Paoloni
>> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou
>> (B); [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
>> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand
>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>> of_pci_range
>>
>> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni
>> <[e-mail]> wrote:
>>>> -----Original Message-----
>>>> From: Bjorn Helgaas [mailto:[e-mail]]
>>>> Sent: 30 July 2015 18:15
>>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:
>>>>>> -----Original Message-----
>>>>>> From: [e-mail] [mailto:linux-pci-
>>>>>> [e-mail]] On Behalf Of Bjorn Helgaas
>>>>>> Sent: Thursday, July 30, 2015 5:15 PM
>>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:
>>
>> [...]
>>
>>>>>>> I don’t think we should rely on [CPU] addresses...what if the
>>>>>> intermediate
>>>>>>> translation layer changes the lower significant bits of the
>> "bus
>>>>>> address"
>>>>>>> to translate into a cpu address?
>>>>>>
>>>>>> Is it really a possiblity that the lower bits could be changed?
>>>>>
>>>>> I've checked all the current deignware users DTs except "pci-
>>>> layerscape"
>>>>> that I could not find:
>>>>> spear1310.dtsi
>>>>> spear1340.dtsi
>>>>> dra7.dtsi
>>>>> imx6qdl.dtsi
>>>>> imx6sx.dtsi
>>>>> keystone.dtsi
>>>>> exynos5440.dtsi
>>>>>
>>>>> None of them modifies the lower bits. To be more precise the only
>> guy
>>>>> that provides another translation layer is "dra7.dtsi":
>>>>> axi0
>>>>> http://lxr.free-
>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
>>>>>
>>>>> axi1
>>>>> http://lxr.free-
>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
>>>>>
>>>>> For this case masking the top 4bits (bits28 to 31) should make the
>> job.
>>
>> IMO, we should just fix this case. After further study, I don't think
>> this is a DW issue, but rather an SOC integration issue.
>>
>> I believe you can just fixup the address in the pp->ops->host_init hook.
>>
> 
> 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 July 31, 2015, 3:09 p.m. UTC | #19

> -----Original Message-----

> From: Kishon Vijay Abraham I [mailto:[e-mail]]

> Sent: Friday, July 31, 2015 3:57 PM

> To: Gabriele Paoloni; Rob Herring

> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou

> (B); [e-mail]; [e-mail]; [e-mail];

> [e-mail]; [e-mail];

> [e-mail]; 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,

> 

> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:

> > [+cc Kishon]

> >

> >> -----Original Message-----

> >> From: [e-mail] [mailto:linux-pci-

> >> [e-mail]] On Behalf Of Rob Herring

> >> Sent: Thursday, July 30, 2015 9:42 PM

> >> To: Gabriele Paoloni

> >> Cc: Bjorn Helgaas; [e-mail]; [e-mail];

> Wangzhou

> >> (B); [e-mail]; [e-mail]; [e-mail];

> >> [e-mail]; [e-mail];

> >> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;

> >> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand

> >> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct

> >> of_pci_range

> >>

> >> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni

> >> <[e-mail]> wrote:

> >>>> -----Original Message-----

> >>>> From: Bjorn Helgaas [mailto:[e-mail]]

> >>>> Sent: 30 July 2015 18:15

> >>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:

> >>>>>> -----Original Message-----

> >>>>>> From: [e-mail] [mailto:linux-pci-

> >>>>>> [e-mail]] On Behalf Of Bjorn Helgaas

> >>>>>> Sent: Thursday, July 30, 2015 5:15 PM

> >>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:

> >>

> >> [...]

> >>

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

> >>>>>> intermediate

> >>>>>>> translation layer changes the lower significant bits of the

> >> "bus

> >>>>>> address"

> >>>>>>> to translate into a cpu address?

> >>>>>>

> >>>>>> Is it really a possiblity that the lower bits could be changed?

> >>>>>

> >>>>> I've checked all the current deignware users DTs except "pci-

> >>>> layerscape"

> >>>>> that I could not find:

> >>>>> spear1310.dtsi

> >>>>> spear1340.dtsi

> >>>>> dra7.dtsi

> >>>>> imx6qdl.dtsi

> >>>>> imx6sx.dtsi

> >>>>> keystone.dtsi

> >>>>> exynos5440.dtsi

> >>>>>

> >>>>> None of them modifies the lower bits. To be more precise the only

> >> guy

> >>>>> that provides another translation layer is "dra7.dtsi":

> >>>>> axi0

> >>>>> http://lxr.free-

> >> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207

> >>>>>

> >>>>> axi1

> >>>>> http://lxr.free-

> >> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241

> >>>>>

> >>>>> For this case masking the top 4bits (bits28 to 31) should make

> the

> >> job.

> >>

> >> IMO, we should just fix this case. After further study, I don't

> think

> >> this is a DW issue, but rather an SOC integration issue.

> >>

> >> I believe you can just fixup the address in the pp->ops->host_init

> hook.

> >>

> >

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



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

> Thanks

> Kishon

Rob Herring July 31, 2015, 4:53 p.m. UTC | #20

On Fri, Jul 31, 2015 at 9:57 AM, Kishon Vijay Abraham I <[e-mail]> wrote:
> +Arnd
>
> Hi,
>
> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
>> [+cc Kishon]
>>
>>> -----Original Message-----
>>> From: [e-mail] [mailto:linux-pci-
>>> [e-mail]] On Behalf Of Rob Herring
>>> Sent: Thursday, July 30, 2015 9:42 PM
>>> To: Gabriele Paoloni
>>> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou
>>> (B); [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail];
>>> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
>>> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand
>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>>> of_pci_range
>>>
>>> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni
>>> <[e-mail]> wrote:
>>>>> -----Original Message-----
>>>>> From: Bjorn Helgaas [mailto:[e-mail]]
>>>>> Sent: 30 July 2015 18:15
>>>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: [e-mail] [mailto:linux-pci-
>>>>>>> [e-mail]] On Behalf Of Bjorn Helgaas
>>>>>>> Sent: Thursday, July 30, 2015 5:15 PM
>>>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:
>>>
>>> [...]
>>>
>>>>>>>> I don’t think we should rely on [CPU] addresses...what if the
>>>>>>> intermediate
>>>>>>>> translation layer changes the lower significant bits of the
>>> "bus
>>>>>>> address"
>>>>>>>> to translate into a cpu address?
>>>>>>>
>>>>>>> Is it really a possiblity that the lower bits could be changed?
>>>>>>
>>>>>> I've checked all the current deignware users DTs except "pci-
>>>>> layerscape"
>>>>>> that I could not find:
>>>>>> spear1310.dtsi
>>>>>> spear1340.dtsi
>>>>>> dra7.dtsi
>>>>>> imx6qdl.dtsi
>>>>>> imx6sx.dtsi
>>>>>> keystone.dtsi
>>>>>> exynos5440.dtsi
>>>>>>
>>>>>> None of them modifies the lower bits. To be more precise the only
>>> guy
>>>>>> that provides another translation layer is "dra7.dtsi":
>>>>>> axi0
>>>>>> http://lxr.free-
>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
>>>>>>
>>>>>> axi1
>>>>>> http://lxr.free-
>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
>>>>>>
>>>>>> For this case masking the top 4bits (bits28 to 31) should make the
>>> job.
>>>
>>> IMO, we should just fix this case. After further study, I don't think
>>> this is a DW issue, but rather an SOC integration issue.
>>>
>>> I believe you can just fixup the address in the pp->ops->host_init hook.
>>>
>>
>> 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

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

Han Jingoo Aug. 3, 2015, 2:41 p.m. UTC | #21

On 2015. 8. 1., at AM 12:09, Gabriele Paoloni <[e-mail]> wrote:
> 
>> -----Original Message-----
>> From: Kishon Vijay Abraham I [mailto:[e-mail]]
>> Sent: Friday, July 31, 2015 3:57 PM
>> To: Gabriele Paoloni; Rob Herring
>> Cc: Bjorn Helgaas; [e-mail]; [e-mail]; Wangzhou
>> (B); [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; 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,
>> 
>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
>>> [+cc Kishon]
>>> 
>>>> -----Original Message-----
>>>> From: [e-mail] [mailto:linux-pci-
>>>> [e-mail]] On Behalf Of Rob Herring
>>>> Sent: Thursday, July 30, 2015 9:42 PM
>>>> To: Gabriele Paoloni
>>>> Cc: Bjorn Helgaas; [e-mail]; [e-mail];
>> Wangzhou
>>>> (B); [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Yuanzhichang; Zhudacai; zhangjukuo;
>>>> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand
>>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>>>> of_pci_range
>>>> 
>>>> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni
>>>> <[e-mail]> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Bjorn Helgaas [mailto:[e-mail]]
>>>>>> Sent: 30 July 2015 18:15
>>>>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: [e-mail] [mailto:linux-pci-
>>>>>>>> [e-mail]] On Behalf Of Bjorn Helgaas
>>>>>>>> Sent: Thursday, July 30, 2015 5:15 PM
>>>>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni wrote:
>>>> 
>>>> [...]
>>>> 
>>>>>>>>> I don’t think we should rely on [CPU] addresses...what if the
>>>>>>>> intermediate
>>>>>>>>> translation layer changes the lower significant bits of the
>>>> "bus
>>>>>>>> address"
>>>>>>>>> to translate into a cpu address?
>>>>>>>> 
>>>>>>>> Is it really a possiblity that the lower bits could be changed?
>>>>>>> 
>>>>>>> I've checked all the current deignware users DTs except "pci-
>>>>>> layerscape"
>>>>>>> that I could not find:
>>>>>>> spear1310.dtsi
>>>>>>> spear1340.dtsi
>>>>>>> dra7.dtsi
>>>>>>> imx6qdl.dtsi
>>>>>>> imx6sx.dtsi
>>>>>>> keystone.dtsi
>>>>>>> exynos5440.dtsi
>>>>>>> 
>>>>>>> None of them modifies the lower bits. To be more precise the only
>>>> guy
>>>>>>> that provides another translation layer is "dra7.dtsi":
>>>>>>> axi0
>>>>>>> http://lxr.free-
>>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
>>>>>>> 
>>>>>>> axi1
>>>>>>> http://lxr.free-
>>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
>>>>>>> 
>>>>>>> For this case masking the top 4bits (bits28 to 31) should make
>> the
>>>> job.
>>>> 
>>>> IMO, we should just fix this case. After further study, I don't
>> think
>>>> this is a DW issue, but rather an SOC integration issue.
>>>> 
>>>> I believe you can just fixup the address in the pp->ops->host_init
>> hook.

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

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

> BTW good to have Arnd opinion too..
>> 
>> Thanks
>> Kishon

Patch

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 8bfda6a..23a5793 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -253,6 +253,7 @@  struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 						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;
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 5751dc5..fe57030 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -198,6 +198,7 @@  int of_pci_get_host_bridge_resources(struct device_node *dev,
 
 	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;
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index d88e81b..865f96e 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -16,6 +16,7 @@  struct of_pci_range {
 	u32 pci_space;
 	u64 pci_addr;
 	u64 cpu_addr;
+	u64 bus_addr;
 	u64 size;
 	u32 flags;
 };