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

Reply via email to