[U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on event buffers more robust

[Dr. Philipp Tomsich]

> On 05 Apr 2017, at 12:25, Marek Vasut <marex at denx.de> wrote:
> 
> On 04/04/2017 10:26 PM, Dr. Philipp Tomsich wrote:
>> 
>>> On 04 Apr 2017, at 22:09, Marek Vasut <marex at 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):

	/*
	 * 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.

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


Regards,
Philipp.