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

[Marek Vasut]

Dear Simon Glass,

> Hi Thierry,
> 
> On Tue, Sep 18, 2012 at 7:54 AM, Thierry Reding
> 
> <thierry.reding at 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 at 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 at 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 at 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