Hi Anup,
On Tue, Dec 4, 2018 at 4:37 PM Anup Patel anup@brainfault.org wrote:
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@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.
Agreed.
Regards, Bin