Re: [RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core
From: Green Wan [mailto:green.wan@sifive.com] Sent: Tuesday, April 13, 2021 5:32 PM Cc: Green Wan; Rick Jian-Zhi Chen(陳建志); Paul Walmsley; Pragnesh Patel; Sean Anderson; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(梁育齊); Brad Kim; open list Subject: [RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core
Add a callback harts_early_init() to ./arch/riscv/cpu/start.S to
Please don't describe the file path because in the following log it can be found obviously. or you can reword as: Add a callback harts_early_init() to start.S
allow different riscv hart perform setup code for each hart as early as possible. Since all the harts enter the callback, they must be able to run the same setup.
Signed-off-by: Green Wan green.wan@sifive.com
arch/riscv/cpu/cpu.c | 15 +++++++++++++++ arch/riscv/cpu/start.S | 12 ++++++++++++ 2 files changed, 27 insertions(+)
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index 85592f5bee..b2b49812c4 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -140,3 +140,18 @@ int arch_early_init_r(void) { return riscv_cpu_probe(); }
+/**
- harts_early_init() - A callback function called by
- ./arch/riscv/cpu/start.S to allow to disable/enable features of each
Don't describe the called path. _weak function is a common usage in U-Boot, if someone grep the U-Boot and will know how to use this callback.
allow to disable/enable features ... it shall be reword as: configure feature settings of each hart (Because someone maybe not just enable or disable features but need to adjust the default value or somewhat)
- core. For example, turn on or off the functional block of CPU harts.
Same meaning as disable/enable features of each hart. It is unnecessary to describe again.
- In a multi-core system, this function must not access shared resources.
- Any access to such resources would probably be better done with
- available_harts_lock held. However, I doubt that any such access will be
The point is atomic instruction but not the available_harts_lock, it is only a variable allocated in memory. You may describe that: Memory access shall be careful here, it shall take care race conditions.
- necessary.
- */
+__weak void harts_early_init(void) +{ +} diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index 8589509e01..a481102960 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -117,6 +117,18 @@ call_board_init_f_0: mv sp, a0 #endif
+#if CONFIG_IS_ENABLED(RISCV_MMODE)
/** Jump to harts_early_init() to perform init for each core. Not* expect to access gd since gd is not initialized. All operations in the* function should affect core itself only. In multi-core system, any access* to common resource or registers outside core should be avoided or need a* protection for multicore.*/
Don't describe too much duplicate words for harts_early_init in start.S and cpu.c. Here you just describe this callback can configure not standard or customized CSRs. And describe race condition issue of memory access in SMP system for harts_early_init() in cpu.c
Thanks, Rick
+call_harts_early_init:
jal harts_early_init+#endif
#ifndef CONFIG_XIP /* * Pick hart to initialize global data and run U-Boot. The other harts -- 2.31.0
Thanks, Rick,
It's very helpful. I'll fix them.
On Wed, Apr 14, 2021 at 11:07 AM Rick Chen rickchen36@gmail.com wrote:
From: Green Wan [mailto:green.wan@sifive.com] Sent: Tuesday, April 13, 2021 5:32 PM Cc: Green Wan; Rick Jian-Zhi Chen(陳建志); Paul Walmsley; Pragnesh Patel; Sean Anderson; Bin Meng; Simon Glass; Atish Patra; Leo Yu-Chi Liang(梁育齊); Brad Kim; open list Subject: [RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core
Add a callback harts_early_init() to ./arch/riscv/cpu/start.S to
Please don't describe the file path because in the following log it can be found obviously. or you can reword as: Add a callback harts_early_init() to start.S
allow different riscv hart perform setup code for each hart as early as possible. Since all the harts enter the callback, they must be able to run the same setup.
Signed-off-by: Green Wan green.wan@sifive.com
arch/riscv/cpu/cpu.c | 15 +++++++++++++++ arch/riscv/cpu/start.S | 12 ++++++++++++ 2 files changed, 27 insertions(+)
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index 85592f5bee..b2b49812c4 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -140,3 +140,18 @@ int arch_early_init_r(void) { return riscv_cpu_probe(); }
+/**
- harts_early_init() - A callback function called by
- ./arch/riscv/cpu/start.S to allow to disable/enable features of each
Don't describe the called path. _weak function is a common usage in U-Boot, if someone grep the U-Boot and will know how to use this callback.
allow to disable/enable features ... it shall be reword as: configure feature settings of each hart (Because someone maybe not just enable or disable features but need to adjust the default value or somewhat)
- core. For example, turn on or off the functional block of CPU harts.
Same meaning as disable/enable features of each hart. It is unnecessary to describe again.
- In a multi-core system, this function must not access shared resources.
- Any access to such resources would probably be better done with
- available_harts_lock held. However, I doubt that any such access will be
The point is atomic instruction but not the available_harts_lock, it is only a variable allocated in memory. You may describe that: Memory access shall be careful here, it shall take care race conditions.
- necessary.
- */
+__weak void harts_early_init(void) +{ +} diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index 8589509e01..a481102960 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -117,6 +117,18 @@ call_board_init_f_0: mv sp, a0 #endif
+#if CONFIG_IS_ENABLED(RISCV_MMODE)
/** Jump to harts_early_init() to perform init for each core. Not* expect to access gd since gd is not initialized. All operations in the* function should affect core itself only. In multi-core system, any access* to common resource or registers outside core should be avoided or need a* protection for multicore.*/Don't describe too much duplicate words for harts_early_init in start.S and cpu.c. Here you just describe this callback can configure not standard or customized CSRs. And describe race condition issue of memory access in SMP system for harts_early_init() in cpu.c
Thanks, Rick
+call_harts_early_init:
jal harts_early_init+#endif
#ifndef CONFIG_XIP /* * Pick hart to initialize global data and run U-Boot. The other harts -- 2.31.0
participants (2)
-
Green Wan -
Rick Chen