nios2: convert cache flush to use dm cpu data

Convert cache flush to use dm cpu data. The cacheflush.c of Linux nios2 arch is copied to arch/nios2/lib/cache.c to replace the cache.S. The cache related functions in cpu.c is moved to cache.c. Both flush_dcache() and flush_icache() are replaced and removed. The flush_dcache_all() now flush icache too, which is

the same as what is done in Linux nios2 arch.

Signed-off-by Thomas Chou <[email protected]>

---
 arch/nios2/cpu/cpu.c           | 15 --------
 arch/nios2/include/asm/cache.h | 13 ++-----
 arch/nios2/lib/bootm.c         |  6 +--
 arch/nios2/lib/cache.S         | 68 ----------------------------------
 arch/nios2/lib/cache.c         | 84
++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 88
insertions(+), 98 deletions(-)
 delete mode 100644 arch/nios2/lib/cache.S
 create mode 100644 arch/nios2/lib/cache.c

arch/nios2/cpu/cpu.c +0 -15

index 5403c0d..8607c95 100644
@@ -29,21 +29,6 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char
 * const argv[]) return 0;
 }
 
-int dcache_status(void)
-{
-	return 1;
-}
-
-void dcache_enable(void)
-{
-	flush_dcache(CONFIG_SYS_DCACHE_SIZE, CONFIG_SYS_DCACHELINE_SIZE);
-}
-
-void dcache_disable(void)
-{
-	flush_dcache(CONFIG_SYS_DCACHE_SIZE, CONFIG_SYS_DCACHELINE_SIZE);
-}
-
 /*
  * COPY EXCEPTION TRAMPOLINE -- copy the tramp to the
  * exception address. Define CONFIG_ROM_STUBS to prevent

(quoted diff) +0 -0

b/arch/nios2/include/asm/cache.h index 9b87c9f..dde43cd 100644

arch/nios2/include/asm/cache.h +0 -4

@@ -8,18 +8,11 @@
 #ifndef __ASM_NIOS2_CACHE_H_
 #define __ASM_NIOS2_CACHE_H_
 
-extern void flush_dcache (unsigned long start, unsigned long size);
-extern void flush_icache (unsigned long start, unsigned long size);
-
 /*
- * Valid L1 data cache line sizes for the NIOS2 architecture are 4, 16,

and 32 - * bytes. If the board configuration has not specified one we default to the - * largest of these values for alignment of DMA buffers.
+ * Valid L1 data cache line sizes for the NIOS2 architecture are 4,
+ * 16, and 32 bytes. We default to the largest of these values for
+ * alignment of DMA buffers.
*/

arch/nios2/include/asm/cache.h +0 -4

-#ifdef CONFIG_SYS_CACHELINE_SIZE
-#define ARCH_DMA_MINALIGN	CONFIG_SYS_CACHELINE_SIZE
-#else
 #define ARCH_DMA_MINALIGN	32
-#endif
 
 #endif /* __ASM_NIOS2_CACHE_H_ */

arch/nios2/lib/bootm.c +0 -3

index c730a3f..4e5c269 100644
@@ -6,9 +6,6 @@
  */
 
 #include <common.h>
-#include <command.h>
-#include <asm/byteorder.h>
-#include <asm/cache.h>
 
 #define NIOS_MAGIC 0x534f494e /* enable command line and initrd passing */
 

@@ -40,8 +37,7 @@ int do_bootm_linux(int flag, int argc, char * const

argv[], bootm_headers_t *ima

arch/nios2/lib/bootm.c +1 -2

 	/* flushes data and instruction caches before calling the kernel */
 	disable_interrupts();
-	flush_dcache((ulong)kernel, CONFIG_SYS_DCACHE_SIZE);
-	flush_icache((ulong)kernel, CONFIG_SYS_ICACHE_SIZE);
+	flush_dcache_all();
 
 	debug("bootargs=%s @ 0x%lx\n", commandline, (ulong)&commandline);
 	debug("initrd=0x%lx-0x%lx\n", (ulong)initrd_start, (ulong)initrd_end);

arch/nios2/lib/cache.S deleted +0 -68

deleted file mode 100644
index 683f005..0000000
@@ -1,68 +0,0 @@
-/*
- * (C) Copyright 2004, Psyent Corporation <www.psyent.com>
- * Scott McNutt <[email protected]>
- *
- * SPDX-License-Identifier:	GPL-2.0+
- */
-
-#include <config.h>
-
-	.text
-
-	.global flush_dcache
-
-flush_dcache:
-	add	r5, r5, r4
-	movhi	r8, %hi(CONFIG_SYS_DCACHELINE_SIZE)
-	ori	r8, r8, %lo(CONFIG_SYS_DCACHELINE_SIZE)
-0:	flushd	0(r4)
-	add	r4, r4, r8
-	bltu	r4, r5, 0b
-	ret
-
-
-	.global flush_icache
-
-flush_icache:
-	add	r5, r5, r4
-	movhi	r8, %hi(CONFIG_SYS_ICACHELINE_SIZE)
-	ori	r8, r8, %lo(CONFIG_SYS_ICACHELINE_SIZE)
-1:	flushi	r4
-	add	r4, r4, r8
-	bltu	r4, r5, 1b
-	ret
-
-	.global flush_dcache_range
-
-flush_dcache_range:
-	movhi	r8, %hi(CONFIG_SYS_DCACHELINE_SIZE)
-	ori	r8, r8, %lo(CONFIG_SYS_DCACHELINE_SIZE)
-0:	flushd	0(r4)
-	add	r4, r4, r8
-	bltu	r4, r5, 0b
-	ret
-
-	.global flush_cache
-
-flush_cache:
-	add	r5, r5, r4
-	mov	r9, r4
-	mov	r10, r5
-
-	movhi	r8, %hi(CONFIG_SYS_DCACHELINE_SIZE)
-	ori	r8, r8, %lo(CONFIG_SYS_DCACHELINE_SIZE)
-0:	flushd	0(r4)
-	add	r4, r4, r8
-	bltu	r4, r5, 0b
-
-	mov	r4, r9
-	mov	r5, r10
-	movhi	r8, %hi(CONFIG_SYS_ICACHELINE_SIZE)
-	ori	r8, r8, %lo(CONFIG_SYS_ICACHELINE_SIZE)
-1:	flushi	r4
-	add	r4, r4, r8
-	bltu	r4, r5, 1b
-
-	sync
-	flushp
-	ret

arch/nios2/lib/cache.c new file +8 -0

new file mode 100644
index 0000000..6f26d8d
@@ -0,0 +1,84 @@
+/*
+ * Copyright (C) 2015 Thomas Chou <[email protected]>
+ * Copyright (C) 2009, Wind River Systems Inc
+ * Implemented by fredrik.markstrom at gmail.com and ivarholmqvist at gmail.com
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+

arch/nios2/lib/cache.c new file +63 -0

+	if (end > start + gd->arch.dcache_size)
+		end = start + gd->arch.dcache_size;
+
+	for (addr = start; addr < end; addr += gd->arch.dcache_line_size) {
+		__asm__ __volatile__ ("   flushd 0(%0)\n"
+					: /* Outputs */
+					: /* Inputs  */ "r"(addr)
+					/* : No clobber */);
+	}
+}
+
+static void __flush_icache(unsigned long start, unsigned long end)
+{
+	unsigned long addr;
+
+	start &= ~(gd->arch.icache_line_size - 1);
+	end += (gd->arch.icache_line_size - 1);
+	end &= ~(gd->arch.icache_line_size - 1);
+
+	if (end > start + gd->arch.icache_size)
+		end = start + gd->arch.icache_size;
+
+	for (addr = start; addr < end; addr += gd->arch.icache_line_size) {
+		__asm__ __volatile__ ("   flushi %0\n"
+					: /* Outputs */
+					: /* Inputs  */ "r"(addr)
+					/* : No clobber */);
+	}
+	__asm__ __volatile(" flushp\n");
+}
+
+void flush_dcache_all(void)
+{
+	__flush_dcache_all(0, gd->arch.dcache_size);
+	__flush_icache(0, gd->arch.icache_size);
+}
+
+void flush_dcache_range(unsigned long start, unsigned long end)
+{
+	__flush_dcache_all(start, end);
+}
+
+void flush_cache(unsigned long start, unsigned long size)
+{
+	__flush_dcache_all(start, start + size);
+	__flush_icache(start, start + size);
+}
+
+/* nios2 data cache is always enabled */
+int dcache_status(void)
+{
+	return 1;
+}
+
+void dcache_enable(void)
+{
+	flush_dcache_all();
+}
+
+void dcache_disable(void)
+{
+	flush_dcache_all();
+}

arch/nios2/lib/cache.c line 18

Quoted 8 lines

arch/nios2/lib/cache.c new file +8 -0

+
+DECLARE_GLOBAL_DATA_PTR;
+
+static void __flush_dcache_all(unsigned long start, unsigned long end)
+{
+     unsigned long addr;
+
+     start &= ~(gd->arch.dcache_line_size - 1);

I'd suggest to use GENMASK() here, but I don't think we picked this from Linux just yet.

arch/nios2/lib/cache.c line 20

Quoted 2 lines

arch/nios2/lib/cache.c new file +2 -0

+     end += (gd->arch.dcache_line_size - 1);
+     end &= ~(gd->arch.dcache_line_size - 1);

Is this an attempt at poor-mans' rounding ? I think you want to implment something like arch/arm/cpu/arm926ejs/cache.c check_cache_range() and NOT do any rounding here. The reason for that is that if you do rounding, you might accidentally corrupt a piece of memory which was just delivered via DMA before you did the flush.

Quoted 5 lines

Is this an attempt at poor-mans' rounding ? I think you want to implment something like arch/arm/cpu/arm926ejs/cache.c check_cache_range() and NOT do any rounding here. The reason for that is that if you do rounding, you might accidentally corrupt a piece of memory which was just delivered via DMA before you did the flush.

The code above is to convert the address to dcache line size. arch/arm/cpu/arm926ejs/cache.c check_cache_range() will skip the cache flushing if it is unaligned to cache line size. I'm not sure how frequent U-boot access to non-aligned cache line size.

Hi Marek,

Quoted 10 lines · Ley Foon Tan

Quoted 5 lines

Is this an attempt at poor-mans' rounding ? I think you want to implment something like arch/arm/cpu/arm926ejs/cache.c check_cache_range() and NOT do any rounding here. The reason for that is that if you do rounding, you might accidentally corrupt a piece of memory which was just delivered via DMA before you did the flush.

The code above is to convert the address to dcache line size. arch/arm/cpu/arm926ejs/cache.c check_cache_range() will skip the cache flushing if it is unaligned to cache line size. I'm not sure how frequent U-boot access to non-aligned cache line size.

Thanks a lot for your looking into this, Ley Foon.

I think we take cache flushing in a different way to arm926ejs.

In nios2 driver programming, we would request all the DMA buffers be aligned to cache line. This is necessary to avoid the cache racing issue as you mention above.

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

In the copy_exception_trampoline() patch, both dcache and icache must be flushed at the exception target address. Though the flush range is only 12 bytes, which won't be aligned.

Thank you for your review.

Best regards,
Thomas

Quoted 5 lines

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

Hi Marek,

Quoted 6 lines · Marek Vasut

Quoted 5 lines

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

Quoted 2 lines

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

I take a look into the cache flush operations in every arch of u-boot. It turns out that the arm926ejs is the only platform that does such cache line range check and skip. All other ARM and all other arch don't. And the cache flush in Linux don't.

Hi Marek,

Quoted 9 lines

Hi Marek,

Quoted 6 lines · Marek Vasut

Quoted 5 lines

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

Quoted 7 lines

Quoted 2 lines

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

I take a look into the cache flush operations in every arch of u-boot. It turns out that the arm926ejs is the only platform that does such cache line range check and skip. All other ARM and all other arch don't. And the cache flush in Linux don't.

+arm11, which is based on the same code.

I see your patch on this range check. I would prefer that the details of cache line configuration be kept inside the cache flush operators, and need not be exposed to drivers.

As drivers might be used by different
arch with different cache implementation, L1,L2..etc. It is not good for drivers to adjust the flush range before passing to cache flush operators.

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

Hi Marek,

Quoted 5 lines

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to allocate DMA buffers so that they are cache aligned.

Quoted 3 lines

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

I would suggest debug check should be left as for debug only. The definition of common functions should be kept as it is more important than coding style.

I debugged DMA issues a lot in the past until I realized the importance of aligned buffers. So there should be a developer's guideline.

But it is even much more difficult when something you believed does not work as expected, what is taken as common sense. It will trap a lot of developers when they called your flush cache functions but was skipped just because, eg, the end of packets are not aligned which is usually the case.

I would suggest that, with the best of my knowledge, please change the range check to a debug probe, and restore the cache flush functions to the common definition.

Quoted 1 line · Thomas Chou

Hi Marek,

Hi,

Quoted 7 lines · Marek Vasut

Quoted 5 lines

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

Quoted 2 lines

I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to allocate DMA buffers so that they are cache aligned.

That and include/memalign.h , which contains macros that are used to align variables.

Quoted 8 lines

Quoted 4 lines

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

I would suggest debug check should be left as for debug only. The definition of common functions should be kept as it is more important than coding style.

Uh yes, that's what arm926 cache functions do, they're debug only.

Quoted 2 lines

I debugged DMA issues a lot in the past until I realized the importance of aligned buffers. So there should be a developer's guideline.

For what exactly?

Quoted 6 lines

But it is even much more difficult when something you believed does not work as expected, what is taken as common sense. It will trap a lot of developers when they called your flush cache functions but was skipped just because, eg, the end of packets are not aligned which is usually the case.

This is good, it should bite them, because this is a bug. If, on the other hand, you will paper over such bugs by adding crap to the cache ops, there will be even worse bugs coming for you, like variables which are sitting in the same cacheline as your unaligned buffer that you want to invalidate or flush will possibly get trashed by such cache operation.

Consider this:

cacheline 0: [ variable A ; buffer B ......... ]
cacheline 1: [ buffer B ......... ; Empty .... ]

Now you do the following:

1) Variable A = 0;
2) Flush buffer B (which is unaligned, so flush cacheline 0 and 1)
3) Start DMA to buffer B
4) Variable A = 1;
5) Check if DMA finished, it did
6) Invalidate buffer B ... oh, but it's unaligned, let's invalidate everything around it, which is cacheline 0 and 1.
7) What is the value of variable A ? Oh, it's fetched from memory and it's 0 there, even though we did set it to 1 ...

Quoted 3 lines

I would suggest that, with the best of my knowledge, please change the range check to a debug probe, and restore the cache flush functions to the common definition.

See above, does my example make it clear why we should never ever hide bugs in the cache ops code ?

Hi Marek,

Quoted 12 lines · Thomas Chou

Quoted 1 line

Hi Marek,

Hi,

Quoted 7 lines · Marek Vasut

Quoted 5 lines

Then you'd also need means to allocate variables to aligned memory location to prevent invalid cache flush. (Linux does this with it's DMA API). We are much simpler and thus this abstraction is still not available. I wonder if the overhead of DMA API would be high or not for U-Boot.

Quoted 5 lines

Quoted 2 lines

I see most people use memalign(ARCH_DMA_MINALIGN, len) in u-boot to allocate DMA buffers so that they are cache aligned.

That and include/memalign.h , which contains macros that are used to align variables.

Yes. It is malloc_cache_aligned(), which should be used to allocate DMA buffer. Thank you for the pointer.

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

It is even worse if the cache flush operators permit incorrect cache flushes or invalidations. Like I mentioned before, this can lead to hard to debug problems when using DMA (at least on ARM).

I would suggest debug check should be left as for debug only. The definition of common functions should be kept as it is more important than coding style.

Uh yes, that's what arm926 cache functions do, they're debug only.

Quoted 4 lines

Quoted 2 lines

I debugged DMA issues a lot in the past until I realized the importance of aligned buffers. So there should be a developer's guideline.

For what exactly?

For u-boot, every DMA buffer must be allocated with malloc_cache_aligned(). Then there will be not variables and DMA buffers cache racing issues as you describe below.

Quoted 31 lines

Quoted 5 lines

But it is even much more difficult when something you believed does not work as expected, what is taken as common sense. It will trap a lot of developers when they called your flush cache functions but was skipped just because, eg, the end of packets are not aligned which is usually the case.

This is good, it should bite them, because this is a bug. If, on the other hand, you will paper over such bugs by adding crap to the cache ops, there will be even worse bugs coming for you, like variables which are sitting in the same cacheline as your unaligned buffer that you want to invalidate or flush will possibly get trashed by such cache operation.

Consider this:

cacheline 0: [ variable A ; buffer B ......... ]
cacheline 1: [ buffer B ......... ; Empty .... ]

Now you do the following:

1) Variable A = 0;
2) Flush buffer B (which is unaligned, so flush cacheline 0 and 1)
3) Start DMA to buffer B
4) Variable A = 1;
5) Check if DMA finished, it did
6) Invalidate buffer B ... oh, but it's unaligned, let's invalidate

everything around it, which is cacheline 0 and 1.

7) What is the value of variable A ? Oh, it's fetched from memory and

Quoted 8 lines

it's 0 there, even though we did set it to 1 ...

Quoted 4 lines

I would suggest that, with the best of my knowledge, please change the range check to a debug probe, and restore the cache flush functions to the common definition.

See above, does my example make it clear why we should never ever hide bugs in the cache ops code ?

It is the drivers' responsibility to follow the guide line above. If there is such a bug, it is not the cache flush ops bug. It is a driver's bug. You may add a probe to show the bug from caller, but you may not call it a bug of cache ops and skip the flush. Given that it is quite common that the return of such cache ops is not checked, few (if not none) will ever know that the flush was skipped.

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

Hi Marek,

Quoted 3 lines · Marek Vasut

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

Thanks for sharing this.

Quoted 4 lines

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

The same rule can be applied to buffer allocated on stack, with the macro you mentioned above. In all, cache line aware allocation on heap or on stack must be used for DMA buffer.

Quoted 5 lines

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

Quoted 4 lines

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

It won't be safer to flush nothing. Sooner or later the cache will be flushed due to data access, even if the cache flush ops is skip.

To solve problem like this, the only solution is to enforce the rule to allocate DMA buffer. It is wrong to skip the flush.

That is bad bad bad, that's even nastier. We really need to fix the drivers, not paper over it in the cache ops.

Quoted 2 lines

To solve problem like this, the only solution is to enforce the rule to allocate DMA buffer. It is wrong to skip the flush.

I absolutelly agree we need aligned allocations for DMA memory areas. But, we also shouldn't hide bugs. And I believe aligning the incorrect arguments to cache functions is not the way to go. We should check the arguments and if someone tries an unaligned cache op, we should scream. What do you think?

Quoted 14 lines · Thomas Chou

Hi Marek,

Quoted 3 lines · Marek Vasut

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

Thanks for sharing this.

Quoted 4 lines

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

Quoted 3 lines

The same rule can be applied to buffer allocated on stack, with the macro you mentioned above. In all, cache line aware allocation on heap or on stack must be used for DMA buffer.

That's correct, they must be used. But sadly, this is not yet the case in all the drivers, which we need to rectify. And how best to rectify this than to scream when someone does such a thing, right ?

Quoted 10 lines

Quoted 9 lines

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

Quoted 2 lines

It won't be safer to flush nothing. Sooner or later the cache will be flushed due to data access, even if the cache flush ops is skip.

That is bad bad bad, that's even nastier. We really need to fix the drivers, not paper over it in the cache ops.

Quoted 3 lines

To solve problem like this, the only solution is to enforce the rule to allocate DMA buffer. It is wrong to skip the flush.

I absolutelly agree we need aligned allocations for DMA memory areas. But, we also shouldn't hide bugs. And I believe aligning the incorrect arguments to cache functions is not the way to go. We should check the arguments and if someone tries an unaligned cache op, we should scream. What do you think?

btw. I think you won't get way too many cache warnings nowadays and we can fix those few remaining way before the 2016.01 is out.

Hi Marek,

Quoted 20 lines · Thomas Chou

Quoted 18 lines

Hi Marek,

Quoted 4 lines · Marek Vasut

There are also DEFINE_CACHE_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() macros which can be used to allocate such stuff on stack. And you sometimes do want to allocate things on stack instead of using malloc().

Thanks for sharing this.

Quoted 4 lines

Sometimes you might want to allocate DMA buffers on stack, for example if you don't have mallocator running yet or if it's more convenient for some reason. So forcing everyone to allocate DMA buffers using malloc is not gonna slide I'm afraid.

The same rule can be applied to buffer allocated on stack, with the macro you mentioned above. In all, cache line aware allocation on heap or on stack must be used for DMA buffer.

Quoted 3 lines

That's correct, they must be used. But sadly, this is not yet the case in all the drivers, which we need to rectify. And how best to rectify this than to scream when someone does such a thing, right ?

Given that there are not so many drivers using DMA in u-boot, as compared to Linux. I would suggest we can walk through and rectify the allocation of DMA buffers.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

The cache flush ops is the best place to scream death and murder if someone tries such unaligned cache operation, so maybe you should even do a printf() there to weed such crappy drivers out for the 2016.01 release.

I agree it's the responsibility of the driver, so if the driver doesn't do things right, it's a bug and the behavior of cache ops is undefined, which might as well be that we do the safer thing here and flush nothing.

Quoted 5 lines

Quoted 2 lines

It won't be safer to flush nothing. Sooner or later the cache will be flushed due to data access, even if the cache flush ops is skip.

That is bad bad bad, that's even nastier. We really need to fix the drivers, not paper over it in the cache ops.

I know how bad it is, with over 35 years work with DMA. grin..

Quoted 9 lines

Quoted 2 lines

To solve problem like this, the only solution is to enforce the rule to allocate DMA buffer. It is wrong to skip the flush.

I absolutelly agree we need aligned allocations for DMA memory areas. But, we also shouldn't hide bugs. And I believe aligning the incorrect arguments to cache functions is not the way to go. We should check the arguments and if someone tries an unaligned cache op, we should scream. What do you think?

Quoted 2 lines

btw. I think you won't get way too many cache warnings nowadays and we can fix those few remaining way before the 2016.01 is out.

I would suggest the "cache alignment check and skip" be removed from cache flush ops, and say out the DMA buffer allocation rule loudly in README, and enforce it by guardianship.

Please allow me to restate the reasons,

1. The cache flush ops are commonly used. Please refer to the "Cache and TLB Flushing Under Linux" doc, linux/Documentation/cachetlb.txt. Violating the defined interface is much worse than violating coding style. It will certainly impact the portability of u-boot. And might introduce more bug than resolve.

2. We all agree that enforcing DMA buffer allocation to cache aligned is the only real solution. Adding such "check and skip" to cache flush ops cannot prevent the flush or solve the problem.

3. Though the flush size of block device are usually aligned, the size of packet are not. Asking the packet drivers to adjust the flush size does not make sense. It is the job of cache flush ops. The debug probe should not override the original purpose. It should be spelled for common understanding.

It is free to your consideration. As it is free and open software. :)

Best regards,
Thomas

Quoted 3 lines

I would suggest the "cache alignment check and skip" be removed from cache flush ops, and say out the DMA buffer allocation rule loudly in README, and enforce it by guardianship.

What exactly do you envision by this "guardianship" ?

Quoted 8 lines

Please allow me to restate the reasons,

1. The cache flush ops are commonly used. Please refer to the "Cache and TLB Flushing Under Linux" doc, linux/Documentation/cachetlb.txt. Violating the defined interface is much worse than violating coding style. It will certainly impact the portability of u-boot. And might introduce more bug than resolve.

I agree with this one.

Quoted 3 lines

2. We all agree that enforcing DMA buffer allocation to cache aligned is the only real solution. Adding such "check and skip" to cache flush ops cannot prevent the flush or solve the problem.

We should probably check-scream-skip here.

Quoted 5 lines

3. Though the flush size of block device are usually aligned, the size of packet are not. Asking the packet drivers to adjust the flush size does not make sense. It is the job of cache flush ops. The debug probe should not override the original purpose. It should be spelled for common understanding.

The socket buffer(s) should be aligned, so network packets should be fine.

Hi Marek,

Quoted 5 lines

Quoted 3 lines

I would suggest the "cache alignment check and skip" be removed from cache flush ops, and say out the DMA buffer allocation rule loudly in README, and enforce it by guardianship.

What exactly do you envision by this "guardianship" ?

I mean the reviews of custodians.

Quoted 22 lines

Quoted 7 lines

Please allow me to restate the reasons,

1. The cache flush ops are commonly used. Please refer to the "Cache and TLB Flushing Under Linux" doc, linux/Documentation/cachetlb.txt. Violating the defined interface is much worse than violating coding style. It will certainly impact the portability of u-boot. And might introduce more bug than resolve.

I agree with this one.

Quoted 3 lines

2. We all agree that enforcing DMA buffer allocation to cache aligned is the only real solution. Adding such "check and skip" to cache flush ops cannot prevent the flush or solve the problem.

We should probably check-scream-skip here.

Quoted 5 lines

3. Though the flush size of block device are usually aligned, the size of packet are not. Asking the packet drivers to adjust the flush size does not make sense. It is the job of cache flush ops. The debug probe should not override the original purpose. It should be spelled for common understanding.

Quoted 2 lines

The socket buffer(s) should be aligned, so network packets should be fine.

While the start of socket buffer might be aligned, the size of the transfer might not for the send ops. It is depended on the net/tcp/ip packets size.

For example, with tftp, there is a lot of unaligned end of packets.

tftp d1000000 u-boot-dtb.bin

flush unaligned d7ff7020-d7ff704e
[repeat ..]

So, such an alarm may be false. And such a skip can be bug.

In fact, for my own projects, I have changed the memory allocation to always cache aligned. And I rarely worry about it ever after.

I look at the net.c of u-boot. There are packets buffer allocated on BSS and stack. I would suggest avoid such programming, and use aligned memory allocation stead.

Hi Marek,

Quoted 6 lines

Quoted 5 lines

In nios2, we don't skip the flushing when the inputs are not aligned like that of arm926ejs. We always flush all cache lines in the range, even if a single byte to flush is in request. So the inputs are rounded to get the lower and upper cache lines range inside the cache flush functions. The caller need not be aware of the detail.

Quoted 2 lines

This is incorrect and all the places which produce these unaligned cache operations must be fixed.

I take a look into the cache flush operations in every arch of u-boot. It turns out that the arm926ejs is the only platform that does such cache line range check and skip. All other ARM and all other arch don't.

And the cache flush in Linux don't.

Quoted 15 lines · Marek Vasut

arch/nios2/lib/cache.c line 18

Quoted 8 lines

arch/nios2/lib/cache.c new file +8 -0

+
+DECLARE_GLOBAL_DATA_PTR;
+
+static void __flush_dcache_all(unsigned long start, unsigned long end)
+{
+     unsigned long addr;
+
+     start &= ~(gd->arch.dcache_line_size - 1);

I'd suggest to use GENMASK() here, but I don't think we picked this from Linux just yet.

Quoted 2 lines

arch/nios2/lib/cache.c new file +2 -0

+     end += (gd->arch.dcache_line_size - 1);
+     end &= ~(gd->arch.dcache_line_size - 1);

Quoted 5 lines

Is this an attempt at poor-mans' rounding ? I think you want to implment something like arch/arm/cpu/arm926ejs/cache.c check_cache_range() and NOT do any rounding here. The reason for that is that if you do rounding, you might accidentally corrupt a piece of memory which was just delivered via DMA before you did the flush.

The code above is to convert the address to dcache line size.

arch/arm/cpu/arm926ejs/cache.c check_cache_range() will skip the cache flushing if it is unaligned to cache line size.

I'm not sure how
frequent U-boot access to non-aligned cache line size.