diff mbox series

[RFC] arch: riscv: cpu: Add callback to init each core

Message ID 20210322092255.303362-1-green.wan@sifive.com
State RFC
Delegated to: Andes
Headers show
Series [RFC] arch: riscv: cpu: Add callback to init each core | expand

Commit Message

Green Wan March 22, 2021, 9:22 a.m. UTC
I am looking for a proper place to add some M-mode code in SPL to
configure features of each core as early as possible. Try to start
the discussion by sending this RFC.

I tried mach_cpu_init() as the start. And not sure whether I used
it correctly, it seems executed only by the core wins lottery in
./arch/riscv/cpu/start.S.

Is there a proper place to add some inits for each core like CSR
in section 7.6 in document of U74-MC Core Complex Manual[1].

There are some alternatives to do it such as

 a) sending IPI to each cores
    Have to wait for IPI init for each core. And the boot core
    have to perform the init by itself and send IPIs to other
    cores. It might not be straightforward.
 b) hook callback, (like this patch)
 c) hard code in ./arch/riscv/cpu/start.c

[1] https://sifive.cdn.prismic.io/sifive/aee0dd4c-d156-496e-a6c4-db0cf54bbe68_sifive_U74MC_rtl_full_20G1.03.00_manual.pdf

Signed-off-by: Green Wan <green.wan@sifive.com>
---
 arch/riscv/cpu/start.S       | 5 +++++
 arch/riscv/lib/spl.c         | 4 ++++
 board/sifive/unmatched/spl.c | 7 +++++++
 3 files changed, 16 insertions(+)

Comments

Bin Meng March 22, 2021, 10:04 a.m. UTC | #1
On Mon, Mar 22, 2021 at 5:23 PM Green Wan <green.wan@sifive.com> wrote:
>
> I am looking for a proper place to add some M-mode code in SPL to
> configure features of each core as early as possible. Try to start
> the discussion by sending this RFC.
>
> I tried mach_cpu_init() as the start. And not sure whether I used
> it correctly, it seems executed only by the core wins lottery in
> ./arch/riscv/cpu/start.S.
>
> Is there a proper place to add some inits for each core like CSR
> in section 7.6 in document of U74-MC Core Complex Manual[1].
>
> There are some alternatives to do it such as
>
>  a) sending IPI to each cores
>     Have to wait for IPI init for each core. And the boot core
>     have to perform the init by itself and send IPIs to other
>     cores. It might not be straightforward.
>  b) hook callback, (like this patch)
>  c) hard code in ./arch/riscv/cpu/start.c
>
> [1] https://sifive.cdn.prismic.io/sifive/aee0dd4c-d156-496e-a6c4-db0cf54bbe68_sifive_U74MC_rtl_full_20G1.03.00_manual.pdf
>
> Signed-off-by: Green Wan <green.wan@sifive.com>
> ---
>  arch/riscv/cpu/start.S       | 5 +++++
>  arch/riscv/lib/spl.c         | 4 ++++
>  board/sifive/unmatched/spl.c | 7 +++++++
>  3 files changed, 16 insertions(+)
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 8589509e01..2629f7e4cc 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -102,6 +102,11 @@ call_board_init_f_0:
>         mv      a0, sp
>         jal     board_init_f_alloc_reserve
>
> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> +call_each_cpu_core:
> +       jal     mach_cpu_core_init
> +#endif

Inserting it here potentially corrupts the return value of
board_init_f_alloc_reserve(), and you need to insert it after sp is
initialized for harts other than the boot hart.

> +
>         /*
>          * Save global data pointer for later. We don't set it here because it
>          * is not initialized yet.
> diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c
> index 8baee07bea..e8261d9c19 100644
> --- a/arch/riscv/lib/spl.c
> +++ b/arch/riscv/lib/spl.c
> @@ -14,6 +14,10 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +__weak void mach_cpu_core_init(void)
> +{
> +}
> +
>  __weak int spl_board_init_f(void)
>  {
>         return 0;
> diff --git a/board/sifive/unmatched/spl.c b/board/sifive/unmatched/spl.c
> index 5e1333b09a..cc466f703a 100644
> --- a/board/sifive/unmatched/spl.c
> +++ b/board/sifive/unmatched/spl.c
> @@ -22,6 +22,13 @@
>  #define MODE_SELECT_SD         0xb
>  #define MODE_SELECT_MASK       GENMASK(3, 0)
>
> +void mach_cpu_core_init(void)
> +{
> +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> +       asm volatile ("csrwi 0x7c1, 0");
> +#endif
> +}
> +
>  int spl_board_init_f(void)
>  {
>         int ret;
> --

Regards,
Bin
Green Wan March 23, 2021, 4:12 a.m. UTC | #2
Hi Bin,

I can move it to the place right after stacks for harts are set up and
right before picking up the lottery hart. (like below) How about the
function name? Do we come up with a better function name?
mach_cpu_core_init() is similar to mach_cpu_init() in include/init.h. It
could cause confusion.

116         /* setup stack */
117 #if CONFIG_IS_ENABLED(SMP)
118         /* tp: hart id */
119         slli    t0, tp, CONFIG_STACK_SIZE_SHIFT
120         sub     sp, a0, t0
121 #else
122         mv      sp, a0
123 #endif
124

move it here

125 #ifndef CONFIG_XIP
126         /*
127          * Pick hart to initialize global data and run U-Boot. The
other harts
128          * wait for initialization to complete.
129          */
130         la      t0, hart_lottery
131         li      t1, 1

On Mon, Mar 22, 2021 at 6:04 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> On Mon, Mar 22, 2021 at 5:23 PM Green Wan <green.wan@sifive.com> wrote:
> >
> > I am looking for a proper place to add some M-mode code in SPL to
> > configure features of each core as early as possible. Try to start
> > the discussion by sending this RFC.
> >
> > I tried mach_cpu_init() as the start. And not sure whether I used
> > it correctly, it seems executed only by the core wins lottery in
> > ./arch/riscv/cpu/start.S.
> >
> > Is there a proper place to add some inits for each core like CSR
> > in section 7.6 in document of U74-MC Core Complex Manual[1].
> >
> > There are some alternatives to do it such as
> >
> >  a) sending IPI to each cores
> >     Have to wait for IPI init for each core. And the boot core
> >     have to perform the init by itself and send IPIs to other
> >     cores. It might not be straightforward.
> >  b) hook callback, (like this patch)
> >  c) hard code in ./arch/riscv/cpu/start.c
> >
> > [1]
> https://sifive.cdn.prismic.io/sifive/aee0dd4c-d156-496e-a6c4-db0cf54bbe68_sifive_U74MC_rtl_full_20G1.03.00_manual.pdf
> >
> > Signed-off-by: Green Wan <green.wan@sifive.com>
> > ---
> >  arch/riscv/cpu/start.S       | 5 +++++
> >  arch/riscv/lib/spl.c         | 4 ++++
> >  board/sifive/unmatched/spl.c | 7 +++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 8589509e01..2629f7e4cc 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -102,6 +102,11 @@ call_board_init_f_0:
> >         mv      a0, sp
> >         jal     board_init_f_alloc_reserve
> >
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > +call_each_cpu_core:
> > +       jal     mach_cpu_core_init
> > +#endif
>
> Inserting it here potentially corrupts the return value of
> board_init_f_alloc_reserve(), and you need to insert it after sp is
> initialized for harts other than the boot hart.
>
> > +
> >         /*
> >          * Save global data pointer for later. We don't set it here
> because it
> >          * is not initialized yet.
> > diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c
> > index 8baee07bea..e8261d9c19 100644
> > --- a/arch/riscv/lib/spl.c
> > +++ b/arch/riscv/lib/spl.c
> > @@ -14,6 +14,10 @@
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > +__weak void mach_cpu_core_init(void)
> > +{
> > +}
> > +
> >  __weak int spl_board_init_f(void)
> >  {
> >         return 0;
> > diff --git a/board/sifive/unmatched/spl.c b/board/sifive/unmatched/spl.c
> > index 5e1333b09a..cc466f703a 100644
> > --- a/board/sifive/unmatched/spl.c
> > +++ b/board/sifive/unmatched/spl.c
> > @@ -22,6 +22,13 @@
> >  #define MODE_SELECT_SD         0xb
> >  #define MODE_SELECT_MASK       GENMASK(3, 0)
> >
> > +void mach_cpu_core_init(void)
> > +{
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > +       asm volatile ("csrwi 0x7c1, 0");
> > +#endif
> > +}
> > +
> >  int spl_board_init_f(void)
> >  {
> >         int ret;
> > --
>
> Regards,
> Bin
>
Bin Meng March 23, 2021, 5:07 a.m. UTC | #3
Hi Green,

On Tue, Mar 23, 2021 at 12:12 PM Green Wan <green.wan@sifive.com> wrote:
>
> Hi Bin,
>
> I can move it to the place right after stacks for harts are set up and right before picking up the lottery hart. (like below) How about the function name? Do we come up with a better function name? mach_cpu_core_init() is similar to mach_cpu_init() in include/init.h. It could cause confusion.

How about riscv_core_early_init(), or riscv_hart_early_init()?

Regards,
Bin
Green Wan March 23, 2021, 7:31 a.m. UTC | #4
On Tue, Mar 23, 2021 at 1:07 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> Hi Green,
>
> On Tue, Mar 23, 2021 at 12:12 PM Green Wan <green.wan@sifive.com> wrote:
> >
> > Hi Bin,
> >
> > I can move it to the place right after stacks for harts are set up and
> right before picking up the lottery hart. (like below) How about the
> function name? Do we come up with a better function name?
> mach_cpu_core_init() is similar to mach_cpu_init() in include/init.h. It
> could cause confusion.
>
> How about riscv_core_early_init(), or riscv_hart_early_init()?
>

I'll vote for riscv_hart_early_init() and create the v2 patch based on it.

Thanks,


> Regards,
> Bin
>
diff mbox series

Patch

diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 8589509e01..2629f7e4cc 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -102,6 +102,11 @@  call_board_init_f_0:
 	mv	a0, sp
 	jal	board_init_f_alloc_reserve
 
+#if CONFIG_IS_ENABLED(RISCV_MMODE)
+call_each_cpu_core:
+	jal	mach_cpu_core_init
+#endif
+
 	/*
 	 * Save global data pointer for later. We don't set it here because it
 	 * is not initialized yet.
diff --git a/arch/riscv/lib/spl.c b/arch/riscv/lib/spl.c
index 8baee07bea..e8261d9c19 100644
--- a/arch/riscv/lib/spl.c
+++ b/arch/riscv/lib/spl.c
@@ -14,6 +14,10 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
+__weak void mach_cpu_core_init(void)
+{
+}
+
 __weak int spl_board_init_f(void)
 {
 	return 0;
diff --git a/board/sifive/unmatched/spl.c b/board/sifive/unmatched/spl.c
index 5e1333b09a..cc466f703a 100644
--- a/board/sifive/unmatched/spl.c
+++ b/board/sifive/unmatched/spl.c
@@ -22,6 +22,13 @@ 
 #define MODE_SELECT_SD		0xb
 #define MODE_SELECT_MASK	GENMASK(3, 0)
 
+void mach_cpu_core_init(void)
+{
+#if CONFIG_IS_ENABLED(RISCV_MMODE)
+	asm volatile ("csrwi 0x7c1, 0");
+#endif
+}
+
 int spl_board_init_f(void)
 {
 	int ret;