[PATCH 00/20] PCI: fix config and I/O Address space memory mappings

[PATCH 00/20] PCI: fix config and I/O Address space memory mappings

[Lorenzo Pieralisi]

PCI local bus specifications (Rev3.0, 3.2.5 "Transaction Ordering
and Posting") strictly require PCI configuration and I/O Address space
write transactions to be non-posted.

Current crop of DT/ACPI PCI host controllers drivers relies on
the ioremap interface to map ECAM and ECAM-derivative PCI config
regions and pci_remap_iospace() to create a VMA for mapping
PCI host bridge I/O Address space transactions to CPU virtual address
space.

On some platforms (ie ARM/ARM64) ioremap fails to comply with the PCI
configuration non-posted write transactions requirement, because it
provides a memory mapping that issues "bufferable" or, in PCI terms
"posted" write transactions. Likewise, the current pci_remap_iospace()
implementation maps the physical address range that the PCI translates
to I/O space cycles to virtual address space through pgprot_device()
attributes that on eg ARM64 provides a memory mapping issuing
posted writes transactions, which is not PCI specifications compliant.

This patch series[1] addresses both issues in one go:

- It updates the pci_remap_iospace() function to use a page mapping
  that guarantees non-posted write transactions for I/O space addresses
- It adds a kernel API to remap PCI config space resources, so that
  architecture can override it with a mapping implementation that
  guarantees PCI specifications compliancy wrt non-posted write
  configuration transactions
- It updates all PCI host controller implementations (and the generic
  ECAM layer) to use the newly introduced mapping interface

Tested on Juno ECAM based interface (DT/ACPI).

Non-ECAM PCI host controller drivers patches need checking to make
sure that:

- I patched the correct resource region mapping for config space
- There are not any other ways to ensure posted-write completion
  in the respective pci_ops that make the relevant patch unnecessary

[1] git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git pci/config-io-mappings-fix

Cc: Pratyush Anand <pratyush.anand@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mingkai Hu <mingkai.hu@freescale.com>
Cc: Tanmay Inamdar <tinamdar@apm.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Wenrui Li <wenrui.li@rock-chips.com>
Cc: Shawn Lin <shawn.lin@rock-chips.com>
Cc: Minghuan Lian <minghuan.Lian@freescale.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Jon Mason <jonmason@broadcom.com>
Cc: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
Cc: Roy Zang <tie-fei.zang@freescale.com>

Lorenzo Pieralisi (20):
  PCI: remove __weak tag from pci_remap_iospace()
  PCI: fix pci_remap_iospace() remap attribute
  asm-generic/io.h: add PCI config space remap interface
  ARM64: implement pci_remap_cfgspace() interface
  ARM: implement pci_remap_cfgspace() interface
  PCI: ECAM: use pci_remap_cfgspace() to map config region
  PCI: implement Devres interface to map PCI config space
  PCI: xilinx: update PCI config space remap function
  PCI: xilinx-nwl: update PCI config space remap function
  PCI: spear13xx: update PCI config space remap function
  PCI: rockchip: update PCI config space remap function
  PCI: qcom: update PCI config space remap function
  PCI: iproc-platform: update PCI config space remap function
  PCI: hisi: update PCI config space remap function
  PCI: designware: update PCI config space remap function
  PCI: armada8k: update PCI config space remap function
  PCI: xgene: update PCI config space remap function
  PCI: tegra: update PCI config space remap function
  PCI: layerscape: update PCI config space remap function
  PCI: keystone-dw: update PCI config space remap function

 Documentation/driver-model/devres.txt  |  6 ++-
 arch/arm/include/asm/io.h              | 10 ++++
 arch/arm/mm/ioremap.c                  |  7 +++
 arch/arm64/include/asm/io.h            | 10 ++++
 drivers/pci/dwc/pci-keystone-dw.c      |  2 +-
 drivers/pci/dwc/pci-layerscape.c       |  2 +-
 drivers/pci/dwc/pcie-armada8k.c        |  2 +-
 drivers/pci/dwc/pcie-designware-host.c | 12 +++--
 drivers/pci/dwc/pcie-hisi.c            |  3 +-
 drivers/pci/dwc/pcie-qcom.c            |  2 +-
 drivers/pci/dwc/pcie-spear13xx.c       |  2 +-
 drivers/pci/ecam.c                     |  5 +-
 drivers/pci/host/pci-tegra.c           |  4 +-
 drivers/pci/host/pci-xgene.c           |  4 +-
 drivers/pci/host/pcie-iproc-platform.c |  3 +-
 drivers/pci/host/pcie-rockchip.c       |  2 +-
 drivers/pci/host/pcie-xilinx-nwl.c     |  2 +-
 drivers/pci/host/pcie-xilinx.c         |  2 +-
 drivers/pci/pci.c                      | 86 +++++++++++++++++++++++++++++++++-
 include/asm-generic/io.h               |  9 ++++
 include/linux/pci.h                    |  5 ++
 21 files changed, 154 insertions(+), 26 deletions(-)

-- 
2.10.0

[PATCH 01/20] PCI: remove __weak tag from pci_remap_iospace()

[Lorenzo Pieralisi]

pci_remap_iospace() is marked as a weak symbol even though
no architecture is currently overriding it; given that its
implementation internals have already code paths that are
arch specific (ie PCI_IOBASE and ioremap_page_range() attributes)
there is no need to leave the weak symbol in the kernel since the
same functionality can be achieved by customizing per-arch the
corresponding functionality.

Remove the __weak symbol from pci_remap_iospace().

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7904d02..bd98674 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3363,7 +3363,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
  *	Only architectures that have memory mapped IO functions defined
  *	(and the PCI_IOBASE value defined) should call this function.
  */
-int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
+int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 {
 #if defined(PCI_IOBASE) && defined(CONFIG_MMU)
 	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
-- 
2.10.0

[PATCH 01/20] PCI: remove __weak tag from pci_remap_iospace()

[Arnd Bergmann]

On Mon, Feb 27, 2017 at 4:14 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> pci_remap_iospace() is marked as a weak symbol even though
> no architecture is currently overriding it; given that its
> implementation internals have already code paths that are
> arch specific (ie PCI_IOBASE and ioremap_page_range() attributes)
> there is no need to leave the weak symbol in the kernel since the
> same functionality can be achieved by customizing per-arch the
> corresponding functionality.
>
> Remove the __weak symbol from pci_remap_iospace().

Good idea,

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Lorenzo Pieralisi]

According to the PCI local bus specifications (Revision 3.0, 3.2.5),
I/O Address space transactions are non-posted. On architectures where
I/O space is implemented through a chunk of memory mapped space mapped
to PCI address space (ie IA64/ARM/ARM64) the memory mapping for the
region backing I/O Address Space transactions determines the I/O
transactions attributes (before the transactions actually reaches the
PCI bus where it is handled according to the PCI specifications).

Current pci_remap_iospace() interface, that is used to map the PCI I/O
Address Space into virtual address space, use pgprot_device() as memory
attribute for the virtual address mapping, that in some architectures
(ie ARM64) provides non-cacheable but write bufferable mappings (ie
posted writes), which clash with the non-posted write behaviour for I/O
Address Space mandated by the PCI specifications.

Update the prot ioremap_page_range() parameter in pci_remap_iospace()
to pgprot_noncached to ensure that the virtual mapping backing
I/O Address Space guarantee non-posted write transactions issued
when addressing I/O Address Space through the MMIO mapping.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd98674..bfb3c6e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 		return -EINVAL;
 
 	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
-				  pgprot_device(PAGE_KERNEL));
+				  pgprot_noncached(PAGE_KERNEL));
 #else
 	/* this architecture does not have memory mapped I/O space,
 	   so this function should never be called */
-- 
2.10.0

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Bjorn Helgaas]

[+cc Luis]

On Mon, Feb 27, 2017 at 03:14:13PM +0000, Lorenzo Pieralisi wrote:
> According to the PCI local bus specifications (Revision 3.0, 3.2.5),
> I/O Address space transactions are non-posted. On architectures where
> I/O space is implemented through a chunk of memory mapped space mapped
> to PCI address space (ie IA64/ARM/ARM64) the memory mapping for the
> region backing I/O Address Space transactions determines the I/O
> transactions attributes (before the transactions actually reaches the
> PCI bus where it is handled according to the PCI specifications).
> 
> Current pci_remap_iospace() interface, that is used to map the PCI I/O
> Address Space into virtual address space, use pgprot_device() as memory
> attribute for the virtual address mapping, that in some architectures
> (ie ARM64) provides non-cacheable but write bufferable mappings (ie
> posted writes), which clash with the non-posted write behaviour for I/O
> Address Space mandated by the PCI specifications.
> 
> Update the prot ioremap_page_range() parameter in pci_remap_iospace()
> to pgprot_noncached to ensure that the virtual mapping backing
> I/O Address Space guarantee non-posted write transactions issued
> when addressing I/O Address Space through the MMIO mapping.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bd98674..bfb3c6e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  		return -EINVAL;
>  
>  	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
> -				  pgprot_device(PAGE_KERNEL));
> +				  pgprot_noncached(PAGE_KERNEL));

pgprot_device() is equivalent to pgprot_noncached() on all arches
except ARM64, and I trust you're doing the right thing on ARM64, so
I'm fine with this from a PCI perspective.

I do find this puzzling because I naively expected pgprot_noncached()
to match up with ioremap_nocache(), and apparently it doesn't.

For example, ARM64 ioremap_nocache() uses PROT_DEVICE_nGnRE, which
doesn't match the MT_DEVICE_nGnRnE in pgprot_noncached().

The point of these patches is to use non-posted mappings.  Apparently
you can do that with pgprot_noncached() here, but ioremap_nocache()
isn't enough for the config space mappings?

I suppose that's a consequence of the pgprot_noncached() vs
ioremap_nocache() mismatch, but this is all extremely confusing.

>  #else
>  	/* this architecture does not have memory mapped I/O space,
>  	   so this function should never be called */
> -- 
> 2.10.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Luis R. Rodriguez]

On Thu, Mar 16, 2017 at 04:48:44PM -0500, Bjorn Helgaas wrote:
> [+cc Luis]
> 
> On Mon, Feb 27, 2017 at 03:14:13PM +0000, Lorenzo Pieralisi wrote:
> > According to the PCI local bus specifications (Revision 3.0, 3.2.5),
> > I/O Address space transactions are non-posted. On architectures where
> > I/O space is implemented through a chunk of memory mapped space mapped
> > to PCI address space (ie IA64/ARM/ARM64) the memory mapping for the
> > region backing I/O Address Space transactions determines the I/O
> > transactions attributes (before the transactions actually reaches the
> > PCI bus where it is handled according to the PCI specifications).
> > 
> > Current pci_remap_iospace() interface, that is used to map the PCI I/O
> > Address Space into virtual address space, use pgprot_device() as memory
> > attribute for the virtual address mapping, that in some architectures
> > (ie ARM64) provides non-cacheable but write bufferable mappings (ie
> > posted writes), 

<sarcasm>
Gee wiz, I am glad this is so well documented.
</sarcasm>

> > which clash with the non-posted write behaviour for I/O
> > Address Space mandated by the PCI specifications.
> > 
> > Update the prot ioremap_page_range() parameter in pci_remap_iospace()
> > to pgprot_noncached to ensure that the virtual mapping backing
> > I/O Address Space guarantee non-posted write transactions issued
> > when addressing I/O Address Space through the MMIO mapping.

How did we end up with pgprot_device() then in the first place Liviu Dudau [0] ?
I ask for two reasons:

a) should we then use a Fixes tag for this patch ?
b) it does not seem clear what the semantics for pgprot_device() or even
   pgprot_noncached(). Can you add some ?

8b921acfeffdb ("PCI: Add pci_remap_iospace() to map bus I/O resources")

Also this patch claims archs can override this call alone, as its __weak.
So is the right thing to do to change pci_remap_iospace() to pgprot_noncached()
or is it for archs to add their own pci_remap_iospace()? If so why ? Without
proper semantics defined for these helpers this is all fuzzy.

> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> >  drivers/pci/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index bd98674..bfb3c6e 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> >  		return -EINVAL;
> >  
> >  	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
> > -				  pgprot_device(PAGE_KERNEL));
> > +				  pgprot_noncached(PAGE_KERNEL));
> 
> pgprot_device() is equivalent to pgprot_noncached() on all arches
> except ARM64, and I trust you're doing the right thing on ARM64, so
> I'm fine with this from a PCI perspective.
> 
> I do find this puzzling because I naively expected pgprot_noncached()
> to match up with ioremap_nocache(), and apparently it doesn't.
> 
> For example, ARM64 ioremap_nocache() uses PROT_DEVICE_nGnRE, which
> doesn't match the MT_DEVICE_nGnRnE in pgprot_noncached().
> 
> The point of these patches is to use non-posted mappings.  Apparently
> you can do that with pgprot_noncached() here, but ioremap_nocache()
> isn't enough for the config space mappings?

This is for iospace it seems, so the other patch I think was for
config space.

  Luis

> I suppose that's a consequence of the pgprot_noncached() vs
> ioremap_nocache() mismatch, but this is all extremely confusing.
> 
> >  #else
> >  	/* this architecture does not have memory mapped I/O space,
> >  	   so this function should never be called */
> > -- 
> > 2.10.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Liviu Dudau]

On Fri, Mar 17, 2017 at 01:33:21AM +0100, Luis R. Rodriguez wrote:
> On Thu, Mar 16, 2017 at 04:48:44PM -0500, Bjorn Helgaas wrote:
> > [+cc Luis]
> > 
> > On Mon, Feb 27, 2017 at 03:14:13PM +0000, Lorenzo Pieralisi wrote:
> > > According to the PCI local bus specifications (Revision 3.0, 3.2.5),
> > > I/O Address space transactions are non-posted. On architectures where
> > > I/O space is implemented through a chunk of memory mapped space mapped
> > > to PCI address space (ie IA64/ARM/ARM64) the memory mapping for the
> > > region backing I/O Address Space transactions determines the I/O
> > > transactions attributes (before the transactions actually reaches the
> > > PCI bus where it is handled according to the PCI specifications).
> > > 
> > > Current pci_remap_iospace() interface, that is used to map the PCI I/O
> > > Address Space into virtual address space, use pgprot_device() as memory
> > > attribute for the virtual address mapping, that in some architectures
> > > (ie ARM64) provides non-cacheable but write bufferable mappings (ie
> > > posted writes), 
> 
> <sarcasm>
> Gee wiz, I am glad this is so well documented.
> </sarcasm>
> 
> > > which clash with the non-posted write behaviour for I/O
> > > Address Space mandated by the PCI specifications.
> > > 
> > > Update the prot ioremap_page_range() parameter in pci_remap_iospace()
> > > to pgprot_noncached to ensure that the virtual mapping backing
> > > I/O Address Space guarantee non-posted write transactions issued
> > > when addressing I/O Address Space through the MMIO mapping.
> 
> How did we end up with pgprot_device() then in the first place Liviu Dudau [0] ?
> I ask for two reasons:

[replying using personal email as the corporate email system is taking its sweet time
to deliver the email to my inbox]

I've asked the people with the right knowledge about the correct API to use (Hi Catalin!),
and during the review it did not throw any red flags. I guess, given Bjorn's comment,
that everyone assumed AArch64 is the same as all other architectures and pgprot_device
is synonymous to pgprot_noncached.

> 
> a) should we then use a Fixes tag for this patch ?

I'm not aware of issues being reported, but Lorenzo might have more info on this.

> b) it does not seem clear what the semantics for pgprot_device() or even
>    pgprot_noncached(). Can you add some ?
> 
> 8b921acfeffdb ("PCI: Add pci_remap_iospace() to map bus I/O resources")
> 
> Also this patch claims archs can override this call alone, as its __weak.
> So is the right thing to do to change pci_remap_iospace() to pgprot_noncached()
> or is it for archs to add their own pci_remap_iospace()? If so why ? Without
> proper semantics defined for these helpers this is all fuzzy.

That was the initial intention, to let arches / platforms overwrite the whole
pci_remap_iospace(). I guess the reality is that no one needs to overwrite it except
for the AArch64 quirk, so probably easier to remove the __weak and fix the attributes for arm64.

Best regards,
Liviu

> 
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > ---
> > >  drivers/pci/pci.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index bd98674..bfb3c6e 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > >  		return -EINVAL;
> > >  
> > >  	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
> > > -				  pgprot_device(PAGE_KERNEL));
> > > +				  pgprot_noncached(PAGE_KERNEL));
> > 
> > pgprot_device() is equivalent to pgprot_noncached() on all arches
> > except ARM64, and I trust you're doing the right thing on ARM64, so
> > I'm fine with this from a PCI perspective.
> > 
> > I do find this puzzling because I naively expected pgprot_noncached()
> > to match up with ioremap_nocache(), and apparently it doesn't.
> > 
> > For example, ARM64 ioremap_nocache() uses PROT_DEVICE_nGnRE, which
> > doesn't match the MT_DEVICE_nGnRnE in pgprot_noncached().
> > 
> > The point of these patches is to use non-posted mappings.  Apparently
> > you can do that with pgprot_noncached() here, but ioremap_nocache()
> > isn't enough for the config space mappings?
> 
> This is for iospace it seems, so the other patch I think was for
> config space.
> 
>   Luis
> 
> > I suppose that's a consequence of the pgprot_noncached() vs
> > ioremap_nocache() mismatch, but this is all extremely confusing.
> > 
> > >  #else
> > >  	/* this architecture does not have memory mapped I/O space,
> > >  	   so this function should never be called */
> > > -- 
> > > 2.10.0
> > > 
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> -- 
> Luis Rodriguez, SUSE LINUX GmbH
> Maxfeldstrasse 5; D-90409 Nuernberg

-- 
   _
 _|_|_
 ('_')
 (?  )?
 |_|_|

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Luis R. Rodriguez]

On Fri, Mar 17, 2017 at 10:43:39AM +0000, Liviu Dudau wrote:
> On Fri, Mar 17, 2017 at 01:33:21AM +0100, Luis R. Rodriguez wrote:
> > a) should we then use a Fixes tag for this patch ?
> 
> I'm not aware of issues being reported, but Lorenzo might have more info on this.

Lorenzo ? If not what exactly made you discover this ? If it is a fix, and only 
ARM64 is implicated, seems like a worthy change to consider for stable for the
sake of stable ARM64 kernels. But, that would leave the PCI config space without
a simple 1 liner fix too -- so maybe its not worth it. Distributions wanting
to support ARM64 however would like to carry these changes, so some annotations
such as Fixes should help.

> > b) it does not seem clear what the semantics for pgprot_device() or even
> >    pgprot_noncached(). Can you add some ?
> > 
> > 8b921acfeffdb ("PCI: Add pci_remap_iospace() to map bus I/O resources")
> > 
> > Also this patch claims archs can override this call alone, as its __weak.
> > So is the right thing to do to change pci_remap_iospace() to pgprot_noncached()
> > or is it for archs to add their own pci_remap_iospace()? If so why ? Without
> > proper semantics defined for these helpers this is all fuzzy.
> 
> That was the initial intention, to let arches / platforms overwrite the whole
> pci_remap_iospace(). I guess the reality is that no one needs to overwrite it except
> for the AArch64 quirk, so probably easier to remove the __weak and fix the attributes for arm64.

Sounds much more reasonable to me.

  Luis

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Lorenzo Pieralisi]

On Fri, Mar 17, 2017 at 05:26:18PM +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 17, 2017 at 10:43:39AM +0000, Liviu Dudau wrote:
> > On Fri, Mar 17, 2017 at 01:33:21AM +0100, Luis R. Rodriguez wrote:
> > > a) should we then use a Fixes tag for this patch ?
> > 
> > I'm not aware of issues being reported, but Lorenzo might have more info on this.
> 
> Lorenzo ? If not what exactly made you discover this ? If it is a fix, and only 
> ARM64 is implicated, seems like a worthy change to consider for stable for the
> sake of stable ARM64 kernels. But, that would leave the PCI config space without
> a simple 1 liner fix too -- so maybe its not worth it. Distributions wanting
> to support ARM64 however would like to carry these changes, so some annotations
> such as Fixes should help.

It started with this thread:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/477353.html

this series is not fixing any current issue I am aware of (but I am not
keen on leaving code as-is either) hence adding a Fixes: tag is problematic.

I would leave stable kernels alone for the time being.

Lorenzo

> > > b) it does not seem clear what the semantics for pgprot_device() or even
> > >    pgprot_noncached(). Can you add some ?
> > > 
> > > 8b921acfeffdb ("PCI: Add pci_remap_iospace() to map bus I/O resources")
> > > 
> > > Also this patch claims archs can override this call alone, as its __weak.
> > > So is the right thing to do to change pci_remap_iospace() to pgprot_noncached()
> > > or is it for archs to add their own pci_remap_iospace()? If so why ? Without
> > > proper semantics defined for these helpers this is all fuzzy.
> > 
> > That was the initial intention, to let arches / platforms overwrite the whole
> > pci_remap_iospace(). I guess the reality is that no one needs to overwrite it except
> > for the AArch64 quirk, so probably easier to remove the __weak and fix the attributes for arm64.
> 
> Sounds much more reasonable to me.
> 
>   Luis

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Bjorn Helgaas]

On Fri, Mar 17, 2017 at 01:33:21AM +0100, Luis R. Rodriguez wrote:
> On Thu, Mar 16, 2017 at 04:48:44PM -0500, Bjorn Helgaas wrote:
> > [+cc Luis]
> > 
> > On Mon, Feb 27, 2017 at 03:14:13PM +0000, Lorenzo Pieralisi wrote:
> > > According to the PCI local bus specifications (Revision 3.0, 3.2.5),
> > > I/O Address space transactions are non-posted. On architectures where
> > > I/O space is implemented through a chunk of memory mapped space mapped
> > > to PCI address space (ie IA64/ARM/ARM64) the memory mapping for the
> > > region backing I/O Address Space transactions determines the I/O
> > > transactions attributes (before the transactions actually reaches the
> > > PCI bus where it is handled according to the PCI specifications).
> > > 
> > > Current pci_remap_iospace() interface, that is used to map the PCI I/O
> > > Address Space into virtual address space, use pgprot_device() as memory
> > > attribute for the virtual address mapping, that in some architectures
> > > (ie ARM64) provides non-cacheable but write bufferable mappings (ie
> > > posted writes), 
> 
> <sarcasm>
> Gee wiz, I am glad this is so well documented.
> </sarcasm>

I'm not sure this is actionable feedback.  The two paragraphs above
seem clear and useful to me.  How would you like to see them improved?

> > > ...
> > > @@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > >  		return -EINVAL;
> > >  
> > >  	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
> > > -				  pgprot_device(PAGE_KERNEL));
> > > +				  pgprot_noncached(PAGE_KERNEL));
> > 
> > ...
> > I do find this puzzling because I naively expected pgprot_noncached()
> > to match up with ioremap_nocache(), and apparently it doesn't.
> > 
> > For example, ARM64 ioremap_nocache() uses PROT_DEVICE_nGnRE, which
> > doesn't match the MT_DEVICE_nGnRnE in pgprot_noncached().
> > 
> > The point of these patches is to use non-posted mappings.  Apparently
> > you can do that with pgprot_noncached() here, but ioremap_nocache()
> > isn't enough for the config space mappings?
> 
> This is for iospace it seems, so the other patch I think was for
> config space.

I understand that 02/20 is for PCI I/O port space and 03/20 is for PCI
config space.  I'm confused because I thought we wanted the same
non-posted mapping for both, but looks like they're different.

Patch 02/20 uses ioremap_page_range(..., pgprot_noncached(PAGE_KERNEL))
to map PCI I/O port space:

  #define pgprot_noncached(prot) \
          __pgprot_modify(prot, PTE_ATTRINDX_MASK, PTE_ATTRINDX(MT_DEVICE_nGnRnE) | PTE_PXN | PTE_UXN)

Patch 03/20 uses ioremap_nocache() to map PCI config space:

  #define ioremap_nocache(addr, size)     __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))

So the I/O port mapping uses MT_DEVICE_nGnRnE, while the config space
mapping uses PROT_DEVICE_nGnRE, which looks different.

Bjorn

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Lorenzo Pieralisi]

On Mon, Mar 20, 2017 at 11:06:36AM -0500, Bjorn Helgaas wrote:

[...]

> > > > @@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > > >  		return -EINVAL;
> > > >  
> > > >  	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
> > > > -				  pgprot_device(PAGE_KERNEL));
> > > > +				  pgprot_noncached(PAGE_KERNEL));
> > > 
> > > ...
> > > I do find this puzzling because I naively expected pgprot_noncached()
> > > to match up with ioremap_nocache(), and apparently it doesn't.
> > > 
> > > For example, ARM64 ioremap_nocache() uses PROT_DEVICE_nGnRE, which
> > > doesn't match the MT_DEVICE_nGnRnE in pgprot_noncached().
> > > 
> > > The point of these patches is to use non-posted mappings.  Apparently
> > > you can do that with pgprot_noncached() here, but ioremap_nocache()
> > > isn't enough for the config space mappings?
> > 
> > This is for iospace it seems, so the other patch I think was for
> > config space.
> 
> I understand that 02/20 is for PCI I/O port space and 03/20 is for PCI
> config space.  I'm confused because I thought we wanted the same
> non-posted mapping for both, but looks like they're different.
> 
> Patch 02/20 uses ioremap_page_range(..., pgprot_noncached(PAGE_KERNEL))
> to map PCI I/O port space:
> 
>   #define pgprot_noncached(prot) \
>           __pgprot_modify(prot, PTE_ATTRINDX_MASK, PTE_ATTRINDX(MT_DEVICE_nGnRnE) | PTE_PXN | PTE_UXN)
> 
> Patch 03/20 uses ioremap_nocache() to map PCI config space:
> 
>   #define ioremap_nocache(addr, size)     __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
> 
> So the I/O port mapping uses MT_DEVICE_nGnRnE, while the config space
> mapping uses PROT_DEVICE_nGnRE, which looks different.

On ARM64 (PATCH 4) and ARM (PATCH 5) we override pci_remap_cfgspace()
with implementations that provide non-posted writes bus attributes,
PATCH 3 is just there to provide a "safe" (well, I need input on that)
fall-back.

Thanks,
Lorenzo

[PATCH 02/20] PCI: fix pci_remap_iospace() remap attribute

[Bjorn Helgaas]

On Mon, Mar 20, 2017 at 04:26:27PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Mar 20, 2017 at 11:06:36AM -0500, Bjorn Helgaas wrote:
> 
> [...]
> 
> > > > > @@ -3375,7 +3375,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > > > >  		return -EINVAL;
> > > > >  
> > > > >  	return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
> > > > > -				  pgprot_device(PAGE_KERNEL));
> > > > > +				  pgprot_noncached(PAGE_KERNEL));
> > > > 
> > > > ...
> > > > I do find this puzzling because I naively expected pgprot_noncached()
> > > > to match up with ioremap_nocache(), and apparently it doesn't.
> > > > 
> > > > For example, ARM64 ioremap_nocache() uses PROT_DEVICE_nGnRE, which
> > > > doesn't match the MT_DEVICE_nGnRnE in pgprot_noncached().
> > > > 
> > > > The point of these patches is to use non-posted mappings.  Apparently
> > > > you can do that with pgprot_noncached() here, but ioremap_nocache()
> > > > isn't enough for the config space mappings?
> > > 
> > > This is for iospace it seems, so the other patch I think was for
> > > config space.
> > 
> > I understand that 02/20 is for PCI I/O port space and 03/20 is for PCI
> > config space.  I'm confused because I thought we wanted the same
> > non-posted mapping for both, but looks like they're different.
> > 
> > Patch 02/20 uses ioremap_page_range(..., pgprot_noncached(PAGE_KERNEL))
> > to map PCI I/O port space:
> > 
> >   #define pgprot_noncached(prot) \
> >           __pgprot_modify(prot, PTE_ATTRINDX_MASK, PTE_ATTRINDX(MT_DEVICE_nGnRnE) | PTE_PXN | PTE_UXN)
> > 
> > Patch 03/20 uses ioremap_nocache() to map PCI config space:
> > 
> >   #define ioremap_nocache(addr, size)     __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
> > 
> > So the I/O port mapping uses MT_DEVICE_nGnRnE, while the config space
> > mapping uses PROT_DEVICE_nGnRE, which looks different.
> 
> On ARM64 (PATCH 4) and ARM (PATCH 5) we override pci_remap_cfgspace()
> with implementations that provide non-posted writes bus attributes,
> PATCH 3 is just there to provide a "safe" (well, I need input on that)
> fall-back.

Ah, thanks, that makes sense to me finally!  After patch 4,
pci_remap_cfgspace() uses PROT_DEVICE_nGnRnE, which does look like the
MT_DEVICE_nGnRnE used in the ioremap_page_range() path.

I wonder if we can get rid of pgprot_device() altogether?  Patch 2
removed the use in pci_remap_iospace(), and patch 18 removes the use
in tegra_pcie_bus_alloc().

Bjorn

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Lorenzo Pieralisi]

The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
Posting") mandate non-posted configuration transactions. As further
highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
Considerations for the Enhanced Configuration Access Mechanism"),
through ECAM and ECAM-derivative configuration mechanism, the memory
mapped transactions from the host CPU into Configuration Requests on the
PCI express fabric may create ordering problems for software because
writes to memory address are typically posted transactions (unless the
architecture can enforce through virtual address mapping non-posted
write transactions behaviour) but writes to Configuration Space are not
posted on the PCI express fabric.

Current DT and ACPI host bridge controllers map PCI configuration space
(ECAM and ECAM-derivative) into the virtual address space through
ioremap() calls, that are non-cacheable device accesses on most
architectures, but may provide "bufferable" or "posted" write semantics
in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
to be buffered in the bus connecting the host CPU to the PCI fabric;
this behaviour, as underlined in the PCIe specifications, may trigger
transactions ordering rules and must be prevented.

Introduce a new generic and explicit API to create a memory
mapping for ECAM and ECAM-derivative config space area that
defaults to ioremap_nocache() (which should provide a sane default
behaviour) but still allowing architectures on which ioremap_nocache()
results in posted write transactions to override the function
call with an arch specific implementation that complies with
the PCI specifications for configuration transactions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 include/asm-generic/io.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ef015e..52dda81 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
 #endif /* CONFIG_GENERIC_IOMAP */
 #endif /* CONFIG_HAS_IOPORT_MAP */
 
+#ifndef pci_remap_cfgspace
+#define pci_remap_cfgspace pci_remap_cfgspace
+static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
+					       size_t size)
+{
+	return ioremap_nocache(offset, size);
+}
+#endif
+
 #ifndef xlate_dev_kmem_ptr
 #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
 static inline void *xlate_dev_kmem_ptr(void *addr)
-- 
2.10.0

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Bjorn Helgaas]

[+cc Luis]

On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
> The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> Posting") mandate non-posted configuration transactions. As further
> highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> Considerations for the Enhanced Configuration Access Mechanism"),
> through ECAM and ECAM-derivative configuration mechanism, the memory
> mapped transactions from the host CPU into Configuration Requests on the
> PCI express fabric may create ordering problems for software because
> writes to memory address are typically posted transactions (unless the
> architecture can enforce through virtual address mapping non-posted
> write transactions behaviour) but writes to Configuration Space are not
> posted on the PCI express fabric.
> 
> Current DT and ACPI host bridge controllers map PCI configuration space
> (ECAM and ECAM-derivative) into the virtual address space through
> ioremap() calls, that are non-cacheable device accesses on most
> architectures, but may provide "bufferable" or "posted" write semantics
> in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> to be buffered in the bus connecting the host CPU to the PCI fabric;
> this behaviour, as underlined in the PCIe specifications, may trigger
> transactions ordering rules and must be prevented.
> 
> Introduce a new generic and explicit API to create a memory
> mapping for ECAM and ECAM-derivative config space area that
> defaults to ioremap_nocache() (which should provide a sane default
> behaviour) but still allowing architectures on which ioremap_nocache()
> results in posted write transactions to override the function
> call with an arch specific implementation that complies with
> the PCI specifications for configuration transactions.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  include/asm-generic/io.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 7ef015e..52dda81 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
>  #endif /* CONFIG_GENERIC_IOMAP */
>  #endif /* CONFIG_HAS_IOPORT_MAP */
>  
> +#ifndef pci_remap_cfgspace
> +#define pci_remap_cfgspace pci_remap_cfgspace
> +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> +					       size_t size)
> +{
> +	return ioremap_nocache(offset, size);
> +}

I'm fine with this conceptually, but I think it would make more sense
if the name weren't specific to PCI or config space, e.g.,
ioremap_nopost() or something.

> +#endif
> +
>  #ifndef xlate_dev_kmem_ptr
>  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
>  static inline void *xlate_dev_kmem_ptr(void *addr)
> -- 
> 2.10.0
> 

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Luis R. Rodriguez]

On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
> [+cc Luis]
> 
> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
> > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> > Posting") mandate non-posted configuration transactions. As further
> > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> > Considerations for the Enhanced Configuration Access Mechanism"),
> > through ECAM and ECAM-derivative configuration mechanism, the memory
> > mapped transactions from the host CPU into Configuration Requests on the
> > PCI express fabric may create ordering problems for software because
> > writes to memory address are typically posted transactions (unless the
> > architecture can enforce through virtual address mapping non-posted
> > write transactions behaviour) but writes to Configuration Space are not
> > posted on the PCI express fabric.
> > 
> > Current DT and ACPI host bridge controllers map PCI configuration space
> > (ECAM and ECAM-derivative) into the virtual address space through
> > ioremap() calls, that are non-cacheable device accesses on most
> > architectures, but may provide "bufferable" or "posted" write semantics
> > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> > to be buffered in the bus connecting the host CPU to the PCI fabric;
> > this behaviour, as underlined in the PCIe specifications, may trigger
> > transactions ordering rules and must be prevented.
> > 
> > Introduce a new generic and explicit API to create a memory
> > mapping for ECAM and ECAM-derivative config space area that
> > defaults to ioremap_nocache() (which should provide a sane default
> > behaviour) but still allowing architectures on which ioremap_nocache()
> > results in posted write transactions to override the function
> > call with an arch specific implementation that complies with
> > the PCI specifications for configuration transactions.

So... I take it this is actually fixing a series of odd issues also,
do we at least have some reports or actual issues ?

> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> >  include/asm-generic/io.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > index 7ef015e..52dda81 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
> >  #endif /* CONFIG_GENERIC_IOMAP */
> >  #endif /* CONFIG_HAS_IOPORT_MAP */
> >  
> > +#ifndef pci_remap_cfgspace
> > +#define pci_remap_cfgspace pci_remap_cfgspace
> > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> > +					       size_t size)
> > +{
> > +	return ioremap_nocache(offset, size);
> > +}
> 
> I'm fine with this conceptually, but I think it would make more sense
> if the name weren't specific to PCI or config space, e.g.,
> ioremap_nopost() or something.

Seems reasonable to me -- but are there other buses that could use this already
as well ? Wouldn't these other buses also run into similar issues ? Can someone
also bounce me a copy of the patches that use this ?

While at it, please add some documentation too, the above commit log is huge,
and yet for the person eyeballing the code they won't have any clue why this
was added exactly. Since this is about helping with picking the right
ioremap due to certain semantics / requirements on the PCI config space,
we should clarify then what exactly are the expectations here. The clearer
you are the less in trouble we can get later.

  Luis

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[John Garry]

On 17/03/2017 00:08, Luis R. Rodriguez wrote:
> On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
>> [+cc Luis]
>>
>> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
>>> The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
>>> Posting") mandate non-posted configuration transactions. As further
>>> highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
>>> Considerations for the Enhanced Configuration Access Mechanism"),
>>> through ECAM and ECAM-derivative configuration mechanism, the memory
>>> mapped transactions from the host CPU into Configuration Requests on the
>>> PCI express fabric may create ordering problems for software because
>>> writes to memory address are typically posted transactions (unless the
>>> architecture can enforce through virtual address mapping non-posted
>>> write transactions behaviour) but writes to Configuration Space are not
>>> posted on the PCI express fabric.
>>>
>>> Current DT and ACPI host bridge controllers map PCI configuration space
>>> (ECAM and ECAM-derivative) into the virtual address space through
>>> ioremap() calls, that are non-cacheable device accesses on most
>>> architectures, but may provide "bufferable" or "posted" write semantics
>>> in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
>>> to be buffered in the bus connecting the host CPU to the PCI fabric;
>>> this behaviour, as underlined in the PCIe specifications, may trigger
>>> transactions ordering rules and must be prevented.
>>>
>>> Introduce a new generic and explicit API to create a memory
>>> mapping for ECAM and ECAM-derivative config space area that
>>> defaults to ioremap_nocache() (which should provide a sane default
>>> behaviour) but still allowing architectures on which ioremap_nocache()
>>> results in posted write transactions to override the function
>>> call with an arch specific implementation that complies with
>>> the PCI specifications for configuration transactions.
>
> So... I take it this is actually fixing a series of odd issues also,
> do we at least have some reports or actual issues ?
>

We (Huawei) originally raised the concern [1], but have not actually 
experienced any related issue.

Thanks,
John

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/477353.html

>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Russell King <linux@armlinux.org.uk>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> ---
>>>  include/asm-generic/io.h | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
>>> index 7ef015e..52dda81 100644
>>> --- a/include/asm-generic/io.h
>>> +++ b/include/asm-generic/io.h
>>> @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
>>>  #endif /* CONFIG_GENERIC_IOMAP */
>>>  #endif /* CONFIG_HAS_IOPORT_MAP */
>>>
>>> +#ifndef pci_remap_cfgspace
>>> +#define pci_remap_cfgspace pci_remap_cfgspace
>>> +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
>>> +					       size_t size)
>>> +{
>>> +	return ioremap_nocache(offset, size);
>>> +}
>>
>> I'm fine with this conceptually, but I think it would make more sense
>> if the name weren't specific to PCI or config space, e.g.,
>> ioremap_nopost() or something.
>
> Seems reasonable to me -- but are there other buses that could use this already
> as well ? Wouldn't these other buses also run into similar issues ? Can someone
> also bounce me a copy of the patches that use this ?
>
> While at it, please add some documentation too, the above commit log is huge,
> and yet for the person eyeballing the code they won't have any clue why this
> was added exactly. Since this is about helping with picking the right
> ioremap due to certain semantics / requirements on the PCI config space,
> we should clarify then what exactly are the expectations here. The clearer
> you are the less in trouble we can get later.
>
>   Luis
>
> .
>

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Bjorn Helgaas]

On Fri, Mar 17, 2017 at 01:08:03AM +0100, Luis R. Rodriguez wrote:
> On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
> > [+cc Luis]
> > 
> > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
> > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> > > Posting") mandate non-posted configuration transactions. As further
> > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> > > Considerations for the Enhanced Configuration Access Mechanism"),
> > > through ECAM and ECAM-derivative configuration mechanism, the memory
> > > mapped transactions from the host CPU into Configuration Requests on the
> > > PCI express fabric may create ordering problems for software because
> > > writes to memory address are typically posted transactions (unless the
> > > architecture can enforce through virtual address mapping non-posted
> > > write transactions behaviour) but writes to Configuration Space are not
> > > posted on the PCI express fabric.
> > > 
> > > Current DT and ACPI host bridge controllers map PCI configuration space
> > > (ECAM and ECAM-derivative) into the virtual address space through
> > > ioremap() calls, that are non-cacheable device accesses on most
> > > architectures, but may provide "bufferable" or "posted" write semantics
> > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> > > to be buffered in the bus connecting the host CPU to the PCI fabric;
> > > this behaviour, as underlined in the PCIe specifications, may trigger
> > > transactions ordering rules and must be prevented.
> > > 
> > > Introduce a new generic and explicit API to create a memory
> > > mapping for ECAM and ECAM-derivative config space area that
> > > defaults to ioremap_nocache() (which should provide a sane default
> > > behaviour) but still allowing architectures on which ioremap_nocache()
> > > results in posted write transactions to override the function
> > > call with an arch specific implementation that complies with
> > > the PCI specifications for configuration transactions.
> > ...

> > > +#ifndef pci_remap_cfgspace
> > > +#define pci_remap_cfgspace pci_remap_cfgspace
> > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> > > +					       size_t size)
> > > +{
> > > +	return ioremap_nocache(offset, size);
> > > +}
> > 
> > I'm fine with this conceptually, but I think it would make more sense
> > if the name weren't specific to PCI or config space, e.g.,
> > ioremap_nopost() or something.
> 
> Seems reasonable to me -- but are there other buses that could use
> this already as well ? Wouldn't these other buses also run into
> similar issues ? Can someone also bounce me a copy of the patches
> that use this ?

I forwarded a copy of the initial posting of all 20 patches to you.

> While at it, please add some documentation too, the above commit log
> is huge, and yet for the person eyeballing the code they won't have
> any clue why this was added exactly. Since this is about helping
> with picking the right ioremap due to certain semantics /
> requirements on the PCI config space, we should clarify then what
> exactly are the expectations here. The clearer you are the less in
> trouble we can get later.

I think the documentation above does contain the critical facts that:

  - Accesses to PCI config space and PCI I/O port space should be
    non-posted

  - ARM64 currently maps them as posted, which may cause ordering
    problems

It's possible the changelog could be made clearer by *removing* text,
but I'm not sure what Lorenzo could *add* to make it better.

This particular patch is generic (not ARM64-specific), so maybe all we
need to say here is that PCI requires non-posted mappings in some
cases, and we currently don't have a generic ioremap() to make them,
so we're adding a way for an arch to provide such an interface.

Bjorn

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Lorenzo Pieralisi]

On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
> [+cc Luis]
> 
> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
> > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> > Posting") mandate non-posted configuration transactions. As further
> > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> > Considerations for the Enhanced Configuration Access Mechanism"),
> > through ECAM and ECAM-derivative configuration mechanism, the memory
> > mapped transactions from the host CPU into Configuration Requests on the
> > PCI express fabric may create ordering problems for software because
> > writes to memory address are typically posted transactions (unless the
> > architecture can enforce through virtual address mapping non-posted
> > write transactions behaviour) but writes to Configuration Space are not
> > posted on the PCI express fabric.
> > 
> > Current DT and ACPI host bridge controllers map PCI configuration space
> > (ECAM and ECAM-derivative) into the virtual address space through
> > ioremap() calls, that are non-cacheable device accesses on most
> > architectures, but may provide "bufferable" or "posted" write semantics
> > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> > to be buffered in the bus connecting the host CPU to the PCI fabric;
> > this behaviour, as underlined in the PCIe specifications, may trigger
> > transactions ordering rules and must be prevented.
> > 
> > Introduce a new generic and explicit API to create a memory
> > mapping for ECAM and ECAM-derivative config space area that
> > defaults to ioremap_nocache() (which should provide a sane default
> > behaviour) but still allowing architectures on which ioremap_nocache()
> > results in posted write transactions to override the function
> > call with an arch specific implementation that complies with
> > the PCI specifications for configuration transactions.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> >  include/asm-generic/io.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > index 7ef015e..52dda81 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
> >  #endif /* CONFIG_GENERIC_IOMAP */
> >  #endif /* CONFIG_HAS_IOPORT_MAP */
> >  
> > +#ifndef pci_remap_cfgspace
> > +#define pci_remap_cfgspace pci_remap_cfgspace
> > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> > +					       size_t size)
> > +{
> > +	return ioremap_nocache(offset, size);
> > +}
> 
> I'm fine with this conceptually, but I think it would make more sense
> if the name weren't specific to PCI or config space, e.g.,
> ioremap_nopost() or something.

I agree with you since there is not much PCI specific in the ioremap
implementation we are introducing (and it may be used more by other
subsystems too), I suspect it is all about finding a reasonable
ioremap_ naming extension (ioremap_nowb() ?).

On the other hand having an explicit pci_remap_cfgspace() interface
would simplify preventing drivers to sneak into the kernel with the
wrong ioremap API for config space (even though when all existing
ones are converted new drivers would just be based on existing ones
so it should be fine).

Suggestions welcome.

Thanks !
Lorenzo

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Lorenzo Pieralisi]

Hi Bjorn, Arnd,

On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
> [+cc Luis]
> 
> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
> > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> > Posting") mandate non-posted configuration transactions. As further
> > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> > Considerations for the Enhanced Configuration Access Mechanism"),
> > through ECAM and ECAM-derivative configuration mechanism, the memory
> > mapped transactions from the host CPU into Configuration Requests on the
> > PCI express fabric may create ordering problems for software because
> > writes to memory address are typically posted transactions (unless the
> > architecture can enforce through virtual address mapping non-posted
> > write transactions behaviour) but writes to Configuration Space are not
> > posted on the PCI express fabric.
> > 
> > Current DT and ACPI host bridge controllers map PCI configuration space
> > (ECAM and ECAM-derivative) into the virtual address space through
> > ioremap() calls, that are non-cacheable device accesses on most
> > architectures, but may provide "bufferable" or "posted" write semantics
> > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> > to be buffered in the bus connecting the host CPU to the PCI fabric;
> > this behaviour, as underlined in the PCIe specifications, may trigger
> > transactions ordering rules and must be prevented.
> > 
> > Introduce a new generic and explicit API to create a memory
> > mapping for ECAM and ECAM-derivative config space area that
> > defaults to ioremap_nocache() (which should provide a sane default
> > behaviour) but still allowing architectures on which ioremap_nocache()
> > results in posted write transactions to override the function
> > call with an arch specific implementation that complies with
> > the PCI specifications for configuration transactions.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> >  include/asm-generic/io.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > index 7ef015e..52dda81 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
> >  #endif /* CONFIG_GENERIC_IOMAP */
> >  #endif /* CONFIG_HAS_IOPORT_MAP */
> >  
> > +#ifndef pci_remap_cfgspace
> > +#define pci_remap_cfgspace pci_remap_cfgspace
> > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> > +					       size_t size)
> > +{
> > +	return ioremap_nocache(offset, size);
> > +}
> 
> I'm fine with this conceptually, but I think it would make more sense
> if the name weren't specific to PCI or config space, e.g.,
> ioremap_nopost() or something.

I would like to respin shortly so I have to make a decision on the
interface, either:

(1) I keep it a PCI only interface (which means I can even move it to
    include/linux/pci.h and arch*/include/asm/pci.h to override it)

or

(2) We add it as a generic ioremap_* interface (I am not sure though
    that's really useful other than to map PCI config space, actually
    the reasoning behind the naming was to limit its usage to PCI
    config space mappings)

I take it as Bjorn is keener on (2), just running a final check before
putting v2 together to make progress.

Thanks !
Lorenzo

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Arnd Bergmann]

On Wed, Mar 22, 2017 at 4:04 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Hi Bjorn, Arnd,
>
> On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
>> [+cc Luis]
>>
>> On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
>> > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
>> > Posting") mandate non-posted configuration transactions. As further
>> > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
>> > Considerations for the Enhanced Configuration Access Mechanism"),
>> > through ECAM and ECAM-derivative configuration mechanism, the memory
>> > mapped transactions from the host CPU into Configuration Requests on the
>> > PCI express fabric may create ordering problems for software because
>> > writes to memory address are typically posted transactions (unless the
>> > architecture can enforce through virtual address mapping non-posted
>> > write transactions behaviour) but writes to Configuration Space are not
>> > posted on the PCI express fabric.
>> >
>> > Current DT and ACPI host bridge controllers map PCI configuration space
>> > (ECAM and ECAM-derivative) into the virtual address space through
>> > ioremap() calls, that are non-cacheable device accesses on most
>> > architectures, but may provide "bufferable" or "posted" write semantics
>> > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
>> > to be buffered in the bus connecting the host CPU to the PCI fabric;
>> > this behaviour, as underlined in the PCIe specifications, may trigger
>> > transactions ordering rules and must be prevented.
>> >
>> > Introduce a new generic and explicit API to create a memory
>> > mapping for ECAM and ECAM-derivative config space area that
>> > defaults to ioremap_nocache() (which should provide a sane default
>> > behaviour) but still allowing architectures on which ioremap_nocache()
>> > results in posted write transactions to override the function
>> > call with an arch specific implementation that complies with
>> > the PCI specifications for configuration transactions.
>> >
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> > Cc: Arnd Bergmann <arnd@arndb.de>
>> > Cc: Will Deacon <will.deacon@arm.com>
>> > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > Cc: Russell King <linux@armlinux.org.uk>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > ---
>> >  include/asm-generic/io.h | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
>> > index 7ef015e..52dda81 100644
>> > --- a/include/asm-generic/io.h
>> > +++ b/include/asm-generic/io.h
>> > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
>> >  #endif /* CONFIG_GENERIC_IOMAP */
>> >  #endif /* CONFIG_HAS_IOPORT_MAP */
>> >
>> > +#ifndef pci_remap_cfgspace
>> > +#define pci_remap_cfgspace pci_remap_cfgspace
>> > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
>> > +                                          size_t size)
>> > +{
>> > +   return ioremap_nocache(offset, size);
>> > +}
>>
>> I'm fine with this conceptually, but I think it would make more sense
>> if the name weren't specific to PCI or config space, e.g.,
>> ioremap_nopost() or something.
>
> I would like to respin shortly so I have to make a decision on the
> interface, either:
>
> (1) I keep it a PCI only interface (which means I can even move it to
>     include/linux/pci.h and arch*/include/asm/pci.h to override it)
>
> or
>
> (2) We add it as a generic ioremap_* interface (I am not sure though
>     that's really useful other than to map PCI config space, actually
>     the reasoning behind the naming was to limit its usage to PCI
>     config space mappings)
>
> I take it as Bjorn is keener on (2), just running a final check before
> putting v2 together to make progress.

I'm fine with either way.

       Arnd

[PATCH 03/20] asm-generic/io.h: add PCI config space remap interface

[Bjorn Helgaas]

On Wed, Mar 22, 2017 at 03:04:03PM +0000, Lorenzo Pieralisi wrote:
> Hi Bjorn, Arnd,
> 
> On Thu, Mar 16, 2017 at 04:12:43PM -0500, Bjorn Helgaas wrote:
> > [+cc Luis]
> > 
> > On Mon, Feb 27, 2017 at 03:14:14PM +0000, Lorenzo Pieralisi wrote:
> > > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> > > Posting") mandate non-posted configuration transactions. As further
> > > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> > > Considerations for the Enhanced Configuration Access Mechanism"),
> > > through ECAM and ECAM-derivative configuration mechanism, the memory
> > > mapped transactions from the host CPU into Configuration Requests on the
> > > PCI express fabric may create ordering problems for software because
> > > writes to memory address are typically posted transactions (unless the
> > > architecture can enforce through virtual address mapping non-posted
> > > write transactions behaviour) but writes to Configuration Space are not
> > > posted on the PCI express fabric.
> > > 
> > > Current DT and ACPI host bridge controllers map PCI configuration space
> > > (ECAM and ECAM-derivative) into the virtual address space through
> > > ioremap() calls, that are non-cacheable device accesses on most
> > > architectures, but may provide "bufferable" or "posted" write semantics
> > > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> > > to be buffered in the bus connecting the host CPU to the PCI fabric;
> > > this behaviour, as underlined in the PCIe specifications, may trigger
> > > transactions ordering rules and must be prevented.
> > > 
> > > Introduce a new generic and explicit API to create a memory
> > > mapping for ECAM and ECAM-derivative config space area that
> > > defaults to ioremap_nocache() (which should provide a sane default
> > > behaviour) but still allowing architectures on which ioremap_nocache()
> > > results in posted write transactions to override the function
> > > call with an arch specific implementation that complies with
> > > the PCI specifications for configuration transactions.
> > > 
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > ---
> > >  include/asm-generic/io.h | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > index 7ef015e..52dda81 100644
> > > --- a/include/asm-generic/io.h
> > > +++ b/include/asm-generic/io.h
> > > @@ -915,6 +915,15 @@ extern void ioport_unmap(void __iomem *p);
> > >  #endif /* CONFIG_GENERIC_IOMAP */
> > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > >  
> > > +#ifndef pci_remap_cfgspace
> > > +#define pci_remap_cfgspace pci_remap_cfgspace
> > > +static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
> > > +					       size_t size)
> > > +{
> > > +	return ioremap_nocache(offset, size);
> > > +}
> > 
> > I'm fine with this conceptually, but I think it would make more sense
> > if the name weren't specific to PCI or config space, e.g.,
> > ioremap_nopost() or something.
> 
> I would like to respin shortly so I have to make a decision on the
> interface, either:
> 
> (1) I keep it a PCI only interface (which means I can even move it to
>     include/linux/pci.h and arch*/include/asm/pci.h to override it)
> 
> or
> 
> (2) We add it as a generic ioremap_* interface (I am not sure though
>     that's really useful other than to map PCI config space, actually
>     the reasoning behind the naming was to limit its usage to PCI
>     config space mappings)
> 
> I take it as Bjorn is keener on (2), just running a final check before
> putting v2 together to make progress.

The point of this seems to be to use non-posted mappings for both I/O
port space and ECAM (config) space.  So I think the most readable way
would be to have generic definitions like this:

  #ifndef ioremap_nopost
  #define ioremap_nopost ioremap_nocache
  #endif

  #ifndef pgprot_nonposted
  #define pgprot_nonposted(prot) pgprot_noncached(prot)
  #endif

and let ARM/ARM64 implement their own versions of those.  Then the
devm code might fit naturally in lib/devres.c, e.g.,

  void __iomem *devm_ioremap_nopost(...) { ... }

and we probably wouldn't need the pci_remap_cfgspace() wrapper, e.g.,
we could do:

  return ioremap_page_range(..., pgprot_nonposted(PAGE_KERNEL));

  cfg->win = ioremap_nopost(...);

But I might be oversimplifying things.

BTW, I tried to apply the v1 series on my "master" branch (v4.11-rc1)
and patch 19 didn't apply cleanly.  It's trivial and I can fix it up
by hand, so not really a problem, just FYI.

Bjorn

[PATCH 04/20] ARM64: implement pci_remap_cfgspace() interface

[Lorenzo Pieralisi]

The PCI bus specifications (rev 3.0, 3.2.5 "Transaction Ordering
and Posting") defines rules for PCI configuration space transactions
ordering and posting, that state that configuration writes
are non-posted transactions.

This rule is reinforced by the ARM v8 architecture reference manual
(issue A.k, Early Write Acknowledgment) that explicitly recommends
that No Early Write Acknowledgment attribute should be used to map
PCI configuration (write) transactions.

Current ioremap interface on ARM64 implements mapping functions
where the Early Write Acknowledgment hint is enabled, so they
cannot be used to map PCI configuration space in a PCI specs
compliant way.

Implement an ARM64 specific pci_remap_cfgspace() interface
that allows to map PCI config region with nGnRnE attributes, providing
a remap function that complies with PCI specifications and the ARMv8
architecture reference manual recommendations.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/io.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 0c00c87..7a03250 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -173,6 +173,16 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
 #define iounmap				__iounmap
 
 /*
+ * PCI configuration space mapping function.
+ *
+ * PCI specifications disallows posted write configuration transactions.
+ * Add an arch specific pci_remap_cfgspace definition that is implemented
+ * through nGnRnE device memory attribute as recommended by the ARM v8
+ * Architecture reference manual Issue A.k B2.8.2 "Device memory".
+ */
+#define pci_remap_cfgspace(addr, size) __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRnE))
+
+/*
  * io{read,write}{16,32,64}be() macros
  */
 #define ioread16be(p)		({ __u16 __v = be16_to_cpu((__force __be16)__raw_readw(p)); __iormb(); __v; })
-- 
2.10.0

[PATCH 05/20] ARM: implement pci_remap_cfgspace() interface

[Lorenzo Pieralisi]

The PCI bus specifications (rev 3.0, 3.2.5 "Transaction Ordering
and Posting") define rules for PCI configuration space transactions
ordering and posting, that state that configuration writes have to
be non-posted transactions.

Current ioremap interface on ARM provides mapping functions that
provide "bufferable" writes transactions (ie ioremap uses MT_DEVICE
memory type) aka posted writes, so PCI host controller drivers have
no arch interface to remap PCI configuration space with memory
attributes that comply with the PCI specifications for configuration
space.

Implement an ARM specific pci_remap_cfgspace() interface that allows to
map PCI config memory regions with MT_UNCACHED memory type (ie strongly
ordered - non-posted writes), providing a remap function that complies
with PCI specifications for config space transactions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Russell King <linux@armlinux.org.uk>
---
 arch/arm/include/asm/io.h | 10 ++++++++++
 arch/arm/mm/ioremap.c     |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 42871fb..74d1b09 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -187,6 +187,16 @@ static inline void pci_ioremap_set_mem_type(int mem_type) {}
 extern int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr);
 
 /*
+ * PCI configuration space mapping function.
+ *
+ * PCI specifications does not allow configuration write
+ * transactions to be posted. Add an arch specific
+ * pci_remap_cfgspace definition that is implemented
+ * through strongly ordered memory mappings.
+ */
+#define pci_remap_cfgspace pci_remap_cfgspace
+void __iomem *pci_remap_cfgspace(resource_size_t res_cookie, size_t size);
+/*
  * Now, pick up the machine-defined IO definitions
  */
 #ifdef CONFIG_NEED_MACH_IO_H
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index ff0eed2..fc91205 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -481,6 +481,13 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 				  __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
+
+void __iomem *pci_remap_cfgspace(resource_size_t res_cookie, size_t size)
+{
+	return arch_ioremap_caller(res_cookie, size, MT_UNCACHED,
+				   __builtin_return_address(0));
+}
+EXPORT_SYMBOL_GPL(pci_remap_cfgspace);
 #endif
 
 /*
-- 
2.10.0

[PATCH 05/20] ARM: implement pci_remap_cfgspace() interface

[Russell King - ARM Linux]

On Mon, Feb 27, 2017 at 03:14:16PM +0000, Lorenzo Pieralisi wrote:
> The PCI bus specifications (rev 3.0, 3.2.5 "Transaction Ordering
> and Posting") define rules for PCI configuration space transactions
> ordering and posting, that state that configuration writes have to
> be non-posted transactions.
> 
> Current ioremap interface on ARM provides mapping functions that
> provide "bufferable" writes transactions (ie ioremap uses MT_DEVICE
> memory type) aka posted writes, so PCI host controller drivers have
> no arch interface to remap PCI configuration space with memory
> attributes that comply with the PCI specifications for configuration
> space.
> 
> Implement an ARM specific pci_remap_cfgspace() interface that allows to
> map PCI config memory regions with MT_UNCACHED memory type (ie strongly
> ordered - non-posted writes), providing a remap function that complies
> with PCI specifications for config space transactions.

Doesn't this have the side effect that configuration writes can bypass
writes to device memory if there isn't an intervening dsb?  (They
probably can do on some CPUs today anyway.)

So, in any case, this looks like an improvement:

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 05/20] ARM: implement pci_remap_cfgspace() interface

[Lorenzo Pieralisi]

Hi Russell,

On Mon, Mar 20, 2017 at 04:43:55PM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 27, 2017 at 03:14:16PM +0000, Lorenzo Pieralisi wrote:
> > The PCI bus specifications (rev 3.0, 3.2.5 "Transaction Ordering
> > and Posting") define rules for PCI configuration space transactions
> > ordering and posting, that state that configuration writes have to
> > be non-posted transactions.
> > 
> > Current ioremap interface on ARM provides mapping functions that
> > provide "bufferable" writes transactions (ie ioremap uses MT_DEVICE
> > memory type) aka posted writes, so PCI host controller drivers have
> > no arch interface to remap PCI configuration space with memory
> > attributes that comply with the PCI specifications for configuration
> > space.
> > 
> > Implement an ARM specific pci_remap_cfgspace() interface that allows to
> > map PCI config memory regions with MT_UNCACHED memory type (ie strongly
> > ordered - non-posted writes), providing a remap function that complies
> > with PCI specifications for config space transactions.
> 
> Doesn't this have the side effect that configuration writes can bypass
> writes to device memory if there isn't an intervening dsb?  (They
> probably can do on some CPUs today anyway.)

I assumed that in plain terms, the difference between MT_DEVICE and
MT_UNCACHED is write posting (aka bufferable) behaviour (across CPU
architecture versions) and that does not affect write ordering rules.

You and Catalin have more insights into ARM 32-bit memory types so
I definitely need your input here to be comprehensive and avoid
pitfalls, let me know if you have some specific CPUs in mind on
which this may trigger a regression.

> So, in any case, this looks like an improvement:
> 
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thank you !

Lorenzo

[PATCH 05/20] ARM: implement pci_remap_cfgspace() interface

[Russell King - ARM Linux]

On Tue, Mar 21, 2017 at 03:26:36PM +0000, Lorenzo Pieralisi wrote:
> I assumed that in plain terms, the difference between MT_DEVICE and
> MT_UNCACHED is write posting (aka bufferable) behaviour (across CPU
> architecture versions) and that does not affect write ordering rules.

Having looked it up in the ARM ARM, I think we're fine.  Device and
strongly ordered accesses are both ordered as one group, so can be
thought of as the same.  The only difference is that Device can
complete from the CPU's perspective before it hits the peripheral,
whereas strongly ordered only completes once it's hit the peripheral.

So this looks like exactly the right thing to do here.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 06/20] PCI: ECAM: use pci_remap_cfgspace() to map config region

[Lorenzo Pieralisi]

Current ECAM kernel implementation uses ioremap() to map the ECAM
configuration space memory region; this is not safe in that on some
architectures the ioremap interface provides mappings that allow posted
write transactions. This, as highlighted in the PCIe specifications
(4.0 - Rev0.3, "Ordering Considerations for the Enhanced Configuration
Address Mechanism"), can create ordering issues for software because
posted writes transactions on the CPU host bus are non posted in the
PCI express fabric.

Update the ioremap() interface to use pci_remap_cfgspace() whose
mapping attributes guarantee that non-posted writes transactions
are issued for memory writes within the ECAM memory mapped address
region.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ecam.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 2fee61b..e2068a4 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -84,12 +84,13 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
 		if (!cfg->winp)
 			goto err_exit_malloc;
 		for (i = 0; i < bus_range; i++) {
-			cfg->winp[i] = ioremap(cfgres->start + i * bsz, bsz);
+			cfg->winp[i] =
+				pci_remap_cfgspace(cfgres->start + i * bsz, bsz);
 			if (!cfg->winp[i])
 				goto err_exit_iomap;
 		}
 	} else {
-		cfg->win = ioremap(cfgres->start, bus_range * bsz);
+		cfg->win = pci_remap_cfgspace(cfgres->start, bus_range * bsz);
 		if (!cfg->win)
 			goto err_exit_iomap;
 	}
-- 
2.10.0

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Lorenzo Pieralisi]

The introduction of the pci_remap_cfgspace() interface allows
PCI host controller drivers to map PCI config space through a
dedicated kernel interface. Current PCI host controller drivers
use the devm_ioremap_* Devres interfaces to map PCI configuration
space regions so in order to update them to the new
pci_remap_cfgspace() mapping interface a new set of Devres interfaces
should be implemented so that PCI host controller drivers can make
use of them.

Introduce two new functions in the PCI kernel layer and Devres
documentation:

- devm_pci_remap_cfgspace()
- devm_pci_remap_cfg_resource()

so that PCI host controller drivers can make use of them to map
PCI configuration space regions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 Documentation/driver-model/devres.txt |  6 ++-
 drivers/pci/pci.c                     | 82 +++++++++++++++++++++++++++++++++++
 include/linux/pci.h                   |  5 +++
 3 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index bf34d5b..e72587f 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -342,8 +342,10 @@ PER-CPU MEM
   devm_free_percpu()
 
 PCI
-  pcim_enable_device()	: after success, all PCI ops become managed
-  pcim_pin_device()	: keep PCI device enabled after release
+  devm_pci_remap_cfgspace()	: ioremap PCI configuration space
+  devm_pci_remap_cfg_resource()	: ioremap PCI configuration space resource
+  pcim_enable_device()		: after success, all PCI ops become managed
+  pcim_pin_device()		: keep PCI device enabled after release
 
 PHY
   devm_usb_get_phy()
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bfb3c6e..1e435c2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3401,6 +3401,88 @@ void pci_unmap_iospace(struct resource *res)
 #endif
 }
 
+/**
+ * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace()
+ * @dev: Generic device to remap IO address for
+ * @offset: BUS offset to map
+ * @size: Size of map
+ *
+ * Managed pci_remap_cfgspace().  Map is automatically unmapped on driver
+ * detach.
+ */
+void __iomem *devm_pci_remap_cfgspace(struct device *dev,
+				      resource_size_t offset,
+				      resource_size_t size)
+{
+	void __iomem **ptr, *addr;
+
+	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return NULL;
+
+	addr = pci_remap_cfgspace(offset, size);
+	if (addr) {
+		*ptr = addr;
+		devres_add(dev, ptr);
+	} else
+		devres_free(ptr);
+
+	return addr;
+}
+EXPORT_SYMBOL(devm_pci_remap_cfgspace);
+
+/**
+ * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource
+ * @dev: generic device to handle the resource for
+ * @res: configuration space resource to be handled
+ *
+ * Checks that a resource is a valid memory region, requests the memory
+ * region and ioremaps with pci_remap_cfgspace() API that ensures the
+ * proper PCI configuration space memory attributes are guaranteed.
+ *
+ * All operations are managed and will be undone on driver detach.
+ *
+ * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
+ * on failure. Usage example:
+ *
+ *	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ *	base = devm_pci_remap_cfg_resource(&pdev->dev, res);
+ *	if (IS_ERR(base))
+ *		return PTR_ERR(base);
+ */
+void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
+					  struct resource *res)
+{
+	resource_size_t size;
+	const char *name;
+	void __iomem *dest_ptr;
+
+	BUG_ON(!dev);
+
+	if (!res || resource_type(res) != IORESOURCE_MEM) {
+		dev_err(dev, "invalid resource\n");
+		return IOMEM_ERR_PTR(-EINVAL);
+	}
+
+	size = resource_size(res);
+	name = res->name ?: dev_name(dev);
+
+	if (!devm_request_mem_region(dev, res->start, size, name)) {
+		dev_err(dev, "can't request region for resource %pR\n", res);
+		return IOMEM_ERR_PTR(-EBUSY);
+	}
+
+	dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
+	if (!dest_ptr) {
+		dev_err(dev, "ioremap failed for resource %pR\n", res);
+		devm_release_mem_region(dev, res->start, size);
+		dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
+	}
+
+	return dest_ptr;
+}
+EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
+
 static void __pci_set_master(struct pci_dev *dev, bool enable)
 {
 	u16 old_cmd, cmd;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 282ed32..5a3588b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1199,6 +1199,11 @@ unsigned long pci_address_to_pio(phys_addr_t addr);
 phys_addr_t pci_pio_to_address(unsigned long pio);
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
 void pci_unmap_iospace(struct resource *res);
+void __iomem *devm_pci_remap_cfgspace(struct device *dev,
+				      resource_size_t offset,
+				      resource_size_t size);
+void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
+					  struct resource *res);
 
 static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
 {
-- 
2.10.0

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Lorenzo Pieralisi]

Arnd, all,

On Mon, Feb 27, 2017 at 03:14:18PM +0000, Lorenzo Pieralisi wrote:
> The introduction of the pci_remap_cfgspace() interface allows
> PCI host controller drivers to map PCI config space through a
> dedicated kernel interface. Current PCI host controller drivers
> use the devm_ioremap_* Devres interfaces to map PCI configuration
> space regions so in order to update them to the new
> pci_remap_cfgspace() mapping interface a new set of Devres interfaces
> should be implemented so that PCI host controller drivers can make
> use of them.
> 
> Introduce two new functions in the PCI kernel layer and Devres
> documentation:
> 
> - devm_pci_remap_cfgspace()
> - devm_pci_remap_cfg_resource()
> 
> so that PCI host controller drivers can make use of them to map
> PCI configuration space regions.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  Documentation/driver-model/devres.txt |  6 ++-
>  drivers/pci/pci.c                     | 82 +++++++++++++++++++++++++++++++++++
>  include/linux/pci.h                   |  5 +++
>  3 files changed, 91 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index bf34d5b..e72587f 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -342,8 +342,10 @@ PER-CPU MEM
>    devm_free_percpu()
>  
>  PCI
> -  pcim_enable_device()	: after success, all PCI ops become managed
> -  pcim_pin_device()	: keep PCI device enabled after release
> +  devm_pci_remap_cfgspace()	: ioremap PCI configuration space
> +  devm_pci_remap_cfg_resource()	: ioremap PCI configuration space resource
> +  pcim_enable_device()		: after success, all PCI ops become managed
> +  pcim_pin_device()		: keep PCI device enabled after release
>  
>  PHY
>    devm_usb_get_phy()
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bfb3c6e..1e435c2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3401,6 +3401,88 @@ void pci_unmap_iospace(struct resource *res)
>  #endif
>  }
>  
> +/**
> + * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace()
> + * @dev: Generic device to remap IO address for
> + * @offset: BUS offset to map
> + * @size: Size of map
> + *
> + * Managed pci_remap_cfgspace().  Map is automatically unmapped on driver
> + * detach.
> + */
> +void __iomem *devm_pci_remap_cfgspace(struct device *dev,
> +				      resource_size_t offset,
> +				      resource_size_t size)
> +{
> +	void __iomem **ptr, *addr;
> +
> +	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	addr = pci_remap_cfgspace(offset, size);

Provided this series is acceptable as-is, I noticed that I have
to add a #define for pci_remap_cfgspace() on all arches that do
not include asm-generic/io.h in their asm/io.h, I missed that,
please shout if you see an easier way to implement a
pci_remap_cfgspace() fall-back to ioremap_nocache() on all arches
that do not override it.

Thanks !
Lorenzo

> +	if (addr) {
> +		*ptr = addr;
> +		devres_add(dev, ptr);
> +	} else
> +		devres_free(ptr);
> +
> +	return addr;
> +}
> +EXPORT_SYMBOL(devm_pci_remap_cfgspace);
> +
> +/**
> + * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource
> + * @dev: generic device to handle the resource for
> + * @res: configuration space resource to be handled
> + *
> + * Checks that a resource is a valid memory region, requests the memory
> + * region and ioremaps with pci_remap_cfgspace() API that ensures the
> + * proper PCI configuration space memory attributes are guaranteed.
> + *
> + * All operations are managed and will be undone on driver detach.
> + *
> + * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
> + * on failure. Usage example:
> + *
> + *	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + *	base = devm_pci_remap_cfg_resource(&pdev->dev, res);
> + *	if (IS_ERR(base))
> + *		return PTR_ERR(base);
> + */
> +void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
> +					  struct resource *res)
> +{
> +	resource_size_t size;
> +	const char *name;
> +	void __iomem *dest_ptr;
> +
> +	BUG_ON(!dev);
> +
> +	if (!res || resource_type(res) != IORESOURCE_MEM) {
> +		dev_err(dev, "invalid resource\n");
> +		return IOMEM_ERR_PTR(-EINVAL);
> +	}
> +
> +	size = resource_size(res);
> +	name = res->name ?: dev_name(dev);
> +
> +	if (!devm_request_mem_region(dev, res->start, size, name)) {
> +		dev_err(dev, "can't request region for resource %pR\n", res);
> +		return IOMEM_ERR_PTR(-EBUSY);
> +	}
> +
> +	dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
> +	if (!dest_ptr) {
> +		dev_err(dev, "ioremap failed for resource %pR\n", res);
> +		devm_release_mem_region(dev, res->start, size);
> +		dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
> +	}
> +
> +	return dest_ptr;
> +}
> +EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
> +
>  static void __pci_set_master(struct pci_dev *dev, bool enable)
>  {
>  	u16 old_cmd, cmd;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 282ed32..5a3588b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1199,6 +1199,11 @@ unsigned long pci_address_to_pio(phys_addr_t addr);
>  phys_addr_t pci_pio_to_address(unsigned long pio);
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
>  void pci_unmap_iospace(struct resource *res);
> +void __iomem *devm_pci_remap_cfgspace(struct device *dev,
> +				      resource_size_t offset,
> +				      resource_size_t size);
> +void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
> +					  struct resource *res);
>  
>  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
>  {
> -- 
> 2.10.0
> 

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Andy Shevchenko]

On Mon, Feb 27, 2017 at 5:14 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> The introduction of the pci_remap_cfgspace() interface allows
> PCI host controller drivers to map PCI config space through a
> dedicated kernel interface. Current PCI host controller drivers
> use the devm_ioremap_* Devres interfaces to map PCI configuration
> space regions so in order to update them to the new
> pci_remap_cfgspace() mapping interface a new set of Devres interfaces
> should be implemented so that PCI host controller drivers can make
> use of them.
>
> Introduce two new functions in the PCI kernel layer and Devres
> documentation:
>
> - devm_pci_remap_cfgspace()
> - devm_pci_remap_cfg_resource()
>
> so that PCI host controller drivers can make use of them to map
> PCI configuration space regions.

Wouldn't you like to be consistent with current PCI API, i.e.:
1. devm_*() functions called pcim_*() in PCI.
2. If you may notice there is no separate pcim_*map*() stuff, they are
dynamically adapting to the case.

?

-- 
With Best Regards,
Andy Shevchenko

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Lorenzo Pieralisi]

Hi Andy,

On Thu, Mar 02, 2017 at 01:54:42AM +0200, Andy Shevchenko wrote:
> On Mon, Feb 27, 2017 at 5:14 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > The introduction of the pci_remap_cfgspace() interface allows
> > PCI host controller drivers to map PCI config space through a
> > dedicated kernel interface. Current PCI host controller drivers
> > use the devm_ioremap_* Devres interfaces to map PCI configuration
> > space regions so in order to update them to the new
> > pci_remap_cfgspace() mapping interface a new set of Devres interfaces
> > should be implemented so that PCI host controller drivers can make
> > use of them.
> >
> > Introduce two new functions in the PCI kernel layer and Devres
> > documentation:
> >
> > - devm_pci_remap_cfgspace()
> > - devm_pci_remap_cfg_resource()
> >
> > so that PCI host controller drivers can make use of them to map
> > PCI configuration space regions.
> 
> Wouldn't you like to be consistent with current PCI API, i.e.:
> 1. devm_*() functions called pcim_*() in PCI.

I thought about that and did not do it because here we are remapping
resources that are _not_ PCI bus resources (ie it is not PCI BARs we
are remapping), keeping the devm_* prefix would be more consistent
to the typical device drivers remapping functions pattern (ie a
typical PCI host controller driver would mix devm_ and pcim_ calls
which is a bit hard to parse), that was my rationale.

I am not too fussed about that either way, I am happy to update it to
pcim_* though, it is Bjorn/Arnd's decision.

> 2. If you may notice there is no separate pcim_*map*() stuff, they are
> dynamically adapting to the case.

I do not understand what you mean here I would ask you to elaborate
a bit more please so that I can do something about it.

Thanks !
Lorenzo

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Andy Shevchenko]

On Thu, Mar 2, 2017 at 2:05 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Thu, Mar 02, 2017 at 01:54:42AM +0200, Andy Shevchenko wrote:
>> On Mon, Feb 27, 2017 at 5:14 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:

+Cc: Tejun, who is initial author of PCI managed resources implementation.

> I thought about that and did not do it because here we are remapping
> resources that are _not_ PCI bus resources (ie it is not PCI BARs we
> are remapping), keeping the devm_* prefix would be more consistent
> to the typical device drivers remapping functions pattern (ie a
> typical PCI host controller driver would mix devm_ and pcim_ calls
> which is a bit hard to parse), that was my rationale.
>
> I am not too fussed about that either way, I am happy to update it to
> pcim_* though, it is Bjorn/Arnd's decision.

I would vote for pcim_*() variant.

>> 2. If you may notice there is no separate pcim_*map*() stuff, they are
>> dynamically adapting to the case.
>
> I do not understand what you mean here I would ask you to elaborate
> a bit more please so that I can do something about it.

Oh, sorry, there are two examples currently, i.e.
pci_enable_msi()/pci_enable_msix() and pci_request_region*() which has
no "m" in the name, but are managed on release by pcim_release().
Some developers consider this as a bad idea, but so far no patch has
been sent to introduce pcim_*() variants of those.

So, regarding to your stuff, I would stick with "pcim" prefix.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Tejun Heo]

Hello,

On Thu, Mar 02, 2017 at 02:50:00PM +0200, Andy Shevchenko wrote:
> > I thought about that and did not do it because here we are remapping
> > resources that are _not_ PCI bus resources (ie it is not PCI BARs we
> > are remapping), keeping the devm_* prefix would be more consistent
> > to the typical device drivers remapping functions pattern (ie a
> > typical PCI host controller driver would mix devm_ and pcim_ calls
> > which is a bit hard to parse), that was my rationale.
> >
> > I am not too fussed about that either way, I am happy to update it to
> > pcim_* though, it is Bjorn/Arnd's decision.
> 
> I would vote for pcim_*() variant.

Me too, for brevity.

> >> 2. If you may notice there is no separate pcim_*map*() stuff, they are
> >> dynamically adapting to the case.
> >
> > I do not understand what you mean here I would ask you to elaborate
> > a bit more please so that I can do something about it.
> 
> Oh, sorry, there are two examples currently, i.e.
> pci_enable_msi()/pci_enable_msix() and pci_request_region*() which has
> no "m" in the name, but are managed on release by pcim_release().
> Some developers consider this as a bad idea, but so far no patch has
> been sent to introduce pcim_*() variants of those.
>
> So, regarding to your stuff, I would stick with "pcim" prefix.

Sounds good to me.

Thanks.

-- 
tejun

[PATCH 07/20] PCI: implement Devres interface to map PCI config space

[Thierry Reding]

On Thu, Mar 02, 2017 at 02:24:06PM -0500, Tejun Heo wrote:
> Hello,
> 
> On Thu, Mar 02, 2017 at 02:50:00PM +0200, Andy Shevchenko wrote:
> > > I thought about that and did not do it because here we are remapping
> > > resources that are _not_ PCI bus resources (ie it is not PCI BARs we
> > > are remapping), keeping the devm_* prefix would be more consistent
> > > to the typical device drivers remapping functions pattern (ie a
> > > typical PCI host controller driver would mix devm_ and pcim_ calls
> > > which is a bit hard to parse), that was my rationale.
> > >
> > > I am not too fussed about that either way, I am happy to update it to
> > > pcim_* though, it is Bjorn/Arnd's decision.
> > 
> > I would vote for pcim_*() variant.
> 
> Me too, for brevity.

devm_* is equally brief. Also, all existing pcim_*() functions take a
struct pci_dev * as their first argument, because they operate on the
PCI devices. However in this case the devm_pci_remap_*() functions do
not operate on PCI devices. Rather they operate on the struct device
that represents the PCI host bridge. Therefore I think devm_ is more
appropriate here.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170302/9057b514/attachment.sig>

[PATCH 08/20] PCI: xilinx: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Cc: Michal Simek <michal.simek@xilinx.com>
---
 drivers/pci/host/pcie-xilinx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index 7f030f5..2fe2df5 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -606,7 +606,7 @@ static int xilinx_pcie_parse_dt(struct xilinx_pcie_port *port)
 		return err;
 	}
 
-	port->reg_base = devm_ioremap_resource(dev, &regs);
+	port->reg_base = devm_pci_remap_cfg_resource(dev, &regs);
 	if (IS_ERR(port->reg_base))
 		return PTR_ERR(port->reg_base);
 
-- 
2.10.0

[PATCH 09/20] PCI: xilinx-nwl: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Cc: Michal Simek <michal.simek@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 4c3e0ab..4b16b26 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -761,7 +761,7 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
 	pcie->phys_pcie_reg_base = res->start;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
-	pcie->ecam_base = devm_ioremap_resource(dev, res);
+	pcie->ecam_base = devm_pci_remap_cfg_resource(dev, res);
 	if (IS_ERR(pcie->ecam_base))
 		return PTR_ERR(pcie->ecam_base);
 	pcie->phys_ecam_base = res->start;
-- 
2.10.0

[PATCH 10/20] PCI: spear13xx: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generate on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Pratyush Anand <pratyush.anand@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/dwc/pcie-spear13xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/pcie-spear13xx.c b/drivers/pci/dwc/pcie-spear13xx.c
index 348f9c5..ea676a1 100644
--- a/drivers/pci/dwc/pcie-spear13xx.c
+++ b/drivers/pci/dwc/pcie-spear13xx.c
@@ -271,7 +271,7 @@ static int spear13xx_pcie_probe(struct platform_device *pdev)
 	}
 
 	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
-	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
 	if (IS_ERR(pci->dbi_base)) {
 		dev_err(dev, "couldn't remap dbi base %p\n", dbi_base);
 		ret = PTR_ERR(pci->dbi_base);
-- 
2.10.0

[PATCH 11/20] PCI: rockchip: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Wenrui Li <wenrui.li@rock-chips.com>
Cc: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/pci/host/pcie-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 26ddd35..690a7b5 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -822,7 +822,7 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 	regs = platform_get_resource_byname(pdev,
 					    IORESOURCE_MEM,
 					    "axi-base");
-	rockchip->reg_base = devm_ioremap_resource(dev, regs);
+	rockchip->reg_base = devm_pci_remap_cfg_resource(dev, regs);
 	if (IS_ERR(rockchip->reg_base))
 		return PTR_ERR(rockchip->reg_base);
 
-- 
2.10.0

[PATCH 12/20] PCI: qcom: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 drivers/pci/dwc/pcie-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
index e36abe0..bf7bd21 100644
--- a/drivers/pci/dwc/pcie-qcom.c
+++ b/drivers/pci/dwc/pcie-qcom.c
@@ -698,7 +698,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pcie->parf);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
-	pci->dbi_base = devm_ioremap_resource(dev, res);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
-- 
2.10.0

[PATCH 13/20] PCI: iproc-platform: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Jon Mason <jonmason@broadcom.com>
---
 drivers/pci/host/pcie-iproc-platform.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index f4909bb..b48d0db 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -67,7 +67,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pcie->base = devm_ioremap(dev, reg.start, resource_size(&reg));
+	pcie->base = devm_pci_remap_cfgspace(dev, reg.start,
+					     resource_size(&reg));
 	if (!pcie->base) {
 		dev_err(dev, "unable to map controller registers\n");
 		return -ENOMEM;
-- 
2.10.0

[PATCH 13/20] PCI: iproc-platform: update PCI config space remap function

[Ray Jui]

Hi Lorenzo,

On 2/27/2017 7:14 AM, Lorenzo Pieralisi wrote:
> PCI configuration space should be mapped with a memory region type that
> generates on the CPU host bus non-posted write transations. Update the
> driver to use the devm_pci_remap_cfg* interface to make sure the correct
> memory mappings for PCI configuration space are used.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Jon Mason <jonmason@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc-platform.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index f4909bb..b48d0db 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -67,7 +67,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	pcie->base = devm_ioremap(dev, reg.start, resource_size(&reg));
> +	pcie->base = devm_pci_remap_cfgspace(dev, reg.start,
> +					     resource_size(&reg));

Note these are NOT config space registers; instead, they are host
controller registers. iProc PCIe controller access config space
registers indirectly through two of the controller registers instead of
directly mapped.

Thanks,

Ray

>  	if (!pcie->base) {
>  		dev_err(dev, "unable to map controller registers\n");
>  		return -ENOMEM;
> 

[PATCH 13/20] PCI: iproc-platform: update PCI config space remap function

[Lorenzo Pieralisi]

Hi Ray,

On Mon, Feb 27, 2017 at 01:21:39PM -0800, Ray Jui wrote:
> Hi Lorenzo,
> 
> On 2/27/2017 7:14 AM, Lorenzo Pieralisi wrote:
> > PCI configuration space should be mapped with a memory region type that
> > generates on the CPU host bus non-posted write transations. Update the
> > driver to use the devm_pci_remap_cfg* interface to make sure the correct
> > memory mappings for PCI configuration space are used.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Ray Jui <rjui@broadcom.com>
> > Cc: Jon Mason <jonmason@broadcom.com>
> > ---
> >  drivers/pci/host/pcie-iproc-platform.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> > index f4909bb..b48d0db 100644
> > --- a/drivers/pci/host/pcie-iproc-platform.c
> > +++ b/drivers/pci/host/pcie-iproc-platform.c
> > @@ -67,7 +67,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > -	pcie->base = devm_ioremap(dev, reg.start, resource_size(&reg));
> > +	pcie->base = devm_pci_remap_cfgspace(dev, reg.start,
> > +					     resource_size(&reg));
> 
> Note these are NOT config space registers; instead, they are host
> controller registers. iProc PCIe controller access config space
> registers indirectly through two of the controller registers instead of
> directly mapped.

Yes but IIUC those registers that allow indirection live in the address
space pointed at by pcie->base, right ? Question is whether it is fine
to access those registers through mappings resulting on posted writes
and that's a question I need your help to answer as I said in the cover
letter.

Thanks a lot for flagging this up, that's exactly the feedback I need.

Lorenzo

> 
> Thanks,
> 
> Ray
> 
> >  	if (!pcie->base) {
> >  		dev_err(dev, "unable to map controller registers\n");
> >  		return -ENOMEM;
> > 

[PATCH 13/20] PCI: iproc-platform: update PCI config space remap function

[Ray Jui]

On 2/28/2017 2:54 AM, Lorenzo Pieralisi wrote:
> Hi Ray,
> 
> On Mon, Feb 27, 2017 at 01:21:39PM -0800, Ray Jui wrote:
>> Hi Lorenzo,
>>
>> On 2/27/2017 7:14 AM, Lorenzo Pieralisi wrote:
>>> PCI configuration space should be mapped with a memory region type that
>>> generates on the CPU host bus non-posted write transations. Update the
>>> driver to use the devm_pci_remap_cfg* interface to make sure the correct
>>> memory mappings for PCI configuration space are used.
>>>
>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Ray Jui <rjui@broadcom.com>
>>> Cc: Jon Mason <jonmason@broadcom.com>
>>> ---
>>>  drivers/pci/host/pcie-iproc-platform.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
>>> index f4909bb..b48d0db 100644
>>> --- a/drivers/pci/host/pcie-iproc-platform.c
>>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>>> @@ -67,7 +67,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>>>  		return ret;
>>>  	}
>>>  
>>> -	pcie->base = devm_ioremap(dev, reg.start, resource_size(&reg));
>>> +	pcie->base = devm_pci_remap_cfgspace(dev, reg.start,
>>> +					     resource_size(&reg));
>>
>> Note these are NOT config space registers; instead, they are host
>> controller registers. iProc PCIe controller access config space
>> registers indirectly through two of the controller registers instead of
>> directly mapped.
> 
> Yes but IIUC those registers that allow indirection live in the address
> space pointed at by pcie->base, right ? Question is whether it is fine
> to access those registers through mappings resulting on posted writes
> and that's a question I need your help to answer as I said in the cover
> letter.

This I'll need to check with our ASIC team, and it may take a while.

Note indirect access to the config registers is done with two registers
within pcie->base, one specifies the
address/bus/device/function/read/write direction and the other specifies
the data. All other registers in the block pointed to by pcie->base
really have nothing to do with config register access. They are used for
other block related configurations.

Thanks,

Ray

> 
> Thanks a lot for flagging this up, that's exactly the feedback I need.
> 
> Lorenzo
> 
>>
>> Thanks,
>>
>> Ray
>>
>>>  	if (!pcie->base) {
>>>  		dev_err(dev, "unable to map controller registers\n");
>>>  		return -ENOMEM;
>>>

[PATCH 14/20] PCI: hisi: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/pci/dwc/pcie-hisi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
index e3e4fed..8042780 100644
--- a/drivers/pci/dwc/pcie-hisi.c
+++ b/drivers/pci/dwc/pcie-hisi.c
@@ -294,10 +294,9 @@ static int hisi_pcie_probe(struct platform_device *pdev)
 	}
 
 	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbi");
-	pci->dbi_base = devm_ioremap_resource(dev, reg);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, reg);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
-
 	platform_set_drvdata(pdev, hisi_pcie);
 
 	ret = hisi_add_pcie_port(hisi_pcie, pdev);
-- 
2.10.0

[PATCH 14/20] PCI: hisi: update PCI config space remap function

[Gabriele Paoloni]

Hi Lorenzo

Many thanks for putting all of this together.

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: 27 February 2017 15:14
> To: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org; linux-arch at vger.kernel.org; Lorenzo
> Pieralisi; Bjorn Helgaas; Gabriele Paoloni; Wangzhou (B)
> Subject: [PATCH 14/20] PCI: hisi: update PCI config space remap
> function
> 
> PCI configuration space should be mapped with a memory region type that
> generates on the CPU host bus non-posted write transations. Update the
> driver to use the devm_pci_remap_cfg* interface to make sure the
> correct
> memory mappings for PCI configuration space are used.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> Cc: Zhou Wang <wangzhou1@hisilicon.com>
> ---
>  drivers/pci/dwc/pcie-hisi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
> index e3e4fed..8042780 100644
> --- a/drivers/pci/dwc/pcie-hisi.c
> +++ b/drivers/pci/dwc/pcie-hisi.c
> @@ -294,10 +294,9 @@ static int hisi_pcie_probe(struct platform_device
> *pdev)
>  	}
> 
>  	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "rc_dbi");
> -	pci->dbi_base = devm_ioremap_resource(dev, reg);
> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, reg);
>  	if (IS_ERR(pci->dbi_base))
>  		return PTR_ERR(pci->dbi_base);
> -
>  	platform_set_drvdata(pdev, hisi_pcie);
> 
>  	ret = hisi_add_pcie_port(hisi_pcie, pdev);
> --
> 2.10.0

I think you missed a couple of places where cfg space is ioremapped.
I have added these and merged with your changes in the patch below:

Thanks again
Gab

---------------------------------
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date: Tue, 21 Feb 2017 15:24:34 +0000
Subject: [PATCH 09/15] PCI: hisi: update PCI config space remap function

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/pci/dwc/pcie-hisi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
index e3e4fed..a5b542c 100644
--- a/drivers/pci/dwc/pcie-hisi.c
+++ b/drivers/pci/dwc/pcie-hisi.c
@@ -99,7 +99,7 @@ static int hisi_pcie_init(struct pci_config_window *cfg)
 		return -ENOMEM;
 	}
 
-	reg_base = devm_ioremap(dev, res->start, resource_size(res));
+	reg_base = devm_pci_remap_cfgspace(dev, res->start, resource_size(res));
 	if (!reg_base)
 		return -ENOMEM;
 
@@ -294,10 +294,9 @@ static int hisi_pcie_probe(struct platform_device *pdev)
 	}
 
 	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbi");
-	pci->dbi_base = devm_ioremap_resource(dev, reg);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, reg);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
-
 	platform_set_drvdata(pdev, hisi_pcie);
 
 	ret = hisi_add_pcie_port(hisi_pcie, pdev);
@@ -358,7 +357,7 @@ static int hisi_pcie_platform_init(struct pci_config_window *cfg)
 		return -EINVAL;
 	}
 
-	reg_base = devm_ioremap(dev, res->start, resource_size(res));
+	reg_base = devm_pci_remap_cfgspace(dev, res->start, resource_size(res));
 	if (!reg_base)
 		return -ENOMEM;
 
-- 
2.7.4

[PATCH 14/20] PCI: hisi: update PCI config space remap function

[Lorenzo Pieralisi]

Hi Gabriele,

On Thu, Mar 02, 2017 at 10:56:16AM +0000, Gabriele Paoloni wrote:
> Hi Lorenzo
> 
> Many thanks for putting all of this together.
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > Sent: 27 February 2017 15:14
> > To: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> > Cc: linux-kernel at vger.kernel.org; linux-arch at vger.kernel.org; Lorenzo
> > Pieralisi; Bjorn Helgaas; Gabriele Paoloni; Wangzhou (B)
> > Subject: [PATCH 14/20] PCI: hisi: update PCI config space remap
> > function
> > 
> > PCI configuration space should be mapped with a memory region type that
> > generates on the CPU host bus non-posted write transations. Update the
> > driver to use the devm_pci_remap_cfg* interface to make sure the
> > correct
> > memory mappings for PCI configuration space are used.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > Cc: Zhou Wang <wangzhou1@hisilicon.com>
> > ---
> >  drivers/pci/dwc/pcie-hisi.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
> > index e3e4fed..8042780 100644
> > --- a/drivers/pci/dwc/pcie-hisi.c
> > +++ b/drivers/pci/dwc/pcie-hisi.c
> > @@ -294,10 +294,9 @@ static int hisi_pcie_probe(struct platform_device
> > *pdev)
> >  	}
> > 
> >  	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "rc_dbi");
> > -	pci->dbi_base = devm_ioremap_resource(dev, reg);
> > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, reg);
> >  	if (IS_ERR(pci->dbi_base))
> >  		return PTR_ERR(pci->dbi_base);
> > -
> >  	platform_set_drvdata(pdev, hisi_pcie);
> > 
> >  	ret = hisi_add_pcie_port(hisi_pcie, pdev);
> > --
> > 2.10.0
> 
> I think you missed a couple of places where cfg space is ioremapped.
> I have added these and merged with your changes in the patch below:
> 
> Thanks again
> Gab
> 
> ---------------------------------
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Date: Tue, 21 Feb 2017 15:24:34 +0000
> Subject: [PATCH 09/15] PCI: hisi: update PCI config space remap function
> 
> PCI configuration space should be mapped with a memory region type that
> generates on the CPU host bus non-posted write transations. Update the
> driver to use the devm_pci_remap_cfg* interface to make sure the correct
> memory mappings for PCI configuration space are used.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> Cc: Zhou Wang <wangzhou1@hisilicon.com>
> ---
>  drivers/pci/dwc/pcie-hisi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
> index e3e4fed..a5b542c 100644
> --- a/drivers/pci/dwc/pcie-hisi.c
> +++ b/drivers/pci/dwc/pcie-hisi.c
> @@ -99,7 +99,7 @@ static int hisi_pcie_init(struct pci_config_window *cfg)
>  		return -ENOMEM;
>  	}
>  
> -	reg_base = devm_ioremap(dev, res->start, resource_size(res));
> +	reg_base = devm_pci_remap_cfgspace(dev, res->start, resource_size(res));
>  	if (!reg_base)
>  		return -ENOMEM;
>  
> @@ -294,10 +294,9 @@ static int hisi_pcie_probe(struct platform_device *pdev)
>  	}
>  
>  	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbi");
> -	pci->dbi_base = devm_ioremap_resource(dev, reg);
> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, reg);
>  	if (IS_ERR(pci->dbi_base))
>  		return PTR_ERR(pci->dbi_base);
> -
>  	platform_set_drvdata(pdev, hisi_pcie);
>  
>  	ret = hisi_add_pcie_port(hisi_pcie, pdev);
> @@ -358,7 +357,7 @@ static int hisi_pcie_platform_init(struct pci_config_window *cfg)
>  		return -EINVAL;
>  	}
>  
> -	reg_base = devm_ioremap(dev, res->start, resource_size(res));
> +	reg_base = devm_pci_remap_cfgspace(dev, res->start, resource_size(res));
>  	if (!reg_base)
>  		return -ENOMEM;

I will fold the changes into v2 (and I hope other host controllers
maintainers will follow suit - I do not have enough knowledge of
all host bridges drivers internals to understand what ioremap calls
need patching).

Thanks !
Lorenzo

[PATCH 14/20] PCI: hisi: update PCI config space remap function

[Gabriele Paoloni]

[...]

> 
> I will fold the changes into v2 (and I hope other host controllers
> maintainers will follow suit - I do not have enough knowledge of
> all host bridges drivers internals to understand what ioremap calls
> need patching).

Ok great, many thanks!

Gab

> 
> Thanks !
> Lorenzo

[PATCH 15/20] PCI: designware: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>
---
 drivers/pci/dwc/pcie-designware-host.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 5ba3349..2b789af 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -338,8 +338,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	}
 
 	if (!pci->dbi_base) {
-		pci->dbi_base = devm_ioremap(dev, pp->cfg->start,
-					resource_size(pp->cfg));
+		pci->dbi_base = devm_pci_remap_cfgspace(dev,
+						pp->cfg->start,
+						resource_size(pp->cfg));
 		if (!pci->dbi_base) {
 			dev_err(dev, "error with ioremap\n");
 			ret = -ENOMEM;
@@ -350,8 +351,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	pp->mem_base = pp->mem->start;
 
 	if (!pp->va_cfg0_base) {
-		pp->va_cfg0_base = devm_ioremap(dev, pp->cfg0_base,
-						pp->cfg0_size);
+		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
+					pp->cfg0_base, pp->cfg0_size);
 		if (!pp->va_cfg0_base) {
 			dev_err(dev, "error with ioremap in function\n");
 			ret = -ENOMEM;
@@ -360,7 +361,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	}
 
 	if (!pp->va_cfg1_base) {
-		pp->va_cfg1_base = devm_ioremap(dev, pp->cfg1_base,
+		pp->va_cfg1_base = devm_pci_remap_cfgspace(dev,
+						pp->cfg1_base,
 						pp->cfg1_size);
 		if (!pp->va_cfg1_base) {
 			dev_err(dev, "error with ioremap\n");
-- 
2.10.0

[PATCH 16/20] PCI: armada8k: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/dwc/pcie-armada8k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/pcie-armada8k.c b/drivers/pci/dwc/pcie-armada8k.c
index 66bac6f..31f5cb1 100644
--- a/drivers/pci/dwc/pcie-armada8k.c
+++ b/drivers/pci/dwc/pcie-armada8k.c
@@ -228,7 +228,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
 
 	/* Get the dw-pcie unit configuration/control registers base. */
 	base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
-	pci->dbi_base = devm_ioremap_resource(dev, base);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, base);
 	if (IS_ERR(pci->dbi_base)) {
 		dev_err(dev, "couldn't remap regs base %p\n", base);
 		ret = PTR_ERR(pci->dbi_base);
-- 
2.10.0

[PATCH 17/20] PCI: xgene: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/host/pci-xgene.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 1a61087..de19898 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -248,7 +248,7 @@ static int xgene_pcie_ecam_init(struct pci_config_window *cfg, u32 ipversion)
 		dev_err(dev, "can't get CSR resource\n");
 		return ret;
 	}
-	port->csr_base = devm_ioremap_resource(dev, &csr);
+	port->csr_base = devm_pci_remap_cfg_resource(dev, &csr);
 	if (IS_ERR(port->csr_base))
 		return PTR_ERR(port->csr_base);
 
@@ -359,7 +359,7 @@ static int xgene_pcie_map_reg(struct xgene_pcie_port *port,
 	struct resource *res;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
-	port->csr_base = devm_ioremap_resource(dev, res);
+	port->csr_base = devm_pci_remap_cfg_resource(dev, res);
 	if (IS_ERR(port->csr_base))
 		return PTR_ERR(port->csr_base);
 
-- 
2.10.0

[PATCH 18/20] PCI: tegra: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use correct memory mapping attributes to map config space
regions to enforce configuration space non-posted writes behaviour.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pci/host/pci-tegra.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index ed8a93f..2618f87 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -380,7 +380,7 @@ static struct tegra_pcie_bus *tegra_pcie_bus_alloc(struct tegra_pcie *pcie,
 						   unsigned int busnr)
 {
 	struct device *dev = pcie->dev;
-	pgprot_t prot = pgprot_device(PAGE_KERNEL);
+	pgprot_t prot = pgprot_noncached(PAGE_KERNEL);
 	phys_addr_t cs = pcie->cs->start;
 	struct tegra_pcie_bus *bus;
 	unsigned int i;
@@ -1962,7 +1962,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		rp->pcie = pcie;
 		rp->np = port;
 
-		rp->base = devm_ioremap_resource(dev, &rp->regs);
+		rp->base = devm_pci_remap_cfg_resource(dev, &rp->regs);
 		if (IS_ERR(rp->base))
 			return PTR_ERR(rp->base);
 
-- 
2.10.0

[PATCH 19/20] PCI: layerscape: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mingkai Hu <mingkai.hu@freescale.com>
Cc: Minghuan Lian <minghuan.Lian@freescale.com>
Cc: Roy Zang <tie-fei.zang@freescale.com>
---
 drivers/pci/dwc/pci-layerscape.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/pci-layerscape.c b/drivers/pci/dwc/pci-layerscape.c
index 175c09e..e39beaa 100644
--- a/drivers/pci/dwc/pci-layerscape.c
+++ b/drivers/pci/dwc/pci-layerscape.c
@@ -281,7 +281,7 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 	pci->ops = pcie->drvdata->dw_pcie_ops;
 
 	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
-	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
-- 
2.10.0

[PATCH 20/20] PCI: keystone-dw: update PCI config space remap function

[Lorenzo Pieralisi]

PCI configuration space should be mapped with a memory region type that
generates on the CPU host bus non-posted write transations. Update the
driver to use the devm_pci_remap_cfg* interface to make sure the correct
memory mappings for PCI configuration space are used.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/pci/dwc/pci-keystone-dw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/pci-keystone-dw.c b/drivers/pci/dwc/pci-keystone-dw.c
index 6b396f6..8bc626e 100644
--- a/drivers/pci/dwc/pci-keystone-dw.c
+++ b/drivers/pci/dwc/pci-keystone-dw.c
@@ -543,7 +543,7 @@ int __init ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
 
 	/* Index 0 is the config reg. space address */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pci->dbi_base = devm_ioremap_resource(dev, res);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
-- 
2.10.0

[PATCH 00/20] PCI: fix config and I/O Address space memory mappings

[Arnd Bergmann]

On Mon, Feb 27, 2017 at 4:14 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:

> This patch series[1] addresses both issues in one go:
>
> - It updates the pci_remap_iospace() function to use a page mapping
>   that guarantees non-posted write transactions for I/O space addresses
> - It adds a kernel API to remap PCI config space resources, so that
>   architecture can override it with a mapping implementation that
>   guarantees PCI specifications compliancy wrt non-posted write
>   configuration transactions
> - It updates all PCI host controller implementations (and the generic
>   ECAM layer) to use the newly introduced mapping interface
>
> Tested on Juno ECAM based interface (DT/ACPI).

This looks all good to me, nice work!

    Arnd

[PATCH 00/20] PCI: fix config and I/O Address space memory mappings

[Lorenzo Pieralisi]

On Wed, Mar 01, 2017 at 05:18:27PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 27, 2017 at 4:14 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> 
> > This patch series[1] addresses both issues in one go:
> >
> > - It updates the pci_remap_iospace() function to use a page mapping
> >   that guarantees non-posted write transactions for I/O space addresses
> > - It adds a kernel API to remap PCI config space resources, so that
> >   architecture can override it with a mapping implementation that
> >   guarantees PCI specifications compliancy wrt non-posted write
> >   configuration transactions
> > - It updates all PCI host controller implementations (and the generic
> >   ECAM layer) to use the newly introduced mapping interface
> >
> > Tested on Juno ECAM based interface (DT/ACPI).
> 
> This looks all good to me, nice work!

Thanks a lot Arnd. There is a pending issue to complete the series,
that is related to asm-generic/io.h, which is not included by all
arches, therefore, on top of adding a default inline for
pci_remap_cfg_space() in asm-generic/io.h I will have to patch
all arches that do not include asm-generic/io.h (eg x86) to
make the series completely functional - ie I will have to add a

#define pci_remap_cfgspace ioremap_nocache

in every given asm/io.h that does not include <asm-generic/io.h>

Another option would consist in adding the default inline for
pci_remap_cfg_space() in asm-generic/pci_iomap.h which seems to
be included by all arches.

I think the first option, even if it requires more extensive
patching is more complete but please all let me know how
you want me to fix this niggle, I am not sure I grasp the background
policy behind asm-generic files entirely so I'd better ask.

Thanks !
Lorenzo