Re: [U-Boot] [PATCH 4/4] armv7: cache: remove flush on un-aligned invalidate
On Wednesday 10 August 2011 11:41 PM, Anton Staaf wrote: On Tue, Aug 9, 2011 at 11:48 PM, Aneesh Vane...@ti.com wrote: Hi Anton, On Tuesday 09 August 2011 10:09 PM, Anton Staaf wrote: I'm not sure what the larger context of this change is, but it seems like a bad idea to me. There are a lot of locations in U-Boot that Please see this thread for the context. http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/105113/focus=105135 Ahh, I missed this thread (Its title became stale). :) But I completely agree with the outcome, we need to fix the unaligned buffer problem. will end up causing an unaligned invalidate (ext2 and dos file system code in particular). And this change will cause those unaligned invalidates to possibly throw away stores to adjacent variables. If No. Those partial cache-lines on the boundary are left alone. They are not invalidated. So, it still affects only the party calling the invalidate. Ahh, you are correct. I missed that the change would cause fewer cache lines to be invalidated. In this case I am much happier with this change. In light of this I still think the warning is a little mild, since it means that the driver that called the invalidate is certainly going to get the wrong values. Perhaps changing it to an error would be good (I realize that functionally it would be identical, but it would be more potent psychologically). I don't think an assert is warranted in this case since as Albert points out it would prevent online debugging of U-Boot which is a very useful way of working. Ok. I will change the warning to an error. br, Aneesh ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] armv7: cache: remove flush on un-aligned invalidate
Hi Anton, Le 09/08/2011 18:39, Anton Staaf a écrit : I'm not sure what the larger context of this change is, but it seems like a bad idea to me. There are a lot of locations in U-Boot that will end up causing an unaligned invalidate (ext2 and dos file system code in particular). And this change will cause those unaligned invalidates to possibly throw away stores to adjacent variables. If you are going to make this change you should at least assert instead of just printing a warning. And there should be a concerted effort to clean up the buffer management in U-Boot so that invalidates will never be unaligned. This is also a departure from the cache management implementations in the Linux kernel, not that U-Boot has to do exactly what they do, but I feel they have the correct implementation, from the perspective of ensuring that all stores actually make it to main memory. There's been a lot of discussion, both in July and August, about this and the the conclusion is that unaligned invalidates (and flushes) cannot reliably be done within the size and speed constraints of an embedded bootloader, and there are no 100% clean solutions without alignment, because with unaligned buffers, one might invalidate a cache line containing also deferred writes, or flush a line onto a buffer currently undergoing DMA. Linux can afford to spend more space and time than U-Boot to solving those issues. OTOH, aligning buffers is a relatively trivial change in the calling code that allows for much simpler and more efficient cache management. The current cache patches on ARM are precisely the start of the effort to align buffers that you suggest, by warning the developer about unaligned invalidates/flushes through the console, and turning any side effect to an inside-effect, so to speak: for instance the recent cache invalidate patch on arm926ejs prevents unaligned invalidates from affecting any other data than the (badly) invalidated buffer itself. You have a point though that maybe a warning is not enough and that unalignments warrant an assert instead. OTOH, that might mean *a lot* of boards will completely cease working in this case, even for live debugging the issue with basic U-Boot commands (which many developers do I am sure). Comments on this warning/assert point? Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] armv7: cache: remove flush on un-aligned invalidate
Hi Anton, On Tuesday 09 August 2011 10:09 PM, Anton Staaf wrote: I'm not sure what the larger context of this change is, but it seems like a bad idea to me. There are a lot of locations in U-Boot that Please see this thread for the context. http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/105113/focus=105135 will end up causing an unaligned invalidate (ext2 and dos file system code in particular). And this change will cause those unaligned invalidates to possibly throw away stores to adjacent variables. If No. Those partial cache-lines on the boundary are left alone. They are not invalidated. So, it still affects only the party calling the invalidate. you are going to make this change you should at least assert instead of just printing a warning. And there should be a concerted effort to clean up the buffer management in U-Boot so that invalidates will never be unaligned. This is also a departure from the cache management implementations in the Linux kernel, not that U-Boot has to do exactly what they do, but I feel they have the correct implementation, from the perspective of ensuring that all stores actually make it to main memory. Yes, I had implemented it in line with the kernel apporach. However, with un-aligned buffers there is no perfect solution anyway. So, I don't have a strong opinion on this. Leaving alone the boundary cache-lines and printing a big warning seems reasonable enough. best regards, Aneesh ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] armv7: cache: remove flush on un-aligned invalidate
On Tue, Aug 9, 2011 at 11:48 PM, Aneesh V ane...@ti.com wrote: Hi Anton, On Tuesday 09 August 2011 10:09 PM, Anton Staaf wrote: I'm not sure what the larger context of this change is, but it seems like a bad idea to me. There are a lot of locations in U-Boot that Please see this thread for the context. http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/105113/focus=105135 Ahh, I missed this thread (Its title became stale). :) But I completely agree with the outcome, we need to fix the unaligned buffer problem. will end up causing an unaligned invalidate (ext2 and dos file system code in particular). And this change will cause those unaligned invalidates to possibly throw away stores to adjacent variables. If No. Those partial cache-lines on the boundary are left alone. They are not invalidated. So, it still affects only the party calling the invalidate. Ahh, you are correct. I missed that the change would cause fewer cache lines to be invalidated. In this case I am much happier with this change. In light of this I still think the warning is a little mild, since it means that the driver that called the invalidate is certainly going to get the wrong values. Perhaps changing it to an error would be good (I realize that functionally it would be identical, but it would be more potent psychologically). I don't think an assert is warranted in this case since as Albert points out it would prevent online debugging of U-Boot which is a very useful way of working. you are going to make this change you should at least assert instead of just printing a warning. And there should be a concerted effort to clean up the buffer management in U-Boot so that invalidates will never be unaligned. This is also a departure from the cache management implementations in the Linux kernel, not that U-Boot has to do exactly what they do, but I feel they have the correct implementation, from the perspective of ensuring that all stores actually make it to main memory. Yes, I had implemented it in line with the kernel apporach. However, with un-aligned buffers there is no perfect solution anyway. So, I don't have a strong opinion on this. Leaving alone the boundary cache-lines and printing a big warning seems reasonable enough. Yup. best regards, Aneesh I have a number of patches to fix unaligned buffer use in the filesystem (ext2, dos, mmc and partition) code that I will start sending upstream today. Some of them will be more RFCs than patch sets. :) Thanks, Anton ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 4/4] armv7: cache: remove flush on un-aligned invalidate
Remove the flush of boundary cache-lines done as part
of invalidate on a non cache-line boundary aligned
buffer
Also, print a warning when this situation is recognized.
Signed-off-by: Aneesh V ane...@ti.com
---
V2:
New in V2
---
arch/arm/cpu/armv7/cache_v7.c | 14 --
arch/arm/lib/cache-pl310.c| 15 +--
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
index 665f025..3dda56b 100644
--- a/arch/arm/cpu/armv7/cache_v7.c
+++ b/arch/arm/cpu/armv7/cache_v7.c
@@ -181,21 +181,23 @@ static void v7_dcache_inval_range(u32 start, u32 stop,
u32 line_len)
u32 mva;
/*
-* If start address is not aligned to cache-line flush the first
-* line to prevent affecting somebody else's buffer
+* If start address is not aligned to cache-line do not
+* invalidate the first cache-line
*/
if (start (line_len - 1)) {
- v7_dcache_clean_inval_range(start, start + 1, line_len);
+ printf(WARNING: %s - start address is not aligned - 0x%08x\n,
+ __func__, start);
/* move to next cache line */
start = (start + line_len - 1) ~(line_len - 1);
}
/*
-* If stop address is not aligned to cache-line flush the last
-* line to prevent affecting somebody else's buffer
+* If stop address is not aligned to cache-line do not
+* invalidate the last cache-line
*/
if (stop (line_len - 1)) {
- v7_dcache_clean_inval_range(stop, stop + 1, line_len);
+ printf(WARNING: %s - stop address is not aligned - 0x%08x\n,
+ __func__, stop);
/* align to the beginning of this cache line */
stop = ~(line_len - 1);
}
diff --git a/arch/arm/lib/cache-pl310.c b/arch/arm/lib/cache-pl310.c
index 36c629c..15fe304 100644
--- a/arch/arm/lib/cache-pl310.c
+++ b/arch/arm/lib/cache-pl310.c
@@ -26,6 +26,7 @@
#include asm/armv7.h
#include asm/pl310.h
#include config.h
+#include common.h
struct pl310_regs *const pl310 = (struct pl310_regs *)CONFIG_SYS_PL310_BASE;
@@ -89,21 +90,23 @@ void v7_outer_cache_inval_range(u32 start, u32 stop)
u32 pa, line_size = 32;
/*
-* If start address is not aligned to cache-line flush the first
-* line to prevent affecting somebody else's buffer
+* If start address is not aligned to cache-line do not
+* invalidate the first cache-line
*/
if (start (line_size - 1)) {
- v7_outer_cache_flush_range(start, start + 1);
+ printf(WARNING: %s - start address is not aligned - 0x%08x\n,
+ __func__, start);
/* move to next cache line */
start = (start + line_size - 1) ~(line_size - 1);
}
/*
-* If stop address is not aligned to cache-line flush the last
-* line to prevent affecting somebody else's buffer
+* If stop address is not aligned to cache-line do not
+* invalidate the last cache-line
*/
if (stop (line_size - 1)) {
- v7_outer_cache_flush_range(stop, stop + 1);
+ printf(WARNING: %s - stop address is not aligned - 0x%08x\n,
+ __func__, stop);
/* align to the beginning of this cache line */
stop = ~(line_size - 1);
}
--
1.7.0.4
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] armv7: cache: remove flush on un-aligned invalidate
I'm not sure what the larger context of this change is, but it seems
like a bad idea to me. There are a lot of locations in U-Boot that
will end up causing an unaligned invalidate (ext2 and dos file system
code in particular). And this change will cause those unaligned
invalidates to possibly throw away stores to adjacent variables. If
you are going to make this change you should at least assert instead
of just printing a warning. And there should be a concerted effort to
clean up the buffer management in U-Boot so that invalidates will
never be unaligned. This is also a departure from the cache
management implementations in the Linux kernel, not that U-Boot has to
do exactly what they do, but I feel they have the correct
implementation, from the perspective of ensuring that all stores
actually make it to main memory.
Thanks,
Anton
On Tue, Aug 9, 2011 at 4:10 AM, Aneesh V ane...@ti.com wrote:
Remove the flush of boundary cache-lines done as part
of invalidate on a non cache-line boundary aligned
buffer
Also, print a warning when this situation is recognized.
Signed-off-by: Aneesh V ane...@ti.com
---
V2:
New in V2
---
arch/arm/cpu/armv7/cache_v7.c | 14 --
arch/arm/lib/cache-pl310.c | 15 +--
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
index 665f025..3dda56b 100644
--- a/arch/arm/cpu/armv7/cache_v7.c
+++ b/arch/arm/cpu/armv7/cache_v7.c
@@ -181,21 +181,23 @@ static void v7_dcache_inval_range(u32 start, u32 stop,
u32 line_len)
u32 mva;
/*
- * If start address is not aligned to cache-line flush the first
- * line to prevent affecting somebody else's buffer
+ * If start address is not aligned to cache-line do not
+ * invalidate the first cache-line
*/
if (start (line_len - 1)) {
- v7_dcache_clean_inval_range(start, start + 1, line_len);
+ printf(WARNING: %s - start address is not aligned -
0x%08x\n,
+ __func__, start);
/* move to next cache line */
start = (start + line_len - 1) ~(line_len - 1);
}
/*
- * If stop address is not aligned to cache-line flush the last
- * line to prevent affecting somebody else's buffer
+ * If stop address is not aligned to cache-line do not
+ * invalidate the last cache-line
*/
if (stop (line_len - 1)) {
- v7_dcache_clean_inval_range(stop, stop + 1, line_len);
+ printf(WARNING: %s - stop address is not aligned - 0x%08x\n,
+ __func__, stop);
/* align to the beginning of this cache line */
stop = ~(line_len - 1);
}
diff --git a/arch/arm/lib/cache-pl310.c b/arch/arm/lib/cache-pl310.c
index 36c629c..15fe304 100644
--- a/arch/arm/lib/cache-pl310.c
+++ b/arch/arm/lib/cache-pl310.c
@@ -26,6 +26,7 @@
#include asm/armv7.h
#include asm/pl310.h
#include config.h
+#include common.h
struct pl310_regs *const pl310 = (struct pl310_regs *)CONFIG_SYS_PL310_BASE;
@@ -89,21 +90,23 @@ void v7_outer_cache_inval_range(u32 start, u32 stop)
u32 pa, line_size = 32;
/*
- * If start address is not aligned to cache-line flush the first
- * line to prevent affecting somebody else's buffer
+ * If start address is not aligned to cache-line do not
+ * invalidate the first cache-line
*/
if (start (line_size - 1)) {
- v7_outer_cache_flush_range(start, start + 1);
+ printf(WARNING: %s - start address is not aligned -
0x%08x\n,
+ __func__, start);
/* move to next cache line */
start = (start + line_size - 1) ~(line_size - 1);
}
/*
- * If stop address is not aligned to cache-line flush the last
- * line to prevent affecting somebody else's buffer
+ * If stop address is not aligned to cache-line do not
+ * invalidate the last cache-line
*/
if (stop (line_size - 1)) {
- v7_outer_cache_flush_range(stop, stop + 1);
+ printf(WARNING: %s - stop address is not aligned - 0x%08x\n,
+ __func__, stop);
/* align to the beginning of this cache line */
stop = ~(line_size - 1);
}
--
1.7.0.4
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
