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

[Marek Vasut]

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.

> 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