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