ARM: l2c: fix support for HW coherent systems in PL310 cache

PATCH New Gregory CLEMENT 2 participants 4 messages Aug 11, 2015 – Aug 12, 2015

Subsystems (2)

Stable branch Arm (32-bit)

lore

Share & tools

lore

Gregory CLEMENT

author

2015-08-11 17:05 UTC #0 lore raw

PATCH 1/1 ARM: l2c: fix support for HW coherent systems in PL310 cache

From: Nadav Haklai <[email protected]>

When a PL310 cache is used in a system that provides hardware coherency, the entire outer cache operations are 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.

This commit extends a previous commit:
98ea2dba65932ffc456b6d7b11b8a0624e2f7b95 which added the io-coherent support for the PL310 cache by also disabling the outer cache flush range operation.

In the current kernel implementation, the outer cache flush range operation is triggered by the dma_alloc function.
This operation can be take place during runtime and in some circumstances may lead to the PCIe/PL310 deadlock on Armada 375/38x SoCs.

[gregory.clement at free-electrons.com]: fix title s/mm/l2c/, add the
stable flag

Signed-off-by Nadav Haklai <[email protected]>

Reviewed-by Ofer Heifetz <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>

Fixes 98ea2dba6593 ("ARM: 8076/1: mm: add support for HW coherent

systems in PL310 cache")
Cc: <[email protected]>

---
 arch/arm/mm/cache-l2x0.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

arch/arm/mm/cache-l2x0.c +3 -4

index 71b3d3309024..5c3148675597 100644
@@ -1266,9 +1266,9 @@ static const struct l2c_init_data of_l2c310_data __initconst = {
 };
 
 /*
- * This is a variant of the of_l2c310_data with .sync set to
- * NULL. Outer sync operations are not needed when the system is I/O
- * coherent, and potentially harmful in certain situations (PCIe/PL310
+ * This is a variant of the of_l2c310_data with .sync and .flush_range set to
+ * NULL. Outer sync and flush range 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 in practice) and needed for secondary CPU

@@ -1287,7 +1287,6 @@ static const struct l2c_init_data of_l2c310_coherent_data __initconst = {
 	.outer_cache = {
 		.inv_range   = l2c210_inv_range,
 		.clean_range = l2c210_clean_range,
-		.flush_range = l2c210_flush_range,
 		.flush_all   = l2c210_flush_all,
 		.disable     = l2c310_disable,
 		.resume      = l2c310_resume,

Signature · 1 line

2.1.0

Catalin Marinas

2015-08-11 17:26 UTC #1 lore raw

PATCH 1/1 ARM: l2c: fix support for HW coherent systems in PL310 cache

Quoted 13 lines · Gregory CLEMENT

From: Nadav Haklai <[email protected]>
When a PL310 cache is used in a system that provides hardware coherency, the entire outer cache operations are 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.
This commit extends a previous commit:
98ea2dba65932ffc456b6d7b11b8a0624e2f7b95 which added the io-coherent support for the PL310 cache by also disabling the outer cache flush range operation.

Quoted 5 lines

In the current kernel implementation, the outer cache flush range operation is triggered by the dma_alloc function.
This operation can be take place during runtime and in some circumstances may lead to the PCIe/PL310 deadlock on Armada 375/38x SoCs.

While this may work around the issue for your specific SoC, I think a better fix is in DMA alloc code to avoid flushing caches for coherent devices. This would be the __dma_clear_buffer() implementation which isn't aware of whether the device is coherent or not.

Signature · 1 line

Catalin

Gregory CLEMENT

author

2015-08-12 08:14 UTC #2 lore raw

PATCH 1/1 ARM: l2c: fix support for HW coherent systems in PL310 cache

Hi Catalin,

Quoted 20 lines · Catalin Marinas

Quoted 13 lines · Gregory CLEMENT
From: Nadav Haklai <[email protected]>
When a PL310 cache is used in a system that provides hardware coherency, the entire outer cache operations are 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.
This commit extends a previous commit:
98ea2dba65932ffc456b6d7b11b8a0624e2f7b95 which added the io-coherent support for the PL310 cache by also disabling the outer cache flush range operation.
Quoted 5 lines
In the current kernel implementation, the outer cache flush range operation is triggered by the dma_alloc function.
This operation can be take place during runtime and in some circumstances may lead to the PCIe/PL310 deadlock on Armada 375/38x SoCs.

Quoted 4 lines

While this may work around the issue for your specific SoC, I think a better fix is in DMA alloc code to avoid flushing caches for coherent devices. This would be the __dma_clear_buffer() implementation which isn't aware of whether the device is coherent or not.

Indeed, the other use of the outer cache flush range is done pretty early in the boot and should not be a problem.

What do you think of the following patch?

I am not totally happy with it because I added a variable to a struct dedicated for the outer cache functions. But using a function at the l2cc driver level would be too much. At the opposite we could directly set a boolean from the l2cc driver. From my point of view it would violate the layer but I might be wrong.

Thanks,

Gregory

 arch/arm/include/asm/outercache.h | 9 +++++++++
 arch/arm/mm/cache-l2x0.c          | 1 +
 arch/arm/mm/dma-mapping.c         | 6 ++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

arch/arm/include/asm/outercache.h +9 -0

index 563b92fc2f41..7f7bbfcf1d32 100644
@@ -35,6 +35,7 @@ struct outer_cache_fns {
 	void (*sync)(void);
 #endif
 	void (*resume)(void);
+	bool is_coherent;
 
 	/* This is an ARM L2C thing */
 	void (*write_sec)(unsigned long, unsigned);

@@ -56,6 +57,14 @@ static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
 }
 
 /**
+ * outer_is_coherent - tell if the outer cache is io coherent
+ */
+static inline bool outer_is_coherent(void)
+{
+	return outer_cache.is_coherent;
+}
+
+/**
  * outer_clean_range - clean dirty outer cache lines
  * @start: starting physical address, inclusive
  * @end: end physical address, exclusive

arch/arm/mm/cache-l2x0.c +1 -0

index 71b3d3309024..0071e276adc0 100644
@@ -1291,6 +1291,7 @@ static const struct l2c_init_data of_l2c310_coherent_data __initconst = {
 		.flush_all   = l2c210_flush_all,
 		.disable     = l2c310_disable,
 		.resume      = l2c310_resume,
+		.is_coherent = true,
 	},
 };

arch/arm/mm/dma-mapping.c +4 -2

index 1ced8a0f7a52..4d87df2c16f9 100644
@@ -241,12 +241,14 @@ static void __dma_clear_buffer(struct page *page, size_t size)
 			page++;
 			size -= PAGE_SIZE;
 		}
-		outer_flush_range(base, end);
+		if (!outer_is_coherent())
+			outer_flush_range(base, end);
 	} else {
 		void *ptr = page_address(page);
 		memset(ptr, 0, size);
 		dmac_flush_range(ptr, ptr + size);
-		outer_flush_range(__pa(ptr), __pa(ptr) + size);
+		if (!outer_is_coherent())
+			outer_flush_range(__pa(ptr), __pa(ptr) + size);
 	}
 }

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Catalin Marinas

2015-08-12 16:57 UTC #3 lore raw

PATCH 1/1 ARM: l2c: fix support for HW coherent systems in PL310 cache

Quoted 30 lines · Gregory CLEMENT

Quoted 20 lines · Catalin Marinas
Quoted 13 lines · Gregory CLEMENT
From: Nadav Haklai <[email protected]>
When a PL310 cache is used in a system that provides hardware coherency, the entire outer cache operations are 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.
This commit extends a previous commit:
98ea2dba65932ffc456b6d7b11b8a0624e2f7b95 which added the io-coherent support for the PL310 cache by also disabling the outer cache flush range operation.
Quoted 5 lines
In the current kernel implementation, the outer cache flush range operation is triggered by the dma_alloc function.
This operation can be take place during runtime and in some circumstances may lead to the PCIe/PL310 deadlock on Armada 375/38x SoCs.
Quoted 4 lines
While this may work around the issue for your specific SoC, I think a better fix is in DMA alloc code to avoid flushing caches for coherent devices. This would be the __dma_clear_buffer() implementation which isn't aware of whether the device is coherent or not.
Indeed, the other use of the outer cache flush range is done pretty early in the boot and should not be a problem.
What do you think of the following patch?

[...]

Quoted 37 lines

 arch/arm/include/asm/outercache.h | 9 +++++++++
 arch/arm/mm/cache-l2x0.c          | 1 +
 arch/arm/mm/dma-mapping.c         | 6 ++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

arch/arm/include/asm/outercache.h +9 -0

index 563b92fc2f41..7f7bbfcf1d32 100644
@@ -35,6 +35,7 @@ struct outer_cache_fns {
 	void (*sync)(void);
 #endif
 	void (*resume)(void);
+	bool is_coherent;
 
 	/* This is an ARM L2C thing */
 	void (*write_sec)(unsigned long, unsigned);

@@ -56,6 +57,14 @@ static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
 }
 
 /**
+ * outer_is_coherent - tell if the outer cache is io coherent
+ */
+static inline bool outer_is_coherent(void)
+{
+	return outer_cache.is_coherent;
+}
+
+/**
  * outer_clean_range - clean dirty outer cache lines
  * @start: starting physical address, inclusive
  * @end: end physical address, exclusive

arch/arm/mm/cache-l2x0.c +0 -0

index 71b3d3309024..0071e276adc0 100644

arch/arm/mm/cache-l2x0.c static const struct l2c_init_data of_l2c310_coherent_data __initconst = { · line 1296

Quoted 6 lines

arch/arm/mm/cache-l2x0.c +1 -0

@@ -1291,6 +1291,7 @@ static const struct l2c_init_data of_l2c310_coherent_data __initconst = {
 		.flush_all   = l2c210_flush_all,
 		.disable     = l2c310_disable,
 		.resume      = l2c310_resume,
+		.is_coherent = true,
 	},
 };

I don't really think we need these. If you need to check a device, we already have is_device_dma_coherent(). Just pass bool argument to __dma_clear_buffer() and fix the calling places. The __dma_alloc() function also takes a "bool coherent" but in this case __alloc_simple_buffer() is only ever used on coherent devices, so you may not even need to check is_device_dma_coherent() (however, there are some patches to add CMA support for coherent devices, I don't know whether they are queued yet).

Quoted 10 lines

arch/arm/mm/dma-mapping.c +1 -1

index 1ced8a0f7a52..4d87df2c16f9 100644
@@ -241,12 +241,14 @@ static void __dma_clear_buffer(struct page *page, size_t size)
 			page++;
 			size -= PAGE_SIZE;
 		}
-		outer_flush_range(base, end);
+		if (!outer_is_coherent())

arch/arm/mm/dma-mapping.c static void __dma_clear_buffer(struct page *page, size_t size) · line 253

Quoted 10 lines

arch/arm/mm/dma-mapping.c +3 -1

+			outer_flush_range(base, end);
 	} else {
 		void *ptr = page_address(page);
 		memset(ptr, 0, size);
 		dmac_flush_range(ptr, ptr + size);
-		outer_flush_range(__pa(ptr), __pa(ptr) + size);
+		if (!outer_is_coherent())
+			outer_flush_range(__pa(ptr), __pa(ptr) + size);
 	}
 }

This would make sense if we have a device which is outer coherent but inner non-coherent, though I doubt this is the case you are trying to fix.

Signature · 1 line

Catalin