Re: [PATCH v3 09/12] riscv: p8700: Add Coherence Manager (CM) and IOCU support

[Yao Zi <ziyao@disroot.org>]

On Tue, Jul 29, 2025 at 04:24:36PM +0000, Uros Stajic wrote:
> From: Chao-ying Fu <cfu@mips.com>
> 
> Add support for Coherence Manager (CM) and IOCU discovery and
> configuration on the P8700 platform.
> 
> Signed-off-by: Chao-ying Fu <cfu@mips.com>
> Signed-off-by: Uros Stajic <uros.stajic@htecgroup.com>
> ---
>  arch/riscv/cpu/p8700/Makefile             |   2 +
>  arch/riscv/cpu/p8700/cache.c              |  10 +++
>  arch/riscv/cpu/p8700/cm-iocu.c            |  75 ++++++++++++++++
>  arch/riscv/cpu/p8700/cm.c                 |  92 +++++++++++++++++++
>  arch/riscv/include/asm/arch-p8700/cm.h    |  61 +++++++++++++
>  arch/riscv/include/asm/arch-p8700/p8700.h |  31 +++++++
>  arch/riscv/include/asm/global_data.h      |   2 +
>  arch/riscv/include/asm/io.h               |  86 ++++++++++++++++++
>  board/mips/boston-riscv/Makefile          |   1 +
>  board/mips/boston-riscv/iocu.c            | 104 ++++++++++++++++++++++
>  10 files changed, 464 insertions(+)
>  create mode 100644 arch/riscv/cpu/p8700/cm-iocu.c
>  create mode 100644 arch/riscv/cpu/p8700/cm.c
>  create mode 100644 arch/riscv/include/asm/arch-p8700/cm.h
>  create mode 100644 board/mips/boston-riscv/iocu.c
> 

...

> diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> index da165858034..fb79d0a8d32 100644
> --- a/arch/riscv/include/asm/io.h
> +++ b/arch/riscv/include/asm/io.h

These changes don't seem to be related to the commit message. Please
split them into a separate patch (if these are really necessary, see
my comments below).

> @@ -15,6 +15,91 @@ static inline void sync(void)
>  {
>  }
>  
> +/*
> + * Generic virtual read/write.  Note that we don't support half-word
> + * read/writes.  We define __arch_*[bl] here, and leave __arch_*w
> + * to the architecture specific code.
> + */

But you do define half-word operations. This comment looks like the one
removed in d5af15bf515 ("riscv: Clean up asm/io.h"), and seems already
out-of-date.

> +#if CONFIG_P8700_RISCV
> +#ifdef CONFIG_ARCH_MAP_SYSMEM
> +static inline void *map_sysmem(phys_addr_t paddr, unsigned long len)
> +{
> +	if (paddr < PHYS_SDRAM_0_SIZE + PHYS_SDRAM_1_SIZE)
> +		paddr = paddr | 0x40000000;
> +	return (void *)(uintptr_t)paddr;
> +}
> +
> +static inline void *unmap_sysmem(const void *vaddr)
> +{
> +	phys_addr_t paddr = (phys_addr_t)vaddr;
> +
> +	paddr = paddr & ~0x40000000;
> +	return (void *)(uintptr_t)paddr;
> +}
> +
> +static inline phys_addr_t map_to_sysmem(const void *ptr)
> +{
> +	return (phys_addr_t)(uintptr_t)ptr;
> +}
> +#endif
> +
> +static inline unsigned char __arch_getb(const volatile void __iomem *mem)
> +{
> +	unsigned char value;
> +
> +	asm volatile("lbu %0,0(%1)" : "=r"(value) : "r"(mem));
> +
> +	return value;
> +}
> +
> +static inline unsigned short __arch_getw(const volatile void __iomem *mem)
> +{
> +	unsigned short value;
> +
> +	asm volatile("lhu %0,0(%1)" : "=r"(value) : "r"(mem));
> +
> +	return value;
> +}
> +
> +static inline unsigned int __arch_getl(const volatile void __iomem *mem)
> +{
> +	unsigned int value;
> +
> +	asm volatile("lw %0,0(%1)" : "=r"(value) : "r"(mem));
> +
> +	return value;
> +}
> +
> +static inline unsigned long long __arch_getq(const volatile void __iomem *mem)
> +{
> +	unsigned long long value;
> +
> +	asm volatile("ld %0,0(%1)" : "=r"(value) : "r"(mem));
> +
> +	return value;
> +}
> +
> +static inline void __arch_putb(unsigned char value, volatile void __iomem *mem)
> +{
> +	asm volatile("sb %0,0(%1)"::"r"(value), "r"(mem));
> +}
> +
> +static inline void __arch_putw(unsigned short value, volatile void __iomem *mem)
> +{
> +	asm volatile("sh %0,0(%1)"::"r"(value), "r"(mem));
> +}
> +
> +static inline void __arch_putl(unsigned int value, volatile void __iomem *mem)
> +{
> +	asm volatile("sw %0,0(%1)"::"r"(value), "r"(mem));
> +}
> +
> +static inline void __arch_putq(unsigned int value, volatile void __iomem *mem)
> +{
> +	asm volatile("sd %0,0(%1)"::"r"(value), "r"(mem));
> +}

I don't see any reason not to use the generic macros for P8700, these
inline assembly should basically behave the same as the C-version below.
Could you please explain it futher?

Thanks,
Yao Zi

> +#else
>  #define __arch_getb(a)			(*(volatile unsigned char *)(a))
>  #define __arch_getw(a)			(*(volatile unsigned short *)(a))
>  #define __arch_getl(a)			(*(volatile unsigned int *)(a))
> @@ -24,6 +109,7 @@ static inline void sync(void)
>  #define __arch_putw(v, a)		(*(volatile unsigned short *)(a) = (v))
>  #define __arch_putl(v, a)		(*(volatile unsigned int *)(a) = (v))
>  #define __arch_putq(v, a)		(*(volatile unsigned long long *)(a) = (v))
> +#endif
>  
>  #define __raw_writeb(v, a)		__arch_putb(v, a)
>  #define __raw_writew(v, a)		__arch_putw(v, a)
> +}