[U-Boot] [PATCH 2/4] cache_v7: Check for dcache enablement in dcache flush functions

[R, Sricharan]

Aneesh,
[snip..]
>>>>>>>> If we are built with D-CACHE enabled but have run 'dcache off' and
>>>>
>>>> then
>>>>>>>>
>>>>>>>> attempt to flush unaligned regions we spam the console with
>>>>
>>>> problems
>>>>>>>>
>>>>>>>> that aren't true (as the cache was off).
>>>>>>>>
>>>>>>>    Today we do cache maintenance operations after the dcache is
>>>>
>>>> turned off.
>>>>>>>
>>>>>>>    One example is before jumping to kernel, we try to invalidate
>>>>
>>>> the caches,
>>>>>>>
>>>>>>>    in cache turned off state. So with this patch those maintenance
>>>>
>>>> calls will
>>>>>>>
>>>>>>>    do nothing, which is not correct.
>>>>>>
>>>>>>
>>>>>> Ah yes,  But, shouldn't we be doing these same operations as part of
>>>>>> turning the cache off?
>>>>>>
>>>>>    The problem is that while turning of dcaches, we flush it, and
>>>>
>>>> turn
>>>>>
>>>>>   cache and MMU off.  But these operations are not happening
>>>>>   automatically in a single call. So there is a chance of  valid
>>>>>   entries present in cache even after it is OFF.
>>>>
>>>>
>>>> I think this is what we need to fix. Otherwise, Tom's change looks good
>>>> to me. How about an invalidate in dcache_disable() or something like
>>>> that?
>>>>
>>>  Correct. So if the cleaning up sequence is fine, then ok.
>>>  So there should be no need to check if caches are OFF inside
>>>  the maintenance functions. Kernel does not looks like doing such checks.
>>>  The real issue looks like we should have had assembly level functions
>>>  to do the cleanup routines for flush-invalidate-turn OFF caches/MM
>>> atomically.
>>
>>   Actually, with something like speculation in the behind, only way of
>> ensuring that
>>    we do not have any valid entries is to do a invalidate all, after
>> turn of caches/mmu.
>>    So this means that we will have a maintenance after turning off.
>
>
> Good point. But we need that invalidate just one last time after the
> disable, right? How about making the cache_status a variable rather
> than reading the C bit. And then disable_cache() shall be like this:
>
> 1. Flush all
> 2. Disable C bit
> 3. Invalidate all
> 4. cache_status = disabled
>
> Maybe, not the best solution. But I can't think of anything better.
> Please note that after this point there won't be any valid entries in
> the cache per ARMv7 Architecture reference manual(section B2.2.2):
>
> "If the cache is disabled, it is guaranteed that no new allocation of memory
> locations into the cache will occur."

  I agree on this sequence. But the only thing is, is it good to turn off
  the caches runtime, because one driver cant do aligned access ?
 Will this then not become a custom, say any other driver which
 sees problem with cache, will then simply go ahead disable and proceed.
  Is that right ?.
 yes, having enabled caches we are missing things like UNCACHED allocations,
 IO etc, but that is going to make boot loaders more heavy, may be.

  In the end, I am not sure which is the right decision to take here :)

Thanks,
 Sricharan