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