[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
--
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
--
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;
> > >  };
--
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

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


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

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
--
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, 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.
--
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, 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.
--
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, 4:50 p.m. UTC | #13

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogbGludXgtcGNpLW93bmVy
QHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS0NCj4gb3duZXJAdmdlci5rZXJuZWwu
b3JnXSBPbiBCZWhhbGYgT2YgQmpvcm4gSGVsZ2Fhcw0KPiBTZW50OiBUaHVyc2RheSwgSnVseSAz
MCwgMjAxNSA1OjE1IFBNDQo+IFRvOiBHYWJyaWVsZSBQYW9sb25pDQo+IENjOiBSb2IgSGVycmlu
ZzsgYXJuZEBhcm5kYi5kZTsgbG9yZW56by5waWVyYWxpc2lAYXJtLmNvbTsgV2FuZ3pob3UgKEIp
Ow0KPiByb2JoK2R0QGtlcm5lbC5vcmc7IGphbWVzLm1vcnNlQGFybS5jb207IExpdml1LkR1ZGF1
QGFybS5jb207IGxpbnV4LQ0KPiBwY2lAdmdlci5rZXJuZWwub3JnOyBsaW51eC1hcm0ta2VybmVs
QGxpc3RzLmluZnJhZGVhZC5vcmc7DQo+IGRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOyBZdWFu
emhpY2hhbmc7IFpodWRhY2FpOyB6aGFuZ2p1a3VvOw0KPiBxaXV6aGVuZmE7IExpZ3Vvemh1IChL
ZW5uZXRoKQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHY2XSBQQ0k6IFN0b3JlIFBDSWUgYnVzIGFk
ZHJlc3MgaW4gc3RydWN0DQo+IG9mX3BjaV9yYW5nZQ0KPiANCj4gT24gVGh1LCBKdWwgMzAsIDIw
MTUgYXQgMDE6NTI6MTNQTSArMDAwMCwgR2FicmllbGUgUGFvbG9uaSB3cm90ZToNCj4gPg0KPiA+
DQo+ID4gPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+ID4gRnJvbTogbGludXgtcGNp
LW93bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS0NCj4gPiA+IG93bmVyQHZn
ZXIua2VybmVsLm9yZ10gT24gQmVoYWxmIE9mIFJvYiBIZXJyaW5nDQo+ID4gPiBTZW50OiBUaHVy
c2RheSwgSnVseSAzMCwgMjAxNSAyOjQzIFBNDQo+ID4gPiBUbzogR2FicmllbGUgUGFvbG9uaQ0K
PiA+ID4gQ2M6IEJqb3JuIEhlbGdhYXM7IGFybmRAYXJuZGIuZGU7IGxvcmVuem8ucGllcmFsaXNp
QGFybS5jb207DQo+IFdhbmd6aG91DQo+ID4gPiAoQik7IHJvYmgrZHRAa2VybmVsLm9yZzsgamFt
ZXMubW9yc2VAYXJtLmNvbTsgTGl2aXUuRHVkYXVAYXJtLmNvbTsNCj4gPiA+IGxpbnV4LXBjaUB2
Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsNCj4g
PiA+IGRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOyBZdWFuemhpY2hhbmc7IFpodWRhY2FpOyB6
aGFuZ2p1a3VvOw0KPiA+ID4gcWl1emhlbmZhOyBMaWd1b3podSAoS2VubmV0aCkNCj4gPiA+IFN1
YmplY3Q6IFJlOiBbUEFUQ0ggdjZdIFBDSTogU3RvcmUgUENJZSBidXMgYWRkcmVzcyBpbiBzdHJ1
Y3QNCj4gPiA+IG9mX3BjaV9yYW5nZQ0KPiA+ID4NCj4gPiA+IE9uIFdlZCwgSnVsIDI5LCAyMDE1
IGF0IDI6NDQgUE0sIEdhYnJpZWxlIFBhb2xvbmkNCj4gPiA+IDxnYWJyaWVsZS5wYW9sb25pQGh1
YXdlaS5jb20+IHdyb3RlOg0KPiA+ID4gPiBIaSBCam9ybg0KPiA+ID4gPg0KPiA+ID4gPiBNYW55
IFRoYW5rcyBmb3IgeW91ciByZXBseQ0KPiA+ID4gPg0KPiA+ID4gPiBJIGhhdmUgY29tbWVudGVk
IGJhY2sgaW5saW5lIHdpdGggcmVzb2x1dGlvbnMgZnJvbSBteSBzaWRlLg0KPiA+ID4gPg0KPiA+
ID4gPiBJZiB5b3UncmUgb2sgd2l0aCB0aGVtIEknbGwgc2VuZCBpdCBvdXQgYSBuZXcgdmVyc2lv
biBpbiB0aGUNCj4gPiA+IGFwcHJvcHJpYXRlIHBhdGNoc2V0DQo+ID4gPiA+DQo+ID4gPiA+IENo
ZWVycw0KPiA+ID4gPg0KPiA+ID4gPiBHYWINCj4gPiA+ID4NCj4gPiA+ID4+IC0tLS0tT3JpZ2lu
YWwgTWVzc2FnZS0tLS0tDQo+ID4gPiA+PiBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWlsdG86Ymhl
bGdhYXNAZ29vZ2xlLmNvbV0NCj4gPiA+ID4+IFNlbnQ6IFdlZG5lc2RheSwgSnVseSAyOSwgMjAx
NSA2OjIxIFBNDQo+ID4gPiA+PiBUbzogR2FicmllbGUgUGFvbG9uaQ0KPiA+ID4gPj4gQ2M6IGFy
bmRAYXJuZGIuZGU7IGxvcmVuem8ucGllcmFsaXNpQGFybS5jb207IFdhbmd6aG91IChCKTsNCj4g
PiA+ID4+IHJvYmgrZHRAa2VybmVsLm9yZzsgamFtZXMubW9yc2VAYXJtLmNvbTsgTGl2aXUuRHVk
YXVAYXJtLmNvbTsNCj4gbGludXgtDQo+ID4gPiA+PiBwY2lAdmdlci5rZXJuZWwub3JnOyBsaW51
eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmc7DQo+ID4gPiA+PiBkZXZpY2V0cmVlQHZn
ZXIua2VybmVsLm9yZzsgWXVhbnpoaWNoYW5nOyBaaHVkYWNhaTsgemhhbmdqdWt1bzsNCj4gPiA+
ID4+IHFpdXpoZW5mYTsgTGlndW96aHUgKEtlbm5ldGgpDQo+ID4gPiA+PiBTdWJqZWN0OiBSZTog
W1BBVENIIHY2XSBQQ0k6IFN0b3JlIFBDSWUgYnVzIGFkZHJlc3MgaW4gc3RydWN0DQo+ID4gPiA+
PiBvZl9wY2lfcmFuZ2UNCj4gPiA+ID4+DQo+ID4gPiA+PiBIaSBHYWJyaWVsZSwNCj4gPiA+ID4+
DQo+ID4gPiA+PiBBcyBmYXIgYXMgSSBjYW4gdGVsbCwgdGhpcyBpcyBub3Qgc3BlY2lmaWMgdG8g
UENJZSwgc28gcGxlYXNlDQo+IHVzZQ0KPiA+ID4gIlBDSSINCj4gPiA+ID4+IGluDQo+ID4gPiA+
PiB0aGUgc3ViamVjdCBhcyBhIGdlbmVyaWMgdGVybSB0aGF0IGluY2x1ZGVzIGJvdGggUENJIGFu
ZCBQQ0llLg0KPiA+ID4gPg0KPiA+ID4gPiBzdXJlIGFncmVlZA0KPiA+ID4gPg0KPiA+ID4gPj4N
Cj4gPiA+ID4+IE9uIE1vbiwgSnVsIDI3LCAyMDE1IGF0IDExOjE3OjAzUE0gKzA4MDAsIEdhYnJp
ZWxlIFBhb2xvbmkgd3JvdGU6DQo+ID4gPiA+PiA+IEZyb206IGdhYnJpZWxlIHBhb2xvbmkgPGdh
YnJpZWxlLnBhb2xvbmlAaHVhd2VpLmNvbT4NCj4gPiA+ID4+ID4NCj4gPiA+ID4+ID4gICAgIFRo
aXMgcGF0Y2ggaXMgbmVlZGVkIHBvcnQgUENJZSBkZXNpZ253YXJlIHRvIG5ldyBEVA0KPiBwYXJz
aW5nDQo+ID4gPiBBUEkNCj4gPiA+ID4+ID4gICAgIEFzIGRpc2N1c3NlZCBpbg0KPiA+ID4gPj4g
PiAgICAgaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvcGlwZXJtYWlsL2xpbnV4LWFybS0NCj4g
a2VybmVsLzIwMTUtDQo+ID4gPiA+PiBKYW51YXJ5LzMxNzc0My5odG1sDQo+ID4gPiA+PiA+ICAg
ICBpbiBkZXNpZ253YXJlIHdlIGhhdmUgYSBwcm9ibGVtIGFzIHRoZSBQQ0kgYWRkcmVzc2VzIGlu
DQo+IHRoZQ0KPiA+ID4gUENJZQ0KPiA+ID4gPj4gY29udHJvbGxlcg0KPiA+ID4gPj4gPiAgICAg
YWRkcmVzcyBzcGFjZSBhcmUgcmVxdWlyZWQgaW4gb3JkZXIgdG8gcGVyZm9ybSBjb3JyZWN0IEhX
DQo+ID4gPiA+PiBvcGVyYXRpb24uDQo+ID4gPiA+PiA+DQo+ID4gPiA+PiA+ICAgICBJbiBvcmRl
ciB0byBzb2x2ZSB0aGlzIHByb2JsZW0gY29tbWl0IGY0YzU1YzVhMyAiUENJOg0KPiA+ID4gZGVz
aWdud2FyZToNCj4gPiA+ID4+ID4gICAgIFByb2dyYW0gQVRVIHdpdGggdW50cmFuc2xhdGVkIGFk
ZHJlc3MiIGFkZGVkIGNvZGUgdG8gcmVhZA0KPiB0aGUNCj4gPiA+ID4+IFBDSWUNCj4gPiA+ID4+
DQo+ID4gPiA+PiBDb252ZW50aW9uYWwgcmVmZXJlbmNlIGlzIDEyLWNoYXIgU0hBMSwgbGlrZSB0
aGlzOg0KPiA+ID4gPj4NCj4gPiA+ID4+ICAgZjRjNTVjNWEzZjdmICgiUENJOiBkZXNpZ253YXJl
OiBQcm9ncmFtIEFUVSB3aXRoIHVudHJhbnNsYXRlZA0KPiA+ID4gPj4gYWRkcmVzcyIpDQo+ID4g
PiA+DQo+ID4gPiA+IEFncmVlZCwgd2lsbCBjaGFuZ2UgdGhpcw0KPiA+ID4gPg0KPiA+ID4gPj4N
Cj4gPiA+ID4+ID4gICAgIGNvbnRyb2xsZXIgc3RhcnQgYWRkcmVzcyBkaXJlY3RseSBmcm9tIHRo
ZSBEVCByYW5nZXMuDQo+ID4gPiA+PiA+DQo+ID4gPiA+PiA+ICAgICBJbiB0aGUgbmV3IERUIHBh
cnNpbmcgQVBJDQo+IG9mX3BjaV9nZXRfaG9zdF9icmlkZ2VfcmVzb3VyY2VzKCkNCj4gPiA+ID4+
IGhpZGVzIHRoZQ0KPiA+ID4gPj4gPiAgICAgRFQgcGFyc2VyIGZyb20gdGhlIGhvc3QgY29udHJv
bGxlciBkcml2ZXJzLCBzbyBpdCBpcyBub3QNCj4gPiA+IHBvc3NpYmxlDQo+ID4gPiA+PiA+ICAg
ICBmb3IgZHJpdmVycyB0byBwYXJzZSB2YWx1ZXMgZGlyZWN0bHkgZnJvbSB0aGUgRFQuDQo+ID4g
PiA+PiA+DQo+ID4gPiA+PiA+ICAgICBJbiBodHRwOi8vd3d3LnNwaW5pY3MubmV0L2xpc3RzL2xp
bnV4LXBjaS9tc2c0MjU0MC5odG1sIHdlDQo+ID4gPiA+PiBhbHJlYWR5IHRyaWVkDQo+ID4gPiA+
PiA+ICAgICB0byB1c2UgdGhlIG5ldyBEVCBwYXJzaW5nIEFQSSBidXQgdGhlcmUgaXMgYSBidWcN
Cj4gKG9idmlvdXNseSkNCj4gPiA+IGluDQo+ID4gPiA+PiBzZXR0aW5nDQo+ID4gPiA+PiA+ICAg
ICB0aGUgPCo+X21vZF9iYXNlIGFkZHJlc3Nlcw0KPiA+ID4gPj4gPiAgICAgQXBwbHlpbmcgdGhp
cyBwYXRjaCB3ZSBjYW4gZWFzaWx5IHNldCAiPCo+X21vZF9iYXNlID0gd2luLQ0KPiA+ID4gPj4g
Pl9fcmVzLnN0YXJ0Ig0KPiA+ID4gPj4NCj4gPiA+ID4+IEJ5IGl0c2VsZiwgdGhpcyBwYXRjaCBh
ZGRzIHNvbWV0aGluZy4gIEl0IHdvdWxkIGhlbHAgbWUNCj4gdW5kZXJzdGFuZA0KPiA+ID4gaXQN
Cj4gPiA+ID4+IGlmDQo+ID4gPiA+PiB0aGUgKnVzZXIqIG9mIHRoaXMgbmV3IHNvbWV0aGluZyB3
ZXJlIGluIHRoZSBzYW1lIHBhdGNoIHNlcmllcy4NCj4gPiA+ID4NCj4gPiA+ID4gdGhlIHVzZXIg
aXM6ICJbUEFUQ0ggdjUgMi81XSBQQ0k6IGRlc2lnbndhcmU6IEFkZCBBUk02NCBzdXBwb3J0Ig0K
PiA+ID4gPiBJIHdpbGwgYXNrIFpob3UgV2FuZyB0byBpbmNsdWRlIHRoaXMgcGF0Y2ggaW4gaGlz
IHBhdGNoc2V0DQo+ID4gPiA+DQo+ID4gPiA+DQo+ID4gPiA+Pg0KPiA+ID4gPj4gPiAgICAgVGhp
cyBwYXRjaCBhZGRzIGEgbmV3IGZpZWxkIGluICJzdHJ1Y3Qgb2ZfcGNpX3JhbmdlIiB0bw0KPiBz
dG9yZQ0KPiA+ID4gdGhlDQo+ID4gPiA+PiA+ICAgICBwY2kgYnVzIHN0YXJ0IGFkZHJlc3M7IGl0
IGZpbGxzIHRoZSBmaWVsZCBpbg0KPiA+ID4gPj4gb2ZfcGNpX3JhbmdlX3BhcnNlcl9vbmUoKTsN
Cj4gPiA+ID4+ID4gICAgIGluIG9mX3BjaV9nZXRfaG9zdF9icmlkZ2VfcmVzb3VyY2VzKCkgaXQg
cmV0cmlldmVzIHRoZQ0KPiA+ID4gcmVzb3VyY2UNCj4gPiA+ID4+IGVudHJ5DQo+ID4gPiA+PiA+
ICAgICBhZnRlciBpdCBpcyBjcmVhdGVkIGFuZCBhZGRlZCB0byB0aGUgcmVzb3VyY2UgbGlzdCBh
bmQNCj4gdXNlcw0KPiA+ID4gPj4gPiAgICAgZW50cnktPl9fcmVzLnN0YXJ0IHRvIHN0b3JlIHRo
ZSBwY2kgY29udHJvbGxlciBhZGRyZXNzDQo+ID4gPiA+Pg0KPiA+ID4gPj4gc3RydWN0IG9mX3Bj
aV9yYW5nZSBpcyBzdGFydGluZyB0byBnZXQgY29uZnVzaW5nIHRvIG5vbi1PRiBmb2xrcw0KPiA+
ID4gbGlrZQ0KPiA+ID4gPj4gbWUuDQo+ID4gPiA+PiBJdCBub3cgY29udGFpbnM6DQo+ID4gPiA+
Pg0KPiA+ID4gPj4gICB1MzIgcGNpX3NwYWNlOw0KPiA+ID4gPj4gICB1NjQgcGNpX2FkZHI7DQo+
ID4gPiA+PiAgIHU2NCBjcHVfYWRkcjsNCj4gPiA+ID4+ICAgdTY0IGJ1c19hZGRyOw0KPiA+ID4g
Pj4NCj4gPiA+ID4+IENhbiB5b3UgZXhwbGFpbiB3aGF0IGFsbCB0aGVzZSB0aGluZ3MgbWVhbiwg
YW5kIG1heWJlIGV2ZW4gYWRkDQo+IG9uZS0NCj4gPiA+IGxpbmUNCj4gPiA+ID4+IGNvbW1lbnRz
IHRvIHRoZSBzdHJ1Y3R1cmU/DQo+ID4gPiA+DQo+ID4gPiA+IHN1cmUgSSBjYW4gYWRkIGNvbW1l
bnRzIGlubGluZSBpbiB0aGUgY29kZQ0KPiA+ID4gPg0KPiA+ID4gPj4NCj4gPiA+ID4+IHBjaV9z
cGFjZTogVGhlIG9ubHkgdXNlcyBJIHNlZSBhcmUgdG8gZGV0ZXJtaW5lIHdoZXRoZXIgdG8gcHJp
bnQNCj4gPiA+ID4+ICJQcmVmZXRjaCIuICBJIGRvbid0IHNlZSBhbnkgcmVhbCBmdW5jdGlvbmFs
aXR5IHRoYXQgdXNlcyB0aGlzLg0KPiA+ID4gPg0KPiA+ID4gPiBMb29raW5nIGF0IHRoZSBjb2Rl
IEkgYWdyZWUuIGl0J3Mgc2VlbXMgdG8gYmUgdXNlZCBvbmx5IGluDQo+IHBvd2VycGMNCj4gPiA+
ID4gYW5kIG1pY3JvYmxhemUgdG8gcHJpbnQgb3V0Lg0KPiA+ID4gPiBIb3dldmVyIGZyb20gbXkg
dW5kZXJzdGFuZGluZyBwY2lfc3BhY2UgaXMgdGhlIHBoeXMuaGkgZmllbGQgb2YNCj4gdGhlDQo+
ID4gPiA+IHJhbmdlcyBwcm9wZXJ0eTogaXQgZGVmaW5lcyB0aGUgcHJvcGVydGllcyBvZiB0aGUg
YWRkcmVzcyBzcGFjZQ0KPiA+ID4gYXNzb2NpYXRlZA0KPiA+ID4gPiB0byB0aGUgUENJIGFkZHJl
c3MuIGlmIHlvdSdyZSBjdXJpb3VzIHlvdSBjYW4gZmluZCBhIG5pY2UgYW5kDQo+IHF1aWNrDQo+
ID4gPiB0byByZWFkDQo+ID4gPiA+ICJndWlkZSIgaW4gaHR0cDovL2RldmljZXRyZWUub3JnL01Q
QzUyMDA6UENJDQo+ID4gPiA+DQo+ID4gPiA+Pg0KPiA+ID4gPj4gcGNpX2FkZHI6IEkgYXNzdW1l
IHRoaXMgaXMgYSBQQ0kgYnVzIGFkZHJlc3MsIGxpa2Ugd2hhdCB5b3UNCj4gd291bGQNCj4gPiA+
IHNlZQ0KPiA+ID4gPj4gaWYNCj4gPiA+ID4+IHlvdSBwdXQgYW4gYW5hbHl6ZXIgb24gdGhlIGJ1
cy9saW5rLiAgVGhpcyBhZGRyZXNzIGNvdWxkIGdvIGluIGENCj4gQkFSLg0KPiA+ID4gPg0KPiA+
ID4gPiBZZXMsIHRoaXMgaXMgdGhlIFBDSSBzdGFydCBhZGRyZXNzIG9mIHRoZSByYW5nZTogcGh5
cy5taWQgKw0KPiBwaHlzLmxvdw0KPiA+ID4gaW4gdGhlDQo+ID4gPiA+IGd1aWRlIG1lbnRpb25l
ZCBhYm92ZQ0KPiA+ID4gPg0KPiA+ID4gPj4NCj4gPiA+ID4+IGNwdV9hZGRyOiBJIGFzc3VtZSB0
aGlzIGlzIGEgQ1BVIHBoeXNpY2FsIGFkZHJlc3MsIGxpa2Ugd2hhdCB5b3UNCj4gPiA+IHdvdWxk
DQo+ID4gPiA+PiBzZWUNCj4gPiA+ID4+IGluIC9wcm9jL2lvbWVtIGFuZCB3aGF0IHlvdSB3b3Vs
ZCBwYXNzIHRvIGlvcmVtYXAoKS4NCj4gPiA+ID4NCj4gPiA+ID4gWWVzIGNvcnJlY3QNCj4gPiA+
ID4NCj4gPiA+ID4+DQo+ID4gPiA+PiBidXNfYWRkcjogPw0KPiA+ID4gPj4NCj4gPiA+ID4NCj4g
PiA+ID4gQWNjb3JkaW5nIHRvIHRoZSBndWlkZSBhYm92ZSwgdGhpcyBpcyB0aGUgYWRkcmVzcyBp
bnRvIHdoaWNoIHRoZQ0KPiA+ID4gcGNpX2FkZHJlc3MNCj4gPiA+ID4gZ2V0IHRyYW5zbGF0ZWQg
dG8gYW5kIHRoYXQgaXMgcGFzc2VkIHRvIHRoZSByb290IGNvbXBsZXguIEJldHdlZW4NCj4gdGhl
DQo+ID4gPiByb290DQo+ID4gPiA+IGNvbXBsZXggYW5kIHRoZSBDUFUgdGhlcmUgY2FuIGJlIGlu
dGVybWVkaWF0ZSB0cmFuc2xhdGlvbiBsYXllcnM6DQo+IHNlZQ0KPiA+ID4gdGhhdCB0bw0KPiA+
ID4gPiBnZXQgcGNpX2FkZHJlc3Mgd2UgY2FsbCAib2ZfdHJhbnNsYXRlX2FkZHJlc3MiOyB0aGlz
IHdpbGwgYXBwbHkNCj4gYWxsDQo+ID4gPiB0aGUNCj4gPiA+ID4gdHJhbnNsYXRpb24gbGF5ZXJz
IChyYW5nZXMgaW4gdGhlIERUKSB0aGF0IGl0IGZpbmRzIHRpbGwgaXQgY29tZXMNCj4gdG8NCj4g
PiA+IHRoZSByb290DQo+ID4gPiA+IG5vZGUgb2YgdGhlIERUICh0aHVzIHJldHJpZXZpbmcgdGhl
IENQVSBhZGRyZXNzKS4NCj4gPiA+ID4gTm93IHNhaWQgdGhhdCwgZm9yIGRlc2lnbndhcmUgd2Ug
bmVlZCB0aGUgZmlyc3QgdHJhbnNsYXRlZCBQQ0kNCj4gPiA+IGFkZHJlc3MsIHRoYXQgd2UgY2Fs
bA0KPiA+ID4NCj4gPiA+IEkgdGhpbmsgeW91IG1lYW4gInRyYW5zbGF0ZWQgQ1BVIGFkZHJlc3Mu
IiBUaGUgZmxvdyBvZiBhZGRyZXNzZXMNCj4gbG9va3MNCj4gPiA+IGxpa2UgdGhpczoNCj4gPiA+
DQo+ID4gPiBDUFUgLT4gQ1BVIGJ1cyBhZGRyZXNzIC0+IGJ1cyBmYWJyaWMgYWRkcmVzcyBkZWNv
ZGluZyAtPiBidXMNCj4gYWRkcmVzcw0KPiA+ID4gLT4gRFcgUENJIC0+IFBDSSBhZGRyZXNzDQo+
ID4gPg0KPiA+ID4gVGhpcyBpcyBxdWl0ZSBjb21tb24gdGhhdCBhbiBJUCBibG9jayBkb2VzIG5v
dCBzZWUgdGhlIGZ1bGwgYWRkcmVzcy4NCj4gPiA+IEl0IGlzIHVudXN1YWwgdGhhdCB0aGUgSVAg
YmxvY2sgbmVlZHMgdG8ga25vdyBpdHMgZnVsbCBhZGRyZXNzIG9uDQo+IHRoZQ0KPiA+ID4gc2xh
dmUgc2lkZS4gSXQgaXMgcXVpdGUgY29tbW9uIGZvciB0aGUgbWFzdGVyIHNpZGUgYW5kIHRoZSBr
ZXJuZWwNCj4gPiA+IGRlYWxzIHdpdGggdGhhdCBhbGwgdGhlIHRpbWUgd2l0aCBETUEgbWFwcGlu
Zw0KPiA+ID4NCj4gPiA+ID4gaGVyZSBidXNfYWRkciBhZnRlciBSb2IgSGVycmluZyBzdWdnZXN0
ZWQgdGhlIG5hbWUuLi5ob25lc3RseSBJDQo+ID4gPiBjYW5ub3QgdGhpbmsgb2YgYQ0KPiA+ID4g
PiBkaWZmZXJlbnQgbmFtZQ0KPiA+ID4NCj4gPiA+IFRoaW5raW5nIGFib3V0IHRoaXMgc29tZSBt
b3JlLCBpcyB0aGlzIHJlYWxseSBhIHRyYW5zbGF0aW9uIHZlcnN1cw0KPiA+ID4ganVzdCBhIHN0
cmlwcGluZyBvZiBoaWdoIGJpdHM/IERvZXMgdGhlIERXIElQIGhhdmUgbGVzcyB0aGFuIDMyLQ0K
PiBiaXRzDQo+ID4gPiBhZGRyZXNzPyBJZiBzbywgd2UgY291bGQgZXhwcmVzcyBkaWZmZXJlbnRs
eSBhbmQganVzdCBtYXNrIHRoZSBDUFUNCj4gPiA+IGFkZHJlc3Mgd2l0aGluIHRoZSBkcml2ZXIg
aW5zdGVhZC4NCj4gPg0KPiA+IEkgZG9u4oCZdCB0aGluayB3ZSBzaG91bGQgcmVseSBvbiBbQ1BV
XSBhZGRyZXNzZXMuLi53aGF0IGlmIHRoZQ0KPiBpbnRlcm1lZGlhdGUNCj4gPiB0cmFuc2xhdGlv
biBsYXllciBjaGFuZ2VzIHRoZSBsb3dlciBzaWduaWZpY2FudCBiaXRzIG9mIHRoZSAiYnVzDQo+
IGFkZHJlc3MiDQo+ID4gdG8gdHJhbnNsYXRlIGludG8gYSBjcHUgYWRkcmVzcz8NCj4gDQo+IElz
IGl0IHJlYWxseSBhIHBvc3NpYmxpdHkgdGhhdCB0aGUgbG93ZXIgYml0cyBjb3VsZCBiZSBjaGFu
Z2VkPw0KDQpJJ3ZlIGNoZWNrZWQgYWxsIHRoZSBjdXJyZW50IGRlaWdud2FyZSB1c2VycyBEVHMg
ZXhjZXB0ICJwY2ktbGF5ZXJzY2FwZSIgDQp0aGF0IEkgY291bGQgbm90IGZpbmQ6DQpzcGVhcjEz
MTAuZHRzaQ0Kc3BlYXIxMzQwLmR0c2kNCmRyYTcuZHRzaQ0KaW14NnFkbC5kdHNpDQppbXg2c3gu
ZHRzaQ0Ka2V5c3RvbmUuZHRzaQ0KZXh5bm9zNTQ0MC5kdHNpDQoNCk5vbmUgb2YgdGhlbSBtb2Rp
ZmllcyB0aGUgbG93ZXIgYml0cy4gVG8gYmUgbW9yZSBwcmVjaXNlIHRoZSBvbmx5IGd1eQ0KdGhh
dCBwcm92aWRlcyBhbm90aGVyIHRyYW5zbGF0aW9uIGxheWVyIGlzICJkcmE3LmR0c2kiOg0KYXhp
MA0KaHR0cDovL2x4ci5mcmVlLWVsZWN0cm9ucy5jb20vc291cmNlL2FyY2gvYXJtL2Jvb3QvZHRz
L2RyYTcuZHRzaSNMMjA3DQoNCmF4aTENCmh0dHA6Ly9seHIuZnJlZS1lbGVjdHJvbnMuY29tL3Nv
dXJjZS9hcmNoL2FybS9ib290L2R0cy9kcmE3LmR0c2kjTDI0MQ0KDQpGb3IgdGhpcyBjYXNlIG1h
c2tpbmcgdGhlIHRvcCA0Yml0cyAoYml0czI4IHRvIDMxKSBzaG91bGQgbWFrZSB0aGUgam9iLg0K
DQpTcGVha2luZyBpbiBnZW5lcmFsIHRlcm1zIHNvIGZhciBJJ3ZlIGFsd2F5cyBzZWVuIGxpbmVh
ciBtYXBwaW5ncw0KdGhhdCBkaWZmZXIgYnkgYml0bWFzayBvZmZzZXQsIGhvd2V2ZXIgbGluZWFy
IGRvZXMgbm90IG1lYW4gdGhhdCB5b3UgDQpjYW5ub3QgYWZmZWN0IHRoZSBsb3dlciBiaXRzOiBl
LmcuIDwweDA+IHRvIDwweDAgKyBzaXplPiBjYW4gbWFwIHRvDQo8MHgwMDAwY2FmZSB0byAweDAw
MDBjYWZlICsgc2l6ZT4sIGJ1dCBJIGd1ZXNzIHRoYXQgZm9yIEhXIGRlc2lnbiByZWFzb25zDQpp
dCBpcyBtdWNoIGVhc2llciB0byByZW1hcCBkaXJlY3RseSB1c2luZyBhIGJpdG1hc2suLi5idXQg
SSB3YXMgbm90IHN1cmUNCmFuZCBJIGRpZG4ndCB0aGluayBhYm91dCB0aGUgcHJvYmxlbXMgdGhh
dCBjYW4gYXJpc2Ugd2l0aCBBQ1BJLg0KDQpJZiB5b3UgdGhpbmsgdGhlIGJpdG1hc2sgaXMgT2sg
dGhlbiBJIGNhbiBkaXJlY3RseSBkZWZpbmUgaXQgaW4NCmRlc2lnbndhcmUgYW5kIHdlIGNhbiBk
cm9wIHRoaXMgcGF0Y2guDQoNClRoYW5rcw0KR2FiDQogDQo+IA0KPiBJIHRoaW5rIHdlJ3JlIHRh
bGtpbmcgYWJvdXQgTU1JTyBoZXJlLCBhbmQgYSBicmlkZ2UgaGFzIGFuIE1NSU8NCj4gd2luZG93
LiAgQSB3aW5kb3cgaXMgYSByYW5nZSBvZiBDUFUgcGh5c2ljYWwgYWRkcmVzc2VzIHRoYXQgdGhl
DQo+IGJyaWRnZSBmb3J3YXJkcyB0byBQQ0kuICBUaGUgUENJIGNvcmUgYXNzdW1lcyB0aGF0IGEg
Q1BVIHBoeXNpY2FsDQo+IGFkZHJlc3MgcmFuZ2UgaXMgbGluZWFybHkgbWFwcGVkIHRvIFBDSSBi
dXMgYWRkcmVzc2VzLCBpLmUuLCBpZiB0aGUNCj4gd2luZG93IGJhc2UgaXMgWCBhbmQgbWFwcyB0
byBQQ0kgYWRkcmVzcyBZLCB0aGVuIGFzIGxvbmcgYXMgWCtuIGlzDQo+IGluc2lkZSB0aGUgd2lu
ZG93LCBpdCBtdXN0IG1hcCB0byBZK24uDQo+IA0KPiBUaGF0IG1lYW5zIHRoZSBsb3ctb3JkZXIg
Yml0cyAodGhlIG9uZXMgdGhhdCBhcmUgdGhlIG9mZnNldCBpbnRvIHRoZQ0KPiB3aW5kb3cpIGNh
bm5vdCBjaGFuZ2UuDQo+IC0tDQo+IFRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5k
IHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1wY2kiIGluDQo+IHRoZSBib2R5IG9mIGEgbWVz
c2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnDQo+IE1vcmUgbWFqb3Jkb21vIGluZm8g
YXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0K
--
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
--
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
--
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 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
--
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 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
--
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

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

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