Hi all, On Mon Sep 15, 2025 at 10:35 AM IST, Anshul Dalal wrote: > Hi Heinrich, > > On Fri Sep 12, 2025 at 5:58 PM IST, Heinrich Schuchardt wrote: >> On 9/12/25 14:16, Anshul Dalal wrote: >>> In dcache_enable, currently the dcache entries are only invalidated when >>> the MMU is not enabled. This causes issues when dcache_enable is called >>> with the MMU already configured, in such cases the existing dcache >>> entries are not flushed which might result in un-expected behavior. >>> >>> This patch invalidates the cache entries on every call of dcache_enable >>> before enabling dcache (by setting CR_C). This makes dcache_enable >>> behave similar to icache_enable as well. >>> >>> Reviewed-by: Dhruva Gole <[email protected]> >>> Reviewed-by: Ilias Apalodimas <[email protected]> >>> Signed-off-by: Anshul Dalal <[email protected]> >>> Tested-by: Wadim Egorov <[email protected]> >>> --- >>> arch/arm/cpu/armv8/cache_v8.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c >>> index 1c1e33bec24..6e662395a83 100644 >>> --- a/arch/arm/cpu/armv8/cache_v8.c >>> +++ b/arch/arm/cpu/armv8/cache_v8.c >>> @@ -830,16 +830,15 @@ void flush_dcache_range(unsigned long start, unsigned >>> long stop) >>> void dcache_enable(void) >>> { >>> /* The data cache is not active unless the mmu is enabled */ >>> - if (!(get_sctlr() & CR_M)) { >>> - invalidate_dcache_all(); >>> - __asm_invalidate_tlb_all(); >>> + if (!mmu_status()) >> >> You are changing the logic here without describing why in the commit >> message: >> >> mmu_status() returns zero if CONFIG_SYS_ICACHE_OFF=y. >> Only for CONFIG_SYS_ICACHE_OFF=n it calls get_sctlr(). >> >> Please, check if the logic change is intended. >> Please, update the commit message. >> > > The bheaviour should be the same for all combinations of ICACHE_OFF and > DCACHE_OFF except for when ICACHE_OFF=y and DCACHE_OFF=n, in such cases > there is the concern that mmu_setup will be called everytime on > dcache_enable after my patch. > > This was not an intended change, thanks for bringing it to my attention. > > I think we should drop the ICACHE_OFF=y definition of mmu_setup which > was added in the commit 268f6ac1f95c ("arm64: Update memcpy_{from, > to}io() helpers"). And only keep the one that calls get_sctlr. If that's > off the table, I can respin the series without the call to mmu_status as > well. >
I have posted the patch[1] removing ICACHE_OFF=y definition of mmu_status, that patch along with this series should address this unintended change. Regards, Anshul [1]: https://lore.kernel.org/u-boot/[email protected]/ >> >>> mmu_setup(); >>> - } >>> >>> /* Set up page tables only once (it is done also by mmu_setup()) */ >>> if (!gd->arch.tlb_fillptr) >>> setup_all_pgtables(); >>> >>> + invalidate_dcache_all(); >>> + __asm_invalidate_tlb_all(); >>> set_sctlr(get_sctlr() | CR_C); >>> } >>>
