[U-Boot] [PATCH 04/19] cpu: Add a RISC-V CPU driver
Auer, Lukas
lukas.auer at aisec.fraunhofer.de
Tue Dec 11 00:03:36 UTC 2018
Hi Bin,
On Fri, 2018-12-07 at 21:59 +0800, Bin Meng wrote:
> Hi Lukas,
>
> On Thu, Nov 15, 2018 at 5:57 AM Auer, Lukas
> <lukas.auer at aisec.fraunhofer.de> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 2018-11-13 at 00:21 -0800, Bin Meng wrote:
> > > This adds a driver for RISC-V CPU. Note the driver will bind
> > > a RISC-V timer driver if "timebase-frequency" property is
> > > present in the device tree.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > ---
> > >
> >
> > Since we have the CPU driver, we could also enable CMD_CPU.
> >
>
> Agreed. Will do in v2.
>
> > > drivers/cpu/Kconfig | 6 +++
> > > drivers/cpu/Makefile | 1 +
> > > drivers/cpu/riscv_cpu.c | 117
> > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 124 insertions(+)
> > > create mode 100644 drivers/cpu/riscv_cpu.c
> > >
> > > diff --git a/drivers/cpu/Kconfig b/drivers/cpu/Kconfig
> > > index d405200..3d5729f 100644
> > > --- a/drivers/cpu/Kconfig
> > > +++ b/drivers/cpu/Kconfig
> > > @@ -13,3 +13,9 @@ config CPU_MPC83XX
> > > select CLK_MPC83XX
> > > help
> > > Support CPU cores for SoCs of the MPC83xx series.
> > > +
> > > +config CPU_RISCV
> > > + bool "Enable RISC-V CPU driver"
> > > + depends on CPU && RISCV
> > > + help
> > > + Support CPU cores for RISC-V architecture.
> > > diff --git a/drivers/cpu/Makefile b/drivers/cpu/Makefile
> > > index 858b037..be0300c 100644
> > > --- a/drivers/cpu/Makefile
> > > +++ b/drivers/cpu/Makefile
> > > @@ -8,4 +8,5 @@ obj-$(CONFIG_CPU) += cpu-uclass.o
> > >
> > > obj-$(CONFIG_ARCH_BMIPS) += bmips_cpu.o
> > > obj-$(CONFIG_CPU_MPC83XX) += mpc83xx_cpu.o
> > > +obj-$(CONFIG_CPU_RISCV) += riscv_cpu.o
> > > obj-$(CONFIG_SANDBOX) += cpu_sandbox.o
> > > diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
> > > new file mode 100644
> > > index 0000000..23917db
> > > --- /dev/null
> > > +++ b/drivers/cpu/riscv_cpu.c
> > > @@ -0,0 +1,117 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2018, Bin Meng <bmeng.cn at gmail.com>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <cpu.h>
> > > +#include <dm.h>
> > > +#include <errno.h>
> > > +#include <dm/device-internal.h>
> > > +#include <dm/lists.h>
> > > +
> > > +static int riscv_cpu_get_desc(struct udevice *dev, char *buf,
> > > int
> > > size)
> > > +{
> > > + const char *isa;
> > > +
> > > + isa = dev_read_string(dev, "riscv,isa");
> > > + if (size < (strlen(isa) + 1))
> > > + return -ENOSPC;
> > > +
> > > + strcpy(buf, isa);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int riscv_cpu_get_info(struct udevice *dev, struct
> > > cpu_info
> > > *info)
> > > +{
> > > + const char *mmu;
> > > +
> > > + dev_read_u32(dev, "clock-frequency", (u32 *)&info-
> > > >cpu_freq);
> > > +
> > > + mmu = dev_read_string(dev, "mmu-type");
> > > + if (!mmu)
> > > + info->features |= BIT(CPU_FEAT_MMU);
> > > +
> >
> > BBL also disables CPUs without an MMU, so it is good to have this
> > information. Where would you disable the CPU in U-Boot? Maybe in
> > arch_fixup_fdt() or in the CPU driver?
> >
>
> I think disabling CPU only needs to be done if booting to S-mode
> payload. If booting to M-mode payload we should leave it as it is.
> arch_fixup_fdt() can be a good place to fix up these sort of things
> but I think that should be another patch.
>
Makes sense. At the moment this is done in BBL anyways, so this is
something we only have to add once we have the OpenSBI.
Thanks,
Lukas
> > > + return 0;
> > > +}
> > > +
> > > +static int riscv_cpu_get_count(struct udevice *dev)
> > > +{
> > > + ofnode node;
> > > + int num = 0;
> > > +
> > > + ofnode_for_each_subnode(node, dev_ofnode(dev->parent)) {
> > > + const char *device_type;
> > > +
> > > + device_type = ofnode_read_string(node,
> > > "device_type");
> > > + if (!device_type)
> > > + continue;
> > > + if (strcmp(device_type, "cpu") == 0)
> > > + num++;
> > > + }
> > > +
> > > + return num;
> > > +}
> > > +
> > > +static int riscv_cpu_bind(struct udevice *dev)
> > > +{
> > > + struct cpu_platdata *plat = dev_get_parent_platdata(dev);
> > > + struct driver *drv;
> > > + struct udevice *timer;
> > > + int ret;
> > > +
> > > + /* save the hart id */
> > > + plat->cpu_id = dev_read_addr(dev);
> > > +
> > > + /* first examine the property in current cpu node */
> > > + ret = dev_read_u32(dev, "timebase-frequency", &plat-
> > > > timebase_freq);
> > >
> > > + /* if not found, then look at the parent /cpus node */
> > > + if (ret)
> > > + dev_read_u32(dev->parent, "timebase-frequency",
> > > + &plat->timebase_freq);
> > > +
> > > + /*
> > > + * Bind riscv-timer driver on hart 0
> > > + *
> > > + * We only instantiate one timer device which is enough for
> > > U-
> > > Boot.
> > > + * Pass the "timebase-frequency" value as the driver data
> > > for
> > > the
> > > + * timer device.
> > > + *
> > > + * Return value is not checked since it's possible that the
> > > timer
> > > + * driver is not included.
> > > + */
> > > + if (!plat->cpu_id && plat->timebase_freq) {
> > > + drv = lists_driver_lookup_name("riscv_timer");
> > > + if (!drv) {
> > > + debug("Cannot find the timer driver, not
> > > included?\n");
> > > + return 0;
> > > + }
> > > +
> > > + device_bind_with_driver_data(dev, drv,
> > > "riscv_timer",
> > > + plat->timebase_freq,
> > > ofnode_null(),
> > > + &timer);
> >
> > You don't need the timer variable here, you can just pass NULL.
> >
>
> Yes, smarter!
>
> > I did not see that you are not checking the return value here, when
> > I
> > wrote my previous email regarding the syscon driver. So it is not
> > actually a problem if two different device implement the
> > functionality
> > for the syscon APIs. I think it is still worth considering to
> > implement
> > something like the misc driver I suggested, however.
> >
>
> Regards,
> Bin
More information about the U-Boot
mailing list