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

[Green Wan]

Hi Rick,

Sorry for the late reply. My working environment got problems. Just
recovered from it. See my reply below.

On Tue, Apr 13, 2021 at 11:33 AM Rick Chen <rickchen36 at gmail.com> wrote:
>
> Hi Green,
>
> > From: Green Wan [mailto:green.wan at sifive.com]
> > Sent: Tuesday, March 30, 2021 1:27 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; u-boot at lists.denx.de
> > Subject: [RFC PATCH v4 1/2] arch: riscv: cpu: Add callback to init each core
> >
> > Add a callback riscv_hart_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 at sifive.com>
> > ---
> >  arch/riscv/cpu/cpu.c   | 15 +++++++++++++++
> >  arch/riscv/cpu/start.S | 14 ++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > index 85592f5bee..1652e51137 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();
> >  }
> > +
> > +/**
> > + * riscv_hart_early_init() - A dummy function called by
>
> Maybe you can rename as harts_early_init(), riscv seems to be extra prefix here.
okay, I'll remove the prefix.

> And dummy sounds like a negative word, it will be better to describe
> it as a callback for customize CSR features ...etc.
>
sure, I can remove 'dummy' and rephrase the comments.

But I wonder whether we all agree the callback is ONLY for customizing
CSRs. It's probably fine to leave the current comments for some
flexibility if any implementation needs to do init differently.

> > + * ./arch/riscv/cpu/start.S to allow to disable/enable features of each core.
> > + * For example, turn on or off the functional block of CPU harts.
> > + *
> > + * 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
> > + * necessary.
> > + */
> > +__weak void riscv_hart_early_init(void)
> > +{
> > +}
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 8589509e01..ab73008f23 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -117,6 +117,20 @@ call_board_init_f_0:
> >         mv      sp, a0
> >  #endif
> >
> > +#if CONFIG_IS_ENABLED(RISCV_MMODE)
> > +       /*
> > +        * Jump to riscv_hart_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.
>
> You can emphasize this init is a callback for customize CSR settings
> for all harts simply.
> Any memory access is prohibited here.

Same here. To satisfy my current need, I'll say 'yes'. ;)
But I don't know whether everyone is with me and want to restrict the
callback only for customizing CRSs.

>
> > +        *
> > +        * A dummy implementation is provided in ./arch/riscv/cpu/cpu.c.
>
> This description for path is not necessary here.
>

Will fix it.

> Thanks,
> Rick
>
> > +        */
> > +call_riscv_hart_early_init:
> > +       jal     riscv_hart_early_init
> > +#endif
> > +
> >  #ifndef CONFIG_XIP
> >         /*
> >          * Pick hart to initialize global data and run U-Boot. The other harts
> > --
> > 2.31.0