[U-Boot] [PATCH 12/19] riscv: Do some basic architecture level cpu initialization

[Bin Meng]

Hi Lukas,

On Fri, Nov 16, 2018 at 7:10 AM Auer, Lukas
<lukas.auer at aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Tue, 2018-11-13 at 00:22 -0800, Bin Meng wrote:
> > Implement arch_cpu_init() to do some basic architecture level cpu
> > initialization, like FPU enable, etc.
> >
> > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > ---
> >
> >  arch/riscv/cpu/cpu.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > index d9f820c..4e508cf 100644
> > --- a/arch/riscv/cpu/cpu.c
> > +++ b/arch/riscv/cpu/cpu.c
> > @@ -5,6 +5,7 @@
> >
> >  #include <common.h>
> >  #include <asm/csr.h>
> > +#include <asm/encoding.h>
> >
> >  /*
> >   * prior_stage_fdt_address must be stored in the data section since
> > it is used
> > @@ -53,3 +54,23 @@ int print_cpuinfo(void)
> >
> >       return 0;
> >  }
> > +
> > +int arch_cpu_init(void)
> > +{
> > +     /* Enable FPU */
> > +     if (supports_extension('d') || supports_extension('f')) {
> > +             csr_write(mstatus, MSTATUS_FS);
>
> This should use csr_set(), so that we don't overwrite the other mstatus
> entries.
>

Ah, yes!

> > +             csr_write(fcsr, 0);
>
> BBL also clears the floating point registers before clearing fcsr.
> Coreboot does neither of these, I am not sure if they are required.
>

It's not required. I believe this is just for sanitizing these registers.

> > +     }
> > +
> > +     /* Enable user/supervisor use of perf counters */
> > +     if (supports_extension('s'))
> > +             csr_write(scounteren, -1);
> > +     csr_write(mcounteren, -1);
>
> Should we really enable all counters, and for both user and supervisor
> code? I would tend to enable only the ones we need. How about only
> enabling the cycle, time, and instret counters (the rest are not
> currently used in the kernel) and only for supervisor code (so in
> mcounteren)?
>

Yes, not all of them are used by current kernel, but they might be
used in the future. To enable all of them is not a bad idea, unless
turning it on brings some consequences.

> > +
> > +     /* Disable paging */
> > +     if (supports_extension('s'))
> > +             csr_write(sptbr, 0);
>
> sptbr has been renamed to satp.
>

Good catch! Will fix in v2.

Regards,
Bin