[U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation
Auer, Lukas
lukas.auer at aisec.fraunhofer.de
Mon Dec 3 23:05:38 UTC 2018
Hi Bin,
On Mon, 2018-12-03 at 16:45 +0800, Bin Meng wrote:
> Hi Anup,
>
> On Mon, Dec 3, 2018 at 4:12 PM Anup Patel <anup at brainfault.org>
> wrote:
> >
> > On Mon, Dec 3, 2018 at 1:27 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Anup,
> > >
> > > On Mon, Dec 3, 2018 at 3:44 PM Anup Patel <anup at brainfault.org>
> > > wrote:
> > > >
> > > > On Mon, Dec 3, 2018 at 1:08 PM Bin Meng <bmeng.cn at gmail.com>
> > > > wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > On Mon, Dec 3, 2018 at 3:31 PM Anup Patel <
> > > > > anup at brainfault.org> wrote:
> > > > > >
> > > > > > On Mon, Dec 3, 2018 at 12:56 PM Bin Meng <
> > > > > > bmeng.cn at gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Anup,
> > > > > > >
> > > > > > > On Mon, Dec 3, 2018 at 3:19 PM Anup Patel <
> > > > > > > anup at brainfault.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Dec 3, 2018 at 12:32 PM Bin Meng <
> > > > > > > > bmeng.cn at gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Anup,
> > > > > > > > >
> > > > > > > > > On Mon, Dec 3, 2018 at 2:43 PM Anup Patel <
> > > > > > > > > anup at brainfault.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Dec 3, 2018 at 12:06 PM Bin Meng <
> > > > > > > > > > bmeng.cn at gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Anup,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Dec 3, 2018 at 1:28 PM Anup Patel <
> > > > > > > > > > > anup at brainfault.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > When running in S-mode, we can use rdtime and
> > > > > > > > > > > > rdtimeh instructions
> > > > > > > > > > > > for reading timer ticks (just like Linux). The
> > > > > > > > > > > > frequency of timer
> > > > > > > > > > > > ticks is passed by prior booting stages in
> > > > > > > > > > > > "timebase-frequency" DT
> > > > > > > > > > > > property of the "/cpus" DT node.
> > > > > > > > > > > >
> > > > > > > > > > > > This patch provides a generic timer
> > > > > > > > > > > > implementation for U-Boot
> > > > > > > > > > > > running in S-mode. For U-Boot running in M-
> > > > > > > > > > > > mode, specific timer
> > > > > > > > > > > > drivers will have to be provided.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Anup Patel <anup at brainfault.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > > arch/Kconfig | 1 -
> > > > > > > > > > > > arch/riscv/Kconfig | 22 +++++++++++----
> > > > > > > > > > > > arch/riscv/lib/Makefile | 1 +
> > > > > > > > > > > > arch/riscv/lib/time.c | 60
> > > > > > > > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > > 4 files changed, 78 insertions(+), 6
> > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > create mode 100644 arch/riscv/lib/time.c
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/arch/Kconfig b/arch/Kconfig
> > > > > > > > > > > > index 9fdd2f7e66..a4fcb3522d 100644
> > > > > > > > > > > > --- a/arch/Kconfig
> > > > > > > > > > > > +++ b/arch/Kconfig
> > > > > > > > > > > > @@ -72,7 +72,6 @@ config RISCV
> > > > > > > > > > > > imply BLK
> > > > > > > > > > > > imply CLK
> > > > > > > > > > > > imply MTD
> > > > > > > > > > > > - imply TIMER
> > > > > > > > > > >
> > > > > > > > > > > No, please don't do this.
> > > > > > > > > >
> > > > > > > > > > I am removing here but selecting it only for M-mode
> > > > > > > > > > U-Boot.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > imply CMD_DM
> > > > > > > > > > > >
> > > > > > > > > > > > config SANDBOX
> > > > > > > > > > > > diff --git a/arch/riscv/Kconfig
> > > > > > > > > > > > b/arch/riscv/Kconfig
> > > > > > > > > > > > index 732a357a99..20a060454b 100644
> > > > > > > > > > > > --- a/arch/riscv/Kconfig
> > > > > > > > > > > > +++ b/arch/riscv/Kconfig
> > > > > > > > > > > > @@ -44,6 +44,23 @@ config ARCH_RV64I
> > > > > > > > > > > >
> > > > > > > > > > > > endchoice
> > > > > > > > > > > >
> > > > > > > > > > > > +choice
> > > > > > > > > > > > + prompt "Run Mode"
> > > > > > > > > > > > + default RISCV_MMODE
> > > > > > > > > > > > +
> > > > > > > > > > > > +config RISCV_MMODE
> > > > > > > > > > > > + bool "Machine"
> > > > > > > > > > > > + select TIMER
> > > > > > > > > > > > + help
> > > > > > > > > > > > + Choose this option to build U-Boot
> > > > > > > > > > > > for RISC-V M-Mode.
> > > > > > > > > > > > +
> > > > > > > > > > > > +config RISCV_SMODE
> > > > > > > > > > > > + bool "Supervisor"
> > > > > > > > > > > > + help
> > > > > > > > > > > > + Choose this option to build U-Boot
> > > > > > > > > > > > for RISC-V S-Mode.
> > > > > > > > > > > > +
> > > > > > > > > > > > +endchoice
> > > > > > > > > > > > +
> > > > > > > > > > >
> > > > > > > > > > > The above changes deserve separate patch, or
> > > > > > > > > > > merge that in the 1st
> > > > > > > > > > > patch in the series.
> > > > > > > > > >
> > > > > > > > > > Sure, I will put Kconfig changes as separate patch.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > config RISCV_ISA_C
> > > > > > > > > > > > bool "Emit compressed instructions"
> > > > > > > > > > > > default y
> > > > > > > > > > > > @@ -55,11 +72,6 @@ config RISCV_ISA_C
> > > > > > > > > > > > config RISCV_ISA_A
> > > > > > > > > > > > def_bool y
> > > > > > > > > > > >
> > > > > > > > > > > > -config RISCV_SMODE
> > > > > > > > > > > > - bool "Run in S-Mode"
> > > > > > > > > > > > - help
> > > > > > > > > > > > - Enable this option to build U-Boot
> > > > > > > > > > > > for RISC-V S-Mode
> > > > > > > > > > > > -
> > > > > > > > > > > > config 32BIT
> > > > > > > > > > > > bool
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/arch/riscv/lib/Makefile
> > > > > > > > > > > > b/arch/riscv/lib/Makefile
> > > > > > > > > > > > index b58db89752..98aa6d40e7 100644
> > > > > > > > > > > > --- a/arch/riscv/lib/Makefile
> > > > > > > > > > > > +++ b/arch/riscv/lib/Makefile
> > > > > > > > > > > > @@ -12,6 +12,7 @@ obj-y += cache.o
> > > > > > > > > > > > obj-y += interrupts.o
> > > > > > > > > > > > obj-y += reset.o
> > > > > > > > > > > > obj-y += setjmp.o
> > > > > > > > > > > > +obj-$(CONFIG_RISCV_SMODE) += time.o
> > > > > > > > > > > >
> > > > > > > > > > > > # For building EFI apps
> > > > > > > > > > > > CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
> > > > > > > > > > > > diff --git a/arch/riscv/lib/time.c
> > > > > > > > > > > > b/arch/riscv/lib/time.c
> > > > > > > > > > > > new file mode 100644
> > > > > > > > > > > > index 0000000000..077e568ca6
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/arch/riscv/lib/time.c
> > > > > > > > > > > > @@ -0,0 +1,60 @@
> > > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > > > > > > +/*
> > > > > > > > > > > > + * Copyright (C) 2018, Anup Patel <
> > > > > > > > > > > > anup at brainfault.org>
> > > > > > > > > > > > + */
> > > > > > > > > > > > +
> > > > > > > > > > > > +#include <common.h>
> > > > > > > > > > > > +#include <fdtdec.h>
> > > > > > > > > > > > +
> > > > > > > > > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > > > > > > > > +
> > > > > > > > > > > > +static unsigned int tbclk;
> > > > > > > > > > > > +
> > > > > > > > > > > > +static void setup_tbclk(void)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + int cpus;
> > > > > > > > > > > > +
> > > > > > > > > > > > + if (!gd->fdt_blob || tbclk)
> > > > > > > > > > > > + return;
> > > > > > > > > > > > +
> > > > > > > > > > > > + cpus = fdt_path_offset(gd->fdt_blob,
> > > > > > > > > > > > "/cpus");
> > > > > > > > > > > > + if (cpus < 0) {
> > > > > > > > > > > > + debug("%s: Missing /cpus
> > > > > > > > > > > > node\n", __func__);
> > > > > > > > > > > > + return;
> > > > > > > > > > > > + }
> > > > > > > > > > > > +
> > > > > > > > > > > > + tbclk = fdtdec_get_int(gd->fdt_blob,
> > > > > > > > > > > > cpus,
> > > > > > > > > > > > + "timebase-
> > > > > > > > > > > > frequency", 1000000);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +ulong notrace get_tbclk(void)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + setup_tbclk();
> > > > > > > > > > > > +
> > > > > > > > > > > > + return tbclk;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +#ifdef CONFIG_64BIT
> > > > > > > > > > > > +uint64_t notrace get_ticks(void)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + unsigned long n;
> > > > > > > > > > > > +
> > > > > > > > > > > > + __asm__ __volatile__ (
> > > > > > > > > > > > + "rdtime %0"
> > > > > > > > > > > > + : "=r" (n));
> > > > > > > > > > > > + return n;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +#else
> > > > > > > > > > > > +uint64_t notrace get_ticks(void)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + uint32_t lo, hi, tmp;
> > > > > > > > > > > > + __asm__ __volatile__ (
> > > > > > > > > > > > + "1:\n"
> > > > > > > > > > > > + "rdtimeh %0\n"
> > > > > > > > > > > > + "rdtime %1\n"
> > > > > > > > > > > > + "rdtimeh %2\n"
> > > > > > > > > > > > + "bne %0, %2, 1b"
> > > > > > > > > > > > + : "=&r" (hi), "=&r" (lo), "=&r"
> > > > > > > > > > > > (tmp));
> > > > > > > > > > > > + return ((uint64_t)hi << 32) | lo;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +#endif
> > > > > > > > > > > > --
> > > > > > > > > > >
> > > > > > > > > > > The proper way to implement timer driver is via
> > > > > > > > > > > DM. Could you please
> > > > > > > > > > > implement a DM timer driver for S-mode?
> > > > > > > > > > >
> > > > > > > > > > > The M-mode timer driver is @
> > > > > > > > > > > http://patchwork.ozlabs.org/patch/996892/, FYI.
> > > > > > > > > >
> > > > > > > > > > "rdtime" is an instruction. It's not an device for
> > > > > > > > > > which we can
> > > > > > > > > > implement DM driver.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes, I know. But that does not mean we cannot write a
> > > > > > > > > driver to do the
> > > > > > > > > paper work.
> > > > > > > >
> > > > > > > > There is no DT node for "rdtime" so how should we probe
> > > > > > > > the DM driver.
> > > > > > > >
> > > > > > >
> > > > > > > I think you can do some modification to create the S-mode
> > > > > > > timer driver
> > > > > > > based on the M-mode driver patch I provided.
> > > > > >
> > > > > > I think you missed my point.
> > > > > >
> > > > > > CLINT is SiFive specific. You have to rename your driver as
> > > > > > clint_timer. The riscv_timer name is in-appropriate.
> > > > > >
> > > > >
> > > > > Yes, I am reconsidering this. Lukas had similar comments, and
> > > > > Rick
> > > > > confirmed that Andes's chip implemented different mtimer/IPI
> > > > > block
> > > > > from SiFive's.
> > > > >
> > > > > > We cannot use CLINT udevice for "rdtime" because:
> > > > > > 1. SOC can implement "rdtime" in HW
> > > > > > 2. SOC can implement some MMIO device (like CLINT)
> > > > > > and emulate "rdtime" in runtime firmware.
> > > > > >
> > > > >
> > > > > It looks you misunderstood things. I was not suggesting you
> > > > > use CLINT
> > > > > device for S-mode timer driver at all. I am not sure why you
> > > > > misunderstood it. Please read my comments carefully.
> > > > >
> > > >
> > > > Can you explain how will the timer probed called if there is no
> > > > matching device in DT?
> > > >
> > >
> > > The timer driver is bound by the CPU driver. See
> > > http://patchwork.ozlabs.org/patch/996902/.
> >
> > Makes sense now. There is no DT based probing. Instead, you
> > are explicitly bind the timer driver.
> >
> > M-mode U-Boot will never use the generic riscv_timer based
> > on "rdtime" instruction. We will mostly have SoC specific timer
> > drivers for M-mode U-Boot.
> >
>
> Unfortunately yes, M-mode timer driver cannot be generic and that's
> what bothered me. It should have been architected clearly in the very
> beginning since the timer is supposed to deliver interrupts directly
> into [M|S]IP on the hart and allowing platform implementation to me
> is
> creating fragmented solutions. If there are other timers available on
> SoC device, I will definitely use them instead.
>
> > I think your CPU driver should only do the explicit device
> > binding if CONFIG_RISCV_SMODE=y.
> >
>
> Yep I haven't figured out a clean way to do the driver binding if
> there are different mtimer implementations, as the mtimer driver name
> has to be made known to the CPU driver. But I think the S-mode driver
> can be generic.
>
How about something like the driver misc/tegra_car.c? So one misc
driver for riscv,clint0, which explicitly binds the M-mode timer and
the syscon driver.
Thanks,
Lukas
> > There is inter-dependency here.
> >
> > My suggestion is, I can include you CPU driver patch and
> > add DM driver for "rdtime" based upon that. Only If you are
> > fine ??
> >
>
> I am fine with that. However there are some review comments that need
> be addressed in my v1 CPU/timer driver series so this should be fixed
> at first. Maybe we can just delay the S-mode timer driver support
> until later, after my series goes in.
>
> Regards,
> Bin
More information about the U-Boot
mailing list