Re: [PATCH v2 01/11] riscv: Add initial support for P8700 SoC

[Uros Stajic <uros.stajic@htecgroup.com>]

On 10. 7. 25. 18:21, ziyao at disroot.org (Yao Zi) wrote:
> On Tue, Jun 24, 2025 at 10:25:30AM +0000, Uros Stajic wrote:
>> From: Chao-ying Fu <cfu at mips.com>
>>
>> Add initial platform support for the P8700-F, a high-performance
>> multi-core RV64GC SoC with optional multi-cluster configuration and
>> hardware multithreading.
>>
>> This patch implements initial support required for U-Boot to run on
>> the P8700-based Boston board.
>>
>> Signed-off-by: Chao-ying Fu <cfu at mips.com>
>> Signed-off-by: Uros Stajic <uros.stajic at htecgroup.com>
>> ---
>>   arch/riscv/Kconfig                          |  11 +
>>   arch/riscv/cpu/Makefile                     |   2 +
>>   arch/riscv/cpu/p8700/Kconfig                |  15 ++
>>   arch/riscv/cpu/p8700/Makefile               |   7 +
>>   arch/riscv/cpu/p8700/cache.c                |  74 ++++++
>>   arch/riscv/cpu/p8700/cpu.c                  |  22 ++
>>   arch/riscv/cpu/p8700/dram.c                 |  37 +++
>>   arch/riscv/cpu/p8700/p8700_platform_setup.S | 155 ++++++++++++
>>   arch/riscv/cpu/start.S                      |   8 +
>>   arch/riscv/dts/Makefile                     |   1 +
>>   arch/riscv/dts/boston-p8700.dts             | 253 ++++++++++++++++++++
>>   arch/riscv/include/asm/arch-p8700/p8700.h   | 110 +++++++++
>>   board/mips/boston-riscv/Kconfig             |  43 ++++
>>   board/mips/boston-riscv/MAINTAINERS         |   9 +
>>   board/mips/boston-riscv/Makefile            |   8 +
>>   board/mips/boston-riscv/boston-lcd.h        |  20 ++
>>   board/mips/boston-riscv/boston-regs.h       |  38 +++
>>   board/mips/boston-riscv/boston-riscv.c      |   9 +
>>   board/mips/boston-riscv/checkboard.c        |  43 ++++
>>   board/mips/boston-riscv/config.mk           |  15 ++
>>   board/mips/boston-riscv/lowlevel_init.S     |  18 ++
>>   board/mips/boston-riscv/reset.c             |  15 ++
>>   configs/boston-p8700_defconfig              |  94 ++++++++
>>   drivers/clk/Kconfig                         |   2 +-
>>   include/asm-generic/global_data.h           |   5 +
>>   include/configs/boston-riscv.h              |  11 +
>>   26 files changed, 1024 insertions(+), 1 deletion(-)
> 
> It's a relatively large patch, and it'll be nice if you could split it
> into several smaller ones, for exampe, one for CPU stuff and one for
> board stuff.
> 
>>   create mode 100644 arch/riscv/cpu/p8700/Kconfig
>>   create mode 100644 arch/riscv/cpu/p8700/Makefile
>>   create mode 100644 arch/riscv/cpu/p8700/cache.c
>>   create mode 100644 arch/riscv/cpu/p8700/cpu.c
>>   create mode 100644 arch/riscv/cpu/p8700/dram.c
>>   create mode 100644 arch/riscv/cpu/p8700/p8700_platform_setup.S
>>   create mode 100644 arch/riscv/dts/boston-p8700.dts
>>   create mode 100644 arch/riscv/include/asm/arch-p8700/p8700.h
>>   create mode 100644 board/mips/boston-riscv/Kconfig
>>   create mode 100644 board/mips/boston-riscv/MAINTAINERS
>>   create mode 100644 board/mips/boston-riscv/Makefile
>>   create mode 100644 board/mips/boston-riscv/boston-lcd.h
>>   create mode 100644 board/mips/boston-riscv/boston-regs.h
>>   create mode 100644 board/mips/boston-riscv/boston-riscv.c
>>   create mode 100644 board/mips/boston-riscv/checkboard.c
>>   create mode 100644 board/mips/boston-riscv/config.mk
>>   create mode 100644 board/mips/boston-riscv/lowlevel_init.S
>>   create mode 100644 board/mips/boston-riscv/reset.c
>>   create mode 100644 configs/boston-p8700_defconfig
>>   create mode 100644 include/configs/boston-riscv.h
> 
> ...
> 
>> diff --git a/arch/riscv/cpu/Makefile b/arch/riscv/cpu/Makefile
>> index 6bf6f911c67..38894861694 100644
>> --- a/arch/riscv/cpu/Makefile
>> +++ b/arch/riscv/cpu/Makefile
>> @@ -5,3 +5,5 @@
>>   extra-y = start.o
>>
>>   obj-y += cpu.o mtrap.o
>> +
>> +obj-$(CONFIG_P8700_RISCV) += p8700/p8700_platform_setup.o
> 
> This could be moved to arch/riscv/cpu/p8700/Makefile. It seems more
> natural to me since arch/riscv/cpu/p8700 contains p8700_platform_setup.S
> 
> ...
> 
>> diff --git a/arch/riscv/cpu/p8700/Makefile b/arch/riscv/cpu/p8700/Makefile
>> new file mode 100644
>> index 00000000000..ecdd232da6f
>> --- /dev/null
>> +++ b/arch/riscv/cpu/p8700/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +#
>> +# Copyright (C) 2021, Chao-ying Fu <cfu at mips.com>
>> +
>> +obj-y += dram.o
>> +obj-y += cpu.o
>> +obj-y += cache.o
> 
> It'll be nice to sort the files in alphabetical order. Same for headers
> included in files below.
> 
>> diff --git a/arch/riscv/cpu/p8700/cache.c b/arch/riscv/cpu/p8700/cache.c
>> new file mode 100644
>> index 00000000000..27641035d80
>> --- /dev/null
>> +++ b/arch/riscv/cpu/p8700/cache.c
>> @@ -0,0 +1,74 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2021, Chao-ying Fu <cfu at mips.com>
>> + */
>> +
>> +#include <cpu_func.h>
>> +#include <asm/global_data.h>
>> +#include <asm/arch-p8700/p8700.h>
>> +
>> +/* NOTE: We force to use a0 in mcache to encode via .word. */
>> +#define cache_loop(start, end, lsize, op) do {				\
>> +	const __typeof__(lsize) __lsize = (lsize);			\
>> +	const register void *addr asm("a0") = (const void *)((start) & ~(__lsize - 1));	\
>> +	const void *aend = (const void *)(((end) - 1) & ~(__lsize - 1));	\
>> +	for (; addr <= aend; addr += __lsize)				\
>> +		asm volatile (".word 0xec0500f3|%0 # force to use %1" \
>> +								::"i"((op) << 20), "r"(addr));	\
> 
> It's unclear what the magical instruction does, could you please add a
> comment and use a meaningful macro to make it easier to understand?
> 
>> +} while (0)
>> +
>> +static unsigned long lsize;
>> +static unsigned long l1d_total_size;
>> +static unsigned long slsize;
>> +
>> +static void probe_cache_config(void)
>> +{
>> +	lsize = 64;
>> +	l1d_total_size = 64 * 1024;
>> +
>> +	int l2_config = 0;
>> +	long address = GCR_L2_CONFIG;
>> +
>> +	asm volatile ("lw %0,0(%1)" : "=r"(l2_config) : "r"(address) : "memory");
> 
> Is it really necessary to use inline assembly? If GCR_L2_CONFIG is an
> MMIO register, readl() should work here.
> 
>> +	int l2_line_size_info = (l2_config >> L2_LINE_SIZE_SHIFT)
>> +				& L2_LINE_SIZE_MASK;
>> +	slsize = (l2_line_size_info == 0) ? 0 : 1 << (l2_line_size_info + 1);
>> +}
>> +
>> +void flush_dcache_range(unsigned long start, unsigned long end)
>> +{
>> +	if (lsize == 0)
>> +		probe_cache_config();
>> +
>> +	/* aend will be miscalculated when size is zero, so we return here */
>> +	if (start >= end)
>> +		return;
>> +
>> +	cache_loop(start, end, lsize, HIT_WRITEBACK_INV_D);
>> +
>> +	/* flush L2 cache */
>> +	if (slsize)
>> +		cache_loop(start, end, slsize, HIT_WRITEBACK_INV_SD);
>> +
>> +	/* ensure cache ops complete before any further memory access */
>> +	asm volatile ("slli x0,x0,1 # ihb");
> 
> This instruction also looks confusing: slli usually means left-shift
> with an immediate in the context of RISC-V, which obviously doesn't
> match the corresponding comment. Could you please provide a macro for
> the instruction, and explain it further if possible? This applies also
> for the asm statement in invalidate_dcache_range().
> 
>> +}
>> +
>> +void invalidate_dcache_range(unsigned long start, unsigned long end)
>> +{
>> +	if (lsize == 0)
>> +		probe_cache_config();
>> +
>> +	/* aend will be miscalculated when size is zero, so we return here */
>> +	if (start >= end)
>> +		return;
>> +
>> +	/* invalidate L2 cache */
>> +	if (slsize)
>> +		cache_loop(start, end, slsize, HIT_INVALIDATE_SD);
>> +
>> +	cache_loop(start, end, lsize, HIT_INVALIDATE_D);
>> +
>> +	/* ensure cache ops complete before any further memory access */
>> +	asm volatile ("slli x0,x0,1 # ihb");
>> +}
>> diff --git a/arch/riscv/cpu/p8700/cpu.c b/arch/riscv/cpu/p8700/cpu.c
>> new file mode 100644
>> index 00000000000..f13c18942f3
>> --- /dev/null
>> +++ b/arch/riscv/cpu/p8700/cpu.c
>> @@ -0,0 +1,22 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, Bin Meng <bmeng.cn at gmail.com>
>> + */
>> +
>> +#include <irq_func.h>
>> +#include <asm/cache.h>
>> +
>> +/*
>> + * cleanup_before_linux() is called just before we call linux
>> + * it prepares the processor for linux
>> + *
>> + * we disable interrupt and caches.
>> + */
>> +int cleanup_before_linux(void)
>> +{
>> +	disable_interrupts();
>> +
>> +	cache_flush();
>> +
>> +	return 0;
>> +}
> 
> We have a weak, general implementation in arch/riscv/cpu.c, thus the
> whole file could be dropped.
> 
> ...
> 
>> diff --git a/arch/riscv/cpu/p8700/p8700_platform_setup.S b/arch/riscv/cpu/p8700/p8700_platform_setup.S
>> new file mode 100644
>> index 00000000000..c2999045175
>> --- /dev/null
>> +++ b/arch/riscv/cpu/p8700/p8700_platform_setup.S
>> @@ -0,0 +1,155 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (C) 2021, Chao-ying Fu <cfu at mips.com>
>> + */
>> +
>> +#include <asm-offsets.h>
>> +#include <config.h>
>> +#include <elf.h>
>> +#include <system-constants.h>
>> +#include <asm/encoding.h>
>> +#include <generated/asm-offsets.h>
>> +#include <asm/arch-p8700/p8700.h>
>> +
>> +.global p8700_platform_setup
>> +.global set_flash_uncached
>> +
>> +p8700_platform_setup:
>> +	move s6, ra
>> +	li	x1,0
>> +	li	x2,0
>> +	li	x3,0
>> +	li	x4,0
>> +	li	x5,0
>> +	li	x6,0
>> +	li	x7,0
>> +	li	x8,0
>> +	li	x9,0
>> +	li	x10,0
>> +	li	x11,0
>> +	li	x12,0
>> +	li	x13,0
>> +	li	x14,0
>> +	li	x15,0
>> +	li	x16,0
>> +	li	x17,0
>> +	li	x18,0
>> +	li	x19,0
>> +	li	x20,0
>> +	li	x21,0
>> +	li	x23,0
>> +	li	x24,0
>> +	li	x25,0
>> +	li	x26,0
>> +	li	x27,0
>> +	li	x28,0
>> +	li	x29,0
>> +	li	x30,0
>> +	li	x31,0
> 
> These instructions miss some space between the two operands.
> 
> ...
> 
>> +	/* Map all PCIe DMA access to its default, non-IOCU, target */
>> +	li	t0,BOSTON_PLAT_NOCPCIE0ADDR
>> +	sw	zero, 0(t0)
>> +	li	t0,BOSTON_PLAT_NOCPCIE1ADDR
>> +	sw	zero, 0(t0)
>> +	li	t0,BOSTON_PLAT_NOCPCIE2ADDR
>> +	sw	zero, 0(t0)
> 
> Here miss some space between the two operands of the li instructions.
> 
>> +	/* Test mhartid */
>> +	beq	a0, zero, 1f
>> +	/* Jump to 0x80000000 */
>> +	li	t0, 0x80000000
> 
> Why choose this specific address? If possible please use a macro instead
> (I noticed it's actually CONFIG_SYS_LOAD_ADDR).
> 
>> +	jr	t0
> 
> ...
> 
>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>> index 7bafdfd390a..e36abb5957b 100644
>> --- a/arch/riscv/cpu/start.S
>> +++ b/arch/riscv/cpu/start.S
>> @@ -39,6 +39,10 @@ secondary_harts_relocation_error:
>>   .section .text
>>   .globl _start
>>   _start:
>> +#ifdef CONFIG_P8700_RISCV
>> +	call p8700_platform_setup
>> +#endif
>> +
>>   #if CONFIG_IS_ENABLED(RISCV_MMODE)
>>   	csrr	a0, CSR_MHARTID
>>   #endif
>> @@ -416,6 +420,10 @@ call_board_init_r:
>>   	mv	a1, s4			/* dest_addr */
>>   	mv	s0, zero		/* fp == NULL */
>>
>> +#ifdef CONFIG_P8700_RISCV
>> +	call set_flash_uncached
>> +#endif
> 
> Is it necessary to call these initialization rountines so early that
> even harts_early_init() doesn't work? It really doesn't look clean
> especially IIRC there's no platform-specific code in
> arch/riscv/cpu/start.S.
> 
>>   /*
>>    * jump to it ...
>>    */
> 
> ...
> 
>> diff --git a/arch/riscv/dts/boston-p8700.dts b/arch/riscv/dts/boston-p8700.dts
>> new file mode 100644
>> index 00000000000..6d700a5675c
>> --- /dev/null
>> +++ b/arch/riscv/dts/boston-p8700.dts
>> @@ -0,0 +1,253 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2021, Chao-ying Fu <cfu at mips.com>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/clock/boston-clock.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +#include <dt-bindings/interrupt-controller/mips-gic.h>
>> +
>> +/ {
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	model = "p8700";
>> +	compatible = "img,boston";
>> +
>> +	chosen {
>> +		stdout-path = &uart0;
>> +		bootargs = "root=/dev/sda rw earlycon console=ttyS0,115200n8r";
>> +	};
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		timebase-frequency = <20000000>;
>> +
>> +		cpu at 0 {
>> +			device_type = "cpu";
>> +			compatible = "riscv";
>> +			mmu-type = "riscv,sv39";
>> +			riscv,isa = "rv64imafdcsu";
> 
> riscv,isa is already deprecated in devicetree-binding upstream, it'll
> be nice if you could provide a riscv,isa-extensions property as well.
> 
>> +			status = "okay";
>> +			reg = <0>;
>> +			clocks = <&clk_boston BOSTON_CLK_CPU>;
>> +			clock-frequency = <20000000>;
>> +			bootph-all;
> 
> These properties should be sorted, compatible goes first, then reg, and
> status should be the last property[1]. This should apply for other nodes
> as well.
> 
>> +		};
>> +	};
> 
> Regards,
> Yao Zi

Hello Yao Zi,

Thank you very much for taking the time to review the patch and for
the detailed feedback. I have addressed the comments in the updated
version and will submit v3 shortly. Please let me know if you have
any additional feedback.

Regards,
Uros Stajic