| Message ID | 20170227151436.18698-6-lorenzo.pieralisi@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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
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.
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 /*
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(+)