> > From: Anup Patel [mailto:a...@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...@gmail.com> wrote: > > > > > > Hi Anup, > > > > > > On Mon, Dec 3, 2018 at 4:12 PM Anup Patel <a...@brainfault.org> wrote: > > > > > > > > On Mon, Dec 3, 2018 at 1:27 PM Bin Meng <bmeng...@gmail.com> wrote: > > > > > > > > > > Hi Anup, > > > > > > > > > > On Mon, Dec 3, 2018 at 3:44 PM Anup Patel <a...@brainfault.org> wrote: > > > > > > > > > > > > On Mon, Dec 3, 2018 at 1:08 PM Bin Meng <bmeng...@gmail.com> > > wrote: > > > > > > > > > > > > > > Hi Anup, > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 3:31 PM Anup Patel <a...@brainfault.org> > > wrote: > > > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 12:56 PM Bin Meng <bmeng...@gmail.com> > > wrote: > > > > > > > > > > > > > > > > > > Hi Anup, > > > > > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 3:19 PM Anup Patel > > > > > > > > > <a...@brainfault.org> > > wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 12:32 PM Bin Meng > > <bmeng...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Anup, > > > > > > > > > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 2:43 PM Anup Patel > > <a...@brainfault.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 12:06 PM Bin Meng > > <bmeng...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Anup, > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Dec 3, 2018 at 1:28 PM Anup Patel > > <a...@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 <a...@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 > > > > > > > > > > > > > > +<a...@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 ? But the merge window is closed. I am not sure if it is appropriate to send a PR at this time. 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. Rick _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
