Patchwork [U-Boot] Cache alignment warnings on Tegra (ARM)

login
register
mail settings
Submitter Tom Warren
Date Sept. 12, 2012, 4:19 p.m.
Message ID <CA+m5__JruHPF6uQQ3=8q9Cojo528qfFaCx7v=3LFYt++Pki4WQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/183405/
State RFC
Headers show

Comments

Tom Warren - Sept. 12, 2012, 4:19 p.m.
Folks,

Stephen Warren has posted an internal bug regarding the cache
alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
the gist:

Executing "mmc dev 0" still yields cache warnings:

Tegra20 (Harmony) # mmc dev 0
ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
mmc0 is current device

I carry the patch below to turn these off, but I'd like to drop it.

commit 37bccb3c67897a8944c458d511dac06389ea8f1e
Author: Stephen Warren <swarren@nvidia.com >
Date: Mon Apr 30 11:39:27 2012 -0600

HACK: Disable cache alignment warnings

They are very annoying and noisy


There have been patches in the past (IIRC) that have tried to ensure
all callers (FS, MMC driver, USB driver, etc.) force their buffers to
the appropriate alignment, but I don't know that we can ever correct
every instance, now or in the future.

Can we start a discussion about what we can do about this warning?
Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
etc.) where Stephen put his #if 0's would be one approach, or changing
the printf() to a debug(), perhaps. As far as I can tell, these
alignment 'errors' don't seem to produce bad data in the transfer.

Thanks,

Tom
Stephen Warren - Sept. 12, 2012, 4:49 p.m.
On 09/12/2012 10:19 AM, Tom Warren wrote:
> Folks,
> 
> Stephen Warren has posted an internal bug regarding the cache
> alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
> the gist:
> 
> Executing "mmc dev 0" still yields cache warnings:
> 
> Tegra20 (Harmony) # mmc dev 0
> ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
> mmc0 is current device
...
> There have been patches in the past (IIRC) that have tried to ensure
> all callers (FS, MMC driver, USB driver, etc.) force their buffers to
> the appropriate alignment, but I don't know that we can ever correct
> every instance, now or in the future.
> 
> Can we start a discussion about what we can do about this warning?
> Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
> etc.) where Stephen put his #if 0's would be one approach, or changing
> the printf() to a debug(), perhaps. As far as I can tell, these
> alignment 'errors' don't seem to produce bad data in the transfer.

I don't think simply turning off the warning is the correct approach; I
believe they represent real problems that can in fact cause data
corruption. I don't believe we have any choice other than to fully solve
the root-cause.
Marek Vasut - Sept. 12, 2012, 10:38 p.m.
Dear Stephen Warren,

> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > Folks,
> > 
> > Stephen Warren has posted an internal bug regarding the cache
> > alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
> > the gist:
> > 
> > Executing "mmc dev 0" still yields cache warnings:
> > 
> > Tegra20 (Harmony) # mmc dev 0
> > ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
> > mmc0 is current device
> 
> ...
> 
> > There have been patches in the past (IIRC) that have tried to ensure
> > all callers (FS, MMC driver, USB driver, etc.) force their buffers to
> > the appropriate alignment, but I don't know that we can ever correct
> > every instance, now or in the future.
> > 
> > Can we start a discussion about what we can do about this warning?
> > Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
> > etc.) where Stephen put his #if 0's would be one approach, or changing
> > the printf() to a debug(), perhaps. As far as I can tell, these
> > alignment 'errors' don't seem to produce bad data in the transfer.
> 
> I don't think simply turning off the warning is the correct approach; I
> believe they represent real problems that can in fact cause data
> corruption. I don't believe we have any choice other than to fully solve
> the root-cause.

Try CONFIG_MMC_BOUNCE_BUFFER or what it was called ... see 
inclued/configs/m28evk.h , I use it there.

Best regards,
Marek Vasut
Stephen Warren - Sept. 12, 2012, 11:10 p.m.
On 09/12/2012 04:38 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 09/12/2012 10:19 AM, Tom Warren wrote:
>>> Folks,
>>>
>>> Stephen Warren has posted an internal bug regarding the cache
>>> alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
>>> the gist:
>>>
>>> Executing "mmc dev 0" still yields cache warnings:
>>>
>>> Tegra20 (Harmony) # mmc dev 0
>>> ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
>>> mmc0 is current device
>>
>> ...
>>
>>> There have been patches in the past (IIRC) that have tried to ensure
>>> all callers (FS, MMC driver, USB driver, etc.) force their buffers to
>>> the appropriate alignment, but I don't know that we can ever correct
>>> every instance, now or in the future.
>>>
>>> Can we start a discussion about what we can do about this warning?
>>> Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
>>> etc.) where Stephen put his #if 0's would be one approach, or changing
>>> the printf() to a debug(), perhaps. As far as I can tell, these
>>> alignment 'errors' don't seem to produce bad data in the transfer.
>>
>> I don't think simply turning off the warning is the correct approach; I
>> believe they represent real problems that can in fact cause data
>> corruption. I don't believe we have any choice other than to fully solve
>> the root-cause.
> 
> Try CONFIG_MMC_BOUNCE_BUFFER or what it was called ... see 
> inclued/configs/m28evk.h , I use it there.

That didn't seem to change anything.

I just re-tested and it looks like there's one single instance of this
cache warning now when running "mmc dev 0"; there used to be hundreds of
them when loading files from eMMC. Perhaps it depends on some runtime
allocation or something though, and I'm just getting lucky and seeing
fewer of them.
Marek Vasut - Sept. 12, 2012, 11:42 p.m.
Dear Stephen Warren,

> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > Dear Stephen Warren,
> > 
> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> >>> Folks,
> >>> 
> >>> Stephen Warren has posted an internal bug regarding the cache
> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
> >>> the gist:
> >>> 
> >>> Executing "mmc dev 0" still yields cache warnings:
> >>> 
> >>> Tegra20 (Harmony) # mmc dev 0
> >>> ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
> >>> mmc0 is current device
> >> 
> >> ...
> >> 
> >>> There have been patches in the past (IIRC) that have tried to ensure
> >>> all callers (FS, MMC driver, USB driver, etc.) force their buffers to
> >>> the appropriate alignment, but I don't know that we can ever correct
> >>> every instance, now or in the future.
> >>> 
> >>> Can we start a discussion about what we can do about this warning?
> >>> Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
> >>> etc.) where Stephen put his #if 0's would be one approach, or changing
> >>> the printf() to a debug(), perhaps. As far as I can tell, these
> >>> alignment 'errors' don't seem to produce bad data in the transfer.
> >> 
> >> I don't think simply turning off the warning is the correct approach; I
> >> believe they represent real problems that can in fact cause data
> >> corruption. I don't believe we have any choice other than to fully solve
> >> the root-cause.
> > 
> > Try CONFIG_MMC_BOUNCE_BUFFER or what it was called ... see
> > inclued/configs/m28evk.h , I use it there.
> 
> That didn't seem to change anything.
> 
> I just re-tested and it looks like there's one single instance of this
> cache warning now when running "mmc dev 0"; there used to be hundreds of
> them when loading files from eMMC. Perhaps it depends on some runtime
> allocation or something though, and I'm just getting lucky and seeing
> fewer of them.

Yes it does, internal unaligned variables passed through to the MMC driver ... 
the bounce buffer should fix that up for you.

Best regards,
Marek Vasut
Simon Glass - Sept. 14, 2012, 3:53 p.m.
Hi,

On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Stephen Warren,
>
>> On 09/12/2012 04:38 PM, Marek Vasut wrote:
>> > Dear Stephen Warren,
>> >
>> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
>> >>> Folks,
>> >>>
>> >>> Stephen Warren has posted an internal bug regarding the cache
>> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
>> >>> the gist:
>> >>>
>> >>> Executing "mmc dev 0" still yields cache warnings:
>> >>>
>> >>> Tegra20 (Harmony) # mmc dev 0
>> >>> ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
>> >>> mmc0 is current device
>> >>
>> >> ...
>> >>
>> >>> There have been patches in the past (IIRC) that have tried to ensure
>> >>> all callers (FS, MMC driver, USB driver, etc.) force their buffers to
>> >>> the appropriate alignment, but I don't know that we can ever correct
>> >>> every instance, now or in the future.
>> >>>
>> >>> Can we start a discussion about what we can do about this warning?
>> >>> Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
>> >>> etc.) where Stephen put his #if 0's would be one approach, or changing
>> >>> the printf() to a debug(), perhaps. As far as I can tell, these
>> >>> alignment 'errors' don't seem to produce bad data in the transfer.
>> >>
>> >> I don't think simply turning off the warning is the correct approach; I
>> >> believe they represent real problems that can in fact cause data
>> >> corruption. I don't believe we have any choice other than to fully solve
>> >> the root-cause.

Yes I agree, and I think it is pretty close - certainly much better
than it used to be. The good thing about them being annoying is that
they will likely get fixed :-)

Regards,
Simon

>> >
>> > Try CONFIG_MMC_BOUNCE_BUFFER or what it was called ... see
>> > inclued/configs/m28evk.h , I use it there.
>>
>> That didn't seem to change anything.
>>
>> I just re-tested and it looks like there's one single instance of this
>> cache warning now when running "mmc dev 0"; there used to be hundreds of
>> them when loading files from eMMC. Perhaps it depends on some runtime
>> allocation or something though, and I'm just getting lucky and seeing
>> fewer of them.
>
> Yes it does, internal unaligned variables passed through to the MMC driver ...
> the bounce buffer should fix that up for you.
>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Thierry Reding - Sept. 15, 2012, 8:01 p.m.
On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> Hi,
> 
> On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Stephen Warren,
> >
> >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> >> > Dear Stephen Warren,
> >> >
> >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> >> >>> Folks,
> >> >>>
> >> >>> Stephen Warren has posted an internal bug regarding the cache
> >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC. Here's
> >> >>> the gist:
> >> >>>
> >> >>> Executing "mmc dev 0" still yields cache warnings:
> >> >>>
> >> >>> Tegra20 (Harmony) # mmc dev 0
> >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned- 0x3fb69908
> >> >>> mmc0 is current device
> >> >>
> >> >> ...
> >> >>
> >> >>> There have been patches in the past (IIRC) that have tried to ensure
> >> >>> all callers (FS, MMC driver, USB driver, etc.) force their buffers to
> >> >>> the appropriate alignment, but I don't know that we can ever correct
> >> >>> every instance, now or in the future.
> >> >>>
> >> >>> Can we start a discussion about what we can do about this warning?
> >> >>> Adding an appropriate #ifdef (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS,
> >> >>> etc.) where Stephen put his #if 0's would be one approach, or changing
> >> >>> the printf() to a debug(), perhaps. As far as I can tell, these
> >> >>> alignment 'errors' don't seem to produce bad data in the transfer.
> >> >>
> >> >> I don't think simply turning off the warning is the correct approach; I
> >> >> believe they represent real problems that can in fact cause data
> >> >> corruption. I don't believe we have any choice other than to fully solve
> >> >> the root-cause.
> 
> Yes I agree, and I think it is pretty close - certainly much better
> than it used to be. The good thing about them being annoying is that
> they will likely get fixed :-)

I think I traced this to the copying of CSD a while back. The problem is
that the transferred buffer is 8 bytes, so there's no way to make it
aligned properly. Unfortunately the entailing discussion did not yield a
solution at the time.

Thierry
Marek Vasut - Sept. 15, 2012, 8:11 p.m.
Dear Thierry Reding,

> On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> > Hi,
> > 
> > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> > > Dear Stephen Warren,
> > > 
> > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > >> > Dear Stephen Warren,
> > >> > 
> > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > >> >>> Folks,
> > >> >>> 
> > >> >>> Stephen Warren has posted an internal bug regarding the cache
> > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
> > >> >>> Here's the gist:
> > >> >>> 
> > >> >>> Executing "mmc dev 0" still yields cache warnings:
> > >> >>> 
> > >> >>> Tegra20 (Harmony) # mmc dev 0
> > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
> > >> >>> 0x3fb69908 mmc0 is current device
> > >> >> 
> > >> >> ...
> > >> >> 
> > >> >>> There have been patches in the past (IIRC) that have tried to
> > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
> > >> >>> buffers to the appropriate alignment, but I don't know that we
> > >> >>> can ever correct every instance, now or in the future.
> > >> >>> 
> > >> >>> Can we start a discussion about what we can do about this warning?
> > >> >>> Adding an appropriate #ifdef
> > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
> > >> >>> his #if 0's would be one approach, or changing the printf() to a
> > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
> > >> >>> don't seem to produce bad data in the transfer.
> > >> >> 
> > >> >> I don't think simply turning off the warning is the correct
> > >> >> approach; I believe they represent real problems that can in fact
> > >> >> cause data corruption. I don't believe we have any choice other
> > >> >> than to fully solve the root-cause.
> > 
> > Yes I agree, and I think it is pretty close - certainly much better
> > than it used to be. The good thing about them being annoying is that
> > they will likely get fixed :-)
> 
> I think I traced this to the copying of CSD a while back. The problem is
> that the transferred buffer is 8 bytes, so there's no way to make it
> aligned properly. Unfortunately the entailing discussion did not yield a
> solution at the time.

And how exactly does the MMC bounce buffer not help here?

> Thierry

Best regards,
Marek Vasut
Thierry Reding - Sept. 15, 2012, 8:19 p.m.
On Sat, Sep 15, 2012 at 10:01:47PM +0200, Thierry Reding wrote:
> I think I traced this to the copying of CSD a while back. The problem is
> that the transferred buffer is 8 bytes, so there's no way to make it
> aligned properly. Unfortunately the entailing discussion did not yield a
> solution at the time.

For reference, below is a link[0] to the patch I proposed at the time
but it was obviously wrong. And it wasn't CSD but rather SCR. The reason
why allocating a larger buffer is not enough is that the MMC core
requests that only 8 bytes be transferred, which is the value that
eventually ends up being passed to the cache invalidation routine.

Thierry

[0]: http://lists.denx.de/pipermail/u-boot/2012-April/123080.html
Thierry Reding - Sept. 15, 2012, 8:41 p.m.
On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> Dear Thierry Reding,
> 
> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> > > Hi,
> > > 
> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> > > > Dear Stephen Warren,
> > > > 
> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > > >> > Dear Stephen Warren,
> > > >> > 
> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > > >> >>> Folks,
> > > >> >>> 
> > > >> >>> Stephen Warren has posted an internal bug regarding the cache
> > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
> > > >> >>> Here's the gist:
> > > >> >>> 
> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> > > >> >>> 
> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
> > > >> >>> 0x3fb69908 mmc0 is current device
> > > >> >> 
> > > >> >> ...
> > > >> >> 
> > > >> >>> There have been patches in the past (IIRC) that have tried to
> > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
> > > >> >>> buffers to the appropriate alignment, but I don't know that we
> > > >> >>> can ever correct every instance, now or in the future.
> > > >> >>> 
> > > >> >>> Can we start a discussion about what we can do about this warning?
> > > >> >>> Adding an appropriate #ifdef
> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
> > > >> >>> his #if 0's would be one approach, or changing the printf() to a
> > > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
> > > >> >>> don't seem to produce bad data in the transfer.
> > > >> >> 
> > > >> >> I don't think simply turning off the warning is the correct
> > > >> >> approach; I believe they represent real problems that can in fact
> > > >> >> cause data corruption. I don't believe we have any choice other
> > > >> >> than to fully solve the root-cause.
> > > 
> > > Yes I agree, and I think it is pretty close - certainly much better
> > > than it used to be. The good thing about them being annoying is that
> > > they will likely get fixed :-)
> > 
> > I think I traced this to the copying of CSD a while back. The problem is
> > that the transferred buffer is 8 bytes, so there's no way to make it
> > aligned properly. Unfortunately the entailing discussion did not yield a
> > solution at the time.
> 
> And how exactly does the MMC bounce buffer not help here?

The problem solved by the bounce buffer is that it is properly cache-
line aligned. However the issue here is not that the buffer is not
properly aligned but rather that the transfer is too small.

When the MMC core transfers the SCR, it requests 8 bytes. The buffer
used to store these 8 bytes is properly allocated. However, those 8
bytes eventually end up as the size of the range that is to be
invalidated. This is the reason for the warning. Address x of the buffer
is cache-line aligned, but x + 8 is never properly aligned and therefore
the code complains.

Thierry
Marek Vasut - Sept. 15, 2012, 8:56 p.m.
Dear Thierry Reding,

> On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> > Dear Thierry Reding,
> > 
> > > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> > > > > Dear Stephen Warren,
> > > > > 
> > > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > > > >> > Dear Stephen Warren,
> > > > >> > 
> > > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > > > >> >>> Folks,
> > > > >> >>> 
> > > > >> >>> Stephen Warren has posted an internal bug regarding the cache
> > > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing
> > > > >> >>> MMC. Here's the gist:
> > > > >> >>> 
> > > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> > > > >> >>> 
> > > > >> >>> Tegra20 (Harmony) # mmc dev 0
> > > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
> > > > >> >>> 0x3fb69908 mmc0 is current device
> > > > >> >> 
> > > > >> >> ...
> > > > >> >> 
> > > > >> >>> There have been patches in the past (IIRC) that have tried to
> > > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force
> > > > >> >>> their buffers to the appropriate alignment, but I don't know
> > > > >> >>> that we can ever correct every instance, now or in the
> > > > >> >>> future.
> > > > >> >>> 
> > > > >> >>> Can we start a discussion about what we can do about this
> > > > >> >>> warning? Adding an appropriate #ifdef
> > > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen
> > > > >> >>> put his #if 0's would be one approach, or changing the
> > > > >> >>> printf() to a debug(), perhaps. As far as I can tell, these
> > > > >> >>> alignment 'errors' don't seem to produce bad data in the
> > > > >> >>> transfer.
> > > > >> >> 
> > > > >> >> I don't think simply turning off the warning is the correct
> > > > >> >> approach; I believe they represent real problems that can in
> > > > >> >> fact cause data corruption. I don't believe we have any choice
> > > > >> >> other than to fully solve the root-cause.
> > > > 
> > > > Yes I agree, and I think it is pretty close - certainly much better
> > > > than it used to be. The good thing about them being annoying is that
> > > > they will likely get fixed :-)
> > > 
> > > I think I traced this to the copying of CSD a while back. The problem
> > > is that the transferred buffer is 8 bytes, so there's no way to make
> > > it aligned properly. Unfortunately the entailing discussion did not
> > > yield a solution at the time.
> > 
> > And how exactly does the MMC bounce buffer not help here?
> 
> The problem solved by the bounce buffer is that it is properly cache-
> line aligned. However the issue here is not that the buffer is not
> properly aligned but rather that the transfer is too small.
> 
> When the MMC core transfers the SCR, it requests 8 bytes. The buffer
> used to store these 8 bytes is properly allocated. However, those 8
> bytes eventually end up as the size of the range that is to be
> invalidated. This is the reason for the warning. Address x of the buffer
> is cache-line aligned, but x + 8 is never properly aligned and therefore
> the code complains.

The bounce buffer also up-aligns the newly buffer size to ARCH_DMA_MINALIGN, so 
you can safely up-align you cacheline flush/invalidate sizes.

> Thierry

Best regards,
Marek Vasut
Simon Glass - Sept. 16, 2012, 2:45 a.m.
Hi,

On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
>> Dear Thierry Reding,
>>
>> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
>> > > Hi,
>> > >
>> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
>> > > > Dear Stephen Warren,
>> > > >
>> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
>> > > >> > Dear Stephen Warren,
>> > > >> >
>> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
>> > > >> >>> Folks,
>> > > >> >>>
>> > > >> >>> Stephen Warren has posted an internal bug regarding the cache
>> > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
>> > > >> >>> Here's the gist:
>> > > >> >>>
>> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
>> > > >> >>>
>> > > >> >>> Tegra20 (Harmony) # mmc dev 0
>> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
>> > > >> >>> 0x3fb69908 mmc0 is current device
>> > > >> >>
>> > > >> >> ...
>> > > >> >>
>> > > >> >>> There have been patches in the past (IIRC) that have tried to
>> > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
>> > > >> >>> buffers to the appropriate alignment, but I don't know that we
>> > > >> >>> can ever correct every instance, now or in the future.
>> > > >> >>>
>> > > >> >>> Can we start a discussion about what we can do about this warning?
>> > > >> >>> Adding an appropriate #ifdef
>> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
>> > > >> >>> his #if 0's would be one approach, or changing the printf() to a
>> > > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
>> > > >> >>> don't seem to produce bad data in the transfer.
>> > > >> >>
>> > > >> >> I don't think simply turning off the warning is the correct
>> > > >> >> approach; I believe they represent real problems that can in fact
>> > > >> >> cause data corruption. I don't believe we have any choice other
>> > > >> >> than to fully solve the root-cause.
>> > >
>> > > Yes I agree, and I think it is pretty close - certainly much better
>> > > than it used to be. The good thing about them being annoying is that
>> > > they will likely get fixed :-)
>> >
>> > I think I traced this to the copying of CSD a while back. The problem is
>> > that the transferred buffer is 8 bytes, so there's no way to make it
>> > aligned properly. Unfortunately the entailing discussion did not yield a
>> > solution at the time.
>>
>> And how exactly does the MMC bounce buffer not help here?
>
> The problem solved by the bounce buffer is that it is properly cache-
> line aligned. However the issue here is not that the buffer is not
> properly aligned but rather that the transfer is too small.
>
> When the MMC core transfers the SCR, it requests 8 bytes. The buffer
> used to store these 8 bytes is properly allocated. However, those 8
> bytes eventually end up as the size of the range that is to be
> invalidated. This is the reason for the warning. Address x of the buffer
> is cache-line aligned, but x + 8 is never properly aligned and therefore
> the code complains.

Would it be too dreadful to define a minimum MMC transfer size, and
set that to the cache line size?

While the bounce buffers are useful, I thought the intent was that
they would be a temporary aeration while we fix the code that calls
MMC?

Regards,
Simon

>
> Thierry
Thierry Reding - Sept. 16, 2012, 6:49 a.m.
On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
> Hi,
> 
> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> >> Dear Thierry Reding,
> >>
> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> >> > > Hi,
> >> > >
> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> >> > > > Dear Stephen Warren,
> >> > > >
> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> >> > > >> > Dear Stephen Warren,
> >> > > >> >
> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> >> > > >> >>> Folks,
> >> > > >> >>>
> >> > > >> >>> Stephen Warren has posted an internal bug regarding the cache
> >> > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
> >> > > >> >>> Here's the gist:
> >> > > >> >>>
> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> >> > > >> >>>
> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
> >> > > >> >>> 0x3fb69908 mmc0 is current device
> >> > > >> >>
> >> > > >> >> ...
> >> > > >> >>
> >> > > >> >>> There have been patches in the past (IIRC) that have tried to
> >> > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
> >> > > >> >>> buffers to the appropriate alignment, but I don't know that we
> >> > > >> >>> can ever correct every instance, now or in the future.
> >> > > >> >>>
> >> > > >> >>> Can we start a discussion about what we can do about this warning?
> >> > > >> >>> Adding an appropriate #ifdef
> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
> >> > > >> >>> his #if 0's would be one approach, or changing the printf() to a
> >> > > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
> >> > > >> >>> don't seem to produce bad data in the transfer.
> >> > > >> >>
> >> > > >> >> I don't think simply turning off the warning is the correct
> >> > > >> >> approach; I believe they represent real problems that can in fact
> >> > > >> >> cause data corruption. I don't believe we have any choice other
> >> > > >> >> than to fully solve the root-cause.
> >> > >
> >> > > Yes I agree, and I think it is pretty close - certainly much better
> >> > > than it used to be. The good thing about them being annoying is that
> >> > > they will likely get fixed :-)
> >> >
> >> > I think I traced this to the copying of CSD a while back. The problem is
> >> > that the transferred buffer is 8 bytes, so there's no way to make it
> >> > aligned properly. Unfortunately the entailing discussion did not yield a
> >> > solution at the time.
> >>
> >> And how exactly does the MMC bounce buffer not help here?
> >
> > The problem solved by the bounce buffer is that it is properly cache-
> > line aligned. However the issue here is not that the buffer is not
> > properly aligned but rather that the transfer is too small.
> >
> > When the MMC core transfers the SCR, it requests 8 bytes. The buffer
> > used to store these 8 bytes is properly allocated. However, those 8
> > bytes eventually end up as the size of the range that is to be
> > invalidated. This is the reason for the warning. Address x of the buffer
> > is cache-line aligned, but x + 8 is never properly aligned and therefore
> > the code complains.
> 
> Would it be too dreadful to define a minimum MMC transfer size, and
> set that to the cache line size?

I did try setting the data size to the cache line size back then, but
that resulted in an error. After that I gave up. I think what we really
need to do is separate the invalidation size from the transfer size in
order to properly handle these situations. Or alternatively pass an
additional buffer size so the code knows how much needs to be
invalidated. AFAICT this is the only location where this actually
happens. All other transfers are typically block sized so they'll be a
multiple of the cache line anyway.

Thierry
Simon Glass - Sept. 17, 2012, 9:39 p.m.
Hi Thierry,

On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
>> Hi,
>>
>> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
>> <thierry.reding@avionic-design.de> wrote:
>> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
>> >> Dear Thierry Reding,
>> >>
>> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
>> >> > > Hi,
>> >> > >
>> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
>> >> > > > Dear Stephen Warren,
>> >> > > >
>> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
>> >> > > >> > Dear Stephen Warren,
>> >> > > >> >
>> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
>> >> > > >> >>> Folks,
>> >> > > >> >>>
>> >> > > >> >>> Stephen Warren has posted an internal bug regarding the cache
>> >> > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
>> >> > > >> >>> Here's the gist:
>> >> > > >> >>>
>> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
>> >> > > >> >>>
>> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
>> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
>> >> > > >> >>> 0x3fb69908 mmc0 is current device
>> >> > > >> >>
>> >> > > >> >> ...
>> >> > > >> >>
>> >> > > >> >>> There have been patches in the past (IIRC) that have tried to
>> >> > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
>> >> > > >> >>> buffers to the appropriate alignment, but I don't know that we
>> >> > > >> >>> can ever correct every instance, now or in the future.
>> >> > > >> >>>
>> >> > > >> >>> Can we start a discussion about what we can do about this warning?
>> >> > > >> >>> Adding an appropriate #ifdef
>> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
>> >> > > >> >>> his #if 0's would be one approach, or changing the printf() to a
>> >> > > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
>> >> > > >> >>> don't seem to produce bad data in the transfer.
>> >> > > >> >>
>> >> > > >> >> I don't think simply turning off the warning is the correct
>> >> > > >> >> approach; I believe they represent real problems that can in fact
>> >> > > >> >> cause data corruption. I don't believe we have any choice other
>> >> > > >> >> than to fully solve the root-cause.
>> >> > >
>> >> > > Yes I agree, and I think it is pretty close - certainly much better
>> >> > > than it used to be. The good thing about them being annoying is that
>> >> > > they will likely get fixed :-)
>> >> >
>> >> > I think I traced this to the copying of CSD a while back. The problem is
>> >> > that the transferred buffer is 8 bytes, so there's no way to make it
>> >> > aligned properly. Unfortunately the entailing discussion did not yield a
>> >> > solution at the time.
>> >>
>> >> And how exactly does the MMC bounce buffer not help here?
>> >
>> > The problem solved by the bounce buffer is that it is properly cache-
>> > line aligned. However the issue here is not that the buffer is not
>> > properly aligned but rather that the transfer is too small.
>> >
>> > When the MMC core transfers the SCR, it requests 8 bytes. The buffer
>> > used to store these 8 bytes is properly allocated. However, those 8
>> > bytes eventually end up as the size of the range that is to be
>> > invalidated. This is the reason for the warning. Address x of the buffer
>> > is cache-line aligned, but x + 8 is never properly aligned and therefore
>> > the code complains.
>>
>> Would it be too dreadful to define a minimum MMC transfer size, and
>> set that to the cache line size?
>
> I did try setting the data size to the cache line size back then, but
> that resulted in an error. After that I gave up. I think what we really
> need to do is separate the invalidation size from the transfer size in
> order to properly handle these situations. Or alternatively pass an
> additional buffer size so the code knows how much needs to be
> invalidated. AFAICT this is the only location where this actually
> happens. All other transfers are typically block sized so they'll be a
> multiple of the cache line anyway.

I suppose the requirement is that the buffer size is large enough, and
is aligned. Even if fewer bytes are transferred than the size of the
buffer, that still solves the problem. As you say, if we had a way of
saying 'here is a 64-byte buffer but only 16 bytes need to be
transferred' then we would be good. If this is really just an MMC
problem then perhaps the fix can be localised there.

Regards,
Simon

>
> Thierry
Thierry Reding - Sept. 18, 2012, 2:54 p.m.
On Mon, Sep 17, 2012 at 02:39:01PM -0700, Simon Glass wrote:
> Hi Thierry,
> 
> On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
> >> Hi,
> >>
> >> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
> >> <thierry.reding@avionic-design.de> wrote:
> >> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> >> >> Dear Thierry Reding,
> >> >>
> >> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> >> >> > > Hi,
> >> >> > >
> >> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
> >> >> > > > Dear Stephen Warren,
> >> >> > > >
> >> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> >> >> > > >> > Dear Stephen Warren,
> >> >> > > >> >
> >> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> >> >> > > >> >>> Folks,
> >> >> > > >> >>>
> >> >> > > >> >>> Stephen Warren has posted an internal bug regarding the cache
> >> >> > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
> >> >> > > >> >>> Here's the gist:
> >> >> > > >> >>>
> >> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> >> >> > > >> >>>
> >> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> >> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
> >> >> > > >> >>> 0x3fb69908 mmc0 is current device
> >> >> > > >> >>
> >> >> > > >> >> ...
> >> >> > > >> >>
> >> >> > > >> >>> There have been patches in the past (IIRC) that have tried to
> >> >> > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
> >> >> > > >> >>> buffers to the appropriate alignment, but I don't know that we
> >> >> > > >> >>> can ever correct every instance, now or in the future.
> >> >> > > >> >>>
> >> >> > > >> >>> Can we start a discussion about what we can do about this warning?
> >> >> > > >> >>> Adding an appropriate #ifdef
> >> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
> >> >> > > >> >>> his #if 0's would be one approach, or changing the printf() to a
> >> >> > > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
> >> >> > > >> >>> don't seem to produce bad data in the transfer.
> >> >> > > >> >>
> >> >> > > >> >> I don't think simply turning off the warning is the correct
> >> >> > > >> >> approach; I believe they represent real problems that can in fact
> >> >> > > >> >> cause data corruption. I don't believe we have any choice other
> >> >> > > >> >> than to fully solve the root-cause.
> >> >> > >
> >> >> > > Yes I agree, and I think it is pretty close - certainly much better
> >> >> > > than it used to be. The good thing about them being annoying is that
> >> >> > > they will likely get fixed :-)
> >> >> >
> >> >> > I think I traced this to the copying of CSD a while back. The problem is
> >> >> > that the transferred buffer is 8 bytes, so there's no way to make it
> >> >> > aligned properly. Unfortunately the entailing discussion did not yield a
> >> >> > solution at the time.
> >> >>
> >> >> And how exactly does the MMC bounce buffer not help here?
> >> >
> >> > The problem solved by the bounce buffer is that it is properly cache-
> >> > line aligned. However the issue here is not that the buffer is not
> >> > properly aligned but rather that the transfer is too small.
> >> >
> >> > When the MMC core transfers the SCR, it requests 8 bytes. The buffer
> >> > used to store these 8 bytes is properly allocated. However, those 8
> >> > bytes eventually end up as the size of the range that is to be
> >> > invalidated. This is the reason for the warning. Address x of the buffer
> >> > is cache-line aligned, but x + 8 is never properly aligned and therefore
> >> > the code complains.
> >>
> >> Would it be too dreadful to define a minimum MMC transfer size, and
> >> set that to the cache line size?
> >
> > I did try setting the data size to the cache line size back then, but
> > that resulted in an error. After that I gave up. I think what we really
> > need to do is separate the invalidation size from the transfer size in
> > order to properly handle these situations. Or alternatively pass an
> > additional buffer size so the code knows how much needs to be
> > invalidated. AFAICT this is the only location where this actually
> > happens. All other transfers are typically block sized so they'll be a
> > multiple of the cache line anyway.
> 
> I suppose the requirement is that the buffer size is large enough, and
> is aligned. Even if fewer bytes are transferred than the size of the
> buffer, that still solves the problem. As you say, if we had a way of
> saying 'here is a 64-byte buffer but only 16 bytes need to be
> transferred' then we would be good. If this is really just an MMC
> problem then perhaps the fix can be localised there.

At least on Tegra that is the only warning that I've seen. I guess a new
member could be added to the struct mmc_data. Alternatively maybe an
extra flag would be better, something like MMC_DATA_CACHE_ALIGNED. It
could be passed anywhere where it is known that the buffer is properly
sized but not a full cache line is transferred.

Thierry
Simon Glass - Sept. 18, 2012, 6:24 p.m.
Hi Thierry,

On Tue, Sep 18, 2012 at 7:54 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Mon, Sep 17, 2012 at 02:39:01PM -0700, Simon Glass wrote:
>> Hi Thierry,
>>
>> On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
>> <thierry.reding@avionic-design.de> wrote:
>> > On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
>> >> Hi,
>> >>
>> >> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
>> >> <thierry.reding@avionic-design.de> wrote:
>> >> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
>> >> >> Dear Thierry Reding,
>> >> >>
>> >> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
>> >> >> > > Hi,
>> >> >> > >
>> >> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> wrote:
>> >> >> > > > Dear Stephen Warren,
>> >> >> > > >
>> >> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
>> >> >> > > >> > Dear Stephen Warren,
>> >> >> > > >> >
>> >> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
>> >> >> > > >> >>> Folks,
>> >> >> > > >> >>>
>> >> >> > > >> >>> Stephen Warren has posted an internal bug regarding the cache
>> >> >> > > >> >>> alignment 'warnings' seen on Tegra20 boards when accessing MMC.
>> >> >> > > >> >>> Here's the gist:
>> >> >> > > >> >>>
>> >> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
>> >> >> > > >> >>>
>> >> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
>> >> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not aligned-
>> >> >> > > >> >>> 0x3fb69908 mmc0 is current device
>> >> >> > > >> >>
>> >> >> > > >> >> ...
>> >> >> > > >> >>
>> >> >> > > >> >>> There have been patches in the past (IIRC) that have tried to
>> >> >> > > >> >>> ensure all callers (FS, MMC driver, USB driver, etc.) force their
>> >> >> > > >> >>> buffers to the appropriate alignment, but I don't know that we
>> >> >> > > >> >>> can ever correct every instance, now or in the future.
>> >> >> > > >> >>>
>> >> >> > > >> >>> Can we start a discussion about what we can do about this warning?
>> >> >> > > >> >>> Adding an appropriate #ifdef
>> >> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where Stephen put
>> >> >> > > >> >>> his #if 0's would be one approach, or changing the printf() to a
>> >> >> > > >> >>> debug(), perhaps. As far as I can tell, these alignment 'errors'
>> >> >> > > >> >>> don't seem to produce bad data in the transfer.
>> >> >> > > >> >>
>> >> >> > > >> >> I don't think simply turning off the warning is the correct
>> >> >> > > >> >> approach; I believe they represent real problems that can in fact
>> >> >> > > >> >> cause data corruption. I don't believe we have any choice other
>> >> >> > > >> >> than to fully solve the root-cause.
>> >> >> > >
>> >> >> > > Yes I agree, and I think it is pretty close - certainly much better
>> >> >> > > than it used to be. The good thing about them being annoying is that
>> >> >> > > they will likely get fixed :-)
>> >> >> >
>> >> >> > I think I traced this to the copying of CSD a while back. The problem is
>> >> >> > that the transferred buffer is 8 bytes, so there's no way to make it
>> >> >> > aligned properly. Unfortunately the entailing discussion did not yield a
>> >> >> > solution at the time.
>> >> >>
>> >> >> And how exactly does the MMC bounce buffer not help here?
>> >> >
>> >> > The problem solved by the bounce buffer is that it is properly cache-
>> >> > line aligned. However the issue here is not that the buffer is not
>> >> > properly aligned but rather that the transfer is too small.
>> >> >
>> >> > When the MMC core transfers the SCR, it requests 8 bytes. The buffer
>> >> > used to store these 8 bytes is properly allocated. However, those 8
>> >> > bytes eventually end up as the size of the range that is to be
>> >> > invalidated. This is the reason for the warning. Address x of the buffer
>> >> > is cache-line aligned, but x + 8 is never properly aligned and therefore
>> >> > the code complains.
>> >>
>> >> Would it be too dreadful to define a minimum MMC transfer size, and
>> >> set that to the cache line size?
>> >
>> > I did try setting the data size to the cache line size back then, but
>> > that resulted in an error. After that I gave up. I think what we really
>> > need to do is separate the invalidation size from the transfer size in
>> > order to properly handle these situations. Or alternatively pass an
>> > additional buffer size so the code knows how much needs to be
>> > invalidated. AFAICT this is the only location where this actually
>> > happens. All other transfers are typically block sized so they'll be a
>> > multiple of the cache line anyway.
>>
>> I suppose the requirement is that the buffer size is large enough, and
>> is aligned. Even if fewer bytes are transferred than the size of the
>> buffer, that still solves the problem. As you say, if we had a way of
>> saying 'here is a 64-byte buffer but only 16 bytes need to be
>> transferred' then we would be good. If this is really just an MMC
>> problem then perhaps the fix can be localised there.
>
> At least on Tegra that is the only warning that I've seen. I guess a new
> member could be added to the struct mmc_data. Alternatively maybe an
> extra flag would be better, something like MMC_DATA_CACHE_ALIGNED. It
> could be passed anywhere where it is known that the buffer is properly
> sized but not a full cache line is transferred.

Yes a flag sounds reasonable. Some will argue that this is messing
with low-level hardware features in a driver, but really it is just a
hint that no bounce buffer is needed. The driver is free to do what it
likes.

Regards,
Simon

>
> Thierry
Marek Vasut - Sept. 18, 2012, 6:37 p.m.
Dear Simon Glass,

> Hi Thierry,
> 
> On Tue, Sep 18, 2012 at 7:54 AM, Thierry Reding
> 
> <thierry.reding@avionic-design.de> wrote:
> > On Mon, Sep 17, 2012 at 02:39:01PM -0700, Simon Glass wrote:
> >> Hi Thierry,
> >> 
> >> On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
> >> 
> >> <thierry.reding@avionic-design.de> wrote:
> >> > On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
> >> >> Hi,
> >> >> 
> >> >> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
> >> >> 
> >> >> <thierry.reding@avionic-design.de> wrote:
> >> >> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> >> >> >> Dear Thierry Reding,
> >> >> >> 
> >> >> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> >> >> >> > > Hi,
> >> >> >> > > 
> >> >> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> 
wrote:
> >> >> >> > > > Dear Stephen Warren,
> >> >> >> > > > 
> >> >> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> >> >> >> > > >> > Dear Stephen Warren,
> >> >> >> > > >> > 
> >> >> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> >> >> >> > > >> >>> Folks,
> >> >> >> > > >> >>> 
> >> >> >> > > >> >>> Stephen Warren has posted an internal bug regarding the
> >> >> >> > > >> >>> cache alignment 'warnings' seen on Tegra20 boards when
> >> >> >> > > >> >>> accessing MMC. Here's the gist:
> >> >> >> > > >> >>> 
> >> >> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> >> >> >> > > >> >>> 
> >> >> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> >> >> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not
> >> >> >> > > >> >>> aligned- 0x3fb69908 mmc0 is current device
> >> >> >> > > >> >> 
> >> >> >> > > >> >> ...
> >> >> >> > > >> >> 
> >> >> >> > > >> >>> There have been patches in the past (IIRC) that have
> >> >> >> > > >> >>> tried to ensure all callers (FS, MMC driver, USB
> >> >> >> > > >> >>> driver, etc.) force their buffers to the appropriate
> >> >> >> > > >> >>> alignment, but I don't know that we can ever correct
> >> >> >> > > >> >>> every instance, now or in the future.
> >> >> >> > > >> >>> 
> >> >> >> > > >> >>> Can we start a discussion about what we can do about
> >> >> >> > > >> >>> this warning? Adding an appropriate #ifdef
> >> >> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where
> >> >> >> > > >> >>> Stephen put his #if 0's would be one approach, or
> >> >> >> > > >> >>> changing the printf() to a debug(), perhaps. As far as
> >> >> >> > > >> >>> I can tell, these alignment 'errors' don't seem to
> >> >> >> > > >> >>> produce bad data in the transfer.
> >> >> >> > > >> >> 
> >> >> >> > > >> >> I don't think simply turning off the warning is the
> >> >> >> > > >> >> correct approach; I believe they represent real
> >> >> >> > > >> >> problems that can in fact cause data corruption. I
> >> >> >> > > >> >> don't believe we have any choice other than to fully
> >> >> >> > > >> >> solve the root-cause.
> >> >> >> > > 
> >> >> >> > > Yes I agree, and I think it is pretty close - certainly much
> >> >> >> > > better than it used to be. The good thing about them being
> >> >> >> > > annoying is that they will likely get fixed :-)
> >> >> >> > 
> >> >> >> > I think I traced this to the copying of CSD a while back. The
> >> >> >> > problem is that the transferred buffer is 8 bytes, so there's
> >> >> >> > no way to make it aligned properly. Unfortunately the entailing
> >> >> >> > discussion did not yield a solution at the time.
> >> >> >> 
> >> >> >> And how exactly does the MMC bounce buffer not help here?
> >> >> > 
> >> >> > The problem solved by the bounce buffer is that it is properly
> >> >> > cache- line aligned. However the issue here is not that the buffer
> >> >> > is not properly aligned but rather that the transfer is too small.
> >> >> > 
> >> >> > When the MMC core transfers the SCR, it requests 8 bytes. The
> >> >> > buffer used to store these 8 bytes is properly allocated. However,
> >> >> > those 8 bytes eventually end up as the size of the range that is
> >> >> > to be invalidated. This is the reason for the warning. Address x
> >> >> > of the buffer is cache-line aligned, but x + 8 is never properly
> >> >> > aligned and therefore the code complains.
> >> >> 
> >> >> Would it be too dreadful to define a minimum MMC transfer size, and
> >> >> set that to the cache line size?
> >> > 
> >> > I did try setting the data size to the cache line size back then, but
> >> > that resulted in an error. After that I gave up. I think what we
> >> > really need to do is separate the invalidation size from the transfer
> >> > size in order to properly handle these situations. Or alternatively
> >> > pass an additional buffer size so the code knows how much needs to be
> >> > invalidated. AFAICT this is the only location where this actually
> >> > happens. All other transfers are typically block sized so they'll be
> >> > a multiple of the cache line anyway.
> >> 
> >> I suppose the requirement is that the buffer size is large enough, and
> >> is aligned. Even if fewer bytes are transferred than the size of the
> >> buffer, that still solves the problem. As you say, if we had a way of
> >> saying 'here is a 64-byte buffer but only 16 bytes need to be
> >> transferred' then we would be good. If this is really just an MMC
> >> problem then perhaps the fix can be localised there.
> > 
> > At least on Tegra that is the only warning that I've seen. I guess a new
> > member could be added to the struct mmc_data. Alternatively maybe an
> > extra flag would be better, something like MMC_DATA_CACHE_ALIGNED. It
> > could be passed anywhere where it is known that the buffer is properly
> > sized but not a full cache line is transferred.
> 
> Yes a flag sounds reasonable. Some will argue that this is messing
> with low-level hardware features in a driver, but really it is just a
> hint that no bounce buffer is needed. The driver is free to do what it
> likes.

What about user passing you unaligned data?

I think I'm missing something here, I think I need a tegra20 board with this 
problem. I fail to see why the bounce buffer doesn't solve this.

> Regards,
> Simon
> 
> > Thierry
Thierry Reding - Sept. 18, 2012, 7 p.m.
On Tue, Sep 18, 2012 at 08:37:44PM +0200, Marek Vasut wrote:
> Dear Simon Glass,
> 
> > Hi Thierry,
> > 
> > On Tue, Sep 18, 2012 at 7:54 AM, Thierry Reding
> > 
> > <thierry.reding@avionic-design.de> wrote:
> > > On Mon, Sep 17, 2012 at 02:39:01PM -0700, Simon Glass wrote:
> > >> Hi Thierry,
> > >> 
> > >> On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
> > >> 
> > >> <thierry.reding@avionic-design.de> wrote:
> > >> > On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
> > >> >> Hi,
> > >> >> 
> > >> >> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
> > >> >> 
> > >> >> <thierry.reding@avionic-design.de> wrote:
> > >> >> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> > >> >> >> Dear Thierry Reding,
> > >> >> >> 
> > >> >> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> > >> >> >> > > Hi,
> > >> >> >> > > 
> > >> >> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut <marex@denx.de> 
> wrote:
> > >> >> >> > > > Dear Stephen Warren,
> > >> >> >> > > > 
> > >> >> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > >> >> >> > > >> > Dear Stephen Warren,
> > >> >> >> > > >> > 
> > >> >> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > >> >> >> > > >> >>> Folks,
> > >> >> >> > > >> >>> 
> > >> >> >> > > >> >>> Stephen Warren has posted an internal bug regarding the
> > >> >> >> > > >> >>> cache alignment 'warnings' seen on Tegra20 boards when
> > >> >> >> > > >> >>> accessing MMC. Here's the gist:
> > >> >> >> > > >> >>> 
> > >> >> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> > >> >> >> > > >> >>> 
> > >> >> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> > >> >> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not
> > >> >> >> > > >> >>> aligned- 0x3fb69908 mmc0 is current device
> > >> >> >> > > >> >> 
> > >> >> >> > > >> >> ...
> > >> >> >> > > >> >> 
> > >> >> >> > > >> >>> There have been patches in the past (IIRC) that have
> > >> >> >> > > >> >>> tried to ensure all callers (FS, MMC driver, USB
> > >> >> >> > > >> >>> driver, etc.) force their buffers to the appropriate
> > >> >> >> > > >> >>> alignment, but I don't know that we can ever correct
> > >> >> >> > > >> >>> every instance, now or in the future.
> > >> >> >> > > >> >>> 
> > >> >> >> > > >> >>> Can we start a discussion about what we can do about
> > >> >> >> > > >> >>> this warning? Adding an appropriate #ifdef
> > >> >> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.) where
> > >> >> >> > > >> >>> Stephen put his #if 0's would be one approach, or
> > >> >> >> > > >> >>> changing the printf() to a debug(), perhaps. As far as
> > >> >> >> > > >> >>> I can tell, these alignment 'errors' don't seem to
> > >> >> >> > > >> >>> produce bad data in the transfer.
> > >> >> >> > > >> >> 
> > >> >> >> > > >> >> I don't think simply turning off the warning is the
> > >> >> >> > > >> >> correct approach; I believe they represent real
> > >> >> >> > > >> >> problems that can in fact cause data corruption. I
> > >> >> >> > > >> >> don't believe we have any choice other than to fully
> > >> >> >> > > >> >> solve the root-cause.
> > >> >> >> > > 
> > >> >> >> > > Yes I agree, and I think it is pretty close - certainly much
> > >> >> >> > > better than it used to be. The good thing about them being
> > >> >> >> > > annoying is that they will likely get fixed :-)
> > >> >> >> > 
> > >> >> >> > I think I traced this to the copying of CSD a while back. The
> > >> >> >> > problem is that the transferred buffer is 8 bytes, so there's
> > >> >> >> > no way to make it aligned properly. Unfortunately the entailing
> > >> >> >> > discussion did not yield a solution at the time.
> > >> >> >> 
> > >> >> >> And how exactly does the MMC bounce buffer not help here?
> > >> >> > 
> > >> >> > The problem solved by the bounce buffer is that it is properly
> > >> >> > cache- line aligned. However the issue here is not that the buffer
> > >> >> > is not properly aligned but rather that the transfer is too small.
> > >> >> > 
> > >> >> > When the MMC core transfers the SCR, it requests 8 bytes. The
> > >> >> > buffer used to store these 8 bytes is properly allocated. However,
> > >> >> > those 8 bytes eventually end up as the size of the range that is
> > >> >> > to be invalidated. This is the reason for the warning. Address x
> > >> >> > of the buffer is cache-line aligned, but x + 8 is never properly
> > >> >> > aligned and therefore the code complains.
> > >> >> 
> > >> >> Would it be too dreadful to define a minimum MMC transfer size, and
> > >> >> set that to the cache line size?
> > >> > 
> > >> > I did try setting the data size to the cache line size back then, but
> > >> > that resulted in an error. After that I gave up. I think what we
> > >> > really need to do is separate the invalidation size from the transfer
> > >> > size in order to properly handle these situations. Or alternatively
> > >> > pass an additional buffer size so the code knows how much needs to be
> > >> > invalidated. AFAICT this is the only location where this actually
> > >> > happens. All other transfers are typically block sized so they'll be
> > >> > a multiple of the cache line anyway.
> > >> 
> > >> I suppose the requirement is that the buffer size is large enough, and
> > >> is aligned. Even if fewer bytes are transferred than the size of the
> > >> buffer, that still solves the problem. As you say, if we had a way of
> > >> saying 'here is a 64-byte buffer but only 16 bytes need to be
> > >> transferred' then we would be good. If this is really just an MMC
> > >> problem then perhaps the fix can be localised there.
> > > 
> > > At least on Tegra that is the only warning that I've seen. I guess a new
> > > member could be added to the struct mmc_data. Alternatively maybe an
> > > extra flag would be better, something like MMC_DATA_CACHE_ALIGNED. It
> > > could be passed anywhere where it is known that the buffer is properly
> > > sized but not a full cache line is transferred.
> > 
> > Yes a flag sounds reasonable. Some will argue that this is messing
> > with low-level hardware features in a driver, but really it is just a
> > hint that no bounce buffer is needed. The driver is free to do what it
> > likes.
> 
> What about user passing you unaligned data?
> 
> I think I'm missing something here, I think I need a tegra20 board with this 
> problem. I fail to see why the bounce buffer doesn't solve this.

The problem here is not that the user is passing unaligned data. Rather
the user, the MMC core in this case, passes a properly aligned buffer
that is too short (8 bytes), so that when the cache invalidation code is
called with the length of 8 bytes it complains that the end of the
buffer isn't properly aligned. The bounce buffer won't solve this
because, instead of the buffer size, the transfer size is passed to the
invalidation routine.

This is by no means Tegra specific. In fact every board that has proper
cache invalidation support should expose this problem.

Thierry
Marek Vasut - Sept. 18, 2012, 7:21 p.m.
Dear Thierry Reding,

> On Tue, Sep 18, 2012 at 08:37:44PM +0200, Marek Vasut wrote:
> > Dear Simon Glass,
> > 
> > > Hi Thierry,
> > > 
> > > On Tue, Sep 18, 2012 at 7:54 AM, Thierry Reding
> > > 
> > > <thierry.reding@avionic-design.de> wrote:
> > > > On Mon, Sep 17, 2012 at 02:39:01PM -0700, Simon Glass wrote:
> > > >> Hi Thierry,
> > > >> 
> > > >> On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
> > > >> 
> > > >> <thierry.reding@avionic-design.de> wrote:
> > > >> > On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
> > > >> >> Hi,
> > > >> >> 
> > > >> >> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
> > > >> >> 
> > > >> >> <thierry.reding@avionic-design.de> wrote:
> > > >> >> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> > > >> >> >> Dear Thierry Reding,
> > > >> >> >> 
> > > >> >> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> > > >> >> >> > > Hi,
> > > >> >> >> > > 
> > > >> >> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut
> > > >> >> >> > > <marex@denx.de>
> > 
> > wrote:
> > > >> >> >> > > > Dear Stephen Warren,
> > > >> >> >> > > > 
> > > >> >> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > > >> >> >> > > >> > Dear Stephen Warren,
> > > >> >> >> > > >> > 
> > > >> >> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > > >> >> >> > > >> >>> Folks,
> > > >> >> >> > > >> >>> 
> > > >> >> >> > > >> >>> Stephen Warren has posted an internal bug regarding
> > > >> >> >> > > >> >>> the cache alignment 'warnings' seen on Tegra20
> > > >> >> >> > > >> >>> boards when accessing MMC. Here's the gist:
> > > >> >> >> > > >> >>> 
> > > >> >> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> > > >> >> >> > > >> >>> 
> > > >> >> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> > > >> >> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not
> > > >> >> >> > > >> >>> aligned- 0x3fb69908 mmc0 is current device
> > > >> >> >> > > >> >> 
> > > >> >> >> > > >> >> ...
> > > >> >> >> > > >> >> 
> > > >> >> >> > > >> >>> There have been patches in the past (IIRC) that
> > > >> >> >> > > >> >>> have tried to ensure all callers (FS, MMC driver,
> > > >> >> >> > > >> >>> USB driver, etc.) force their buffers to the
> > > >> >> >> > > >> >>> appropriate alignment, but I don't know that we
> > > >> >> >> > > >> >>> can ever correct every instance, now or in the
> > > >> >> >> > > >> >>> future.
> > > >> >> >> > > >> >>> 
> > > >> >> >> > > >> >>> Can we start a discussion about what we can do
> > > >> >> >> > > >> >>> about this warning? Adding an appropriate #ifdef
> > > >> >> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.)
> > > >> >> >> > > >> >>> where Stephen put his #if 0's would be one
> > > >> >> >> > > >> >>> approach, or changing the printf() to a debug(),
> > > >> >> >> > > >> >>> perhaps. As far as I can tell, these alignment
> > > >> >> >> > > >> >>> 'errors' don't seem to produce bad data in the
> > > >> >> >> > > >> >>> transfer.
> > > >> >> >> > > >> >> 
> > > >> >> >> > > >> >> I don't think simply turning off the warning is the
> > > >> >> >> > > >> >> correct approach; I believe they represent real
> > > >> >> >> > > >> >> problems that can in fact cause data corruption. I
> > > >> >> >> > > >> >> don't believe we have any choice other than to fully
> > > >> >> >> > > >> >> solve the root-cause.
> > > >> >> >> > > 
> > > >> >> >> > > Yes I agree, and I think it is pretty close - certainly
> > > >> >> >> > > much better than it used to be. The good thing about them
> > > >> >> >> > > being annoying is that they will likely get fixed :-)
> > > >> >> >> > 
> > > >> >> >> > I think I traced this to the copying of CSD a while back.
> > > >> >> >> > The problem is that the transferred buffer is 8 bytes, so
> > > >> >> >> > there's no way to make it aligned properly. Unfortunately
> > > >> >> >> > the entailing discussion did not yield a solution at the
> > > >> >> >> > time.
> > > >> >> >> 
> > > >> >> >> And how exactly does the MMC bounce buffer not help here?
> > > >> >> > 
> > > >> >> > The problem solved by the bounce buffer is that it is properly
> > > >> >> > cache- line aligned. However the issue here is not that the
> > > >> >> > buffer is not properly aligned but rather that the transfer is
> > > >> >> > too small.
> > > >> >> > 
> > > >> >> > When the MMC core transfers the SCR, it requests 8 bytes. The
> > > >> >> > buffer used to store these 8 bytes is properly allocated.
> > > >> >> > However, those 8 bytes eventually end up as the size of the
> > > >> >> > range that is to be invalidated. This is the reason for the
> > > >> >> > warning. Address x of the buffer is cache-line aligned, but x
> > > >> >> > + 8 is never properly aligned and therefore the code
> > > >> >> > complains.
> > > >> >> 
> > > >> >> Would it be too dreadful to define a minimum MMC transfer size,
> > > >> >> and set that to the cache line size?
> > > >> > 
> > > >> > I did try setting the data size to the cache line size back then,
> > > >> > but that resulted in an error. After that I gave up. I think what
> > > >> > we really need to do is separate the invalidation size from the
> > > >> > transfer size in order to properly handle these situations. Or
> > > >> > alternatively pass an additional buffer size so the code knows
> > > >> > how much needs to be invalidated. AFAICT this is the only
> > > >> > location where this actually happens. All other transfers are
> > > >> > typically block sized so they'll be a multiple of the cache line
> > > >> > anyway.
> > > >> 
> > > >> I suppose the requirement is that the buffer size is large enough,
> > > >> and is aligned. Even if fewer bytes are transferred than the size
> > > >> of the buffer, that still solves the problem. As you say, if we had
> > > >> a way of saying 'here is a 64-byte buffer but only 16 bytes need to
> > > >> be transferred' then we would be good. If this is really just an
> > > >> MMC problem then perhaps the fix can be localised there.
> > > > 
> > > > At least on Tegra that is the only warning that I've seen. I guess a
> > > > new member could be added to the struct mmc_data. Alternatively
> > > > maybe an extra flag would be better, something like
> > > > MMC_DATA_CACHE_ALIGNED. It could be passed anywhere where it is
> > > > known that the buffer is properly sized but not a full cache line is
> > > > transferred.
> > > 
> > > Yes a flag sounds reasonable. Some will argue that this is messing
> > > with low-level hardware features in a driver, but really it is just a
> > > hint that no bounce buffer is needed. The driver is free to do what it
> > > likes.
> > 
> > What about user passing you unaligned data?
> > 
> > I think I'm missing something here, I think I need a tegra20 board with
> > this problem. I fail to see why the bounce buffer doesn't solve this.
> 
> The problem here is not that the user is passing unaligned data. Rather
> the user, the MMC core in this case, passes a properly aligned buffer
> that is too short (8 bytes), so that when the cache invalidation code is
> called with the length of 8 bytes it complains that the end of the
> buffer isn't properly aligned. The bounce buffer won't solve this
> because, instead of the buffer size, the transfer size is passed to the
> invalidation routine.

Sure, but after you apply the bounce buffer, you can safely invalidate the whole 
cacheline, so align it up and be done with it.

> This is by no means Tegra specific. In fact every board that has proper
> cache invalidation support should expose this problem.

Yea of course, the arm926ejs had this trouble too, see the mxs MMC driver and 
arm926 cache.c

> Thierry
Thierry Reding - Sept. 18, 2012, 7:29 p.m.
On Tue, Sep 18, 2012 at 09:21:14PM +0200, Marek Vasut wrote:
> Dear Thierry Reding,
> 
> > On Tue, Sep 18, 2012 at 08:37:44PM +0200, Marek Vasut wrote:
> > > Dear Simon Glass,
> > > 
> > > > Hi Thierry,
> > > > 
> > > > On Tue, Sep 18, 2012 at 7:54 AM, Thierry Reding
> > > > 
> > > > <thierry.reding@avionic-design.de> wrote:
> > > > > On Mon, Sep 17, 2012 at 02:39:01PM -0700, Simon Glass wrote:
> > > > >> Hi Thierry,
> > > > >> 
> > > > >> On Sat, Sep 15, 2012 at 11:49 PM, Thierry Reding
> > > > >> 
> > > > >> <thierry.reding@avionic-design.de> wrote:
> > > > >> > On Sat, Sep 15, 2012 at 07:45:30PM -0700, Simon Glass wrote:
> > > > >> >> Hi,
> > > > >> >> 
> > > > >> >> On Sat, Sep 15, 2012 at 1:41 PM, Thierry Reding
> > > > >> >> 
> > > > >> >> <thierry.reding@avionic-design.de> wrote:
> > > > >> >> > On Sat, Sep 15, 2012 at 10:11:54PM +0200, Marek Vasut wrote:
> > > > >> >> >> Dear Thierry Reding,
> > > > >> >> >> 
> > > > >> >> >> > On Fri, Sep 14, 2012 at 08:53:32AM -0700, Simon Glass wrote:
> > > > >> >> >> > > Hi,
> > > > >> >> >> > > 
> > > > >> >> >> > > On Wed, Sep 12, 2012 at 4:42 PM, Marek Vasut
> > > > >> >> >> > > <marex@denx.de>
> > > 
> > > wrote:
> > > > >> >> >> > > > Dear Stephen Warren,
> > > > >> >> >> > > > 
> > > > >> >> >> > > >> On 09/12/2012 04:38 PM, Marek Vasut wrote:
> > > > >> >> >> > > >> > Dear Stephen Warren,
> > > > >> >> >> > > >> > 
> > > > >> >> >> > > >> >> On 09/12/2012 10:19 AM, Tom Warren wrote:
> > > > >> >> >> > > >> >>> Folks,
> > > > >> >> >> > > >> >>> 
> > > > >> >> >> > > >> >>> Stephen Warren has posted an internal bug regarding
> > > > >> >> >> > > >> >>> the cache alignment 'warnings' seen on Tegra20
> > > > >> >> >> > > >> >>> boards when accessing MMC. Here's the gist:
> > > > >> >> >> > > >> >>> 
> > > > >> >> >> > > >> >>> Executing "mmc dev 0" still yields cache warnings:
> > > > >> >> >> > > >> >>> 
> > > > >> >> >> > > >> >>> Tegra20 (Harmony) # mmc dev 0
> > > > >> >> >> > > >> >>> ERROR: v7_dcache_inval_range- stop address is not
> > > > >> >> >> > > >> >>> aligned- 0x3fb69908 mmc0 is current device
> > > > >> >> >> > > >> >> 
> > > > >> >> >> > > >> >> ...
> > > > >> >> >> > > >> >> 
> > > > >> >> >> > > >> >>> There have been patches in the past (IIRC) that
> > > > >> >> >> > > >> >>> have tried to ensure all callers (FS, MMC driver,
> > > > >> >> >> > > >> >>> USB driver, etc.) force their buffers to the
> > > > >> >> >> > > >> >>> appropriate alignment, but I don't know that we
> > > > >> >> >> > > >> >>> can ever correct every instance, now or in the
> > > > >> >> >> > > >> >>> future.
> > > > >> >> >> > > >> >>> 
> > > > >> >> >> > > >> >>> Can we start a discussion about what we can do
> > > > >> >> >> > > >> >>> about this warning? Adding an appropriate #ifdef
> > > > >> >> >> > > >> >>> (CONFIG_SYS_NO_CACHE_ALIGNMENT_WARNINGS, etc.)
> > > > >> >> >> > > >> >>> where Stephen put his #if 0's would be one
> > > > >> >> >> > > >> >>> approach, or changing the printf() to a debug(),
> > > > >> >> >> > > >> >>> perhaps. As far as I can tell, these alignment
> > > > >> >> >> > > >> >>> 'errors' don't seem to produce bad data in the
> > > > >> >> >> > > >> >>> transfer.
> > > > >> >> >> > > >> >> 
> > > > >> >> >> > > >> >> I don't think simply turning off the warning is the
> > > > >> >> >> > > >> >> correct approach; I believe they represent real
> > > > >> >> >> > > >> >> problems that can in fact cause data corruption. I
> > > > >> >> >> > > >> >> don't believe we have any choice other than to fully
> > > > >> >> >> > > >> >> solve the root-cause.
> > > > >> >> >> > > 
> > > > >> >> >> > > Yes I agree, and I think it is pretty close - certainly
> > > > >> >> >> > > much better than it used to be. The good thing about them
> > > > >> >> >> > > being annoying is that they will likely get fixed :-)
> > > > >> >> >> > 
> > > > >> >> >> > I think I traced this to the copying of CSD a while back.
> > > > >> >> >> > The problem is that the transferred buffer is 8 bytes, so
> > > > >> >> >> > there's no way to make it aligned properly. Unfortunately
> > > > >> >> >> > the entailing discussion did not yield a solution at the
> > > > >> >> >> > time.
> > > > >> >> >> 
> > > > >> >> >> And how exactly does the MMC bounce buffer not help here?
> > > > >> >> > 
> > > > >> >> > The problem solved by the bounce buffer is that it is properly
> > > > >> >> > cache- line aligned. However the issue here is not that the
> > > > >> >> > buffer is not properly aligned but rather that the transfer is
> > > > >> >> > too small.
> > > > >> >> > 
> > > > >> >> > When the MMC core transfers the SCR, it requests 8 bytes. The
> > > > >> >> > buffer used to store these 8 bytes is properly allocated.
> > > > >> >> > However, those 8 bytes eventually end up as the size of the
> > > > >> >> > range that is to be invalidated. This is the reason for the
> > > > >> >> > warning. Address x of the buffer is cache-line aligned, but x
> > > > >> >> > + 8 is never properly aligned and therefore the code
> > > > >> >> > complains.
> > > > >> >> 
> > > > >> >> Would it be too dreadful to define a minimum MMC transfer size,
> > > > >> >> and set that to the cache line size?
> > > > >> > 
> > > > >> > I did try setting the data size to the cache line size back then,
> > > > >> > but that resulted in an error. After that I gave up. I think what
> > > > >> > we really need to do is separate the invalidation size from the
> > > > >> > transfer size in order to properly handle these situations. Or
> > > > >> > alternatively pass an additional buffer size so the code knows
> > > > >> > how much needs to be invalidated. AFAICT this is the only
> > > > >> > location where this actually happens. All other transfers are
> > > > >> > typically block sized so they'll be a multiple of the cache line
> > > > >> > anyway.
> > > > >> 
> > > > >> I suppose the requirement is that the buffer size is large enough,
> > > > >> and is aligned. Even if fewer bytes are transferred than the size
> > > > >> of the buffer, that still solves the problem. As you say, if we had
> > > > >> a way of saying 'here is a 64-byte buffer but only 16 bytes need to
> > > > >> be transferred' then we would be good. If this is really just an
> > > > >> MMC problem then perhaps the fix can be localised there.
> > > > > 
> > > > > At least on Tegra that is the only warning that I've seen. I guess a
> > > > > new member could be added to the struct mmc_data. Alternatively
> > > > > maybe an extra flag would be better, something like
> > > > > MMC_DATA_CACHE_ALIGNED. It could be passed anywhere where it is
> > > > > known that the buffer is properly sized but not a full cache line is
> > > > > transferred.
> > > > 
> > > > Yes a flag sounds reasonable. Some will argue that this is messing
> > > > with low-level hardware features in a driver, but really it is just a
> > > > hint that no bounce buffer is needed. The driver is free to do what it
> > > > likes.
> > > 
> > > What about user passing you unaligned data?
> > > 
> > > I think I'm missing something here, I think I need a tegra20 board with
> > > this problem. I fail to see why the bounce buffer doesn't solve this.
> > 
> > The problem here is not that the user is passing unaligned data. Rather
> > the user, the MMC core in this case, passes a properly aligned buffer
> > that is too short (8 bytes), so that when the cache invalidation code is
> > called with the length of 8 bytes it complains that the end of the
> > buffer isn't properly aligned. The bounce buffer won't solve this
> > because, instead of the buffer size, the transfer size is passed to the
> > invalidation routine.
> 
> Sure, but after you apply the bounce buffer, you can safely invalidate the whole 
> cacheline, so align it up and be done with it.

That's what I proposed to do last time around but it was NAK'ed. At the
time I didn't ensure that the buffer was actually big enough, which is
why people didn't like it (data on the stack after the DMA buffer might
be invalidated as well).

> > This is by no means Tegra specific. In fact every board that has proper
> > cache invalidation support should expose this problem.
> 
> Yea of course, the arm926ejs had this trouble too, see the mxs MMC driver and 
> arm926 cache.c

I suppose we could do something like this as well. If we make sure that
the buffer is properly aligned and and sized, we could pass the aligned
end address to invalidate_dcache_range().

Thierry
Marek Vasut - Sept. 18, 2012, 7:36 p.m.
Dear Thierry Reding,

[...]

> > Sure, but after you apply the bounce buffer, you can safely invalidate
> > the whole cacheline, so align it up and be done with it.
> 
> That's what I proposed to do last time around but it was NAK'ed.

By who?

> At the
> time I didn't ensure that the buffer was actually big enough, which is
> why people didn't like it (data on the stack after the DMA buffer might
> be invalidated as well).

Correct, thus the bounce buffer.

> > > This is by no means Tegra specific. In fact every board that has proper
> > > cache invalidation support should expose this problem.
> > 
> > Yea of course, the arm926ejs had this trouble too, see the mxs MMC driver
> > and arm926 cache.c
> 
> I suppose we could do something like this as well. If we make sure that
> the buffer is properly aligned and and sized, we could pass the aligned
> end address to invalidate_dcache_range().
> 
> Thierry

Best regards,
Marek Vasut
Thierry Reding - Sept. 18, 2012, 8:04 p.m.
On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
> Dear Thierry Reding,
> 
> [...]
> 
> > > Sure, but after you apply the bounce buffer, you can safely invalidate
> > > the whole cacheline, so align it up and be done with it.
> > 
> > That's what I proposed to do last time around but it was NAK'ed.
> 
> By who?

I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
valid reason, so I'm not complaining.

> > At the
> > time I didn't ensure that the buffer was actually big enough, which is
> > why people didn't like it (data on the stack after the DMA buffer might
> > be invalidated as well).
> 
> Correct, thus the bounce buffer.

I don't think we even need the bounce buffer. All that needs to be done
is guarantee that the buffers passed to the MMC driver are properly
aligned and sized.

Thierry
Simon Glass - Sept. 18, 2012, 8:28 p.m.
Hi,

On Tue, Sep 18, 2012 at 1:04 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
>> Dear Thierry Reding,
>>
>> [...]
>>
>> > > Sure, but after you apply the bounce buffer, you can safely invalidate
>> > > the whole cacheline, so align it up and be done with it.
>> >
>> > That's what I proposed to do last time around but it was NAK'ed.
>>
>> By who?
>
> I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
> valid reason, so I'm not complaining.
>
>> > At the
>> > time I didn't ensure that the buffer was actually big enough, which is
>> > why people didn't like it (data on the stack after the DMA buffer might
>> > be invalidated as well).
>>
>> Correct, thus the bounce buffer.
>
> I don't think we even need the bounce buffer. All that needs to be done
> is guarantee that the buffers passed to the MMC driver are properly
> aligned and sized.
>
> Thierry

Perhaps a point to make here is that we really don't want every driver
(or even driver stack) implementing bounce buffers to when it is not a
huge effort to change the code that calls them (typically filesystem
code) to do the right thing. The code will be smaller and more
efficient if the alignment issues are dealt with at source IMO.

Regards,
Simon
Marek Vasut - Sept. 18, 2012, 9:20 p.m.
Dear Thierry Reding,

> On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
> > Dear Thierry Reding,
> > 
> > [...]
> > 
> > > > Sure, but after you apply the bounce buffer, you can safely
> > > > invalidate the whole cacheline, so align it up and be done with it.
> > > 
> > > That's what I proposed to do last time around but it was NAK'ed.
> > 
> > By who?
> 
> I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
> valid reason, so I'm not complaining.
> 
> > > At the
> > > time I didn't ensure that the buffer was actually big enough, which is
> > > why people didn't like it (data on the stack after the DMA buffer might
> > > be invalidated as well).
> > 
> > Correct, thus the bounce buffer.
> 
> I don't think we even need the bounce buffer. All that needs to be done
> is guarantee that the buffers passed to the MMC driver are properly
> aligned and sized.

If you resize the MMC structures and call sizeof() on them to get the size of 
the transfer, the MMC won't work correctly anymore.

> Thierry

Best regards,
Marek Vasut
Marek Vasut - Sept. 18, 2012, 9:21 p.m.
Dear Simon Glass,

> Hi,
> 
> On Tue, Sep 18, 2012 at 1:04 PM, Thierry Reding
> 
> <thierry.reding@avionic-design.de> wrote:
> > On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
> >> Dear Thierry Reding,
> >> 
> >> [...]
> >> 
> >> > > Sure, but after you apply the bounce buffer, you can safely
> >> > > invalidate the whole cacheline, so align it up and be done with it.
> >> > 
> >> > That's what I proposed to do last time around but it was NAK'ed.
> >> 
> >> By who?
> > 
> > I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
> > valid reason, so I'm not complaining.
> > 
> >> > At the
> >> > time I didn't ensure that the buffer was actually big enough, which is
> >> > why people didn't like it (data on the stack after the DMA buffer
> >> > might be invalidated as well).
> >> 
> >> Correct, thus the bounce buffer.
> > 
> > I don't think we even need the bounce buffer. All that needs to be done
> > is guarantee that the buffers passed to the MMC driver are properly
> > aligned and sized.
> > 
> > Thierry
> 
> Perhaps a point to make here is that we really don't want every driver
> (or even driver stack) implementing bounce buffers to when it is not a
> huge effort to change the code that calls them (typically filesystem
> code) to do the right thing. The code will be smaller and more
> efficient if the alignment issues are dealt with at source IMO.

You need the BB for user-case, when user gives you misaligned buffer.

Best regards,
Marek Vasut
Simon Glass - Sept. 18, 2012, 10:42 p.m.
Hi,

On Tue, Sep 18, 2012 at 2:21 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Simon Glass,
>
>> Hi,
>>
>> On Tue, Sep 18, 2012 at 1:04 PM, Thierry Reding
>>
>> <thierry.reding@avionic-design.de> wrote:
>> > On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
>> >> Dear Thierry Reding,
>> >>
>> >> [...]
>> >>
>> >> > > Sure, but after you apply the bounce buffer, you can safely
>> >> > > invalidate the whole cacheline, so align it up and be done with it.
>> >> >
>> >> > That's what I proposed to do last time around but it was NAK'ed.
>> >>
>> >> By who?
>> >
>> > I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
>> > valid reason, so I'm not complaining.
>> >
>> >> > At the
>> >> > time I didn't ensure that the buffer was actually big enough, which is
>> >> > why people didn't like it (data on the stack after the DMA buffer
>> >> > might be invalidated as well).
>> >>
>> >> Correct, thus the bounce buffer.
>> >
>> > I don't think we even need the bounce buffer. All that needs to be done
>> > is guarantee that the buffers passed to the MMC driver are properly
>> > aligned and sized.
>> >
>> > Thierry
>>
>> Perhaps a point to make here is that we really don't want every driver
>> (or even driver stack) implementing bounce buffers to when it is not a
>> huge effort to change the code that calls them (typically filesystem
>> code) to do the right thing. The code will be smaller and more
>> efficient if the alignment issues are dealt with at source IMO.
>
> You need the BB for user-case, when user gives you misaligned buffer.

Yes, although I think you are talking about non-filesystem (i.e. raw)
access, and it would be odd to read a kernel to a non-cached-aligned
address. If we want to support that, we can (perhaps even in the
command itself)., but at least U-Boot's own code should ideally not
generate unaligned access.

Regards,
Simon

>
> Best regards,
> Marek Vasut
Marek Vasut - Sept. 18, 2012, 10:44 p.m.
Dear Simon Glass,

> Hi,
> 
> On Tue, Sep 18, 2012 at 2:21 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Simon Glass,
> > 
> >> Hi,
> >> 
> >> On Tue, Sep 18, 2012 at 1:04 PM, Thierry Reding
> >> 
> >> <thierry.reding@avionic-design.de> wrote:
> >> > On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
> >> >> Dear Thierry Reding,
> >> >> 
> >> >> [...]
> >> >> 
> >> >> > > Sure, but after you apply the bounce buffer, you can safely
> >> >> > > invalidate the whole cacheline, so align it up and be done with
> >> >> > > it.
> >> >> > 
> >> >> > That's what I proposed to do last time around but it was NAK'ed.
> >> >> 
> >> >> By who?
> >> > 
> >> > I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
> >> > valid reason, so I'm not complaining.
> >> > 
> >> >> > At the
> >> >> > time I didn't ensure that the buffer was actually big enough, which
> >> >> > is why people didn't like it (data on the stack after the DMA
> >> >> > buffer might be invalidated as well).
> >> >> 
> >> >> Correct, thus the bounce buffer.
> >> > 
> >> > I don't think we even need the bounce buffer. All that needs to be
> >> > done is guarantee that the buffers passed to the MMC driver are
> >> > properly aligned and sized.
> >> > 
> >> > Thierry
> >> 
> >> Perhaps a point to make here is that we really don't want every driver
> >> (or even driver stack) implementing bounce buffers to when it is not a
> >> huge effort to change the code that calls them (typically filesystem
> >> code) to do the right thing. The code will be smaller and more
> >> efficient if the alignment issues are dealt with at source IMO.
> > 
> > You need the BB for user-case, when user gives you misaligned buffer.
> 
> Yes, although I think you are talking about non-filesystem (i.e. raw)

Like fatload ? Fatload doesn't use any interim buffer either, so not only raw.

> access, and it would be odd to read a kernel to a non-cached-aligned
> address. If we want to support that, we can (perhaps even in the
> command itself)., but at least U-Boot's own code should ideally not
> generate unaligned access.

Correct

> 
> Regards,
> Simon
> 
> > Best regards,
> > Marek Vasut

Best regards,
Marek Vasut
Thierry Reding - Sept. 19, 2012, 5:45 a.m.
On Wed, Sep 19, 2012 at 12:44:27AM +0200, Marek Vasut wrote:
> Dear Simon Glass,
> 
> > Hi,
> > 
> > On Tue, Sep 18, 2012 at 2:21 PM, Marek Vasut <marex@denx.de> wrote:
> > > Dear Simon Glass,
> > > 
> > >> Hi,
> > >> 
> > >> On Tue, Sep 18, 2012 at 1:04 PM, Thierry Reding
> > >> 
> > >> <thierry.reding@avionic-design.de> wrote:
> > >> > On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
> > >> >> Dear Thierry Reding,
> > >> >> 
> > >> >> [...]
> > >> >> 
> > >> >> > > Sure, but after you apply the bounce buffer, you can safely
> > >> >> > > invalidate the whole cacheline, so align it up and be done with
> > >> >> > > it.
> > >> >> > 
> > >> >> > That's what I proposed to do last time around but it was NAK'ed.
> > >> >> 
> > >> >> By who?
> > >> > 
> > >> > I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
> > >> > valid reason, so I'm not complaining.
> > >> > 
> > >> >> > At the
> > >> >> > time I didn't ensure that the buffer was actually big enough, which
> > >> >> > is why people didn't like it (data on the stack after the DMA
> > >> >> > buffer might be invalidated as well).
> > >> >> 
> > >> >> Correct, thus the bounce buffer.
> > >> > 
> > >> > I don't think we even need the bounce buffer. All that needs to be
> > >> > done is guarantee that the buffers passed to the MMC driver are
> > >> > properly aligned and sized.
> > >> > 
> > >> > Thierry
> > >> 
> > >> Perhaps a point to make here is that we really don't want every driver
> > >> (or even driver stack) implementing bounce buffers to when it is not a
> > >> huge effort to change the code that calls them (typically filesystem
> > >> code) to do the right thing. The code will be smaller and more
> > >> efficient if the alignment issues are dealt with at source IMO.
> > > 
> > > You need the BB for user-case, when user gives you misaligned buffer.
> > 
> > Yes, although I think you are talking about non-filesystem (i.e. raw)
> 
> Like fatload ? Fatload doesn't use any interim buffer either, so not only raw.
> 
> > access, and it would be odd to read a kernel to a non-cached-aligned
> > address. If we want to support that, we can (perhaps even in the
> > command itself)., but at least U-Boot's own code should ideally not
> > generate unaligned access.
> 
> Correct

Okay, so basically we should be able to define CONFIG_MMC_BOUNCE_BUFFER
on Tegra and have the driver assume that whole cache lines can always be
flushed, i.e. round up the flush length to a full cache line. Is that
it?

Thierry
Thierry Reding - Sept. 19, 2012, 5:46 a.m.
On Tue, Sep 18, 2012 at 11:20:30PM +0200, Marek Vasut wrote:
> Dear Thierry Reding,
> 
> > On Tue, Sep 18, 2012 at 09:36:18PM +0200, Marek Vasut wrote:
> > > Dear Thierry Reding,
> > > 
> > > [...]
> > > 
> > > > > Sure, but after you apply the bounce buffer, you can safely
> > > > > invalidate the whole cacheline, so align it up and be done with it.
> > > > 
> > > > That's what I proposed to do last time around but it was NAK'ed.
> > > 
> > > By who?
> > 
> > I think it was Simon Glass and Mike Frysinger. They NAK'ed it for very
> > valid reason, so I'm not complaining.
> > 
> > > > At the
> > > > time I didn't ensure that the buffer was actually big enough, which is
> > > > why people didn't like it (data on the stack after the DMA buffer might
> > > > be invalidated as well).
> > > 
> > > Correct, thus the bounce buffer.
> > 
> > I don't think we even need the bounce buffer. All that needs to be done
> > is guarantee that the buffers passed to the MMC driver are properly
> > aligned and sized.
> 
> If you resize the MMC structures and call sizeof() on them to get the size of 
> the transfer, the MMC won't work correctly anymore.

That's exactly what my first attempt was back then. It was a very naive
attempt at getting the MMC core to pass the correct size and as you said
it didn't work.

Thierry

Patch

diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
index 1b4e808..9031ea1 100644
--- a/arch/arm/cpu/armv7/cache_v7.c
+++ b/arch/arm/cpu/armv7/cache_v7.c
@@ -185,8 +185,10 @@  static void v7_dcache_inval_range(u32 start, u32
stop, u32 line_len)
* invalidate the first cache-line
*/
if (start & (line_len- 1)) {
+#if 0
printf("ERROR: %s- start address is not aligned- 0x%08x\n",
__func__, start);
+#endif
/* move to next cache line */
start = (start + line_len- 1) & ~(line_len- 1);
}
@@ -196,8 +198,10 @@  static void v7_dcache_inval_range(u32 start, u32
stop, u32 line_len)
* invalidate the last cache-line
*/
if (stop & (line_len- 1)) {
+#if 0
printf("ERROR: %s- stop address is not aligned- 0x%08x\n",
__func__, stop);
+#endif
/* align to the beginning of this cache line */
stop &= ~(line_len- 1);
}
diff --git a/arch/arm/lib/cache-pl310.c b/arch/arm/lib/cache-pl310.c
index 21d13f7..d8a343c 100644
--- a/arch/arm/lib/cache-pl310.c
+++ b/arch/arm/lib/cache-pl310.c
@@ -94,8 +94,10 @@  void v7_outer_cache_inval_range(u32 start, u32 stop)
* invalidate the first cache-line
*/
if (start & (line_size- 1)) {
+#if 0
printf("ERROR: %s- start address is not aligned- 0x%08x\n",
__func__, start);
+#endif
/* move to next cache line */
start = (start + line_size- 1) & ~(line_size- 1);
}
@@ -105,8 +107,10 @@  void v7_outer_cache_inval_range(u32 start, u32 stop)
* invalidate the last cache-line
*/
if (stop & (line_size- 1)) {
+#if 0
printf("ERROR: %s- stop address is not aligned- 0x%08x\n",
__func__, stop);
+#endif
/* align to the beginning of this cache line */
stop &= ~(line_size- 1);
}