[U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation

[Anup Patel <anup@brainfault.org>]

On Tue, Dec 4, 2018 at 1:44 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Rick,
>
> On Tue, Dec 4, 2018 at 3:12 PM Rick Chen <rickchen36@gmail.com> wrote:
> >
> > > > From: Anup Patel [mailto:anup at brainfault.org]
> > > > Sent: Monday, December 03, 2018 6:30 PM
> > > > To: Bin Meng
> > > > Cc: Rick Jian-Zhi Chen(陳建志); Lukas Auer; Alexander Graf; Palmer Dabbelt;
> > > > Atish Patra; Christoph Hellwig; U-Boot Mailing List
> > > > Subject: Re: [PATCH v7 4/4] RISC-V: Add S-mode timer implementation
> > > >
> > > > On Mon, Dec 3, 2018 at 2:16 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > On Mon, Dec 3, 2018 at 4:12 PM Anup Patel <anup@brainfault.org> wrote:
> > > > > >
> > > > > > On Mon, Dec 3, 2018 at 1:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Anup,
> > > > > > >
> > > > > > > On Mon, Dec 3, 2018 at 3:44 PM Anup Patel <anup@brainfault.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Dec 3, 2018 at 1:08 PM Bin Meng <bmeng.cn@gmail.com>
> > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi Anup,
> > > > > > > > >
> > > > > > > > > On Mon, Dec 3, 2018 at 3:31 PM Anup Patel <anup@brainfault.org>
> > > > wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Dec 3, 2018 at 12:56 PM Bin Meng <bmeng.cn@gmail.com>
> > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Anup,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Dec 3, 2018 at 3:19 PM Anup Patel <anup@brainfault.org>
> > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Dec 3, 2018 at 12:32 PM Bin Meng
> > > > <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Anup,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Dec 3, 2018 at 2:43 PM Anup Patel
> > > > <anup@brainfault.org> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 12:06 PM Bin Meng
> > > > <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi Anup,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 1:28 PM Anup Patel
> > > > <anup@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@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@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.
> > > >
> > > > Even, I think timer CSRs should be clearly defined and accessible from both M and
> > > > S modes.
> > > >
> > > > >
> > > > > > 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.
> > > > >
> > > >
> > > > I think first three patches of this series can go in without S-mode timer patch.
> > > >
> > > > Regards,
> > > > Anup
> >
> > Hi Anup
> >
> > get_ticks( ) is indeed an old way which shall be replace by dm timer.
> > atcpit100_timer is  SoC dm timer which is used by ae350.
> > Maybe Bin is trying to say that rdtime shall be called in
> > riscv_timer_get_count( ) in riscv_timer.c
> > And then trigger a SBI call to M-mode and get mtime when u-boot is
> > running in S-mode, right ?
> >
>
> Correct.
>
> > But the merge window is closed.
> > I am not sure if it is appropriate to send a PR at this time.
> >
>
> I think it is OK to send the PR as the first 3 patches of S-mode
> support are pretty much RISC-V specific.

Yes, please go ahead with first 3 patches.

>
> > Maybe I can push your first three patches of this series into u-boot-riscv.git
> > And send a PR to master when next merge window open.
> >
>
> For my patch series, I will send v2 soon to address the comments and I
> hope they can be included in v2019.01 release too, and Anup can send
> out the S-mode timer support on top of that.

Sure, I will base riscv_timer on your v2.

I have thought about this more....

The rdtime based riscv_timer will be enabled by default for S-mode U-Boot
but for M-mode U-Boot people can write their own DM timer driver or use
riscv_timer driver if their CPU support rdtime in M-mode.

Regards,
Anup