RISCV: Support pm cpu ops for SMP #63884
Conversation
|
Hello @quic-lingutla, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
carlocaione,
edersondisouza,
fkokosinski,
katsuster,
mgielda and
tgorochowik
arch/riscv/core/smp.c
Outdated
| #ifdef CONFIG_PM_CPU_OPS | ||
| uintptr_t entry = (uintptr_t)&__start; | ||
|
|
||
| if (pm_cpu_on(cpu_num, entry)) { |
There was a problem hiding this comment.
I don't get it. Is RISCV supporting PSCI and SMCCC now? What PM CPU ops driver is this using?
There was a problem hiding this comment.
I am not sure why PM CPU ops tightly coupled with PSCI or SMCC. RISCV SMP's arch_start_cpu() assumes secondary core is already powered up and looping at boot_secondary_core, but some platforms needs to do initialization (either via opensbi or regular MMIO to enable clocks, etc,.) to bring up secondary cores.
As of now, RISCV doesn't have any PM CPU Ops driver, but platforms can choose to implement these pm cpu ops weak functions to bringup secondary cores.
There was a problem hiding this comment.
My point is exactly that RISCV doesn't have any PM CPU ops driver in tree so you are adding this for some out-of-tree platform?
There was a problem hiding this comment.
Right, as of now the platform is out of tree.
There was a problem hiding this comment.
ok, do you plan to upstream it? I'm not willing to push code into the tree that is basically dead code without any user for it.
There was a problem hiding this comment.
Summarizing the offline conversation we had...
This code is in support of a microcontroller core embedded in a larger SoC design so there's not a clear path to share a buildable platform. In this particular platform the driver implementing the CPU Ops talks to some proprietary power/clock/reset controlling hardware.
As a separate discussion supporting Zephyr-on-SBI on an upstream platform (at least QEMU) would be really interesting.
| @@ -18,6 +19,10 @@ volatile struct { | |||
| volatile uintptr_t riscv_cpu_wake_flag; | |||
| volatile void *riscv_cpu_sp; | |||
|
|
|||
| #ifdef CONFIG_PM_CPU_OPS | |||
| extern void __start(void); | |||
There was a problem hiding this comment.
should this be CONFIG_KERNEL_ENTRY?
There was a problem hiding this comment.
Are you referring to the above #ifdef check ? or function declaration ?
If its for function declaration,
I am not sure, if we have any helpers to declare function like:
extern void CONFIG_KERNEL_ENTRY(void);
There was a problem hiding this comment.
uhm, I see. This should really be fixed somehow, it is really useless to have CONFIG_KERNEL_ENTRY and not being able to use it in C code (but only in the linker script).
Add pm cpu ops to call the platform specific implementations for bringing up secondary cores. Signed-off-by: Lingutla Chandrasekhar <[email protected]>
quic-lingutla
force-pushed
the
riscv_pm_cpu_ops
branch
from
95705d5
to
19aaede
Compare
|
Hi @quic-lingutla! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
Add pm cpu ops to call the platform specific implementations for bringing up secondary cores.