Re: [PATCH -next v7 04/10] arm: armv8: invalidate dcache entries on dcache_enable

[Anshul Dalal <anshuld@ti.com>]

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 <d-gole@ti.com>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> Signed-off-by: Anshul Dalal <anshuld@ti.com>
>> Tested-by: Wadim Egorov <w.egorov@phytec.de>
>> ---
>>   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.

CC+: Patrice Chotard, Patrick Delaunay (the authors of above commit)

Regards,
Anshul

>
>>   		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);
>>   }
>>