[U-Boot] [PATCH] mmc:dcache: Cache line size aligned internal MMC buffers

Mon Aug 29 22:12:31 CEST 2011

On Wed, Aug 24, 2011 at 1:13 PM, Anton Staaf <robotboy at google.com> wrote:
> On Wed, Aug 24, 2011 at 12:18 PM, Lukasz Majewski <majess1982 at gmail.com> wrote:
>> On Wed, 24 Aug 2011 11:25:42 -0700
>> Anton Staaf <robotboy at google.com> wrote:
>>
>>> On Wed, Aug 24, 2011 at 11:12 AM, Wolfgang Denk <wd at denx.de> wrote:
>>> > Dear Anton Staaf,
>>> >
>>> > In message
>>> > <CAF6FioWBsjFa+QdFNHEQGO50eLoqZMc--Yt3-iYX6DWr=hcOrg at 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
>>> >
>>> > --
>>> > 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: wd at denx.de Brain: an apparatus with
>>> > which we think we think.    - Ambrose Bierce
>>> >
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>> Regards,
>> Lukasz Majewski
>>
>