[U-Boot] [RESEND PATCH v2 08/15] riscv: Add a helper routine to print CPU information

[Auer, Lukas]

Hi Bin,

On Tue, 2018-09-18 at 16:53 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Tue, Sep 18, 2018 at 5:59 AM Auer, Lukas
> <lukas.auer at aisec.fraunhofer.de> wrote:
> > 
> > Hi Bin,
> > 
> > On Mon, 2018-09-17 at 12:55 +0800, Bin Meng wrote:
> > > Hi Lukas,
> > > 
> > > On Mon, Sep 17, 2018 at 4:54 AM Auer, Lukas
> > > <lukas.auer at aisec.fraunhofer.de> wrote:
> > > > 
> > > > Hi Bin,
> > > > 
> > > > On Mon, 2018-09-10 at 21:54 -0700, Bin Meng wrote:
> > > > > This adds a helper routine to print CPU information.
> > > > > Currently
> > > > > it prints all the instruction set extensions that the
> > > > > processor
> > > > > core supports.
> > > > > 
> > > > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > > > ---
> > > > > 
> > > > > Changes in v2: None
> > > > > 
> > > > >  arch/riscv/Makefile          |   1 +
> > > > >  arch/riscv/cpu/Makefile      |   5 ++
> > > > >  arch/riscv/cpu/cpu.c         |  49 +++++++++++++++++
> > > > >  arch/riscv/include/asm/csr.h | 124
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 179 insertions(+)
> > > > >  create mode 100644 arch/riscv/cpu/Makefile
> > > > >  create mode 100644 arch/riscv/cpu/cpu.c
> > > > >  create mode 100644 arch/riscv/include/asm/csr.h
> > > > > 
> > > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > > index 084888a..af432e1 100644
> > > > > --- a/arch/riscv/Makefile
> > > > > +++ b/arch/riscv/Makefile
> > > > > @@ -5,5 +5,6 @@
> > > > > 
> > > > >  head-y := arch/riscv/cpu/$(CPU)/start.o
> > > > > 
> > > > > +libs-y += arch/riscv/cpu/
> > > > >  libs-y += arch/riscv/cpu/$(CPU)/
> > > > >  libs-y += arch/riscv/lib/
> > > > > diff --git a/arch/riscv/cpu/Makefile
> > > > > b/arch/riscv/cpu/Makefile
> > > > > new file mode 100644
> > > > > index 0000000..63de163
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/cpu/Makefile
> > > > > @@ -0,0 +1,5 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0+
> > > > > +#
> > > > > +# Copyright (C) 2018, Bin Meng <bmeng.cn at gmail.com>
> > > > > +
> > > > > +obj-y += cpu.o
> > > > > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> > > > > new file mode 100644
> > > > > index 0000000..ae57fb8
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/cpu/cpu.c
> > > > > @@ -0,0 +1,49 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * Copyright (C) 2018, Bin Meng <bmeng.cn at gmail.com>
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <asm/csr.h>
> > > > > +
> > > > > +enum {
> > > > > +     ISA_INVALID = 0,
> > > > > +     ISA_32BIT,
> > > > > +     ISA_64BIT,
> > > > > +     ISA_128BIT
> > > > > +};
> > > > > +
> > > > > +static const char * const isa_bits[] = {
> > > > > +     [ISA_INVALID] = NULL,
> > > > > +     [ISA_32BIT]   = "32",
> > > > > +     [ISA_64BIT]   = "64",
> > > > > +     [ISA_128BIT]  = "128"
> > > > > +};
> > > > > +
> > > > > +static inline bool supports_extension(char ext)
> > > > > +{
> > > > > +     return csr_read(misa) & (1 << (ext - 'a'));
> > > > > +}
> > > > > +
> > > > > +int print_cpuinfo(void)
> > > > > +{
> > > > > +     char name[32];
> > > > > +     char *s = name;
> > > > > +     int bit;
> > > > > +
> > > > > +     s += sprintf(name, "rv");
> > > > > +     bit = csr_read(misa) >> (sizeof(long) * 8 - 2);
> > > > > +     s += sprintf(s, isa_bits[bit]);
> > > > > +
> > > > > +     supports_extension('i') ? *s++ = 'i' : 'r';
> > > > > +     supports_extension('m') ? *s++ = 'm' : 'i';
> > > > > +     supports_extension('a') ? *s++ = 'a' : 's';
> > > > > +     supports_extension('f') ? *s++ = 'f' : 'c';
> > > > > +     supports_extension('d') ? *s++ = 'd' : '-';
> > > > > +     supports_extension('c') ? *s++ = 'c' : 'v';
> > > > > +     *s++ = '\0';
> > > > 
> > > > Why are you not using the ISA string "riscv,isa" from the
> > > > device
> > > > tree?
> > > > This way, the code is not limited to machine mode and we do not
> > > > have
> > > > update it if the set of extensions to check for changes.
> > > 
> > > I wanted to use hardware provided information whenever possible.
> > > Reading from DT can be a last resort. I think we wanted to have
> > > U-
> > > Boot
> > > running in machine mode, no?
> > > 
> > > Regards,
> > > Bin
> > > 
> > 
> > Ok, I see. I don't entirely like that this has to be updated if we
> > want
> > to display new extensions and it's currently actually missing the
> > 'u'
> > (user) and 's' (supervisor) extensions, which are present in the
> > misa
> > of RISC-V QEMU. It is fine for now, I think.
> > 
> 
> Yes, let's leave this for now. Currently it seems only prints "GC"
> extension is enough (IMAFDC).
> 

That makes sense.

> > Yes, we want to run u-boot in machine mode for now, especially if
> > we
> > want to add an SBI implementation to u-boot. Not much is needed to
> > run
> > it in supervisor mode though, which is why I mentioned this. It
> > also
> > allows us to use an SBI implementation external to u-boot, should
> > we
> > want / need that.
> > 
> 
> Let's see how things go. My personal preference would be only using
> U-Boot though :)
> 
> Regards,
> Bin

Yes that certainly is the best solution, at least for now. :) I think a
separate SBI implementation only starts to make sense, if it becomes a
lot more complicated / feature-rich than it is now.

Thanks,
Lukas