From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Date: Mon, 24 Oct 2016 11:44:04 +0100 Subject: [U-Boot] [Resend RFC PATCH 1/2] armv8: Fix dcache disable function In-Reply-To: References: <1476476277-10527-1-git-send-email-york.sun@nxp.com> <1476476277-10527-2-git-send-email-york.sun@nxp.com> Message-ID: <20161024104404.GD15620@leverpostej> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, Sorry for joining this a bit late; apologies if the below re-treads ground already covered. On Wed, Oct 19, 2016 at 09:25:02AM -0600, Stephen Warren wrote: > On 10/14/2016 02:17 PM, York Sun wrote: > >Current code turns off d-cache first, then flush all levels of cache. > >This results data loss. As soon as d-cache is off, the dirty cache > >is discarded according to the test on LS2080A. This issue was not > >seen as long as external L3 cache was flushed to push the data to > >main memory. However, external L3 cache is not guaranteed to have > >the data. To fix this, flush the d-cache by way/set first to make > >sure cache is clean before turning it off. > > >diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > > >@@ -478,9 +478,9 @@ void dcache_disable(void) > > >+ flush_dcache_all(); > > set_sctlr(sctlr & ~(CR_C|CR_M)); > > > >- flush_dcache_all(); > > __asm_invalidate_tlb_all(); > > I talked to Mark Rutland at ARM, and I believe the current code is > correct. Well, almost, but not quite. It's a long story... ;) I gave a primer [1,2] on the details at ELC earlier this year, which may or may not be useful. The big details are: * Generaly "Flush" is ambiguous/meaningless. Here you seem to want clean+invalidate. * Set/Way operations are for IMPLEMENTATION DEFINED (i.e. SoC-specific) cache maintenance sequences, and are not truly portable (e.g. not affecting system caches). I assume that an earlier boot stage initialised the caches prior to U-Boot. Given that, you *only* need to perform maintenance for the memory you have (at any point) mapped with cacheable attrbiutes, which should be a small subset of the PA space. With ARMv8-A, broadcast maintenance to the PoC should affect all relevant caches (assuming you use the correct shareability attributes). * You *cannot* write a dcache disable routine in C, as the compiler can perform a number of implicit memory accesses (e.g. stack, globals, GOT). For that alone, I do not believe the code above is correct. Note that we have seen this being an issue in practice, before we got rid of Set/Way ops from arm64 Linux (see commit 5e051531447259e5). * Your dcache disable code *must* be clean to the PoC, prior to execution, or instruction fetches could see stale data. You can first *clean* this to the PoC, which is sufficient to avoid the problems above. * The SCTLR_ELx.{C,I} bits do not enable/disable caches; they merely activate/deactiveate cacheable attributes on data/instruction fetches. Note that cacheable instruction fetches can allocate into unified/data caches. Also, note that the I bit is independent of the C bit, and the attributes it provides differ when the M bit is clear. Generally, I would advise that at all times M == C == I, as that leads to the least surprise. Thanks, Mark. [1] http://events.linuxfoundation.org/sites/events/files/slides/slides_17.pdf [2] https://www.youtube.com/watch?v=F0SlIMHRnLk