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:
- SOC can implement "rdtime" in HW
- 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