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

[Thierry Reding]

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 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.
> > 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120918/31a52732/attachment.pgp>