ARM: implement workaround for Cortex-A9/PL310/PCIe deadlock

Russell, Will, Catalin,

This patch series adresses a problem that affects the newer Marvell Armada 375 and 38x SOCs, based on Cortex-A9+PL310, combined with the Marvell PCIe hardware unit. When the hardware I/O coherency is enabled, the combination of Cortex-A9/PL310/Marvell PCIe hardware unit will quickly cause a deadlock when the PCIe bus is stressed.

The workaround for this problem has been suggested by ARM, and consists in two things:

(1) Map the PCIe regions as strongly-ordered

(2) Disable the outer cache sync of the PL310 when hardware I/O coherency is used, since it is unneeded and causes the deadlock.

The following four patches address the problem in the following way:

* PATCH 1/4 adds a small API in arch/arm/mm/ioremap.c to allow sub-architectures to override the memory type used for PCI I/O mappings.

* PATCH 2/4 extends the l2x0 cache driver with a new property "arm,io-coherent", valid for the PL310, which makes the driver disable the outer cache sync operation. This patch should be routed through Russell's tree.

* PATCH 3/4 actually implements the Armada 375/38x workaround, by using MT_UNCACHED for PCI memory mappings, and adding the "arm,io-coherent" property to the cache controller Device Tree node when appropriate (i.e, when hardware I/O coherency is enabled). This patch has no build dependency on the two previous patches. It has already been merged by the mvebu maintainers.

* PATCH 4/4 uses the API introduced in PATCH 1/4 to map PCI I/O regions as strongly ordered. It should also go through the mvebu maintainers tree, but was kept separate from PATCH 3/4, since 1/ PATCH 3 has already been merged and 2/ PATCH 4 has a build dependency on PATCH 1, which may cause a delay in merging of PATCH 4.

Changes since v4:

- Re-introduce the patch to allow sub-architectures to override the memory type used for PCI I/O mappings, since switching to strongly-ordered for all platforms does not seem to be well accepted/understood at this point.

- Remove the of_device_is_compatible() check for the PL310, when testing for 'arm,io-coherent'. Suggested by Rob Herring. However, the code tetsing 'arm,io-coherent' cannot be moved into pl310_of_setup(), because this function is called *before* the 'outer_cache' structure is initialized.

- Add a separate patch to use the pci_ioremap_set_mem_type() API in mach-mvebu/coherency.c.

Changes since v3:

- Withdrawn all Acked-by tags since the changes compared to v3 are quite significant.

- Instead of introducing a small mechanism to allow each sub-architecture to override the memory type used for PCI I/O mappings, simply make all of them mapped MT_UNCACHED instead of MT_DEVICE, as suggested by Arnd Bergmann. This also has the nice consequence that there is no longer a build dependency between PATCH 3/3 and PATCH 1/3. Suggested by Arnd Bergmann.

- Change the name of the new property of the PL310 DT binding from the too generic 'dma-coherent' to 'arm,io-coherent'. Suggested by Rob Herring.

- Instead of adding a complete set of L2 cache operations in cache-l2x0.c, simply nullify the outer_cache.sync operation when 'arm,io-coherent' is specified. Suggested by Rob Herring.

- Move the Armada 375/38x specific code from mach-mvebu/board-v7.c to mach-mvebu/coherency.c, which makes more sense. Suggested by Arnd Bergmann.

Changes since v2:

- Added Acked-by from Catalin on "ARM: mm: allow sub-architectures to override PCI I/O memory type".

- Dropped the patch fixing the of_update_property() function, since we're no longer using it.

- Instead of using a different compatible string to identify PL310 used in an I/O coherent configuration, use a separate boolean property. Suggested by Catalin.

- Rework the mach-mvebu/coherency.c to add the boolean property "dma-coherent" when needed instead of updating the compatible string of the cache controller.

Changes since v1:

- Instead of introducing separate l2x0 initialization functions, rely on a separate compatible string to identify whether we're coherent or not. The compatible string *has* to be modified at runtime, because Armada 375 and 38x are only I/O coherent when in SMP mode. In non-SMP mode, they are not I/O coherent, so we cannot change the DT to 'arm,pl310-coherent-cache'.

- Addition of the drivers/of fix to be able to use of_update_property() early and fix up the PL310 compatible string, as explained in the previous item.

Thanks!

Thomas

Thomas Petazzoni (4):
ARM: mm: allow sub-architectures to override PCI I/O memory type
ARM: mm: add support for HW coherent systems in PL310
ARM: mvebu: implement L2/PCIe deadlock workaround
ARM: mvebu: use pci_ioremap_set_mem_type() to map PCI I/O as strongly
ordered

 Documentation/devicetree/bindings/arm/l2cc.txt |  3 ++
 arch/arm/include/asm/io.h                      |  6 ++++
 arch/arm/mach-mvebu/coherency.c                | 40 ++++++++++++++++++++++++++
 arch/arm/mm/cache-l2x0.c                       | 13 +++++++++
 arch/arm/mm/ioremap.c                          |  9 +++++-
 5 files changed, 70 insertions(+), 1 deletion(-)

1.9.3

Due to a design incompatibility between the PCIe Marvell controller and the Cortex-A9, stressing PCIe devices with a lot of traffic quickly causes a deadlock.

One part of the workaround for this is to have all PCIe regions mapped as strongly-ordered (MT_UNCACHED) instead of the default MT_DEVICE. While the arch_ioremap_caller() mechanism allows sub-architecture code to override ioremap(), used to map PCIe memory regions, there isn't such a mechanism to override the behavior of pci_ioremap_io().

This commit adds the arch_pci_ioremap_mem_type variable, initialized to MT_DEVICE by default, and that sub-architecture code can override. We have chosen to expose a single variable rather than offering the possibility of overriding the entire pci_ioremap_io(), because implementing pci_ioremap_io() requires calling functions (get_mem_type()) that are private to the arch/arm/mm/ code.

---
 arch/arm/include/asm/io.h | 6 ++++++
 arch/arm/mm/ioremap.c     | 9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

index 8aa4cca..3d23418 100644
@@ -179,6 +179,12 @@ static inline void __iomem *__typesafe_io(unsigned long addr)
 /* PCI fixed i/o mapping */
 #define PCI_IO_VIRT_BASE	0xfee00000
 
+#if defined(CONFIG_PCI)
+void pci_ioremap_set_mem_type(int mem_type);
+#else
+static inline void pci_ioremap_set_mem_type(int mem_type) {}
+#endif
+
 extern int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr);
 
 /*

index f9c32ba..d1e5ad7 100644
@@ -438,6 +438,13 @@ void __arm_iounmap(volatile void __iomem *io_addr)
 EXPORT_SYMBOL(__arm_iounmap);
 
 #ifdef CONFIG_PCI
+static int pci_ioremap_mem_type = MT_DEVICE;
+
+void pci_ioremap_set_mem_type(int mem_type)
+{
+	pci_ioremap_mem_type = mem_type;
+}
+
 int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 {
 	BUG_ON(offset + SZ_64K > IO_SPACE_LIMIT);

@@ -445,7 +452,7 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 	return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
 				  PCI_IO_VIRT_BASE + offset + SZ_64K,
 				  phys_addr,
-				  __pgprot(get_mem_type(MT_DEVICE)->prot_pte));
+				  __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
 #endif

1.9.3

When a PL310 cache is used on a system that provides hardware coherency, the outer cache sync operation is useless, and can be skipped. Moreover, on some systems, it is harmful as it causes deadlocks between the Marvell coherency mechanism, the Marvell PCIe controller and the Cortex-A9.

To avoid this, this commit introduces a new Device Tree property 'arm,io-coherent' for the L2 cache controller node, valid only for the PL310 cache. It identifies the usage of the PL310 cache in an I/O coherent configuration. Internally, it makes the driver disable the outer cache sync operation.

Note that technically speaking, a fully coherent system wouldn't require any of the other .outer_cache operations. However, in practice, when booting secondary CPUs, these are not yet coherent, and therefore a set of cache maintenance operations are necessary at this point. This explains why we keep the other .outer_cache operations and only ->sync is disabled.

While in theory any write to a PL310 register could cause the deadlock, in practice, disabling ->sync is sufficient to workaround the deadlock, since the other cache maintenance operations are only used in very specific situations.

---
 Documentation/devicetree/bindings/arm/l2cc.txt |  3 +++
 arch/arm/mm/cache-l2x0.c                       | 13 +++++++++++++
 2 files changed, 16 insertions(+)

index b513cb8..af527ee 100644
@@ -40,6 +40,9 @@ Optional properties:
 - arm,filter-ranges : <start length> Starting address and length of window to
   filter. Addresses in the filter window are directed to the M1 port. Other
   addresses will go to the M0 port.
+- arm,io-coherent : indicates that the system is operating in an hardware
+  I/O coherent mode. Valid only when the arm,pl310-cache compatible
+  string is used.
 - interrupts : 1 combined interrupt.
 - cache-id-part: cache id part number to be used if it is not present
   on hardware

index 7abde2ce..30f4476 100644
@@ -1005,6 +1005,19 @@ int __init l2x0_of_init(u32 aux_val, u32 aux_mask)
 
 	of_init = true;
 	memcpy(&outer_cache, &data->outer_cache, sizeof(outer_cache));
+
+	/*
+	 * outer sync operations are not needed when the system is I/O
+	 * coherent, and potentially harmful in certain situations
+	 * (PCIe/PL310 deadlock on Armada 375/38x due to hardware I/O
+	 * coherency). The other operations are kept because they are
+	 * infrequent (therefore do not cause the deadlock) and needed
+	 * for secondary CPU boot and other power management
+	 * activities.
+	 */
+	if (of_property_read_bool(np, "arm,io-coherent"))
+		outer_cache.sync = NULL;
+
 	l2x0_init(l2x0_base, aux_val, aux_mask);
 
 	return 0;

1.9.3

The Marvell Armada 375 and Armada 38x SOCs, which use the Cortex-A9 CPU core, the PL310 cache and the Marvell PCIe hardware block are affected a L2/PCIe deadlock caused by a system erratum when hardware I/O coherency is used.

This deadlock can be avoided by mapping the PCIe memory areas as strongly-ordered (note: MT_UNCACHED is strongly-ordered), and by removing the outer cache sync done in software. This is implemented in this patch by:

* Registering a custom arch_ioremap_caller function that allows to make sure PCI memory regions are mapped MT_UNCACHED.

* Adding at runtime the 'arm,io-coherent' property to the PL310 cache controller. This cannot be done permanently in the DT, because the hardware I/O coherency can only be enabled when CONFIG_SMP is enabled, in the current kernel situation.

---
 arch/arm/mach-mvebu/coherency.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

index d5a975b..f6be9c6 100644
@@ -31,6 +31,7 @@
 #include <linux/clk.h>
 #include <asm/smp_plat.h>
 #include <asm/cacheflush.h>
+#include <asm/mach/map.h>
 #include "armada-370-xp.h"
 #include "coherency.h"
 #include "mvebu-soc-id.h"

@@ -308,9 +309,47 @@ static void __init armada_370_coherency_init(struct device_node *np)
 	set_cpu_coherent();
 }
 
+/*
+ * This ioremap hook is used on Armada 375/38x to ensure that PCIe
+ * memory areas are mapped as MT_UNCACHED instead of MT_DEVICE. This
+ * is needed as a workaround for a deadlock issue between the PCIe
+ * interface and the cache controller.
+ */
+static void __iomem *
+armada_pcie_wa_ioremap_caller(phys_addr_t phys_addr, size_t size,
+			      unsigned int mtype, void *caller)
+{
+	struct resource pcie_mem;
+
+	mvebu_mbus_get_pcie_mem_aperture(&pcie_mem);
+
+	if (pcie_mem.start <= phys_addr && (phys_addr + size) <= pcie_mem.end)
+		mtype = MT_UNCACHED;
+
+	return __arm_ioremap_caller(phys_addr, size, mtype, caller);
+}
+
 static void __init armada_375_380_coherency_init(struct device_node *np)
 {
+	struct device_node *cache_dn;
+
 	coherency_cpu_base = of_iomap(np, 0);
+	arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
+
+	/*
+	 * Add the PL310 property "arm,io-coherent". This makes sure the
+	 * outer sync operation is not used, which allows to
+	 * workaround the system erratum that causes deadlocks when
+	 * doing PCIe in an SMP situation on Armada 375 and Armada
+	 * 38x.
+	 */
+	for_each_compatible_node(cache_dn, NULL, "arm,pl310-cache") {
+		struct property *p;
+
+		p = kzalloc(sizeof(*p), GFP_KERNEL);
+		p->name = kstrdup("arm,io-coherent", GFP_KERNEL);
+		of_add_property(cache_dn, p);
+	}
 }
 
 static int coherency_type(void)

1.9.3

Part of the workaround for the PCIe/SMP/PL310 deadlock on Armada 375/38x is to map PCI mappings strongly ordered. Mapping PCI memory regions as strongly ordered was already done thanks to the arch_ioremap_caller mechanism. This patch does the same for the PCI I/O regions by using the new pci_ioremap_set_mem_type() function.

---
This patch is kept separate from the rest of the mach-mvebu/coherency.c code for the workaround, as this patch has a build dependency on the new API. Since the new API patch will go through Russell's tree, and this patch through the mvebu tree, there might be some merging issue, or even the need to delay the merging of this patch.

---
 arch/arm/mach-mvebu/coherency.c | 1 +
 1 file changed, 1 insertion(+)

index f6be9c6..0215614 100644
@@ -335,6 +335,7 @@ static void __init armada_375_380_coherency_init(struct device_node *np)
 
 	coherency_cpu_base = of_iomap(np, 0);
 	arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
+	pci_ioremap_set_mem_type(MT_UNCACHED);
 
 	/*
 	 * Add the PL310 property "arm,io-coherent". This makes sure the

1.9.3

Dear Catalin Marinas,

I justed tested in !CONFIG_SMP on Armada 38x, and writing to a PCI I/O mapping works in both the MT_UNCACHED and MT_DEVICE cases.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com