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

[Anton Staaf]

On Mon, Aug 29, 2011 at 2:23 PM, Scott Wood <scottwood at freescale.com> wrote:
> On 08/29/2011 03:58 PM, Anton Staaf wrote:
>> On Mon, Aug 29, 2011 at 1:47 PM, Scott Wood <scottwood at 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
>
>