Re: [PATCH 3/7] cache: andes_llcache: improve performance of LLC operation

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3879 bytes --]

On Mon, Mar 30, 2026 at 06:27:20PM +0800, Hui Min Mina Chou wrote:
> Eliminate get_cpu() on !CONFIG_SMP and switch readl/writel to their

Where is the get_cpu() that you're talking about eliminating here?

> relaxed variants to remove unnecessary fence instructions on I/O
> memory access. The platform specification defines all I/O regions are
> on channel 0 (point-to-point strongly ordered), so explicit fences are
> not required [1][2][3]. Explicit memory barriers (mb) are added before
> and after the CCTL loop to ensure overall memory consistency.
> 

> Also fix hart ID mapping by switching to cpuid_to_hartid_map() instead
> of using the logical CPU ID directly. In AMP setups (e.g. Linux on
> Hart 1, RTOS on Hart 0), Linux sees itself as CPU 0 but must access
> Hart 1's CCTL registers, so using the logical ID would cause accidental
> interference with other cores.

This seems like it should be a separate fix for sure.

> 
> [1] platform spec 2.1.1: https://github.com/riscvarchive/riscv-platform-specs/blob/main/riscv-platform-spec.adoc?plain=1#L169
> [2] privileged spec 3.6.5: https://github.com/riscv/riscv-isa-manual/blob/main/src/machine.adoc?plain=1#L2835
> [3] riscv: asm/mmio.h: https://gitea.andestech.com/RD-SW/linux/src/branch/ast-v5_4_0-branch/arch/riscv/include/asm/mmio.h#L105
> 
> Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
> Signed-off-by: Hui Min Mina Chou <minachou@andestech.com>
> ---
>  drivers/cache/andes_llcache.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cache/andes_llcache.c b/drivers/cache/andes_llcache.c
> index d318b8009f7f..57f666bc537a 100644
> --- a/drivers/cache/andes_llcache.c
> +++ b/drivers/cache/andes_llcache.c
> @@ -66,7 +66,7 @@ static struct andes_priv andes_priv;
>  /* LLC operations */
>  static inline uint32_t andes_cpu_llc_get_cctl_status(void)
>  {
> -	return readl(andes_priv.llc_base + ANDES_LLC_REG_CCTL_STATUS_OFFSET_C0);
> +	return readl_relaxed(andes_priv.llc_base + ANDES_LLC_REG_CCTL_STATUS_OFFSET_C0);
>  }
>  
>  static void andes_cpu_cache_operation(unsigned long start, unsigned long end,
> @@ -74,16 +74,22 @@ static void andes_cpu_cache_operation(unsigned long start, unsigned long end,
>  {
>  	unsigned long line_size = andes_priv.andes_cache_line_size;
>  	void __iomem *base = andes_priv.llc_base;
> -	int mhartid = smp_processor_id();
>  	unsigned long pa;
> +	int mhartid = 0;
>  
> +	if (IS_ENABLED(CONFIG_SMP))
> +		mhartid = cpuid_to_hartid_map(get_cpu());

But I dunno why this dance is required. Can't you just retain the call
to smp_processor_id() and pass the result unconditionally to
cpuid_to_hartid_map()? Or just make it a oneliner with
cpuid_to_hartid_map(smp_processor_id())?

> +	else
> +		mhartid = cpuid_to_hartid_map(0);
> +
> +	mb(); /* complete earlier memory accesses before the cache flush */
>  	while (end > start) {
>  		csr_write(CSR_UCCTLBEGINADDR, start);
>  		csr_write(CSR_UCCTLCOMMAND, l1_op);
>  
>  		pa = virt_to_phys((void *)start);
> -		writel(pa, base + ANDES_LLC_REG_CCTL_ACC_OFFSET_BY_CORE(mhartid));
> -		writel(llc_op, base + ANDES_LLC_REG_CCTL_CMD_OFFSET_BY_CORE(mhartid));
> +		writel_relaxed(pa, base + ANDES_LLC_REG_CCTL_ACC_OFFSET_BY_CORE(mhartid));
> +		writel_relaxed(llc_op, base + ANDES_LLC_REG_CCTL_CMD_OFFSET_BY_CORE(mhartid));
>  		while ((andes_cpu_llc_get_cctl_status() &
>  			ANDES_LLC_CCTL_STATUS_MASK_BY_CORE(mhartid)) !=
>  			ANDES_LLC_CCTL_STATUS_IDLE)
> @@ -91,6 +97,10 @@ static void andes_cpu_cache_operation(unsigned long start, unsigned long end,
>  
>  		start += line_size;
>  	}
> +	mb(); /* issue later memory accesses after the cache flush */
> +
> +	if (IS_ENABLED(CONFIG_SMP))
> +		put_cpu();
>  }
>  
>  /* Write-back L1 and LLC entry */
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]