[RFC,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
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(+)

> 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