Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wed, Aug 24, 2011 at 1:13 PM, Anton Staaf robot...@google.com wrote: On Wed, Aug 24, 2011 at 12:18 PM, Lukasz Majewski majess1...@gmail.com wrote: On Wed, 24 Aug 2011 11:25:42 -0700 Anton Staaf robot...@google.com wrote: On Wed, Aug 24, 2011 at 11:12 AM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message CAF6FioWBsjFa+QdFNHEQGO50eLoqZMc--Yt3-iYX6DWr=hc...@mail.gmail.com you wrote: 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Ok, so one problem solved :-). Ahh, I misunderstood your question. I thought you were asking about the need to know the cache line size. Not it's specific implementation as a function call. In which case, I agree, and am happy to use CONFIG_SYS_CACHE_LINE_SIZE, though I don't see any definitions of CONFIG_SYS_CACHE_LINE_SIZE in the entire U-Boot tree. What have I missed? There are some similar definitions: ./include/configs/atstk1006.h:#define CONFIG_SYS_DCACHE_LINESZ 32 ./include/configs/atngw100.h:#define CONFIG_SYS_DCACHE_LINESZ 32 ./include/configs/favr-32-ezkit.h:#define CONFIG_SYS_DCACHE_LINESZ 32 I would assume that these should be changed to the one mentioned by Wolfgang. But this still leaves us with a question of how to make a simple, easy to use macro for allocating cache line size aligned stack buffers. I'll work on that some more and see if I can come up with something. OK, better late than never, I've run down all of the solutions I can think of. Below are three solutions and one non-solution, how they would be used, and what I see as pro's and con's for each. 1) Mikes's macro #define DMA_ALIGN_SIZE(size) \ (((size) + CONFIG_SYS_CACHELINE_SIZE - 1) #define DMA_DECLARE_BUFFER(type, name, size) \ void __##name[DMA_ALIGN_SIZE(size * sizeof(type))]; \ type * name = __##name ~(CONFIG_SYS_CACHELINE_SIZE - 1)); DMA_DECLARE_BUFFER(int, buffer, 100); Pros: Doesn't use alloca, doesn't use malloc and doesn't use new GCC alignment attribute abilities. This makes it the most portable, and the most supported solution. Cons: It's a pretty ugly macro that obfuscates the creation of multiple variables. Thus I would say it falls into the macro magic category. It looks like this is already working it's way into a patch. So the rest of my comments below may be moot. 2) Use alloca wrapped in a macro: #define alloca_cacheline_alligned(size) alloca(size + CONFIG_SYS_CACHELINE_SIZE - 1) ~(CONFIG_SYS_CACHELINE_SIZE - 1) int * buffer = alloca_cacheline_alligned(100 * sizeof(int)); Pros: It is fairly obvious what the above code is intended to do, even to someone that doesn't know the underlying implementation of the macro. Cons: Wolfgang does not want alloca in the U-Boot codebase. I would like to know, mainly for my education, why you do not want alloca, but are OK with dynamic array sizing (as in the first solution above). My understanding is that they pretty much equate to the same thing. What is it about alloca that is objectionable? 3) Use GCC specific alignment attribute: #define CACHLINE_ALIGNED __attribute__ ((aligned (CONFIG_SYS_CACHELINE_SIZE))) int buffer[100] CACHELINE_ALIGNED; Pros: The declaration of the buffer is even simpler and more obvious, no use of alloca at all. Cons: This doesn't work in any version of GCC before October 2010. Meaning that it probably doesn't work in whatever compiler you're using. It's really too bad that this isn't a usable solution. I suppose that we could switch to it at some point when we expect U-Boot to only be compiled by versions of GCC that support this. By the way, the failure mode here is pretty bad. If you compile the above code with an older GCC it will silently fail to align the variable. :( 4) Use memalign and free: int * buffer = memalign(CONFIG_SYS_CACHELINE_SIZE, 100 * sizeof(int)); free(buffer); Pros: portable. Cons: Heap instead of Stack allocation. The result is that it's slower, and requires more manual management of buffers. Also, Wolfgang is opposed to this solution. I think I agree with this not being a great solution. I do wonder if it would be useful to consider a dedicated buffer management API that could be used to allocate and free cache line aligned power of two buffers. Perhaps something like a buddy heap for simplicity. Thanks, Anton Thanks, Anton Thanks, Anton Best regards, Wolfgang Denk
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Anton Staaf, In message caf6fiowhhqbvqwwprusb69s2mpfbtoiazxc8x56bkogjmzx...@mail.gmail.com you wrote: I would like to know, mainly for my education, why you do not want alloca, but are OK with dynamic array sizing (as in the first solution above). My understanding is that they pretty much equate to the same thing. What is it about alloca that is objectionable? First, I've got bitten by alloca() before, when GCC misbehaved. Admitted, that was in some _very_ old version, but such experience sticks. Second, the behaviour of alloca() depends on a number of non-obvious settings and compiler flags, and results are not always obvious. rom the man page: Notes on the GNU Version Normally, gcc(1) translates calls to alloca() with inlined code. This is not done when either the -ansi, -std=c89, -std=c99, or the -fno-builtin option is given (and the header alloca.h is not included). But beware! By default the glibc version of stdlib.h includes alloca.h and that contains the line: #define alloca(size) __builtin_alloca (size) with messy consequences if one has a private version of this function. This is nothing I want to have in any production software. OK, you may argument that U-Boot has a strictly controlled environment, but anyway. Third, the man page says: NOTES The alloca() function is machine- and compiler-dependent. For certain applications, its use can improve efficiency compared to the use of malloc(3) plus free(3). In certain cases, it can also simplify memory deallocation in applications that use longjmp(3) or siglongjmp(3). Otherwise, its use is discouraged. In my opinion, U-Boot falls into the otherwise category... I think I agree with this not being a great solution. I do wonder if it would be useful to consider a dedicated buffer management API that could be used to allocate and free cache line aligned power of two buffers. Perhaps something like a buddy heap for simplicity. The longer I read this thread, the less frightening Mikes's macro becomes (compared tho the alternatives). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Though a program be but three lines long, someday it will have to be maintained. - The Tao of Programming ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On 08/29/2011 03:12 PM, Anton Staaf wrote:
1) Mikes's macro
#define DMA_ALIGN_SIZE(size) \
(((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_DECLARE_BUFFER(type, name, size) \
void __##name[DMA_ALIGN_SIZE(size * sizeof(type))]; \
type * name = __##name ~(CONFIG_SYS_CACHELINE_SIZE - 1));
DMA_DECLARE_BUFFER(int, buffer, 100);
This doesn't compile, and it tries to round the buffer down below its
starting point.
After fixing the more obvious issues, I get error: initializer element
is not constant.
There might be no way to express and-by-constant as a relocation.
You could set the pointer at runtime, though, and remove some of the
macrification:
#define DMA_ALIGN_SIZE(size) \
((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_ALIGN_ADDR(addr) \
(DMA_ALIGN_SIZE(addr) (CONFIG_SYS_CACHELINE_SIZE - 1))
int buffer_unaligned[DMA_ALIGN_SIZE(100)];
int *buffer;
some_init_func()
{
buffer = (int *)(DMA_ALIGN_ADDR((uintptr_t)buffer_unaligned));
}
3) Use GCC specific alignment attribute:
#define CACHLINE_ALIGNED __attribute__ ((aligned (CONFIG_SYS_CACHELINE_SIZE)))
int buffer[100] CACHELINE_ALIGNED;
Pros: The declaration of the buffer is even simpler and more obvious,
no use of alloca at all.
Cons: This doesn't work in any version of GCC before October 2010.
Meaning that it probably doesn't work in whatever compiler you're
using.
It's really too bad that this isn't a usable solution. I suppose that
we could switch to it at some point when we expect U-Boot to only be
compiled by versions of GCC that support this. By the way, the
failure mode here is pretty bad. If you compile the above code with
an older GCC it will silently fail to align the variable. :(
If the decision is made to depend on newer compilers, U-Boot could check
for it and #error out if the compiler is too old (possibly just in the
files that depend on this feature).
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 29, 2011 at 1:47 PM, Scott Wood scottw...@freescale.com wrote:
On 08/29/2011 03:12 PM, Anton Staaf wrote:
1) Mikes's macro
#define DMA_ALIGN_SIZE(size) \
(((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_DECLARE_BUFFER(type, name, size) \
void __##name[DMA_ALIGN_SIZE(size * sizeof(type))]; \
type * name = __##name ~(CONFIG_SYS_CACHELINE_SIZE - 1));
DMA_DECLARE_BUFFER(int, buffer, 100);
This doesn't compile, and it tries to round the buffer down below its
starting point.
You are correct. I wrote that one as a modification of mikes initial
proposal. I should have caught the incorrect rounding when I did.
The patch that Lukasz sent titled dcache: Dcache line size aligned
stack buffer allocation has a correct implementation.
After fixing the more obvious issues, I get error: initializer element
is not constant.
I think this requires the use of -std=c99 or GCC extensions. More
specifics can be found here:
http://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
There might be no way to express and-by-constant as a relocation.
You could set the pointer at runtime, though, and remove some of the
macrification:
#define DMA_ALIGN_SIZE(size) \
((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_ALIGN_ADDR(addr) \
(DMA_ALIGN_SIZE(addr) (CONFIG_SYS_CACHELINE_SIZE - 1))
int buffer_unaligned[DMA_ALIGN_SIZE(100)];
int *buffer;
some_init_func()
{
buffer = (int *)(DMA_ALIGN_ADDR((uintptr_t)buffer_unaligned));
}
:) This was one of my suggestions earlier on a different thread. It
was rejected there, I believe because it makes things less clear.
3) Use GCC specific alignment attribute:
#define CACHLINE_ALIGNED __attribute__ ((aligned
(CONFIG_SYS_CACHELINE_SIZE)))
int buffer[100] CACHELINE_ALIGNED;
Pros: The declaration of the buffer is even simpler and more obvious,
no use of alloca at all.
Cons: This doesn't work in any version of GCC before October 2010.
Meaning that it probably doesn't work in whatever compiler you're
using.
It's really too bad that this isn't a usable solution. I suppose that
we could switch to it at some point when we expect U-Boot to only be
compiled by versions of GCC that support this. By the way, the
failure mode here is pretty bad. If you compile the above code with
an older GCC it will silently fail to align the variable. :(
If the decision is made to depend on newer compilers, U-Boot could check
for it and #error out if the compiler is too old (possibly just in the
files that depend on this feature).
Yup, I think whatever macro we come up with we can simplify it
eventually using the GCC attribute solution and we can make the macro
conditional on compiler version.
Thanks,
Anton
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 29, 2011 at 1:35 PM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message caf6fiowhhqbvqwwprusb69s2mpfbtoiazxc8x56bkogjmzx...@mail.gmail.com you wrote: I would like to know, mainly for my education, why you do not want alloca, but are OK with dynamic array sizing (as in the first solution above). My understanding is that they pretty much equate to the same thing. What is it about alloca that is objectionable? First, I've got bitten by alloca() before, when GCC misbehaved. Admitted, that was in some _very_ old version, but such experience sticks. Very fair, I have programming constructs I avoid for the same reason. *cough* exceptions *cough* Second, the behaviour of alloca() depends on a number of non-obvious settings and compiler flags, and results are not always obvious. rom the man page: Notes on the GNU Version Normally, gcc(1) translates calls to alloca() with inlined code. This is not done when either the -ansi, -std=c89, -std=c99, or the -fno-builtin option is given (and the header alloca.h is not included). But beware! By default the glibc version of stdlib.h includes alloca.h and that contains the line: #define alloca(size) __builtin_alloca (size) with messy consequences if one has a private version of this function. This is nothing I want to have in any production software. OK, you may argument that U-Boot has a strictly controlled environment, but anyway. Yes, I don't expect that we will allow a custom implementation of alloca into U-Boot, but it's a good point that the compiler treats it in strange ways. One alternative here would be to always call __builtin_alloca... This would probably have other issues though. Third, the man page says: NOTES The alloca() function is machine- and compiler-dependent. For certain applications, its use can improve efficiency compared to the use of malloc(3) plus free(3). In certain cases, it can also simplify memory deallocation in applications that use longjmp(3) or siglongjmp(3). Otherwise, its use is discouraged. In my opinion, U-Boot falls into the otherwise category... Yup, and another man page in the Bugs section says: The alloca() function is machine and compiler dependent. On many systems its implementation is buggy. Its use is discouraged. Which seems even worse. So, OK, I think we've run this to the ground. And we can always find and replace all of the uses of Mikes macro easily if we think of something better later. Thank you, Anton I think I agree with this not being a great solution. I do wonder if it would be useful to consider a dedicated buffer management API that could be used to allocate and free cache line aligned power of two buffers. Perhaps something like a buddy heap for simplicity. The longer I read this thread, the less frightening Mikes's macro becomes (compared tho the alternatives). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Though a program be but three lines long, someday it will have to be maintained. - The Tao of Programming ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On 08/29/2011 03:58 PM, Anton Staaf wrote:
On Mon, Aug 29, 2011 at 1:47 PM, Scott Wood scottw...@freescale.com wrote:
On 08/29/2011 03:12 PM, Anton Staaf wrote:
1) Mikes's macro
#define DMA_ALIGN_SIZE(size) \
(((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_DECLARE_BUFFER(type, name, size) \
void __##name[DMA_ALIGN_SIZE(size * sizeof(type))]; \
type * name = __##name ~(CONFIG_SYS_CACHELINE_SIZE - 1));
DMA_DECLARE_BUFFER(int, buffer, 100);
This doesn't compile, and it tries to round the buffer down below its
starting point.
You are correct. I wrote that one as a modification of mikes initial
proposal. I should have caught the incorrect rounding when I did.
The patch that Lukasz sent titled dcache: Dcache line size aligned
stack buffer allocation has a correct implementation.
With the version in that patch I get the slightly different error:
initializer element is not computable at load time. Seems like whether
you cast the address to (type *) or (void *) determines which error you
get. This is with GCC 4.5.1 (powerpc) and 4.6.0 (x86). Maybe it's
arch-dependent, based on available relocation types.
Also, shouldn't the array be of type char rather than char *?
How do you make the declaration static?
After fixing the more obvious issues, I get error: initializer element
is not constant.
I think this requires the use of -std=c99 or GCC extensions. More
specifics can be found here:
http://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
-std=c99 doesn't help.
The problem isn't the array itself, it's the pointer initializer.
You could set the pointer at runtime, though, and remove some of the
macrification:
#define DMA_ALIGN_SIZE(size) \
((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_ALIGN_ADDR(addr) \
(DMA_ALIGN_SIZE(addr) (CONFIG_SYS_CACHELINE_SIZE - 1))
int buffer_unaligned[DMA_ALIGN_SIZE(100)];
int *buffer;
some_init_func()
{
buffer = (int *)(DMA_ALIGN_ADDR((uintptr_t)buffer_unaligned));
}
:) This was one of my suggestions earlier on a different thread. It
was rejected there, I believe because it makes things less clear.
So, the complex macro is bad because it obscures things, and this
version is bad because it doesn't? :-)
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 29, 2011 at 2:23 PM, Scott Wood scottw...@freescale.com wrote:
On 08/29/2011 03:58 PM, Anton Staaf wrote:
On Mon, Aug 29, 2011 at 1:47 PM, Scott Wood scottw...@freescale.com wrote:
On 08/29/2011 03:12 PM, Anton Staaf wrote:
1) Mikes's macro
#define DMA_ALIGN_SIZE(size) \
(((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_DECLARE_BUFFER(type, name, size) \
void __##name[DMA_ALIGN_SIZE(size * sizeof(type))]; \
type * name = __##name ~(CONFIG_SYS_CACHELINE_SIZE - 1));
DMA_DECLARE_BUFFER(int, buffer, 100);
This doesn't compile, and it tries to round the buffer down below its
starting point.
You are correct. I wrote that one as a modification of mikes initial
proposal. I should have caught the incorrect rounding when I did.
The patch that Lukasz sent titled dcache: Dcache line size aligned
stack buffer allocation has a correct implementation.
With the version in that patch I get the slightly different error:
initializer element is not computable at load time. Seems like whether
you cast the address to (type *) or (void *) determines which error you
get. This is with GCC 4.5.1 (powerpc) and 4.6.0 (x86). Maybe it's
arch-dependent, based on available relocation types.
Also, shouldn't the array be of type char rather than char *?
Yes, you are correct, it should be a char. That may be the problem.
How do you make the declaration static?
you can't with this version of the macro. Are there cases where you
need the buffer to be static?
Thanks,
Anton
After fixing the more obvious issues, I get error: initializer element
is not constant.
I think this requires the use of -std=c99 or GCC extensions. More
specifics can be found here:
http://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
-std=c99 doesn't help.
The problem isn't the array itself, it's the pointer initializer.
You could set the pointer at runtime, though, and remove some of the
macrification:
#define DMA_ALIGN_SIZE(size) \
((size) + CONFIG_SYS_CACHELINE_SIZE - 1)
#define DMA_ALIGN_ADDR(addr) \
(DMA_ALIGN_SIZE(addr) (CONFIG_SYS_CACHELINE_SIZE - 1))
int buffer_unaligned[DMA_ALIGN_SIZE(100)];
int *buffer;
some_init_func()
{
buffer = (int *)(DMA_ALIGN_ADDR((uintptr_t)buffer_unaligned));
}
:) This was one of my suggestions earlier on a different thread. It
was rejected there, I believe because it makes things less clear.
So, the complex macro is bad because it obscures things, and this
version is bad because it doesn't? :-)
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On 08/29/2011 04:54 PM, Anton Staaf wrote: On Mon, Aug 29, 2011 at 2:23 PM, Scott Wood scottw...@freescale.com wrote: With the version in that patch I get the slightly different error: initializer element is not computable at load time. Seems like whether you cast the address to (type *) or (void *) determines which error you get. This is with GCC 4.5.1 (powerpc) and 4.6.0 (x86). Maybe it's arch-dependent, based on available relocation types. Also, shouldn't the array be of type char rather than char *? Yes, you are correct, it should be a char. That may be the problem. It didn't make a difference. How do you make the declaration static? you can't with this version of the macro. Are there cases where you need the buffer to be static? I think you'd want it to be static more often than not. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 29, 2011 at 3:03 PM, Scott Wood scottw...@freescale.com wrote: On 08/29/2011 04:54 PM, Anton Staaf wrote: On Mon, Aug 29, 2011 at 2:23 PM, Scott Wood scottw...@freescale.com wrote: With the version in that patch I get the slightly different error: initializer element is not computable at load time. Seems like whether you cast the address to (type *) or (void *) determines which error you get. This is with GCC 4.5.1 (powerpc) and 4.6.0 (x86). Maybe it's arch-dependent, based on available relocation types. Also, shouldn't the array be of type char rather than char *? Yes, you are correct, it should be a char. That may be the problem. It didn't make a difference. Hmm, I haven't played with this code yet, so I can't say for sure. But it would certainly throw a wrench in the works if Mike's solution was also unavailable on some platforms. By chance, you aren't also adding static here are you? I could certainly see that that wouldn't work. How do you make the declaration static? you can't with this version of the macro. Are there cases where you need the buffer to be static? I think you'd want it to be static more often than not. If the buffer is allocated at file scope, then yes, we would want it to be static. But not at function scope. It would no longer be allocated on the stack in that case, and that was frowned upon earlier. Thanks, Anton -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On 08/29/2011 05:49 PM, Anton Staaf wrote: How do you make the declaration static? you can't with this version of the macro. Are there cases where you need the buffer to be static? I think you'd want it to be static more often than not. If the buffer is allocated at file scope, then yes, we would want it to be static. But not at function scope. It would no longer be allocated on the stack in that case, and that was frowned upon earlier. Ah, that's the issue. I was trying to declare it at file scope. :-P -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 29, 2011 at 4:01 PM, Scott Wood scottw...@freescale.com wrote: On 08/29/2011 05:49 PM, Anton Staaf wrote: How do you make the declaration static? you can't with this version of the macro. Are there cases where you need the buffer to be static? I think you'd want it to be static more often than not. If the buffer is allocated at file scope, then yes, we would want it to be static. But not at function scope. It would no longer be allocated on the stack in that case, and that was frowned upon earlier. Ah, that's the issue. I was trying to declare it at file scope. :-P Thank goodness, I was trying to figure out how that couldn't be a valid GCC complaint. :) Thanks, Anton -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Hi, On Tue, 23 Aug 2011 23:00:59 -0400 Mike Frysinger vap...@gentoo.org wrote: On Tuesday, August 23, 2011 18:42:46 Wolfgang Denk wrote: Mike Frysinger wrote: Why cannot we define a macro that declares a (sufficiently sized) buffer on the stack and provides and a pointer to a (correctly aligned) address in this buffer? isnt that what i already posted and you NAK-ed ? :) DMA_DECLARE_BUFFER(...) I just NAKed this specific implementation. well, it's hard to come up with a good one without knowing the parameters to work within ;). i'm not stuck on any particular implementation, i just want the helper to be simple to use and hard to screw up. the trouble with doing something like: char foo[func_to_do_round(size)]; is that size is not in # of bytes but is number of elements. so the point of my (i dont deny) complicated cpp logic was so that the internal implementation took on more of the work leaving the user (which we all know can easily mess things up) with a very simple macro: DMA_DECLARE_BUFFER(buffer type, variable name, num elements); -mike After reading dcache related threads I'd like to sum them up: 1. alloca() - not acceptable to u-boot mainline by Wolfgang. I agree that alloca is NOT the way to go. 2. malloc/memalign - avoidable to use 3. Mike's DMA_DECLARE_BUFFER(buffer type, variable name, size in bytes) solution looks OK for me, at least for the stack allocated data (e.g. ext_csd). However this proposed implementation has been NAK'ed by Wolfgang. I'm curious how this macro should look like? Is it only matter of code reordering or other approach shall be found? 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. However, I think, that proposed get_dcache_line_size() implementation ( http://patchwork.ozlabs.org/patch/111048/ ) is more programmer friendly (one don't need to exactly know the dcache line size on a particular SoC). -- Best regards, Lukasz Majewski Samsung Poland RD Center Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Lukasz Majewski, In message 20110824120744.097ba2c5@lmajewski.digital.local you wrote: After reading dcache related threads I'd like to sum them up: 1. alloca() - not acceptable to u-boot mainline by Wolfgang. I agree that alloca is NOT the way to go. 2. malloc/memalign - avoidable to use 3. Mike's DMA_DECLARE_BUFFER(buffer type, variable name, size in bytes) solution looks OK for me, at least for the stack allocated data (e.g. ext_csd). However this proposed implementation has been NAK'ed by Wolfgang. I'm curious how this macro should look like? Is it only matter of code reordering or other approach shall be found? I think I'd like to see a macro that can be used like this: void *dma_buffer_pointer = DMA_BUFFER(size_in_bytes); 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You know, after a woman's raised a family and so on, she wants to start living her own life. Whose life she's _been_ living, then? - Terry Pratchett, _Witches Abroad_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Hi Wolfgang,
I think I'd like to see a macro that can be used like this:
void *dma_buffer_pointer = DMA_BUFFER(size_in_bytes);
Frankly speaking I don't know any easy way to define buffer this way
in a macro (at least without digging deep enough to C preprocessor
programming tricks). Maybe Mike or Anton knows.
The void *dma_buffer_pointer = DMA_BUFFER(size_in_bytes); approach
needs to do following things in macro:
#define DMA_BUFFER(100) \
char buf[100 + get_dcache_line_size]; \
unsigned long tmp = (unsigned long) buf; \
void* buf_out = (void*) ((tmp + (get_dcache_line_size() - 1))
~(get_dcache_line_size() - 1))
The problem here is to assign the buf_out pointer to the void
*dma_buffer_pointer. How can we return the void *buf_out?
For comparison please look to this solution:
#define ALIGN_ADDR(addr) ((void *)(((unsigned long) addr +
get_dcache_line_size() - 1)\ ~(get_dcache_line_size() - 1)))
#define DMA_DECLARE_BUFFER(type, name, size) \
char *__##name[size + get_dcache_line_size()]; \
type *name = ALIGN_ADDR(__##name);
int mmc_startup(struct mmc *mmc)
{
/* Some declarations */
/* char ext_csd[512]; */
DMA_DECLARE_BUFFER(char, ext_csd, 512);
printf(%s: ptr:%p\n, __func__, ext_csd);
/* rest of the code */
}
Here the DMA_DECLARE_BUFFER really defines the buffer as an automatic
variable with this function scope. This is tested and works :-)
4. get_dcache_line_size() can be simply defined as
#define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we
_really_ want to save a few bytes.
Actually I fail to understand why we would ever need
get_dcache_line_size() in a boot loader.
If I can ask for clarification here.
Shall not u-boot read on fly the cache line size (as it is now done) or
you don't like the get_cache_line_size defined as function?
Going further shall the get_cache_line_size be removed completely and
replaced with CONFIG_SYS_CACHE_LINE_SIZE?
Best regards,
Wolfgang Denk
--
Best regards,
Lukasz Majewski
Samsung Poland RD Center
Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 17:48:50 Anton Staaf wrote:
I wasn't going to say it. :) How about something like this, which is
very similar to what you had Mike, but doesn't define the array in the
macro. It's a bit clearer what is going on, but also requires a bit
more work at each use.
#define DCACHE_RESIZE(size) ((size) + get_dcache_line_size() - 1)
#define DCACHE_ALIGN(buffer) ((buffer) + get_dcache_line_size() - 1)
~(get_dcache_line_size() - 1)
char buffer[DCACHE_RESIZE(100)];
as long as people always use a byte-sized type (i.e. char), this should work.
obviously using u32 buffer[...] will be bad.
It would be awesome if the idea below worked, but it can't because the
array is popped when the ({...}) scope is exited I believe.
yes, i believe you are correct.
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wednesday, August 24, 2011 09:25:53 Wolfgang Denk wrote:
Lukasz Majewski wrote:
3. Mike's DMA_DECLARE_BUFFER(buffer type, variable name,
size in bytes)
solution looks OK for me, at least for the stack allocated data (e.g.
ext_csd).
However this proposed implementation has been NAK'ed by Wolfgang.
I'm curious how this macro should look like? Is it only matter of code
reordering or other approach shall be found?
I think I'd like to see a macro that can be used like this:
void *dma_buffer_pointer = DMA_BUFFER(size_in_bytes);
as Anton highlighted, i'm not sure this is possible. DMA_BUFFER() would have
to be defined with ({...}), and any storage declared in there is in new scope,
so as soon as you exit that scope, any storage declared is invalid for use
outside of it. the only thing that is good for is producing a single value
(i.e. an int or char or a pointer etc..., but not storage).
if a person who knows more about gcc internals in this regard could comment,
that'd be great :).
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wed, Aug 24, 2011 at 6:25 AM, Wolfgang Denk w...@denx.de wrote: Dear Lukasz Majewski, In message 20110824120744.097ba2c5@lmajewski.digital.local you wrote: After reading dcache related threads I'd like to sum them up: 1. alloca() - not acceptable to u-boot mainline by Wolfgang. I agree that alloca is NOT the way to go. 2. malloc/memalign - avoidable to use 3. Mike's DMA_DECLARE_BUFFER(buffer type, variable name, size in bytes) solution looks OK for me, at least for the stack allocated data (e.g. ext_csd). However this proposed implementation has been NAK'ed by Wolfgang. I'm curious how this macro should look like? Is it only matter of code reordering or other approach shall be found? I think I'd like to see a macro that can be used like this: void *dma_buffer_pointer = DMA_BUFFER(size_in_bytes); 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these buffers need to be cache line size aligned is because unaligned cache invalidates are not possible to do in a safe way. The problem is that invalidating a partial cache line requires invalidating the entire line. And the other part of the line can contain nearby variables (especially if the buffer is stack allocated), so we have to first flush the cache line to be safe. However, that flush will clobber the values that the DMA engine wrote to main memory that are not reflected in the cache. Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You know, after a woman's raised a family and so on, she wants to start living her own life. Whose life she's _been_ living, then? - Terry Pratchett, _Witches Abroad_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wednesday, August 24, 2011 13:27:05 Anton Staaf wrote: On Wed, Aug 24, 2011 at 6:25 AM, Wolfgang Denk wrote: Lukasz Majewski wrote: 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these buffers need to be cache line size aligned is because unaligned cache invalidates are not possible to do in a safe way. The problem is that invalidating a partial cache line requires invalidating the entire line. And the other part of the line can contain nearby variables (especially if the buffer is stack allocated), so we have to first flush the cache line to be safe. However, that flush will clobber the values that the DMA engine wrote to main memory that are not reflected in the cache. right, and that's why i want to hide it be hind a dma buffer allocate API so that people can just say i want a dma safe buffer rather than having to delve into these details. they'll inevitable get confused and screw it up. ;) -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Anton Staaf, In message CAF6FioWBsjFa+QdFNHEQGO50eLoqZMc--Yt3-iYX6DWr=hc...@mail.gmail.com you wrote: 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Brain: an apparatus with which we think we think.- Ambrose Bierce ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wed, Aug 24, 2011 at 11:12 AM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message CAF6FioWBsjFa+QdFNHEQGO50eLoqZMc--Yt3-iYX6DWr=hc...@mail.gmail.com you wrote: 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Ahh, I misunderstood your question. I thought you were asking about the need to know the cache line size. Not it's specific implementation as a function call. In which case, I agree, and am happy to use CONFIG_SYS_CACHE_LINE_SIZE, though I don't see any definitions of CONFIG_SYS_CACHE_LINE_SIZE in the entire U-Boot tree. What have I missed? Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Brain: an apparatus with which we think we think. - Ambrose Bierce ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Anton Staaf, In message caf6fiowm6mezmmdr6+i8tgolg-en4tihtocaupbh0o7vqha...@mail.gmail.com you wrote: No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Ahh, I misunderstood your question. I thought you were asking about the need to know the cache line size. Not it's specific implementation as a function call. In which case, I agree, and am happy to use CONFIG_SYS_CACHE_LINE_SIZE, though I don't see any definitions of CONFIG_SYS_CACHE_LINE_SIZE in the entire U-Boot tree. What have I missed? I copy pasted the name, incorrectly. It's CONFIG_SYS_CACHELINE_SIZE You will find this being used: - grep -R CONFIG_SYS_CACHELINE_SIZE * | wc -l 276 Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are certain things men must do to remain men. -- Kirk, The Ultimate Computer, stardate 4929.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wed, 24 Aug 2011 11:25:42 -0700 Anton Staaf robot...@google.com wrote: On Wed, Aug 24, 2011 at 11:12 AM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message CAF6FioWBsjFa+QdFNHEQGO50eLoqZMc--Yt3-iYX6DWr=hc...@mail.gmail.com you wrote: 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Ok, so one problem solved :-). Ahh, I misunderstood your question. I thought you were asking about the need to know the cache line size. Not it's specific implementation as a function call. In which case, I agree, and am happy to use CONFIG_SYS_CACHE_LINE_SIZE, though I don't see any definitions of CONFIG_SYS_CACHE_LINE_SIZE in the entire U-Boot tree. What have I missed? There are some similar definitions: ./include/configs/atstk1006.h:#define CONFIG_SYS_DCACHE_LINESZ 32 ./include/configs/atngw100.h:#define CONFIG_SYS_DCACHE_LINESZ 32 ./include/configs/favr-32-ezkit.h:#define CONFIG_SYS_DCACHE_LINESZ 32 Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Brain: an apparatus with which we think we think. - Ambrose Bierce ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot Regards, Lukasz Majewski ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wed, Aug 24, 2011 at 12:04 PM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message caf6fiowm6mezmmdr6+i8tgolg-en4tihtocaupbh0o7vqha...@mail.gmail.com you wrote: No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Ahh, I misunderstood your question. I thought you were asking about the need to know the cache line size. Not it's specific implementation as a function call. In which case, I agree, and am happy to use CONFIG_SYS_CACHE_LINE_SIZE, though I don't see any definitions of CONFIG_SYS_CACHE_LINE_SIZE in the entire U-Boot tree. What have I missed? I copy pasted the name, incorrectly. It's CONFIG_SYS_CACHELINE_SIZE You will find this being used: - grep -R CONFIG_SYS_CACHELINE_SIZE * | wc -l 276 :) Hah, thanks. I tried a number of permutations, but never the right one. -Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are certain things men must do to remain men. -- Kirk, The Ultimate Computer, stardate 4929.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Wed, Aug 24, 2011 at 12:18 PM, Lukasz Majewski majess1...@gmail.com wrote: On Wed, 24 Aug 2011 11:25:42 -0700 Anton Staaf robot...@google.com wrote: On Wed, Aug 24, 2011 at 11:12 AM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message CAF6FioWBsjFa+QdFNHEQGO50eLoqZMc--Yt3-iYX6DWr=hc...@mail.gmail.com you wrote: 4. get_dcache_line_size() can be simply defined as #define get_dcache_line_size() CONFIG_SYS_CACHE_LINE_SIZE if we _really_ want to save a few bytes. Actually I fail to understand why we would ever need get_dcache_line_size() in a boot loader. It is required so that we can correctly allocate buffers that will be used by DMA engines to read or write data. The reason that these No, it is definitely NOT needed for this purpose - we have been using CONFIG_SYS_CACHE_LINE_SIZE for more than a decade without problems, so I don't really understand why we now would need a new function for this? Ok, so one problem solved :-). Ahh, I misunderstood your question. I thought you were asking about the need to know the cache line size. Not it's specific implementation as a function call. In which case, I agree, and am happy to use CONFIG_SYS_CACHE_LINE_SIZE, though I don't see any definitions of CONFIG_SYS_CACHE_LINE_SIZE in the entire U-Boot tree. What have I missed? There are some similar definitions: ./include/configs/atstk1006.h:#define CONFIG_SYS_DCACHE_LINESZ 32 ./include/configs/atngw100.h:#define CONFIG_SYS_DCACHE_LINESZ 32 ./include/configs/favr-32-ezkit.h:#define CONFIG_SYS_DCACHE_LINESZ 32 I would assume that these should be changed to the one mentioned by Wolfgang. But this still leaves us with a question of how to make a simple, easy to use macro for allocating cache line size aligned stack buffers. I'll work on that some more and see if I can come up with something. Thanks, Anton Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Brain: an apparatus with which we think we think. - Ambrose Bierce ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot Regards, Lukasz Majewski ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Hi Mike, On Mon, 22 Aug 2011 12:08:42 -0400 Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 03:29:52 Lukasz Majewski wrote: On Fri, 19 Aug 2011 11:35:50 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 11:28:18 Lukasz Majewski wrote: On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: also, what is the code size increase with your patch ? Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B np if it gives significant (more than system noise) speedups. any details on that ? No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). so you're papering over a bug in some controller's cache handling ? shouldnt you fix the controller in question by having it flush its caches ? aligning random buffers to make cache issues go away isnt the right way for anything. -mike Please look into the following patch: http://patchwork.ozlabs.org/patch/110576/ It seems that only flushing/invalidating buffers is not enough. -- Best regards, Lukasz Majewski Samsung Poland RD Center Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Hi Anton, On Mon, 22 Aug 2011 11:57:57 -0700 Anton Staaf robot...@google.com wrote: drivers/mmc/mmc.c: ext_csd in mmc_change_freq is allocated on the stac drivers/mmc/mmc.c: scr and switch_status in sd_change_freq are allocated on the stack. drivers/mmc/mmc.c: ext_csd in mmc_startup is allocated on the stack. This allocations are already fixed: http://patchwork.ozlabs.org/patch/110300/ http://patchwork.ozlabs.org/patch/109790/ If any doubts/comments/ideas, please let me know :-) But that's probably not right. I should probably send them now so we can see a number of examples of the problem and get a better idea of how to fix it. Discussion is always welcome. -- Best regards, Lukasz Majewski Samsung Poland RD Center Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tue, Aug 23, 2011 at 2:19 AM, Lukasz Majewski l.majew...@samsung.com wrote: Hi Anton, On Mon, 22 Aug 2011 11:57:57 -0700 Anton Staaf robot...@google.com wrote: drivers/mmc/mmc.c: ext_csd in mmc_change_freq is allocated on the stac drivers/mmc/mmc.c: scr and switch_status in sd_change_freq are allocated on the stack. drivers/mmc/mmc.c: ext_csd in mmc_startup is allocated on the stack. This allocations are already fixed: http://patchwork.ozlabs.org/patch/110300/ http://patchwork.ozlabs.org/patch/109790/ Yup, should have said that you'd already taken care of some of these. I'll send a patch now for another unaligned buffer in the mmc code that isn't in your patch set. Thanks, Anton If any doubts/comments/ideas, please let me know :-) But that's probably not right. I should probably send them now so we can see a number of examples of the problem and get a better idea of how to fix it. Discussion is always welcome. -- Best regards, Lukasz Majewski Samsung Poland RD Center Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 05:19:39 Lukasz Majewski wrote: On Mon, 22 Aug 2011 11:57:57 -0700 Anton Staaf wrote: drivers/mmc/mmc.c: ext_csd in mmc_change_freq is allocated on the stac drivers/mmc/mmc.c: scr and switch_status in sd_change_freq are allocated on the stack. drivers/mmc/mmc.c: ext_csd in mmc_startup is allocated on the stack. This allocations are already fixed: http://patchwork.ozlabs.org/patch/110300/ http://patchwork.ozlabs.org/patch/109790/ If any doubts/comments/ideas, please let me know :-) hmm, i wish we had a memalign_alloca(). and all this copy pasting of get_dcache_line_size() makes me unhappy as we're encoding too-low-of-a-level logic into funcs. what about adding a new func like: #define dma_buffer_alloca(size) and it would take care of allocating a big enough aligned buffer on the stack. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tue, Aug 23, 2011 at 10:30 AM, Mike Frysinger vap...@gentoo.org wrote: On Tuesday, August 23, 2011 05:19:39 Lukasz Majewski wrote: On Mon, 22 Aug 2011 11:57:57 -0700 Anton Staaf wrote: drivers/mmc/mmc.c: ext_csd in mmc_change_freq is allocated on the stac drivers/mmc/mmc.c: scr and switch_status in sd_change_freq are allocated on the stack. drivers/mmc/mmc.c: ext_csd in mmc_startup is allocated on the stack. This allocations are already fixed: http://patchwork.ozlabs.org/patch/110300/ http://patchwork.ozlabs.org/patch/109790/ If any doubts/comments/ideas, please let me know :-) hmm, i wish we had a memalign_alloca(). and all this copy pasting of get_dcache_line_size() makes me unhappy as we're encoding too-low-of-a-level logic into funcs. what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. But neither of these are really a problem for most U-Boot targets. Are you thinking something like: #define dma_buffer_alloca(size) alloca(size + get_dcache_line_size() - 1) ~(get_dcache_line_size() - 1); Subtracting one from the total allocated size is technically correct, but could fail poorly if the get_dcache_line_size function returned 0 for some reason. Perhaps because it's called on a target with no cache so the implementer figured 0 was as good as any value to return. I have a nagging suspicion that I'm forgetting something though. I know I looked at using alloca first when I was starting to deal with cache and DMA issues on the Tegra. And I seem to recall a reason not to use it. But it's not coming back to me now. Perhaps someone else will think of it. :) Thanks, Anton and it would take care of allocating a big enough aligned buffer on the stack. -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 14:12:09 Anton Staaf wrote: On Tue, Aug 23, 2011 at 10:30 AM, Mike Frysinger wrote: On Tuesday, August 23, 2011 05:19:39 Lukasz Majewski wrote: On Mon, 22 Aug 2011 11:57:57 -0700 Anton Staaf wrote: drivers/mmc/mmc.c: ext_csd in mmc_change_freq is allocated on the stac drivers/mmc/mmc.c: scr and switch_status in sd_change_freq are allocated on the stack. drivers/mmc/mmc.c: ext_csd in mmc_startup is allocated on the stack. This allocations are already fixed: http://patchwork.ozlabs.org/patch/110300/ http://patchwork.ozlabs.org/patch/109790/ If any doubts/comments/ideas, please let me know :-) hmm, i wish we had a memalign_alloca(). and all this copy pasting of get_dcache_line_size() makes me unhappy as we're encoding too-low-of-a-level logic into funcs. what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. But neither of these are really a problem for most U-Boot targets. and more importantly, we're already doing these things on the stack ;) Are you thinking something like: #define dma_buffer_alloca(size) alloca(size + get_dcache_line_size() - 1) ~(get_dcache_line_size() - 1); yes, exactly Subtracting one from the total allocated size is technically correct, but could fail poorly if the get_dcache_line_size function returned 0 for some reason. Perhaps because it's called on a target with no cache so the implementer figured 0 was as good as any value to return. i'm not sure we should cater to buggy get_dcache_line_size implementations I have a nagging suspicion that I'm forgetting something though. I know I looked at using alloca first when I was starting to deal with cache and DMA issues on the Tegra. And I seem to recall a reason not to use it. But it's not coming back to me now. Perhaps someone else will think of it. :) if the stack lives in a place that dma can't access, but that'd already be a problem for these funcs -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 14:12:09 Anton Staaf wrote: On Tue, Aug 23, 2011 at 10:30 AM, Mike Frysinger wrote: what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. oh, and that doesnt preclude also adding a dma_buffer_malloc(size). it's just that all the cases here are already on the stack, so i was focusing on that. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tue, Aug 23, 2011 at 11:36 AM, Mike Frysinger vap...@gentoo.org wrote: On Tuesday, August 23, 2011 14:12:09 Anton Staaf wrote: On Tue, Aug 23, 2011 at 10:30 AM, Mike Frysinger wrote: what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. oh, and that doesnt preclude also adding a dma_buffer_malloc(size). it's just that all the cases here are already on the stack, so i was focusing on that. -mike Yup, totally reasonable. Lukasz, would you like to add the above macro to your dcache cache line size patch? Then we can change our other patches to use it instead of memalign. Thanks, Anton ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Anton Staaf, In message CAF6FioXeFQP2a0VaZOVOTmxPvJxF4=kamS18=2=p6tfnefw...@mail.gmail.com you wrote: what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. But neither of these are really a problem for most U-Boot targets. Are you thinking something like: #define dma_buffer_alloca(size) alloca(size + get_dcache_line_size() - 1) ~(get_dcache_line_size() - 1); I don't think I will accept any alloca() based code into mainline. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are three principal ways to lose money: wine, women, and en- gineers. While the first two are more pleasant, the third is by far the more certain. -- Baron Rothschild, ca. 1800 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tue, Aug 23, 2011 at 1:12 PM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message CAF6FioXeFQP2a0VaZOVOTmxPvJxF4=kamS18=2=p6tfnefw...@mail.gmail.com you wrote: what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. But neither of these are really a problem for most U-Boot targets. Are you thinking something like: #define dma_buffer_alloca(size) alloca(size + get_dcache_line_size() - 1) ~(get_dcache_line_size() - 1); I don't think I will accept any alloca() based code into mainline. Ahh, that must have been my reason for not using it in the first place, a premonition. :) So then, to guide our efforts, what is a more suitable solution? Would you prefer we stick with the existing path of calling memalign and passing it the cache size by directly calling get_dcache_line_size? Or would you prefer something more like a dma_buffer_malloc function that allocates on the heap a cache line size aligned buffer and returns it? Hmm, that will require a bit of thought because we need to recover the original pointer returned by malloc to cleanly free the buffer later. Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are three principal ways to lose money: wine, women, and en- gineers. While the first two are more pleasant, the third is by far the more certain. -- Baron Rothschild, ca. 1800 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 16:12:03 Wolfgang Denk wrote: Anton Staaf wrote: what about adding a new func like: #define dma_buffer_alloca(size) I generally avoid large allocations on the stack, they can confuse virtual stack management and blow out small embedded stacks. But neither of these are really a problem for most U-Boot targets. Are you thinking something like: #define dma_buffer_alloca(size) alloca(size + get_dcache_line_size() - 1) ~(get_dcache_line_size() - 1); I don't think I will accept any alloca() based code into mainline. hmm, for some reason i thought we were using alloca. but i dont see any consumers in u-boot itself. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 16:27:26 Anton Staaf wrote: So then, to guide our efforts, what is a more suitable solution? Would you prefer we stick with the existing path of calling memalign and passing it the cache size by directly calling get_dcache_line_size? Or would you prefer something more like a dma_buffer_malloc function that allocates on the heap a cache line size aligned buffer and returns it? memalign() is simply a malloc() with offset fudging, so dma_buffer_malloc() is the way to go imo. anything that involves end code having to figure out how to align things itself is asking for pain. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tue, Aug 23, 2011 at 1:37 PM, Mike Frysinger vap...@gentoo.org wrote: On Tuesday, August 23, 2011 16:27:26 Anton Staaf wrote: So then, to guide our efforts, what is a more suitable solution? Would you prefer we stick with the existing path of calling memalign and passing it the cache size by directly calling get_dcache_line_size? Or would you prefer something more like a dma_buffer_malloc function that allocates on the heap a cache line size aligned buffer and returns it? memalign() is simply a malloc() with offset fudging, so dma_buffer_malloc() is the way to go imo. anything that involves end code having to figure out how to align things itself is asking for pain. Indeed, I had temporarily forgotten about memalign it seems. :/ Does it make more sense to put such a function into Lukasz's patch, or a separate patch? Thanks, Anton -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Mike Frysinger, In message 201108231637.05845.vap...@gentoo.org you wrote: On Tuesday, August 23, 2011 16:27:26 Anton Staaf wrote: So then, to guide our efforts, what is a more suitable solution? Would you prefer we stick with the existing path of calling memalign and passing it the cache size by directly calling get_dcache_line_size? Or would you prefer something more like a dma_buffer_malloc function that allocates on the heap a cache line size aligned buffer and returns it? memalign() is simply a malloc() with offset fudging, so dma_buffer_malloc() is the way to go imo. anything that involves end code having to figure out how to align things itself is asking for pain. I would like to avoid using any malloc code here. We have to keep in mind that such code changes will spread, and will be copied into driver code, file systems, etc. which might be used (and even required, for example for NAND or SDCard booting systems) before relocation - but malloc becomes available only after relocation. Why cannot we define a macro that declares a (sufficiently sized) buffer on the stack and provides and a pointer to a (correctly aligned) address in this buffer? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are three ways to get something done: (1) Do it yourself. (2) Hire someone to do it for you. (3) Forbid your kids to do it. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 17:06:41 Anton Staaf wrote: On Tue, Aug 23, 2011 at 1:37 PM, Mike Frysinger vap...@gentoo.org wrote: On Tuesday, August 23, 2011 16:27:26 Anton Staaf wrote: So then, to guide our efforts, what is a more suitable solution? Would you prefer we stick with the existing path of calling memalign and passing it the cache size by directly calling get_dcache_line_size? Or would you prefer something more like a dma_buffer_malloc function that allocates on the heap a cache line size aligned buffer and returns it? memalign() is simply a malloc() with offset fudging, so dma_buffer_malloc() is the way to go imo. anything that involves end code having to figure out how to align things itself is asking for pain. Indeed, I had temporarily forgotten about memalign it seems. :/ Does it make more sense to put such a function into Lukasz's patch, or a separate patch? Lukasz' havent been merged yet, so imo it makes more sense to put the sane framework in place and then fix the relevant code on top of that -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 17:09:37 Wolfgang Denk wrote: Mike Frysinger wrote: On Tuesday, August 23, 2011 16:27:26 Anton Staaf wrote: So then, to guide our efforts, what is a more suitable solution? Would you prefer we stick with the existing path of calling memalign and passing it the cache size by directly calling get_dcache_line_size? Or would you prefer something more like a dma_buffer_malloc function that allocates on the heap a cache line size aligned buffer and returns it? memalign() is simply a malloc() with offset fudging, so dma_buffer_malloc() is the way to go imo. anything that involves end code having to figure out how to align things itself is asking for pain. I would like to avoid using any malloc code here. We have to keep in mind that such code changes will spread, and will be copied into driver code, file systems, etc. which might be used (and even required, for example for NAND or SDCard booting systems) before relocation - but malloc becomes available only after relocation. Why cannot we define a macro that declares a (sufficiently sized) buffer on the stack and provides and a pointer to a (correctly aligned) address in this buffer? isnt that what i already posted and you NAK-ed ? :) DMA_DECLARE_BUFFER(...) -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tue, Aug 23, 2011 at 2:32 PM, Mike Frysinger vap...@gentoo.org wrote:
On Tuesday, August 23, 2011 17:09:37 Wolfgang Denk wrote:
Mike Frysinger wrote:
On Tuesday, August 23, 2011 16:27:26 Anton Staaf wrote:
So then, to guide our efforts, what is a more suitable solution?
Would you prefer we stick with the existing path of calling memalign
and passing it the cache size by directly calling
get_dcache_line_size? Or would you prefer something more like a
dma_buffer_malloc function that allocates on the heap a cache line
size aligned buffer and returns it?
memalign() is simply a malloc() with offset fudging, so
dma_buffer_malloc() is the way to go imo. anything that involves end
code having to figure out how to align things itself is asking for pain.
I would like to avoid using any malloc code here. We have to keep in
mind that such code changes will spread, and will be copied into
driver code, file systems, etc. which might be used (and even
required, for example for NAND or SDCard booting systems) before
relocation - but malloc becomes available only after relocation.
Why cannot we define a macro that declares a (sufficiently sized)
buffer on the stack and provides and a pointer to a (correctly
aligned) address in this buffer?
isnt that what i already posted and you NAK-ed ? :)
DMA_DECLARE_BUFFER(...)
I wasn't going to say it. :) How about something like this, which is
very similar to what you had Mike, but doesn't define the array in the
macro. It's a bit clearer what is going on, but also requires a bit
more work at each use.
#define DCACHE_RESIZE(size) ((size) + get_dcache_line_size() - 1)
#define DCACHE_ALIGN(buffer) ((buffer) + get_dcache_line_size() - 1)
~(get_dcache_line_size() - 1)
char buffer[DCACHE_RESIZE(100)];
char * aligned = DCACHE_ALIGN(buffer);
It would be awesome if the idea below worked, but it can't because the
array is popped when the ({...}) scope is exited I believe.
#define allocate_dma_buffer_on_stack(size) \
({ \
char _dma_buffer[(size) + get_dcache_line_size() - 1]; \
\
_dma_buffer ~(get_dcache_line_size() - 1); \
})
And you could use it like:
char * buffer = allocate_dma_buffer_on_stack(100);
If anyone can think of a way to make this work that would be excellent...
Thanks,
Anton
-mike
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Dear Mike Frysinger, In message 201108231732.39791.vap...@gentoo.org you wrote: Why cannot we define a macro that declares a (sufficiently sized) buffer on the stack and provides and a pointer to a (correctly aligned) address in this buffer? isnt that what i already posted and you NAK-ed ? :) DMA_DECLARE_BUFFER(...) I just NAKed this specific implementation. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Vor allem kein Gedanke! Nichts ist kompromittierender als ein Gedan- ke!- Friedrich Wilhelm Nietzsche _Der Fall Wagner_ (1888) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Tuesday, August 23, 2011 18:42:46 Wolfgang Denk wrote: Mike Frysinger wrote: Why cannot we define a macro that declares a (sufficiently sized) buffer on the stack and provides and a pointer to a (correctly aligned) address in this buffer? isnt that what i already posted and you NAK-ed ? :) DMA_DECLARE_BUFFER(...) I just NAKed this specific implementation. well, it's hard to come up with a good one without knowing the parameters to work within ;). i'm not stuck on any particular implementation, i just want the helper to be simple to use and hard to screw up. the trouble with doing something like: char foo[func_to_do_round(size)]; is that size is not in # of bytes but is number of elements. so the point of my (i dont deny) complicated cpp logic was so that the internal implementation took on more of the work leaving the user (which we all know can easily mess things up) with a very simple macro: DMA_DECLARE_BUFFER(buffer type, variable name, num elements); -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Hi Mike, On Fri, 19 Aug 2011 11:35:50 -0400 Mike Frysinger vap...@gentoo.org wrote: On Friday, August 19, 2011 11:28:18 Lukasz Majewski wrote: On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: + cache_align_buf = memalign(get_dcache_line_size(), nowhere do i see get_dcache_line_size() defined Please look to the following post: http://patchwork.ozlabs.org/patch/110501/ and another related with this issue: http://patchwork.ozlabs.org/patch/110300/ if you're posting patches with dependencies, you need to mention them explicitly (below the --- area), or send proper patch series ([PATCH N/M]). ignoring that, this patch will break all arches except arm. that's bad mmmkay. you probably need to move that weak def out of arm's cache.c and into like lib/cache.c. Yes, I will prepare two patch series: One addressing the get_dcache_line_size function for all u-boot architectures. Another patch series will address changes to the drivers/mmc.c file. also, what is the code size increase with your patch ? Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B np if it gives significant (more than system noise) speedups. any details on that ? -mike No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). I'm particularly interested if the approach with memalign and get_dcache_line_size is the preferred way to go. I will think about some reliable ways to measure the MMC performance with enabled and disabled MMC subsystem. -- Best regards, Lukasz Majewski Samsung Poland RD Center Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Monday, August 22, 2011 03:29:52 Lukasz Majewski wrote: On Fri, 19 Aug 2011 11:35:50 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 11:28:18 Lukasz Majewski wrote: On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: also, what is the code size increase with your patch ? Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B np if it gives significant (more than system noise) speedups. any details on that ? No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). so you're papering over a bug in some controller's cache handling ? shouldnt you fix the controller in question by having it flush its caches ? aligning random buffers to make cache issues go away isnt the right way for anything. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 22, 2011 at 9:08 AM, Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 03:29:52 Lukasz Majewski wrote: On Fri, 19 Aug 2011 11:35:50 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 11:28:18 Lukasz Majewski wrote: On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: also, what is the code size increase with your patch ? Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B np if it gives significant (more than system noise) speedups. any details on that ? No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). so you're papering over a bug in some controller's cache handling ? shouldnt you fix the controller in question by having it flush its caches ? aligning random buffers to make cache issues go away isnt the right way for anything. -mike No, this isn't something that can be fixed in the controller driver code. This is a fundamental problem with buffers in U-Boot that needs to be resolved by aligning all buffers used for DMA. The main problem is that invalidating a non-cache line aligned buffer is not a safe operation. There have been a number of threads discussing this. The general consensus was to make attempting to invalidate an unaligned buffer an error and then to clean up the unaligned buffers as we find them. Lukasz, I also have been using memalign to clean up accesses in local patches, so you've got my vote there. I am curious as to whether we should provide a single block allocation API or if each subsection should add lazy memalign allocations to create aligned buffers when they are needed... Thanks, Anton ___ 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
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Monday, August 22, 2011 06:42:06 PM Anton Staaf wrote: On Mon, Aug 22, 2011 at 9:08 AM, Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 03:29:52 Lukasz Majewski wrote: On Fri, 19 Aug 2011 11:35:50 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 11:28:18 Lukasz Majewski wrote: On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: also, what is the code size increase with your patch ? Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B np if it gives significant (more than system noise) speedups. any details on that ? No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). so you're papering over a bug in some controller's cache handling ? shouldnt you fix the controller in question by having it flush its caches ? aligning random buffers to make cache issues go away isnt the right way for anything. -mike No, this isn't something that can be fixed in the controller driver code. This is a fundamental problem with buffers in U-Boot that needs to be resolved by aligning all buffers used for DMA. The main problem is that invalidating a non-cache line aligned buffer is not a safe operation. There have been a number of threads discussing this. The general consensus was to make attempting to invalidate an unaligned buffer an error and then to clean up the unaligned buffers as we find them. Lukasz, I also have been using memalign to clean up accesses in local patches, so you've got my vote there. I am curious as to whether we should provide a single block allocation API or if each subsection should add lazy memalign allocations to create aligned buffers when they are needed... Maybe some dma_allocate_aligned() would be cool. And probably control the alignment with some #define CONFIG PLATFORM_ALIGNMENT_SIZE ? Cheers Thanks, Anton ___ 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Monday, August 22, 2011 12:42:06 Anton Staaf wrote: On Mon, Aug 22, 2011 at 9:08 AM, Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 03:29:52 Lukasz Majewski wrote: No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). so you're papering over a bug in some controller's cache handling ? shouldnt you fix the controller in question by having it flush its caches ? aligning random buffers to make cache issues go away isnt the right way for anything. No, this isn't something that can be fixed in the controller driver code. This is a fundamental problem with buffers in U-Boot that needs to be resolved by aligning all buffers used for DMA. The main problem is that invalidating a non-cache line aligned buffer is not a safe operation. There have been a number of threads discussing this. The general consensus was to make attempting to invalidate an unaligned buffer an error and then to clean up the unaligned buffers as we find them. Linux has taken the approach of the callers providing dma safe buffers instead of having lower layers trying to fix things up all the time. and for obvious reasons: you avoid the double allocation, the memcpy, and you avoid unclear documentation between layers where multiple layers attempt to fixup the callers thus futher multiplying the duplicated allocations/copies/etc... so in this case, why not fix the caller that is passing a dma unaligned buffer down to the mmc code ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Yes, this would be a much preferable approach. The only problem that I encountered when attempting to do that in the Chromium U-Boot was that there are exposed stand alone U-Boot API's that can pass in pointers that make their way all the way down to the device driver. Two solutions occurred to me. One was to put an Error or assert as high up as possible and expect violators to fix their code, the other was to patch things up in lower layers (right now we have a bounce buffer in the tegra MMC driver to deal with unaligned buffers). My intent was that this would give us a functional U-Boot while we took the time to clean up as many of the unaligned buffers that U-Boot uses, and then tackle the question of stand alone applications. -Anton On Mon, Aug 22, 2011 at 10:17 AM, Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 12:42:06 Anton Staaf wrote: On Mon, Aug 22, 2011 at 9:08 AM, Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 03:29:52 Lukasz Majewski wrote: No tests performed yet. The goal of those patches is to preserve the MMC subsystem functionality when dcache is enabled (the ext_csd[512] corruption is observed with d-cache enabled). so you're papering over a bug in some controller's cache handling ? shouldnt you fix the controller in question by having it flush its caches ? aligning random buffers to make cache issues go away isnt the right way for anything. No, this isn't something that can be fixed in the controller driver code. This is a fundamental problem with buffers in U-Boot that needs to be resolved by aligning all buffers used for DMA. The main problem is that invalidating a non-cache line aligned buffer is not a safe operation. There have been a number of threads discussing this. The general consensus was to make attempting to invalidate an unaligned buffer an error and then to clean up the unaligned buffers as we find them. Linux has taken the approach of the callers providing dma safe buffers instead of having lower layers trying to fix things up all the time. and for obvious reasons: you avoid the double allocation, the memcpy, and you avoid unclear documentation between layers where multiple layers attempt to fixup the callers thus futher multiplying the duplicated allocations/copies/etc... so in this case, why not fix the caller that is passing a dma unaligned buffer down to the mmc code ? -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Monday, August 22, 2011 14:15:22 Anton Staaf wrote: please dont top post Yes, this would be a much preferable approach. The only problem that I encountered when attempting to do that in the Chromium U-Boot was that there are exposed stand alone U-Boot API's that can pass in pointers that make their way all the way down to the device driver. Two solutions occurred to me. One was to put an Error or assert as high up as possible and expect violators to fix their code, the other was to patch things up in lower layers (right now we have a bounce buffer in the tegra MMC driver to deal with unaligned buffers). My intent was that this would give us a functional U-Boot while we took the time to clean up as many of the unaligned buffers that U-Boot uses, and then tackle the question of stand alone applications. could you elaborate on the code paths in question ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Mon, Aug 22, 2011 at 11:31 AM, Mike Frysinger vap...@gentoo.org wrote: On Monday, August 22, 2011 14:15:22 Anton Staaf wrote: please dont top post Sorry about that. Yes, this would be a much preferable approach. The only problem that I encountered when attempting to do that in the Chromium U-Boot was that there are exposed stand alone U-Boot API's that can pass in pointers that make their way all the way down to the device driver. Two solutions occurred to me. One was to put an Error or assert as high up as possible and expect violators to fix their code, the other was to patch things up in lower layers (right now we have a bounce buffer in the tegra MMC driver to deal with unaligned buffers). My intent was that this would give us a functional U-Boot while we took the time to clean up as many of the unaligned buffers that U-Boot uses, and then tackle the question of stand alone applications. could you elaborate on the code paths in question ? Heh, now I've got to go fine them again. The ub_dev_read function in the example/api/glue.c is an example of a use of the public syscall API that as far as I can tell passes down the buffer pointer all the way to the driver layer. As for the unaligned buffers I've run into: fs/ext2/dev.c: sec_buf is allocated on the stack and thus is not guaranteed to be aligned. drivers/mmc/mmc.c: ext_csd in mmc_change_freq is allocated on the stack. drivers/mmc/mmc.c: scr and switch_status in sd_change_freq are allocated on the stack. drivers/mmc/mmc.c: ext_csd in mmc_startup is allocated on the stack. disk/part_efi.c: gpt_header in print_part_efi is allocated on the stack. disk/part_efi.c: gpt_header in get_partition_info_efi is allocated on the stack. disk/part_efi.c: legacy_mbr in test_part_efi is allocated on the stack. disk/part_efi.c: pte in alloc_read_gpt_entries is allocated using malloc (not memalign). I have working solutions to all of these but they generally use the lazy allocation of an aligned buffer as needed pattern. I wanted to see the results of Lukasz current thread before sending them upstream. But that's probably not right. I should probably send them now so we can see a number of examples of the problem and get a better idea of how to fix it. Thanks, Anton -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: + cache_align_buf = memalign(get_dcache_line_size(), nowhere do i see get_dcache_line_size() defined also, what is the code size increase with your patch ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
Hi Mike, On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger vap...@gentoo.org wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: + cache_align_buf = memalign(get_dcache_line_size(), nowhere do i see get_dcache_line_size() defined also, what is the code size increase with your patch ? -mike Please look to the following post: http://patchwork.ozlabs.org/patch/110501/ and another related with this issue: http://patchwork.ozlabs.org/patch/110300/ Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B -- Best regards, Lukasz Majewski Samsung Poland RD Center Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers
On Friday, August 19, 2011 11:28:18 Lukasz Majewski wrote: On Fri, 19 Aug 2011 09:57:10 -0400 Mike Frysinger wrote: On Friday, August 19, 2011 05:25:13 Lukasz Majewski wrote: + cache_align_buf = memalign(get_dcache_line_size(), nowhere do i see get_dcache_line_size() defined Please look to the following post: http://patchwork.ozlabs.org/patch/110501/ and another related with this issue: http://patchwork.ozlabs.org/patch/110300/ if you're posting patches with dependencies, you need to mention them explicitly (below the --- area), or send proper patch series ([PATCH N/M]). ignoring that, this patch will break all arches except arm. that's bad mmmkay. you probably need to move that weak def out of arm's cache.c and into like lib/cache.c. also, what is the code size increase with your patch ? Code size overhead (s5p_goni target): Without proposed changes: 167928 B (u-boot.bin) With changes: 168208 B (u-boot.bin) Delta: 280 B np if it gives significant (more than system noise) speedups. any details on that ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
