[U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation
Bin Meng
bmeng.cn at gmail.com
Mon Dec 3 08:45:47 UTC 2018
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.
> 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