ARM926ejs: Add routines to invalidate D-Cache

PATCH New Hong Xu 4 participants 10 messages Aug 8, 2011 – Aug 10, 2011 Arm (32-bit)

Share & tools

Hong Xu

author

2011-08-08 03:20 UTC #0 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch data written by DMA controller from RAM).

Tested on AT91SAM9261EK with Peripheral DMA controller.

Signed-off-by Hong Xu <[email protected]>

Tested-by Elen Song <[email protected]>

CC Albert Aribaud <[email protected]>

CC Aneesh V <[email protected]>

CC Reinhard Meyer <[email protected]>

CC Heiko Schocher <[email protected]>

---
V2:
Per Albert's suggestion, add invalidate_dcache_range

V3:
invalidate_dcache_range emits warning when detecting unaligned buffer

invalidate_dcache_range won't clean any adjacent cache line when detecting unaligned buffer and only round up/down the buffer address

 arch/arm/lib/cache.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

arch/arm/lib/cache.c +54 -0

index 92b61a2..9e6aa47 100644
@@ -53,3 +53,57 @@ void	__flush_dcache_all(void)
 }
 void	flush_dcache_all(void)
 	__attribute__((weak, alias("__flush_dcache_all")));
+
+/*
+ * The buffer range to be invalidated is [start, stop)
+ */
+void __invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+	int cache_line_len;
+	unsigned long mva;
+
+#ifdef CONFIG_ARM926EJS
+#ifdef CONFIG_SYS_CACHELINE_SIZE
+	cache_line_len = CONFIG_SYS_CACHELINE_SIZE;
+#else
+	/*
+	 * ARM926EJ-S Technical Reference Manual, Chap 2.3.1 Table 2-9
+	 * only b'10, aka. 32 bytes cache line len is valid
+	 */
+	cache_line_len = 32;
+#endif
+	mva = start;
+	if ((mva & (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, starting "
+			"address: 0x%08x\n", __FUNCTION__, start);
+		mva &= ~(cache_line_len - 1);
+	}
+	if ((stop & (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, ending "
+			"address: 0x%08x\n", __FUNCTION__, stop);
+		stop = (stop | (cache_line_len - 1)) + 1;
+	}
+
+	while (mva < stop) {
+		asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(mva));
+		mva += cache_line_len;
+	}
+
+	/* Drain the WB */
+	asm("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
+#endif
+
+	return;
+}
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+	__attribute__((weak, alias("__invalidate_dcache_range")));
+
+void __invalidate_dcache_all(void)
+{
+#ifdef CONFIG_ARM926EJS
+	asm("mcr p15, 0, %0, c7, c6, 0" : : "r" (0));
+#endif
+	return;
+}
+void  invalidate_dcache_all(void)
+	__attribute__((weak, alias("__invalidate_dcache_all")));

Signature · 1 line

1.7.6

Albert ARIBAUD

2011-08-08 08:01 UTC #1 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

Hi Hong Xu,

Le 08/08/2011 05:20, Hong Xu a ?crit :

Quoted 22 lines

After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch data written by DMA controller from RAM).
Tested on AT91SAM9261EK with Peripheral DMA controller.
Signed-off-by Hong Xu<[email protected]>
Tested-by Elen Song<[email protected]>
CC Albert Aribaud<[email protected]>
CC Aneesh V<[email protected]>
CC Reinhard Meyer<[email protected]>
CC Heiko Schocher<[email protected]>
---
V2:
Per Albert's suggestion, add invalidate_dcache_range
V3:
invalidate_dcache_range emits warning when detecting unaligned buffer
invalidate_dcache_range won't clean any adjacent cache line when detecting unaligned buffer and only round up/down the buffer address

arch/arm/lib/cache.c void __flush_dcache_all(void) · line 77

Quoted 3 lines

arch/arm/lib/cache.c +3 -0

+	mva = start;
+	if ((mva&  (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, starting "

I'd rather have a message about "cache", not "buffer", e.g.

printf("WARNING: %s - start address %x is not aligned\n"

     __FUNCTION__, start);

arch/arm/lib/cache.c void __flush_dcache_all(void) · line 83

Quoted 5 lines

arch/arm/lib/cache.c +5 -0

+		mva&= ~(cache_line_len - 1);
+	}
+	if ((stop&  (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, ending "
+			"address: 0x%08x\n", __FUNCTION__, stop);

Ditto.

arch/arm/lib/cache.c void __flush_dcache_all(void) · line 90

Quoted 7 lines

arch/arm/lib/cache.c +7 -0

+		stop = (stop | (cache_line_len - 1)) + 1;
+	}
+
+	while (mva<  stop) {
+		asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(mva));
+		mva += cache_line_len;
+	}

Thinking more about the degenerate case -- why not round *up* the start address, and round *down* the stop address, that is, *reduce* the area to the aligned portion rather than *expand* it into the unknown? That would make data in "partially owned" cache lines safe from unwanted invalidation. OTOH, it would not completely invalidate the caller's data, but at least the malfunction would appear in the faulty calling code, not elsewhere.

Opinions?

Amicalement,

Signature · 1 line

Albert.

Hong Xu

author

2011-08-08 08:58 UTC #2 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

Hi Albert,

Quoted 5 lines · Albert ARIBAUD

Hi Hong Xu,
Le 08/08/2011 05:20, Hong Xu a ?crit :
Quoted 2 lines
After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch

[...]

Quoted 10 lines

Quoted 5 lines
unaligned buffer and only round up/down the buffer address
+ mva = start;
+ if ((mva& (cache_line_len - 1)) != 0) {
+ printf("WARNING: %s - unaligned buffer detected, starting "
I'd rather have a message about "cache", not "buffer", e.g.
printf("WARNING: %s - start address %x is not aligned\n" __FUNCTION__, start);

Quoted 7 lines

Quoted 5 lines
+ mva&= ~(cache_line_len - 1);
+ }
+ if ((stop& (cache_line_len - 1)) != 0) {
+ printf("WARNING: %s - unaligned buffer detected, ending "
+ "address: 0x%08x\n", __FUNCTION__, stop);
Ditto.

Quoted 16 lines

Quoted 7 lines
+ stop = (stop | (cache_line_len - 1)) + 1;
+ }
+
+ while (mva< stop) {
+ asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(mva));
+ mva += cache_line_len;
+ }
Thinking more about the degenerate case -- why not round *up* the start address, and round *down* the stop address, that is, *reduce* the area to the aligned portion rather than *expand* it into the unknown? That would make data in "partially owned" cache lines safe from unwanted invalidation. OTOH, it would not completely invalidate the caller's data, but at least the malfunction would appear in the faulty calling code, not elsewhere.

Quoted 1 line

Opinions?

Agree :)

BR,
Eric

Marek Vasut

2011-08-08 17:34 UTC #3 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

In reply to Albert ARIBAUD

Quoted 26 lines · Albert ARIBAUD

Hi Hong Xu,
Le 08/08/2011 05:20, Hong Xu a ?crit :
Quoted 23 lines
After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch data written by DMA controller from RAM).
Tested on AT91SAM9261EK with Peripheral DMA controller.
Signed-off-by Hong Xu<[email protected]>
Tested-by Elen Song<[email protected]>
CC Albert Aribaud<[email protected]>
CC Aneesh V<[email protected]>
CC Reinhard Meyer<[email protected]>
CC Heiko Schocher<[email protected]>
---
V2:
Per Albert's suggestion, add invalidate_dcache_range
V3:
invalidate_dcache_range emits warning when detecting unaligned buffer
invalidate_dcache_range won't clean any adjacent cache line when detecting unaligned buffer and only round up/down the buffer address

Quoted 8 lines

arch/arm/lib/cache.c void __flush_dcache_all(void) · line 77
Quoted 3 lines
arch/arm/lib/cache.c +3 -0
+	mva = start;
+	if ((mva&  (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, starting "
I'd rather have a message about "cache", not "buffer", e.g.
printf("WARNING: %s - start address %x is not aligned\n"
     __FUNCTION__, start);

__func__ is prefered in linux kernel :-)

Quoted 8 lines

arch/arm/lib/cache.c void __flush_dcache_all(void) · line 83
Quoted 6 lines
arch/arm/lib/cache.c +5 -0
+		mva&= ~(cache_line_len - 1);
+	}
+	if ((stop&  (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, ending "
+			"address: 0x%08x\n", __FUNCTION__, stop);
Ditto.

Ditto.

Quoted 16 lines

arch/arm/lib/cache.c void __flush_dcache_all(void) · line 90
Quoted 8 lines
arch/arm/lib/cache.c +7 -0
+		stop = (stop | (cache_line_len - 1)) + 1;
+	}
+
+	while (mva<  stop) {
+		asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(mva));
+		mva += cache_line_len;
+	}
Thinking more about the degenerate case -- why not round *up* the start address, and round *down* the stop address, that is, *reduce* the area to the aligned portion rather than *expand* it into the unknown? That would make data in "partially owned" cache lines safe from unwanted invalidation. OTOH, it would not completely invalidate the caller's data, but at least the malfunction would appear in the faulty calling code, not elsewhere.

That'd introduce even stranger behaviour and it'd be even more sickening to debug

Quoted 4 lines

Opinions?
Amicalement,

Albert ARIBAUD

2011-08-08 17:56 UTC #4 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

Le 08/08/2011 19:34, Marek Vasut a ?crit :

Quoted 10 lines

Quoted 7 lines
Thinking more about the degenerate case -- why not round *up* the start address, and round *down* the stop address, that is, *reduce* the area to the aligned portion rather than *expand* it into the unknown? That would make data in "partially owned" cache lines safe from unwanted invalidation. OTOH, it would not completely invalidate the caller's data, but at least the malfunction would appear in the faulty calling code, not elsewhere.
That'd introduce even stranger behaviour and it'd be even more sickening to debug

I tend to agree neither on the "stranger" part, nor on the "sickening". Can you develop your viewpoint? For your reference, here is mine:

As for "stranger": for me, an unexpected value (either a modification or an absence thereof) in a variable that no code is using right now, that is stranger than an unexpected value in a DMA buffer we just happen to have recently invalidated.

As for "sickening": for me, when a bug occurs, one of the requisites is to look up the console output, where a message about an unaligned cache invalidate address will stand out and quite quickly point at the root cause for that weird DMA buffer content problem -- OTOH, with "expanded" invalidates, the DMA buffer is OK so one might disregard the warning and not link it far later with that weird problem where an obscure global variable couldn't keep its value.

I do understand though that I dismissed flush-in-invalidate with an argument of "does what it says on the tin", and that one could say my proposal makes the function do less of what it says on the tin; my point was about the nature of the action, not its extent.

IOW, the sticker on the tin also says "use only with cacheline-aligned start and stop", so fair's fair: if you pass unaligned ranges, don't expect the invalidate to extend beyond the aligned part.

Amicalement,

Signature · 1 line

Albert.

Hong Xu

author

2011-08-09 01:57 UTC #5 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

In reply to Marek Vasut

Hi Marek Vasut,

Quoted 33 lines · Marek Vasut

Quoted 32 lines · Albert ARIBAUD
Hi Hong Xu,
Le 08/08/2011 05:20, Hong Xu a ?crit :
Quoted 23 lines
After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch data written by DMA controller from RAM).
Tested on AT91SAM9261EK with Peripheral DMA controller.
Signed-off-by Hong Xu<[email protected]>
Tested-by Elen Song<[email protected]>
CC Albert Aribaud<[email protected]>
CC Aneesh V<[email protected]>
CC Reinhard Meyer<[email protected]>
CC Heiko Schocher<[email protected]>
---
V2:
Per Albert's suggestion, add invalidate_dcache_range
V3:
invalidate_dcache_range emits warning when detecting unaligned buffer
invalidate_dcache_range won't clean any adjacent cache line when detecting unaligned buffer and only round up/down the buffer address
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 77
Quoted 3 lines
arch/arm/lib/cache.c +3 -0
+	mva = start;
+	if ((mva&   (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, starting "
I'd rather have a message about "cache", not "buffer", e.g.

Quoted 4 lines

Quoted 2 lines
printf("WARNING: %s - start address %x is not aligned\n"
__FUNCTION__, start);
__func__ is prefered in linux kernel :-)

__func__ is C99 standard. __FUNCTION__ appears more in U-Boot. ;-) GCC manual says some older GCC only recognize __FUNCTION__ . If we rely on GCC, it looks __FUNCTION__ will reduce troubles.

BR,
Eric

Quoted 11 lines

Quoted 8 lines
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 83
Quoted 6 lines
arch/arm/lib/cache.c +5 -0
+		mva&= ~(cache_line_len - 1);
+	}
+	if ((stop&   (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, ending "
+			"address: 0x%08x\n", __FUNCTION__, stop);
Ditto.
Ditto.

[...]

Marek Vasut

2011-08-09 19:55 UTC #6 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

Quoted 38 lines · Hong Xu

Hi Marek Vasut,
Quoted 35 lines · Marek Vasut
Quoted 34 lines · Albert ARIBAUD
Hi Hong Xu,
Le 08/08/2011 05:20, Hong Xu a ?crit :
Quoted 25 lines
After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch data written by DMA controller from RAM).
Tested on AT91SAM9261EK with Peripheral DMA controller.
Signed-off-by Hong Xu<[email protected]>
Tested-by Elen Song<[email protected]>
CC Albert Aribaud<[email protected]>
CC Aneesh V<[email protected]>
CC Reinhard Meyer<[email protected]>
CC Heiko Schocher<[email protected]>
---
V2:
Per Albert's suggestion, add invalidate_dcache_range
V3:
invalidate_dcache_range emits warning when detecting unaligned buffer
invalidate_dcache_range won't clean any adjacent cache line when detecting unaligned buffer and only round up/down the buffer address
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 77
Quoted 3 lines
arch/arm/lib/cache.c +3 -0
+	mva = start;
+	if ((mva&   (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, starting "
I'd rather have a message about "cache", not "buffer", e.g.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
printf("WARNING: %s - start address %x is not aligned\n"
__FUNCTION__, start);
__func__ is prefered in linux kernel :-)
__func__ is C99 standard. __FUNCTION__ appears more in U-Boot. ;-)

This doesn't mean it's correct ;-) "majority proof" isn't a proof really.

Quoted 2 lines

GCC manual says some older GCC only recognize __FUNCTION__ . If we rely on GCC, it looks __FUNCTION__ will reduce troubles.

Do we support such ancient versions of GCC anyway ? Just to be clear, I'm fine with either way, just my 2.7183 cents ;-)

Quoted 15 lines

BR,
Eric
Quoted 9 lines
Quoted 7 lines
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 83
Quoted 5 lines
arch/arm/lib/cache.c +5 -0
+		mva&= ~(cache_line_len - 1);
+	}
+	if ((stop&   (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, ending "
+			"address: 0x%08x\n", __FUNCTION__, stop);
Ditto.
Ditto.
[...]

Hong Xu

author

2011-08-10 01:45 UTC #7 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

Hi Marek Vasut,

Quoted 4 lines · Marek Vasut

Quoted 3 lines · Hong Xu
Hi Marek Vasut,
On 08/09/2011 01:34 AM, Marek Vasut wrote:

[...]

Quoted 8 lines

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
printf("WARNING: %s - start address %x is not aligned\n"
__FUNCTION__, start);
__func__ is prefered in linux kernel :-)
__func__ is C99 standard. __FUNCTION__ appears more in U-Boot. ;-)

Quoted 7 lines

This doesn't mean it's correct ;-) "majority proof" isn't a proof really.
Quoted 2 lines
GCC manual says some older GCC only recognize __FUNCTION__ . If we rely on GCC, it looks __FUNCTION__ will reduce troubles.
Do we support such ancient versions of GCC anyway ? Just to be clear, I'm fine with either way, just my 2.7183 cents ;-)

Agree. Just after last reply, I reconsidered the situation in the tearoom. __func__ looks better. ;-) I'll resend the patch soon, thanks.

BR,
Eric

Marek Vasut

2011-08-10 02:46 UTC #8 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

Quoted 19 lines · Hong Xu

Hi Marek Vasut,
Quoted 4 lines · Marek Vasut
Quoted 3 lines · Hong Xu
Hi Marek Vasut,
On 08/09/2011 01:34 AM, Marek Vasut wrote:
[...]
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
printf("WARNING: %s - start address %x is not aligned\n"
__FUNCTION__, start);
__func__ is prefered in linux kernel :-)
__func__ is C99 standard. __FUNCTION__ appears more in U-Boot. ;-)
This doesn't mean it's correct ;-) "majority proof" isn't a proof really.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
GCC manual says some older GCC only recognize __FUNCTION__ . If we rely on GCC, it looks __FUNCTION__ will reduce troubles.
Do we support such ancient versions of GCC anyway ? Just to be clear, I'm fine with either way, just my 2.7183 cents ;-)
Agree. Just after last reply, I reconsidered the situation in the tearoom. __func__ looks better. ;-) I'll resend the patch soon, thanks.

Thanks, I'm very much looking forward to this patch in mainline.

Quoted 3 lines

BR,
Eric

Aneesh V

2011-08-09 11:05 UTC #9 lore raw

PATCH 1/1 ARM926ejs: Add routines to invalidate D-Cache

In reply to Marek Vasut

Hi Marek Vasut,

Quoted 65 lines · Marek Vasut

Quoted 26 lines · Albert ARIBAUD
Hi Hong Xu,
Le 08/08/2011 05:20, Hong Xu a ?crit :
Quoted 23 lines
After DMA operation, we need to maintain D-Cache coherency. So that the DCache must be invalidated (hence CPU will fetch data written by DMA controller from RAM).
Tested on AT91SAM9261EK with Peripheral DMA controller.
Signed-off-by Hong Xu<[email protected]>
Tested-by Elen Song<[email protected]>
CC Albert Aribaud<[email protected]>
CC Aneesh V<[email protected]>
CC Reinhard Meyer<[email protected]>
CC Heiko Schocher<[email protected]>
---
V2:
Per Albert's suggestion, add invalidate_dcache_range
V3:
invalidate_dcache_range emits warning when detecting unaligned buffer
invalidate_dcache_range won't clean any adjacent cache line when detecting unaligned buffer and only round up/down the buffer address
Quoted 8 lines
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 77
Quoted 3 lines
arch/arm/lib/cache.c +3 -0
+	mva = start;
+	if ((mva&   (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, starting "
I'd rather have a message about "cache", not "buffer", e.g.
printf("WARNING: %s - start address %x is not aligned\n"
__FUNCTION__, start);
__func__ is prefered in linux kernel :-)
Quoted 8 lines
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 83
Quoted 6 lines
arch/arm/lib/cache.c +5 -0
+		mva&= ~(cache_line_len - 1);
+	}
+	if ((stop&   (cache_line_len - 1)) != 0) {
+		printf("WARNING: %s - unaligned buffer detected, ending "
+			"address: 0x%08x\n", __FUNCTION__, stop);
Ditto.
Ditto.
Quoted 16 lines
arch/arm/lib/cache.c void __flush_dcache_all(void) · line 90
Quoted 8 lines
arch/arm/lib/cache.c +7 -0
+		stop = (stop | (cache_line_len - 1)) + 1;
+	}
+
+	while (mva<   stop) {
+		asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(mva));
+		mva += cache_line_len;
+	}
Thinking more about the degenerate case -- why not round *up* the start address, and round *down* the stop address, that is, *reduce* the area to the aligned portion rather than *expand* it into the unknown? That would make data in "partially owned" cache lines safe from unwanted invalidation. OTOH, it would not completely invalidate the caller's data, but at least the malfunction would appear in the faulty calling code, not elsewhere.

Quoted 2 lines

That'd introduce even stranger behaviour and it'd be even more sickening to debug

I think the warning messages printed here will greatly help in debugging such issues.

best regards,
Aneesh