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

[Anshul Dalal <anshuld@ti.com>]

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 <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.
>

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/20250916112205.238811-1-anshuld@ti.com/

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