[RFC PATCH v5 1/2] arch: riscv: cpu: Add callback to init each core

[Green Wan]

Thanks, Rick,

It's very helpful. I'll fix them.


On Wed, Apr 14, 2021 at 11:07 AM Rick Chen <rickchen36 at gmail.com> wrote:
>
> > From: Green Wan [mailto:green.wan at 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 at 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