part_dos: allocate cacheline aligned buffers

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Quoted 4 lines · On Tuesday 24 April 2012 03:53:44 Thierry Reding wrote:

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

this is still wrong. all you've done is bypass the error message without addressing the underlying problem -- you're invalidating too much.

Quoted 3 lines · Mike Frysinger

Quoted 2 lines · On Tuesday 24 April 2012 03:53:44 Thierry Reding wrote:

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a

warning

Quoted 1 line

Quoted 1 line

from the ARM v7 cache handling code, this patch makes sure that

complete

Quoted 4 lines

Quoted 1 line

cachelines are flushed.

this is still wrong. all you've done is bypass the error message without addressing the underlying problem -- you're invalidating too much.

Reading 8 bytes is always less than a cacheline, so we don't have much choice, do we? We could of course always read a whole cacheline even if only
8 bytes are requested, but does that have any advantage over reading 8 bytes
and then invalidating the cacheline?

Or maybe I'm missing the point.

Thierry

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Thierry,

Quoted 19 lines · Thierry Reding

Quoted 3 lines · Mike Frysinger

Quoted 2 lines · On Tuesday 24 April 2012 03:53:44 Thierry Reding wrote:

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a

warning

Quoted 1 line

Quoted 1 line

from the ARM v7 cache handling code, this patch makes sure that

complete

Quoted 4 lines

Quoted 1 line

cachelines are flushed.

this is still wrong. all you've done is bypass the error message without addressing the underlying problem -- you're invalidating too much.

Reading 8 bytes is always less than a cacheline, so we don't have much choice, do we? We could of course always read a whole cacheline even if only
8 bytes are requested, but does that have any advantage over reading 8 bytes
and then invalidating the cacheline?

Quoted 2 lines

Or maybe I'm missing the point.

Well the point is that you can read 8 bytes but you still must use a buffer that is large enough for DMA activity. So the caller must allocate a buffer larger than 8 bytes.

I worry that what you have done will just introduce obscure bugs, since we will potentially invalidate stack variants (for example) and lose their values.

With the case problems, we are trying to fix them at source (i.e. at the higher level).

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

Quoted 6 lines

I worry that what you have done will just introduce obscure bugs, since we will potentially invalidate stack variants (for example) and lose their values.

With the case problems, we are trying to fix them at source (i.e. at the higher level).

I understand. The question then becomes how to best fix the passed in size. Always passing the size of a complete cacheline in the SEND_SCR command doesn't seem like a better option because it may have other implications on other hardware.

Quoted 1 line · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

yes, that shows what we're talking about.
int sd_change_freq(struct mmc *mmc)

{
       ... stack vars ...
       int err;
       ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2);
       struct mmc_data data;
       int timeout;
       ... stack vars ...
...
       data.dest = (char *)scr;
       data.blocksize = 8;
       data.blocks = 1;
       data.flags = MMC_DATA_READ;
       err = mmc_send_cmd(mmc, &cmd, &data);

...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
struct mmc_data *data)
{
...

        if (data->flags & MMC_DATA_READ) {
                if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1))
                       printf("Warning: unaligned read from %p "
                               "may fail\n", data->dest);
                invalidate_dcache_range((ulong)data->dest,
                       (ulong)data->dest +
                        data->blocks * data->blocksize);
       }

...

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack layout is:
[random stuff]

0x100   [scr[0]]
0x104   [scr[1]]
0x108   [err]
0x10a   [timeout]

[more stuff]

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that
weren't written back to external memory yet.

Quoted 5 lines

Quoted 5 lines

I worry that what you have done will just introduce obscure bugs, since we will potentially invalidate stack variants (for example) and lose their values.

With the case problems, we are trying to fix them at source (i.e. at

the

Quoted 3 lines

Quoted 1 line

higher level).

I understand. The question then becomes how to best fix the passed in

size.

Quoted 2 lines

Always passing the size of a complete cacheline in the SEND_SCR command doesn't seem like a better option because it may have other implications

on

Quoted 1 line

other hardware.

i proposed this in a previous thread, but i think Simon missed it.

perhaps the warning in the core code could be dropped and all your changes in
fringe code obsoleted (such as these USB patches): when it detects that an address is starting on an unaligned boundary, flush that line first, and then
let it be invalidated. accordingly, when the end length is on an unaligned boundary, do the same flush-then-invalidate step. this should also make things
work without a (significant) loss in performance. if anything, i suspect the
overhead of doing runtime buffer size calculations and manually aligning pointers (which is what ALLOC_CACHE_ALIGN_BUFFER does) is a wash compared to
partially flushing cache lines in the core ...

Simon: what do you think of this last idea ?

-mike

Quoted 1 line · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack
layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108
[err] 0x10a [timeout] [more stuff]

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.

[PATCH 2/2] mmc: tegra: invalidate complete cachelines

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Signed-off-by: Thierry Reding <thierry.reding at
avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1
file changed, 4 insertions(+), 3 deletions(-)

drivers/mmc/tegra2_mmc.c +0 -0

index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++

b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { +
ulong end = (ulong)data->dest + + roundup(data->blocks *
data->blocksize, + ARCH_DMA_MINALIGN); if
((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); -
invalidate_dcache_range((ulong)data->dest, - (ulong)data->dest
+ - data->blocks * data->blocksize); +
invalidate_dcache_range((ulong)data->dest, end); } }

--

Quoted 2 lines · Mike Frysinger

Quoted 1 line · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

This is a follow-up to:

http://lists.denx.de/pipermail/u-boot/2012-April/123080.html

which was referenced from:

http://lists.denx.de/pipermail/u-boot/2012-September/133641.html

Quoted 14 lines

yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...

Quoted 5 lines

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack
layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108
[err] 0x10a [timeout] [more stuff]

That's not the stack layout. scr is allocated (as quoted above) using ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for scr is always cache-aligned in address and size, even if the code only uses 8 bytes of it.

Quoted 5 lines

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.

So, this problem won't actually occur here.

So, Thierry's proposed solution (which I'll re-quote below) would actually work just fine:

Quoted 24 lines

[PATCH 2/2] mmc: tegra: invalidate complete cachelines

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Signed-off-by: Thierry Reding <thierry.reding at
avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1
file changed, 4 insertions(+), 3 deletions(-)

drivers/mmc/tegra2_mmc.c +0 -0

index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++

b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { +
ulong end = (ulong)data->dest + + roundup(data->blocks *
data->blocksize, + ARCH_DMA_MINALIGN); if
((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); -
invalidate_dcache_range((ulong)data->dest, - (ulong)data->dest
+ - data->blocks * data->blocksize); +
invalidate_dcache_range((ulong)data->dest, end); } }

Quoted 1 line

--

... so long as we require any struct mmc_data data's .dest field to point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.

So, I'd like to propose that we take Thierry's fix, perhaps having validated (or implemented some way of enforcing) that ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.

Hi Stephen,

Quoted 74 lines · Stephen Warren

Quoted 2 lines · Mike Frysinger

Quoted 1 line · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

This is a follow-up to:

http://lists.denx.de/pipermail/u-boot/2012-April/123080.html

which was referenced from:

http://lists.denx.de/pipermail/u-boot/2012-September/133641.html

Quoted 14 lines

yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...

Quoted 5 lines

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack
layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108
[err] 0x10a [timeout] [more stuff]

That's not the stack layout. scr is allocated (as quoted above) using ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for scr is always cache-aligned in address and size, even if the code only uses 8 bytes of it.

Quoted 5 lines

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.

So, this problem won't actually occur here.

So, Thierry's proposed solution (which I'll re-quote below) would

actually work just fine:

Quoted 26 lines

[PATCH 2/2] mmc: tegra: invalidate complete cachelines

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Signed-off-by: Thierry Reding <thierry.reding at
avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1
file changed, 4 insertions(+), 3 deletions(-)

drivers/mmc/tegra2_mmc.c +0 -0

index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++

b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { + ulong end = (ulong)data->dest + +
roundup(data->blocks * data->blocksize, +

             ARCH_DMA_MINALIGN); if ((uintptr_t)data->dest &

(ARCH_DMA_MINALIGN - 1)) printf("Warning: unaligned read from %p " "may fail\n", data->dest); - invalidate_dcache_range((ulong)data->dest, -
(ulong)data->dest + - data->blocks *
data->blocksize); + invalidate_dcache_range((ulong)data->dest, end); } }

Quoted 8 lines

Quoted 1 line

--

... so long as we require any struct mmc_data data's .dest field to point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.

So, I'd like to propose that we take Thierry's fix, perhaps having validated (or implemented some way of enforcing) that ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.

That sounds ok to me.

It is important to avoid silent failure if we can. Are you proposing to pass a 'size' parameter along with any buffer pointer, or something else?

Dear Simon Glass,

Quoted 88 lines

Hi Stephen,

Quoted 74 lines · Stephen Warren

Quoted 2 lines · Mike Frysinger

Quoted 1 line · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

This is a follow-up to:

http://lists.denx.de/pipermail/u-boot/2012-April/123080.html

which was referenced from:

http://lists.denx.de/pipermail/u-boot/2012-September/133641.html

Quoted 14 lines

yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...

Quoted 5 lines

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack
layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108
[err] 0x10a [timeout] [more stuff]

That's not the stack layout. scr is allocated (as quoted above) using ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for scr is always cache-aligned in address and size, even if the code only uses 8 bytes of it.

Quoted 5 lines

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.

So, this problem won't actually occur here.

So, Thierry's proposed solution (which I'll re-quote below) would

actually work just fine:

Quoted 26 lines

[PATCH 2/2] mmc: tegra: invalidate complete cachelines

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Signed-off-by: Thierry Reding <thierry.reding at
avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1
file changed, 4 insertions(+), 3 deletions(-)

drivers/mmc/tegra2_mmc.c +0 -0

index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++

b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { + ulong end = (ulong)data->dest + +
roundup(data->blocks * data->blocksize, +

             ARCH_DMA_MINALIGN); if ((uintptr_t)data->dest &

(ARCH_DMA_MINALIGN - 1)) printf("Warning: unaligned read from %p " "may fail\n", data->dest); - invalidate_dcache_range((ulong)data->dest, -
(ulong)data->dest + - data->blocks *
data->blocksize); + invalidate_dcache_range((ulong)data->dest, end); } }

Quoted 8 lines

Quoted 1 line

--

... so long as we require any struct mmc_data data's .dest field to point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.

So, I'd like to propose that we take Thierry's fix, perhaps having validated (or implemented some way of enforcing) that ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.

That sounds ok to me.

Quoted 3 lines

It is important to avoid silent failure if we can. Are you proposing to pass a 'size' parameter along with any buffer pointer, or something else?

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Quoted 5 lines

Dear Simon Glass,

Quoted 3 lines

Hi Stephen,

On Fri, Nov 2, 2012 at 1:22 PM, Stephen Warren <[email protected]>

Quoted 87 lines

Quoted 87 lines

Quoted 76 lines · Mike Frysinger

Quoted 2 lines · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

Quoted 1 line

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

This is a follow-up to:

http://lists.denx.de/pipermail/u-boot/2012-April/123080.html

which was referenced from:

http://lists.denx.de/pipermail/u-boot/2012-September/133641.html

Quoted 14 lines

yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...

Quoted 5 lines

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack
layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108
[err] 0x10a [timeout] [more stuff]

That's not the stack layout. scr is allocated (as quoted above) using ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for scr is always cache-aligned in address and size, even if the code only uses 8 bytes of it.

Quoted 5 lines

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.

So, this problem won't actually occur here.

So, Thierry's proposed solution (which I'll re-quote below) would

actually work just fine:

Quoted 28 lines

[PATCH 2/2] mmc: tegra: invalidate complete cachelines

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Signed-off-by: Thierry Reding <thierry.reding at
avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1
file changed, 4 insertions(+), 3 deletions(-)

drivers/mmc/tegra2_mmc.c +0 -0

index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++

b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { + ulong end = (ulong)data->dest + +
roundup(data->blocks * data->blocksize, +

             ARCH_DMA_MINALIGN); if ((uintptr_t)data->dest &

(ARCH_DMA_MINALIGN - 1)) printf("Warning: unaligned read from %p " "may fail\n", data->dest); - invalidate_dcache_range((ulong)data->dest, -
(ulong)data->dest + - data->blocks *
data->blocksize); + invalidate_dcache_range((ulong)data->dest, end); } }

Quoted 8 lines

Quoted 1 line

--

... so long as we require any struct mmc_data data's .dest field to point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.

So, I'd like to propose that we take Thierry's fix, perhaps having validated (or implemented some way of enforcing) that ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.

That sounds ok to me.

Quoted 7 lines

Quoted 3 lines

It is important to avoid silent failure if we can. Are you proposing to pass a 'size' parameter along with any buffer pointer, or something else?

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why the driver is flushing caches!)

I guess we only support the use of aligned addresses, so e.g. the following would work:

ext2load mmc 0:1 0x00100000 /file

but the following wouldn't:

ext2load mmc 0:1 0x00100004 /file

which while I suppose it is an artificial restriction, hasn't been an issue in practice.

Dear Stephen Warren,

Quoted 1 line · Marek Vasut

Quoted 12 lines

Quoted 3 lines

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why the driver is flushing caches!)

I guess we only support the use of aligned addresses, so e.g. the following would work:

ext2load mmc 0:1 0x00100000 /file

Quoted 6 lines

but the following wouldn't:

ext2load mmc 0:1 0x00100004 /file

which while I suppose it is an artificial restriction, hasn't been an issue in practice.

Then just enable the bounce buffer and it will work ;-)

Quoted 3 lines

Dear Stephen Warren,

Quoted 1 line · Marek Vasut

...

Quoted 16 lines

Quoted 16 lines

Quoted 3 lines

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why the driver is flushing caches!)

I guess we only support the use of aligned addresses, so e.g. the following would work:

ext2load mmc 0:1 0x00100000 /file

but the following wouldn't:

ext2load mmc 0:1 0x00100004 /file

Quoted 4 lines

Quoted 2 lines

which while I suppose it is an artificial restriction, hasn't been an issue in practice.

Then just enable the bounce buffer and it will work ;-)

You suggested that last time, and it made no difference then... In fact, the config option you mentioned isn't used anywhere in the srouce tree except adding bouncebuf.o

into the build right now; which config option
do you think I should use?

Dear Stephen Warren,

Quoted 25 lines · Marek Vasut

Quoted 3 lines

Dear Stephen Warren,

Quoted 1 line · Marek Vasut

...

Quoted 19 lines

Quoted 18 lines

Quoted 3 lines

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why the driver is flushing caches!)

I guess we only support the use of aligned addresses, so e.g. the following would work:

ext2load mmc 0:1 0x00100000 /file

but the following wouldn't:

ext2load mmc 0:1 0x00100004 /file

which while I suppose it is an artificial restriction, hasn't been an issue in practice.

Quoted 5 lines

Quoted 1 line

Then just enable the bounce buffer and it will work ;-)

You suggested that last time, and it made no difference then... In fact, the config option you mentioned isn't used anywhere in the srouce tree except adding bouncebuf.o

Bouncebuf.o ?

Quoted 2 lines

into the build right now; which config option
do you think I should use?

CONFIG_BOUNCE_BUFFER

It's used on mx28-based boards.

Quoted 29 lines

Dear Stephen Warren,

Quoted 27 lines · Marek Vasut

Quoted 3 lines

Dear Stephen Warren,

Quoted 1 line · Marek Vasut

...

Quoted 20 lines

Quoted 10 lines

Quoted 3 lines

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why the driver is flushing caches!)

I guess we only support the use of aligned addresses, so e.g. the following would work:

Quoted 8 lines

ext2load mmc 0:1 0x00100000 /file

but the following wouldn't:

ext2load mmc 0:1 0x00100004 /file

which while I suppose it is an artificial restriction, hasn't been an issue in practice.

Then just enable the bounce buffer and it will work ;-)

Quoted 5 lines

Quoted 3 lines

You suggested that last time, and it made no difference then... In fact, the config option you mentioned isn't used anywhere in the srouce tree except adding bouncebuf.o

Bouncebuf.o ?

common/bouncebuf.c

Quoted 6 lines

Quoted 2 lines

into the build right now; which config option
do you think I should use?

CONFIG_BOUNCE_BUFFER

It's used on mx28-based boards.

Ahah, there's much more to it than just defining the config option, and grep'ing for the config option doesn't reveal that.

Defining CONFIG_BOUNCE_BUFFER simply pulls in bouncebuf.o into the build. It's then up to the individual MMC/... driver to actually make use of the new functions - i.e. explicitly call e.g. bounce_buffer_start(). The MXS MMC driver calls these without any ifdef CONFIG_BOUNCE_BUFFER, and hence doesn't show up when you grep for that. I had assumed that such functionality would be a conditional part of the MMC core rather than the individual drivers, hence had missed the usage in the MXS driver.

So, that config option plus some changes to the Tegra MMC driver might work - I'll take a look. However, making that change will simply hide any unaligned buffer and reduce performance, so I'd still prefer to require that buffers be aligned in the future where possible.

Quoted 27 lines · Marek Vasut

Dear Stephen Warren,

Quoted 25 lines · Marek Vasut

Quoted 3 lines

Dear Stephen Warren,

Quoted 1 line · Marek Vasut

...

Quoted 19 lines

Quoted 18 lines

Quoted 3 lines

Dumb question -- might be unrelated. Does the tegra mmc driver do DMA? And if so, what happens if you do raw read to unaligned address (aka. how come you don't need the bounce buffer)?

Yes, it does DMA, I believe. (At least if it doesn't, I have no idea why the driver is flushing caches!)

I guess we only support the use of aligned addresses, so e.g. the following would work:

ext2load mmc 0:1 0x00100000 /file

but the following wouldn't:

ext2load mmc 0:1 0x00100004 /file

which while I suppose it is an artificial restriction, hasn't been an issue in practice.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Then just enable the bounce buffer and it will work ;-)

You suggested that last time, and it made no difference then... In fact, the config option you mentioned isn't used anywhere in the srouce tree except adding bouncebuf.o

Bouncebuf.o ?

common/bouncebuf.c

Quoted 6 lines

Quoted 2 lines

into the build right now; which config option
do you think I should use?

CONFIG_BOUNCE_BUFFER

It's used on mx28-based boards.

Ahah, there's much more to it than just defining the config option, and grep'ing for the config option doesn't reveal that.

Defining CONFIG_BOUNCE_BUFFER simply pulls in bouncebuf.o into the build. It's then up to the individual MMC/... driver to actually make use of the new functions - i.e. explicitly call e.g. bounce_buffer_start(). The MXS MMC driver calls these without any ifdef CONFIG_BOUNCE_BUFFER, and hence doesn't show up when you grep for that. I had assumed that such functionality would be a conditional part of the MMC core rather than the individual drivers, hence had missed the usage in the MXS driver.

So, that config option plus some changes to the Tegra MMC driver might work - I'll take a look. However, making that change will simply hide any unaligned buffer and reduce performance, so I'd still prefer to require that buffers be aligned in the future where possible.

Hi Stephen,

Quoted 72 lines · Stephen Warren

Quoted 2 lines · Mike Frysinger

Quoted 1 line · On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:

For reference, see sd_change_freq() in drivers/mmc/mmc.c.

This is a follow-up to:

http://lists.denx.de/pipermail/u-boot/2012-April/123080.html

which was referenced from:

http://lists.denx.de/pipermail/u-boot/2012-September/133641.html

Quoted 14 lines

yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...

static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...

Quoted 5 lines

so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack
layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108
[err] 0x10a [timeout] [more stuff]

That's not the stack layout. scr is allocated (as quoted above) using ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for scr is always cache-aligned in address and size, even if the code only uses 8 bytes of it.

Quoted 5 lines

this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.

So, this problem won't actually occur here.

So, Thierry's proposed solution (which I'll re-quote below) would actually work just fine:

Quoted 24 lines

[PATCH 2/2] mmc: tegra: invalidate complete cachelines

The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.

Signed-off-by: Thierry Reding <thierry.reding at
avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1
file changed, 4 insertions(+), 3 deletions(-)

drivers/mmc/tegra2_mmc.c +0 -0

index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++

b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { +
ulong end = (ulong)data->dest + + roundup(data->blocks *
data->blocksize, + ARCH_DMA_MINALIGN); if
((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning:
unaligned read from %p " "may fail\n", data->dest); -
invalidate_dcache_range((ulong)data->dest, - (ulong)data->dest
+ - data->blocks * data->blocksize); +
invalidate_dcache_range((ulong)data->dest, end); } }

Quoted 8 lines

Quoted 1 line

--

... so long as we require any struct mmc_data data's .dest field to point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.

So, I'd like to propose that we take Thierry's fix, perhaps having validated (or implemented some way of enforcing) that ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.

That sounds ok to me.

It is important to avoid silent failure if we can. Are you proposing to pass a 'size' parameter along with any buffer pointer, or something else?

Some hardware requires the data buffers to be cacheline-aligned to make sure DMA operations can be properly executed.

This patch uses the ALLOC_CACHE_ALIGN_BUFFER macro to allocate buffers with proper alignment. The same was already done for EFI partitions in commit f75dd58 "part_efi: dcache: allocate cacheline aligned buffers".

Signed-off-by Thierry Reding <[email protected]>

---
This fixes boot failures on Avionic Design Plutux and Medcom boards.

?disk/part_dos.c | ? ?6 +++---
?1 file changed, 3 insertions(+), 3 deletions(-)

disk/part_dos.c +1 -1

index b5bcb37..c028aaf 100644
@@ -87,7 +87,7 @@ static int test_block_type(unsigned char *buffer)
 
 ?int test_part_dos (block_dev_desc_t *dev_desc)
 ?{
- ? ? ? unsigned char buffer[dev_desc->blksz];
+ ? ? ? ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 

? ? ? ?if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) || ? ? ? ? ? ?(buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) ||

disk/part_dos.c +2 -3

@@ -102,7 +102,7 @@ int test_part_dos (block_dev_desc_t *dev_desc)
 ?static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_sector, int relative,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int part_num)
 ?{
- ? ? ? unsigned char buffer[dev_desc->blksz];
+ ? ? ? ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 ? ? ? ?dos_partition_t *pt;
 ? ? ? ?int i;
 

@@ -166,7 +166,7 @@ static int get_partition_info_extended (block_dev_desc_t *dev_desc, int ext_part
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int relative, int part_num,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int which_part, disk_partition_t *info)
 ?{
- ? ? ? unsigned char buffer[dev_desc->blksz];
+ ? ? ? ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 ? ? ? ?dos_partition_t *pt;
 ? ? ? ?int i;
 
--

1.7.10

Quoted 3 lines · Thierry Reding

Some hardware requires the data buffers to be cacheline-aligned to make sure DMA operations can be properly executed.

Quoted 5 lines

This patch uses the ALLOC_CACHE_ALIGN_BUFFER macro to allocate buffers with proper alignment. The same was already done for EFI partitions in commit f75dd58 "part_efi: dcache: allocate cacheline aligned buffers".

Signed-off-by Thierry Reding <[email protected]>

Acked-by Simon Glass <[email protected]>