From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dr. Philipp Tomsich Date: Wed, 5 Apr 2017 12:57:44 +0200 Subject: [U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on event buffers more robust In-Reply-To: References: <1491241798-37084-1-git-send-email-philipp.tomsich@theobroma-systems.com> <8bb1f19c-a7f5-bbc8-3a0d-6c2de73a7410@denx.de> <52CB490A-0557-4C75-99B9-B9137D85C789@theobroma-systems.com> <0274b567-62c0-e801-2a6d-779f87c9ab9e@denx.de> <5DC61317-5AFD-4BEA-929E-F0F9B431D5F5@theobroma-systems.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de > On 05 Apr 2017, at 12:25, Marek Vasut wrote: > > On 04/04/2017 10:26 PM, Dr. Philipp Tomsich wrote: >> >>> On 04 Apr 2017, at 22:09, Marek Vasut 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.