[U-Boot] [PATCH v7 0/4] RISC-V S-mode support

[U-Boot] [PATCH v7 0/4] RISC-V S-mode support

[Anup Patel]

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

[U-Boot] [PATCH v7 1/4] riscv: Add kconfig option to run U-Boot in S-mode

[Anup Patel]

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

[U-Boot] [PATCH v7 2/4] riscv: qemu: Use different SYS_TEXT_BASE for S-mode

[Anup Patel]

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

[U-Boot] [PATCH v7 3/4] riscv: Add S-mode defconfigs for QEMU virt machine

[Anup Patel]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Auer, Lukas]

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

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

[Rick Chen]

> > 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

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

[Bin Meng]

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

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

[Anup Patel]

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

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

[Bin Meng]

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

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

[Rick Chen]

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