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

[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.

Regards,
Simon

>
> Thierry