On 04/05/2017 12:57 PM, Dr. Philipp Tomsich wrote:
> 
>> On 05 Apr 2017, at 12:25, Marek Vasut <ma...@denx.de> wrote:
>>
>> On 04/04/2017 10:26 PM, Dr. Philipp Tomsich wrote:
>>>
>>>> On 04 Apr 2017, at 22:09, Marek Vasut <ma...@denx.de> wrote:
>>>>
>>>>> The DWC3 flush expands to a clean+invalidate. It is not wrong, as long as
>>>>> it is used as in my patch:
>>>>> a. before the first time data is expected to be written by the peripheral 
>>>>> (i.e.
>>>>> before the peripheral is started)—to ensure that the cache line is not 
>>>>> cached
>>>>> any longer…
>>>>
>>>> So invalidate() is enough ?
>>>
>>> If I had to write this from scratch, I’d got with the paranoid sequence of:
>>>
>>>     handler():
>>>     {
>>>             invalidate
>>>             do my stuff
>>>             clean
>>>     }
>>>
>>> However, some architectures in U-Boot (e.g. ARMv8) don’t implement the
>>> invalidate verb. Given this, I’d rather stay as close to what’s already 
>>> there.
>>
>> invalidate_dcache_range() must be implemented if flush_dcache_range()
>> is, otherwise it's a bug.
> 
> The ARMv8 implementation for invalidate currently maps back to a 
> clean+invalidate
> (see arch/arm/cpu/armv8/cache_v8.c):

Hm, interesting and unusual. OK

>       /*
>        * Invalidates range in all levels of D-cache/unified cache
>        */
>       void invalidate_dcache_range(unsigned long start, unsigned long stop)
>       {
>               __asm_flush_dcache_range(start, stop);
>       }
> 
>       /*
>        * Flush range(clean & invalidate) from all levels of D-cache/unified 
> cache
>        */
>       void flush_dcache_range(unsigned long start, unsigned long stop)
>       {
>               __asm_flush_dcache_range(start, stop);
>       }
> 
> I am a bit scared of either using (as this clearly is mislabeled) or changing 
> (as
> other code might depend on things being as they are) the invalidate-function
> for ARMv8 at this point.

The naming is OK, flush == push data from cache one level down ;
invalidate == mark cacheline invalid so data would be loaded from next
level .

>>> Note that using flush (i.e. clean+invalidate) aligns with how caches are
>>> managed throughout various other drivers in U-Boot.
>>>
>>>>
>>>>> b. after the driver modifies any buffers (i.e. anything modified will be 
>>>>> written
>>>>> back) and before it next reads the buffers expecting possibly changed data
>>>>> (i.e. invalidating).
>>>>
>>>> So flush+invalidate ? Keep in mind this driver may not be used on
>>>> ARMv7/v8 only …
>>>
>>> Yes, a clean+invalidate.
>>> The flush_dcache_range(…, …) function in U-Boot implements C+I semantics
>>> at least on arm, arm64, avr32, powerpc, xtensa …
>>
>> flush on arm926 does not invalidate the cacheline iirc .
> 
> I dug up an ARMv5 architecture manual (I didn’t think I’d ever need this 
> again) to
> look at what the ARM926 does.
> 
> Here’s the code for reference:
>       void flush_dcache_range(unsigned long start, unsigned long stop)
>       {
>               if (!check_cache_range(start, stop))
>                       return;
> 
>               while (start < stop) {
>                       asm volatile("mcr p15, 0, %0, c7, c14, 1\n" : : 
> "r"(start));
>                       start += CONFIG_SYS_CACHELINE_SIZE;
>               }
> 
>               asm volatile("mcr p15, 0, %0, c7, c10, 4\n" : : "r"(0));
>       }
> 
> c7 are the cache-management functions, with the following opcodes:
> —     “c7, c14, 1” is “Clean and invalidate data cache line” on the modified 
> virtual-address (MVA)
> —     “c7, c10, 4” is "Data Synchronization Barrier (formerly Drain Write 
> Buffer)"
> 
> This discussion shows that (at some future point… and no, I am not 
> volunteering
> for this) the naming of the cache-maintenance functions and the documentation 
> for
> them should be reworked to avoid any confusion for the casual driver 
> developer.
> 
> I’ll just go ahead and put together a v2 that also addresses the pointer-size 
> concerns,
> as I don’t want to have the fix for our DWC3 issue held up by this.

OK

-- 
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to