Re: [U-Boot] [PATCH 04/19] cpu: Add a RISC-V CPU driver

2018-12-10 Thread Auer, Lukas 
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
>  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 
> > > ---
> > > 
> > 
> > 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 000..23917db
> > > --- /dev/null
> > > +++ b/drivers/cpu/riscv_cpu.c
> > > @@ -0,0 +1,117 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2018, Bin Meng 
> > > + */
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +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

Re: [U-Boot] [PATCH 04/19] cpu: Add a RISC-V CPU driver

2018-12-07 Thread Bin Meng 
Hi Lukas,

On Thu, Nov 15, 2018 at 5:57 AM Auer, Lukas
 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 
> > ---
> >
>
> 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 000..23917db
> > --- /dev/null
> > +++ b/drivers/cpu/riscv_cpu.c
> > @@ -0,0 +1,117 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2018, Bin Meng 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +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.

> > + 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"

Re: [U-Boot] [PATCH 04/19] cpu: Add a RISC-V CPU driver

2018-11-14 Thread Auer, Lukas 
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 
> ---
> 

Since we have the CPU driver, we could also enable CMD_CPU.

>  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 000..23917db
> --- /dev/null
> +++ b/drivers/cpu/riscv_cpu.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, Bin Meng 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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?

> + 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.

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.

Thanks,
Lukas

> + }
> +
> + return 0;
> +}
> +
> +static const struct cpu_ops riscv

[U-Boot] [PATCH 04/19] cpu: Add a RISC-V CPU driver

2018-11-13 Thread Bin Meng 
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 
---

 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 000..23917db
--- /dev/null
+++ b/drivers/cpu/riscv_cpu.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018, Bin Meng 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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);
+
+   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);
+   }
+
+   return 0;
+}
+
+static const struct cpu_ops riscv_cpu_ops = {
+   .get_desc   = riscv_cpu_get_desc,
+   .get_info   = riscv_cpu_get_info,
+   .get_count  = riscv_cpu_get_count,
+};
+
+static const struct udevice_id riscv_cpu_ids[] = {
+   { .compatible = "riscv" },
+   { }
+};
+
+U_BOOT_DRIVER(riscv_cpu) = {
+   .name = "riscv_cpu",
+   .id = UCLASS_CPU,
+   .of_match = riscv_cpu_ids,
+   .bind = riscv_cpu_bind,
+   .ops = &riscv_cpu_ops,
+   .flags = DM_FLAG_PRE_RELOC,
+};
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot