* [U-Boot] [PATCH v7 0/4] RISC-V S-mode support
@ 2018-12-03 5:27 Anup Patel
2018-12-03 5:27 ` [U-Boot] [PATCH v7 1/4] riscv: Add kconfig option to run U-Boot in S-mode Anup Patel
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Anup Patel @ 2018-12-03 5:27 UTC (permalink / raw)
To: u-boot
This patchset allows us runing u-boot in S-mode which is
useful on platforms where M-mode runtime firmware is an
independent firmware and u-boot is used as last stage OS
bootloader.
The patchset based upon git://git.denx.de/u-boot-riscv.git
and is tested on QEMU in both M-mode and S-mode.
For S-mode testing, we have used u-boot.bin as payload of
latest BBL (at commit 6ebd0f2a46255d0c76dad3c05b16c1d154795d26)
applied with following changes:
diff --git a/machine/emulation.c b/machine/emulation.c
index 132e977..def75e1 100644
--- a/machine/emulation.c
+++ b/machine/emulation.c
@@ -162,6 +162,12 @@ static inline int emulate_read_csr(int num, uintptr_t mstatus, uintptr_t* result
switch (num)
{
+ case CSR_MISA:
+ *result = read_csr(misa);
+ return 0;
+ case CSR_MHARTID:
+ *result = read_csr(mhartid);
+ return 0;
case CSR_CYCLE:
if (!((counteren >> (CSR_CYCLE - CSR_CYCLE)) & 1))
return -1;
Changes since v6:
- Added patch for S-mode timer implementation
Changes since v5:
- Dropped PATCH4 to remove redundant a2 store on DRAM base in start.S
because it will taken care by Rick as separate patch
- Added MODE_PREFIX() macro to generate mode specific CSR names
Changes since v4:
- Rebased series based on commit 52923c6db7f00e0197ec894c8c1bb8a7681974bb
of git://git.denx.de/u-boot-riscv.git
- Added a patch to remove redundant a2 store on DRAM base. This
store was creating problem booting U-Boot in S-mode using BBL.
Changes since v3:
- Replaced 'u-boot' with 'U-Boot' in commit message
- Dropped 'an' in RISCV_SMODE kconfig option help message
- Added appropriate #ifdef in arch/riscv/lib/interrupts.c
Changes since v2:
- Dropped 'default n" from RISCV_SMODE kconfig option
- Replaced '-smode_' in defconfig names with '_smode_'
Changes since v1:
- Rebased upon latest git://git.denx.de/u-boot-riscv.git
- Add details in cover letter for running u-boot in S-mode
using BBL
Anup Patel (4):
riscv: Add kconfig option to run U-Boot in S-mode
riscv: qemu: Use different SYS_TEXT_BASE for S-mode
riscv: Add S-mode defconfigs for QEMU virt machine
RISC-V: Add S-mode timer implementation
arch/Kconfig | 1 -
arch/riscv/Kconfig | 17 ++++++++
arch/riscv/cpu/start.S | 23 ++++++----
arch/riscv/include/asm/encoding.h | 6 +++
arch/riscv/lib/Makefile | 1 +
arch/riscv/lib/interrupts.c | 31 +++++++++----
arch/riscv/lib/time.c | 60 ++++++++++++++++++++++++++
board/emulation/qemu-riscv/Kconfig | 3 +-
board/emulation/qemu-riscv/MAINTAINERS | 2 +
configs/qemu-riscv32_smode_defconfig | 10 +++++
configs/qemu-riscv64_smode_defconfig | 11 +++++
11 files changed, 146 insertions(+), 19 deletions(-)
create mode 100644 arch/riscv/lib/time.c
create mode 100644 configs/qemu-riscv32_smode_defconfig
create mode 100644 configs/qemu-riscv64_smode_defconfig
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [U-Boot] [PATCH v7 1/4] riscv: Add kconfig option to run U-Boot in S-mode 2018-12-03 5:27 [U-Boot] [PATCH v7 0/4] RISC-V S-mode support Anup Patel @ 2018-12-03 5:27 ` Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 2/4] riscv: qemu: Use different SYS_TEXT_BASE for S-mode Anup Patel ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Anup Patel @ 2018-12-03 5:27 UTC (permalink / raw) To: u-boot This patch adds kconfig option RISCV_SMODE to run U-Boot in S-mode. When this opition is enabled we use s<xyz> CSRs instead of m<xyz> CSRs. It is important to note that there is no equivalent S-mode CSR for misa and mhartid CSRs so we expect M-mode runtime firmware (BBL or equivalent) to emulate misa and mhartid CSR read. In-future, we will have more patches to avoid accessing misa and mhartid CSRs from S-mode. Signed-off-by: Anup Patel <anup@brainfault.org> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Tested-by: Bin Meng <bmeng.cn@gmail.com> Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de> --- arch/riscv/Kconfig | 5 +++++ arch/riscv/cpu/start.S | 23 +++++++++++++++-------- arch/riscv/include/asm/encoding.h | 6 ++++++ arch/riscv/lib/interrupts.c | 31 ++++++++++++++++++++++--------- 4 files changed, 48 insertions(+), 17 deletions(-) diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 3e0af55e71..732a357a99 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -55,6 +55,11 @@ 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/cpu/start.S b/arch/riscv/cpu/start.S index 15e1b8199a..3f055bdb7e 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -41,10 +41,10 @@ _start: li t0, CONFIG_SYS_SDRAM_BASE SREG a2, 0(t0) la t0, trap_entry - csrw mtvec, t0 + csrw MODE_PREFIX(tvec), t0 /* mask all interrupts */ - csrw mie, zero + csrw MODE_PREFIX(ie), zero /* Enable cache */ jal icache_enable @@ -166,7 +166,7 @@ fix_rela_dyn: */ la t0, trap_entry add t0, t0, t6 - csrw mtvec, t0 + csrw MODE_PREFIX(tvec), t0 clear_bss: la t0, __bss_start /* t0 <- rel __bss_start in FLASH */ @@ -238,17 +238,24 @@ trap_entry: SREG x29, 29*REGBYTES(sp) SREG x30, 30*REGBYTES(sp) SREG x31, 31*REGBYTES(sp) - csrr a0, mcause - csrr a1, mepc + csrr a0, MODE_PREFIX(cause) + csrr a1, MODE_PREFIX(epc) mv a2, sp jal handle_trap - csrw mepc, a0 + csrw MODE_PREFIX(epc), a0 +#ifdef CONFIG_RISCV_SMODE +/* + * Remain in S-mode after sret + */ + li t0, SSTATUS_SPP +#else /* * Remain in M-mode after mret */ li t0, MSTATUS_MPP - csrs mstatus, t0 +#endif + csrs MODE_PREFIX(status), t0 LREG x1, 1*REGBYTES(sp) LREG x2, 2*REGBYTES(sp) LREG x3, 3*REGBYTES(sp) @@ -281,4 +288,4 @@ trap_entry: LREG x30, 30*REGBYTES(sp) LREG x31, 31*REGBYTES(sp) addi sp, sp, 32*REGBYTES - mret + MODE_PREFIX(ret) diff --git a/arch/riscv/include/asm/encoding.h b/arch/riscv/include/asm/encoding.h index 9ea50ce640..97cf906aa6 100644 --- a/arch/riscv/include/asm/encoding.h +++ b/arch/riscv/include/asm/encoding.h @@ -7,6 +7,12 @@ #ifndef RISCV_CSR_ENCODING_H #define RISCV_CSR_ENCODING_H +#ifdef CONFIG_RISCV_SMODE +#define MODE_PREFIX(__suffix) s##__suffix +#else +#define MODE_PREFIX(__suffix) m##__suffix +#endif + #define MSTATUS_UIE 0x00000001 #define MSTATUS_SIE 0x00000002 #define MSTATUS_HIE 0x00000004 diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c index 903a1c4cd5..3aff006977 100644 --- a/arch/riscv/lib/interrupts.c +++ b/arch/riscv/lib/interrupts.c @@ -34,17 +34,30 @@ int disable_interrupts(void) return 0; } -ulong handle_trap(ulong mcause, ulong epc, struct pt_regs *regs) +ulong handle_trap(ulong cause, ulong epc, struct pt_regs *regs) { - ulong is_int; + ulong is_irq, irq; - is_int = (mcause & MCAUSE_INT); - if ((is_int) && ((mcause & MCAUSE_CAUSE) == IRQ_M_EXT)) - external_interrupt(0); /* handle_m_ext_interrupt */ - else if ((is_int) && ((mcause & MCAUSE_CAUSE) == IRQ_M_TIMER)) - timer_interrupt(0); /* handle_m_timer_interrupt */ - else - _exit_trap(mcause, epc, regs); + is_irq = (cause & MCAUSE_INT); + irq = (cause & ~MCAUSE_INT); + + if (is_irq) { + switch (irq) { + case IRQ_M_EXT: + case IRQ_S_EXT: + external_interrupt(0); /* handle external interrupt */ + break; + case IRQ_M_TIMER: + case IRQ_S_TIMER: + timer_interrupt(0); /* handle timer interrupt */ + break; + default: + _exit_trap(cause, epc, regs); + break; + }; + } else { + _exit_trap(cause, epc, regs); + } return epc; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 2/4] riscv: qemu: Use different SYS_TEXT_BASE for S-mode 2018-12-03 5:27 [U-Boot] [PATCH v7 0/4] RISC-V S-mode support Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 1/4] riscv: Add kconfig option to run U-Boot in S-mode Anup Patel @ 2018-12-03 5:27 ` Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 3/4] riscv: Add S-mode defconfigs for QEMU virt machine Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation Anup Patel 3 siblings, 0 replies; 23+ messages in thread From: Anup Patel @ 2018-12-03 5:27 UTC (permalink / raw) To: u-boot When u-boot runs in S-mode, the M-mode runtime firmware (BBL or equivalent) uses memory range in 0x80000000 to 0x80200000. Due to this, we cannot use 0x80000000 as SYS_TEXT_BASE when running in S-mode. Instead for S-mode, we use 0x80200000 as SYS_TEXT_BASE. Even Linux RISC-V kernel ignores/reserves memory range 0x80000000 to 0x80200000 because it runs in S-mode. Signed-off-by: Anup Patel <anup@brainfault.org> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Tested-by: Bin Meng <bmeng.cn@gmail.com> Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de> --- board/emulation/qemu-riscv/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/board/emulation/qemu-riscv/Kconfig b/board/emulation/qemu-riscv/Kconfig index 33ca253432..56bb5337d4 100644 --- a/board/emulation/qemu-riscv/Kconfig +++ b/board/emulation/qemu-riscv/Kconfig @@ -13,7 +13,8 @@ config SYS_CONFIG_NAME default "qemu-riscv" config SYS_TEXT_BASE - default 0x80000000 + default 0x80000000 if !RISCV_SMODE + default 0x80200000 if RISCV_SMODE config BOARD_SPECIFIC_OPTIONS # dummy def_bool y -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 3/4] riscv: Add S-mode defconfigs for QEMU virt machine 2018-12-03 5:27 [U-Boot] [PATCH v7 0/4] RISC-V S-mode support Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 1/4] riscv: Add kconfig option to run U-Boot in S-mode Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 2/4] riscv: qemu: Use different SYS_TEXT_BASE for S-mode Anup Patel @ 2018-12-03 5:27 ` Anup Patel 2018-12-03 5:27 ` [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation Anup Patel 3 siblings, 0 replies; 23+ messages in thread From: Anup Patel @ 2018-12-03 5:27 UTC (permalink / raw) To: u-boot This patch adds S-mode defconfigs for QEMU virt machine so that we can run u-boot in S-mode on QEMU using M-mode runtime firmware (BBL or equivalent). Signed-off-by: Anup Patel <anup@brainfault.org> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Tested-by: Bin Meng <bmeng.cn@gmail.com> Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de> --- board/emulation/qemu-riscv/MAINTAINERS | 2 ++ configs/qemu-riscv32_smode_defconfig | 10 ++++++++++ configs/qemu-riscv64_smode_defconfig | 11 +++++++++++ 3 files changed, 23 insertions(+) create mode 100644 configs/qemu-riscv32_smode_defconfig create mode 100644 configs/qemu-riscv64_smode_defconfig diff --git a/board/emulation/qemu-riscv/MAINTAINERS b/board/emulation/qemu-riscv/MAINTAINERS index 3c6eb4f844..c701c83d77 100644 --- a/board/emulation/qemu-riscv/MAINTAINERS +++ b/board/emulation/qemu-riscv/MAINTAINERS @@ -4,4 +4,6 @@ S: Maintained F: board/emulation/qemu-riscv/ F: include/configs/qemu-riscv.h F: configs/qemu-riscv32_defconfig +F: configs/qemu-riscv32_smode_defconfig F: configs/qemu-riscv64_defconfig +F: configs/qemu-riscv64_smode_defconfig diff --git a/configs/qemu-riscv32_smode_defconfig b/configs/qemu-riscv32_smode_defconfig new file mode 100644 index 0000000000..0a84ec1874 --- /dev/null +++ b/configs/qemu-riscv32_smode_defconfig @@ -0,0 +1,10 @@ +CONFIG_RISCV=y +CONFIG_TARGET_QEMU_VIRT=y +CONFIG_RISCV_SMODE=y +CONFIG_DISTRO_DEFAULTS=y +CONFIG_NR_DRAM_BANKS=1 +CONFIG_FIT=y +CONFIG_DISPLAY_CPUINFO=y +CONFIG_DISPLAY_BOARDINFO=y +# CONFIG_CMD_MII is not set +CONFIG_OF_PRIOR_STAGE=y diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig new file mode 100644 index 0000000000..b012443370 --- /dev/null +++ b/configs/qemu-riscv64_smode_defconfig @@ -0,0 +1,11 @@ +CONFIG_RISCV=y +CONFIG_TARGET_QEMU_VIRT=y +CONFIG_ARCH_RV64I=y +CONFIG_RISCV_SMODE=y +CONFIG_DISTRO_DEFAULTS=y +CONFIG_NR_DRAM_BANKS=1 +CONFIG_FIT=y +CONFIG_DISPLAY_CPUINFO=y +CONFIG_DISPLAY_BOARDINFO=y +# CONFIG_CMD_MII is not set +CONFIG_OF_PRIOR_STAGE=y -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 5:27 [U-Boot] [PATCH v7 0/4] RISC-V S-mode support Anup Patel ` (2 preceding siblings ...) 2018-12-03 5:27 ` [U-Boot] [PATCH v7 3/4] riscv: Add S-mode defconfigs for QEMU virt machine Anup Patel @ 2018-12-03 5:27 ` Anup Patel 2018-12-03 6:36 ` Bin Meng 3 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 5:27 UTC (permalink / raw) To: u-boot 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 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 + 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 -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 5:27 ` [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation Anup Patel @ 2018-12-03 6:36 ` Bin Meng 2018-12-03 6:43 ` Anup Patel 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-03 6:36 UTC (permalink / raw) To: u-boot 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. > 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. > 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. Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 6:36 ` Bin Meng @ 2018-12-03 6:43 ` Anup Patel 2018-12-03 7:01 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 6:43 UTC (permalink / raw) To: u-boot 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. This is similar to how U-Boot ARM64 uses CNTPCT_EL0 register. We cannot use DM driver here. Regards, Anup ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 6:43 ` Anup Patel @ 2018-12-03 7:01 ` Bin Meng 2018-12-03 7:19 ` Anup Patel 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-03 7:01 UTC (permalink / raw) To: u-boot 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. > This is similar to how U-Boot ARM64 uses CNTPCT_EL0 register. > > We cannot use DM driver here. > The issue is that eventually we will migrate all U-Boot device drivers to DM, which means get_tbclk() and get_ticks() you implemented will be broken someday in the future. Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:01 ` Bin Meng @ 2018-12-03 7:19 ` Anup Patel 2018-12-03 7:26 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 7:19 UTC (permalink / raw) To: u-boot 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. > > > This is similar to how U-Boot ARM64 uses CNTPCT_EL0 register. > > > > We cannot use DM driver here. > > > > The issue is that eventually we will migrate all U-Boot device drivers > to DM, which means get_tbclk() and get_ticks() you implemented will be > broken someday in the future. Yes, I agree we should have DM driver whenever possible but sometime there is no DT bindings for things like timer because it's part of CPU instruction or CSRs Regards, Anup ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:19 ` Anup Patel @ 2018-12-03 7:26 ` Bin Meng 2018-12-03 7:30 ` Anup Patel 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-03 7:26 UTC (permalink / raw) To: u-boot 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. In the riscv_timer_probe(), do not require syscon driver to be loaded, ie: not calling syscon_get_by_driver_data(). In the riscv_timer_get_count(), implement that with rdtime for S-mode. > > > > > This is similar to how U-Boot ARM64 uses CNTPCT_EL0 register. > > > > > > We cannot use DM driver here. > > > > > > > The issue is that eventually we will migrate all U-Boot device drivers > > to DM, which means get_tbclk() and get_ticks() you implemented will be > > broken someday in the future. > > Yes, I agree we should have DM driver whenever possible but sometime > there is no DT bindings for things like timer because it's part of CPU > instruction or CSRs > Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:26 ` Bin Meng @ 2018-12-03 7:30 ` Anup Patel 2018-12-03 7:38 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 7:30 UTC (permalink / raw) To: u-boot 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. 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. > > In the riscv_timer_probe(), do not require syscon driver to be loaded, > ie: not calling syscon_get_by_driver_data(). In the > riscv_timer_get_count(), implement that with rdtime for S-mode. There is no udevice for "rdtime" or time CSRs. CLINT is a SiFive specific device. Please rename your driver. Regards, Anup ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:30 ` Anup Patel @ 2018-12-03 7:38 ` Bin Meng 2018-12-03 7:44 ` Anup Patel 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-03 7:38 UTC (permalink / raw) To: u-boot 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. > > > > In the riscv_timer_probe(), do not require syscon driver to be loaded, > > ie: not calling syscon_get_by_driver_data(). In the > > riscv_timer_get_count(), implement that with rdtime for S-mode. > > There is no udevice for "rdtime" or time CSRs. > > CLINT is a SiFive specific device. Please rename your driver. > Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:38 ` Bin Meng @ 2018-12-03 7:44 ` Anup Patel 2018-12-03 7:57 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 7:44 UTC (permalink / raw) To: u-boot 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? Regards, Anup ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:44 ` Anup Patel @ 2018-12-03 7:57 ` Bin Meng 2018-12-03 8:12 ` Anup Patel 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-03 7:57 UTC (permalink / raw) To: u-boot 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/. Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 7:57 ` Bin Meng @ 2018-12-03 8:12 ` Anup Patel 2018-12-03 8:45 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 8:12 UTC (permalink / raw) To: u-boot 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. I think your CPU driver should only do the explicit device binding if CONFIG_RISCV_SMODE=y. 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 ?? Regards, Anup ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 8:12 ` Anup Patel @ 2018-12-03 8:45 ` Bin Meng 2018-12-03 10:29 ` Anup Patel 2018-12-03 23:05 ` Auer, Lukas 0 siblings, 2 replies; 23+ messages in thread From: Bin Meng @ 2018-12-03 8:45 UTC (permalink / raw) To: u-boot 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. > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 8:45 ` Bin Meng @ 2018-12-03 10:29 ` Anup Patel [not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA3A54CF4@ATCPCS16.andestech.com> 2018-12-03 23:05 ` Auer, Lukas 1 sibling, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-03 10:29 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <752D002CFF5D0F4FA35C0100F1D73F3FA3A54CF4@ATCPCS16.andestech.com>]
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation [not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA3A54CF4@ATCPCS16.andestech.com> @ 2018-12-04 7:13 ` Rick Chen 2018-12-04 8:14 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Rick Chen @ 2018-12-04 7:13 UTC (permalink / raw) To: u-boot > > 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 ? 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-04 7:13 ` Rick Chen @ 2018-12-04 8:14 ` Bin Meng 2018-12-04 8:37 ` Anup Patel 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-04 8:14 UTC (permalink / raw) To: u-boot 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. > 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. Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-04 8:14 ` Bin Meng @ 2018-12-04 8:37 ` Anup Patel 2018-12-04 9:36 ` Bin Meng 0 siblings, 1 reply; 23+ messages in thread From: Anup Patel @ 2018-12-04 8:37 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-04 8:37 ` Anup Patel @ 2018-12-04 9:36 ` Bin Meng 2018-12-05 2:30 ` Rick Chen 0 siblings, 1 reply; 23+ messages in thread From: Bin Meng @ 2018-12-04 9:36 UTC (permalink / raw) To: u-boot 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 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. > Agreed. Regards, Bin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-04 9:36 ` Bin Meng @ 2018-12-05 2:30 ` Rick Chen 0 siblings, 0 replies; 23+ messages in thread From: Rick Chen @ 2018-12-05 2:30 UTC (permalink / raw) To: u-boot Bin Meng <bmeng.cn@gmail.com> 於 2018年12月4日 週二 下午5:36寫道： > > 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 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. > > > > Agreed. > > Regards, > Bin I have applied to u-boot-riscv/master Travis is running ... Then I will send a PR later Thanks Rick ^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation 2018-12-03 8:45 ` Bin Meng 2018-12-03 10:29 ` Anup Patel @ 2018-12-03 23:05 ` Auer, Lukas 1 sibling, 0 replies; 23+ messages in thread From: Auer, Lukas @ 2018-12-03 23:05 UTC (permalink / raw) To: u-boot Hi Bin, On Mon, 2018-12-03 at 16:45 +0800, Bin Meng 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 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@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. > How about something like the driver misc/tegra_car.c? So one misc driver for riscv,clint0, which explicitly binds the M-mode timer and the syscon driver. Thanks, Lukas > > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-12-05 2:30 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-03 5:27 [U-Boot] [PATCH v7 0/4] RISC-V S-mode support Anup Patel
2018-12-03 5:27 ` [U-Boot] [PATCH v7 1/4] riscv: Add kconfig option to run U-Boot in S-mode Anup Patel
2018-12-03 5:27 ` [U-Boot] [PATCH v7 2/4] riscv: qemu: Use different SYS_TEXT_BASE for S-mode Anup Patel
2018-12-03 5:27 ` [U-Boot] [PATCH v7 3/4] riscv: Add S-mode defconfigs for QEMU virt machine Anup Patel
2018-12-03 5:27 ` [U-Boot] [PATCH v7 4/4] RISC-V: Add S-mode timer implementation Anup Patel
2018-12-03 6:36 ` Bin Meng
2018-12-03 6:43 ` Anup Patel
2018-12-03 7:01 ` Bin Meng
2018-12-03 7:19 ` Anup Patel
2018-12-03 7:26 ` Bin Meng
2018-12-03 7:30 ` Anup Patel
2018-12-03 7:38 ` Bin Meng
2018-12-03 7:44 ` Anup Patel
2018-12-03 7:57 ` Bin Meng
2018-12-03 8:12 ` Anup Patel
2018-12-03 8:45 ` Bin Meng
2018-12-03 10:29 ` Anup Patel
[not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA3A54CF4@ATCPCS16.andestech.com>
2018-12-04 7:13 ` Rick Chen
2018-12-04 8:14 ` Bin Meng
2018-12-04 8:37 ` Anup Patel
2018-12-04 9:36 ` Bin Meng
2018-12-05 2:30 ` Rick Chen
2018-12-03 23:05 ` Auer, Lukas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.