* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
@ 2020-08-14 17:45 Heinrich Schuchardt
2020-08-14 17:52 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2020-08-14 17:45 UTC (permalink / raw)
To: u-boot
On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we
have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in
SMODE.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
arch/riscv/Kconfig | 16 +++++++++++-----
arch/riscv/cpu/fu540/Kconfig | 3 ++-
arch/riscv/cpu/generic/Kconfig | 3 ++-
arch/riscv/include/asm/global_data.h | 2 +-
arch/riscv/lib/Makefile | 2 +-
5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 009a545fcf..96c386225b 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -153,12 +153,18 @@ config 64BIT
bool
config SIFIVE_CLINT
- bool
- depends on RISCV_MMODE || SPL_RISCV_MMODE
+ bool "SiFive's Core Local Interruptor (CLINT) driver"
select REGMAP
select SYSCON
- select SPL_REGMAP if SPL
- select SPL_SYSCON if SPL
+ help
+ The SiFive CLINT block holds memory-mapped control and status registers
+ associated with software and timer interrupts.
+
+config SPL_SIFIVE_CLINT
+ bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"
+ depends on SPL
+ select SPL_REGMAP
+ select SPL_SYSCON
help
The SiFive CLINT block holds memory-mapped control and status registers
associated with software and timer interrupts.
@@ -186,7 +192,7 @@ config ANDES_PLMT
associated with timer tick.
config RISCV_RDTIME
- bool
+ bool "Timer API via rdtime instruction"
default y if RISCV_SMODE || SPL_RISCV_SMODE
help
The provides the riscv_get_time() API that is implemented using the
diff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig
index 2dcad8e27f..8b2c83039f 100644
--- a/arch/riscv/cpu/fu540/Kconfig
+++ b/arch/riscv/cpu/fu540/Kconfig
@@ -8,7 +8,8 @@ config SIFIVE_FU540
imply CPU
imply CPU_RISCV
imply RISCV_TIMER
- imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
+ imply SIFIVE_CLINT if RISCV_MMODE
+ imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
imply CMD_CPU
imply SPL_CPU_SUPPORT
imply SPL_OPENSBI
diff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig
index b2cb155d6d..62fcadd710 100644
--- a/arch/riscv/cpu/generic/Kconfig
+++ b/arch/riscv/cpu/generic/Kconfig
@@ -8,7 +8,8 @@ config GENERIC_RISCV
imply CPU
imply CPU_RISCV
imply RISCV_TIMER
- imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
+ imply SIFIVE_CLINT if RISCV_MMODE
+ imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
imply CMD_CPU
imply SPL_CPU_SUPPORT
imply SPL_OPENSBI
diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
index 2eb14815bc..b89b469d41 100644
--- a/arch/riscv/include/asm/global_data.h
+++ b/arch/riscv/include/asm/global_data.h
@@ -18,7 +18,7 @@
struct arch_global_data {
long boot_hart; /* boot hart id */
phys_addr_t firmware_fdt_addr;
-#ifdef CONFIG_SIFIVE_CLINT
+#if CONFIG_IS_ENABLED(SIFIVE_CLINT)
void __iomem *clint; /* clint base address */
#endif
#ifdef CONFIG_ANDES_PLIC
diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index 6c503ff2b2..861d7b489f 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
obj-$(CONFIG_CMD_GO) += boot.o
obj-y += cache.o
+obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o
ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y)
-obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
obj-$(CONFIG_ANDES_PLIC) += andes_plic.o
obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o
else
--
2.28.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-14 17:45 [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE Heinrich Schuchardt
@ 2020-08-14 17:52 ` Anup Patel
2020-08-14 17:59 ` Heinrich Schuchardt
0 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-08-14 17:52 UTC (permalink / raw)
To: u-boot
On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we
> have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
Can you elaborate why ?
The rdtime instruction should generate an illegal instruction fault which
OpenSBI will emulate.
Regards,
Anup
>
> Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in
> SMODE.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> arch/riscv/Kconfig | 16 +++++++++++-----
> arch/riscv/cpu/fu540/Kconfig | 3 ++-
> arch/riscv/cpu/generic/Kconfig | 3 ++-
> arch/riscv/include/asm/global_data.h | 2 +-
> arch/riscv/lib/Makefile | 2 +-
> 5 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 009a545fcf..96c386225b 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -153,12 +153,18 @@ config 64BIT
> bool
>
> config SIFIVE_CLINT
> - bool
> - depends on RISCV_MMODE || SPL_RISCV_MMODE
> + bool "SiFive's Core Local Interruptor (CLINT) driver"
> select REGMAP
> select SYSCON
> - select SPL_REGMAP if SPL
> - select SPL_SYSCON if SPL
> + help
> + The SiFive CLINT block holds memory-mapped control and status registers
> + associated with software and timer interrupts.
> +
> +config SPL_SIFIVE_CLINT
> + bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"
> + depends on SPL
> + select SPL_REGMAP
> + select SPL_SYSCON
> help
> The SiFive CLINT block holds memory-mapped control and status registers
> associated with software and timer interrupts.
> @@ -186,7 +192,7 @@ config ANDES_PLMT
> associated with timer tick.
>
> config RISCV_RDTIME
> - bool
> + bool "Timer API via rdtime instruction"
> default y if RISCV_SMODE || SPL_RISCV_SMODE
> help
> The provides the riscv_get_time() API that is implemented using the
> diff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig
> index 2dcad8e27f..8b2c83039f 100644
> --- a/arch/riscv/cpu/fu540/Kconfig
> +++ b/arch/riscv/cpu/fu540/Kconfig
> @@ -8,7 +8,8 @@ config SIFIVE_FU540
> imply CPU
> imply CPU_RISCV
> imply RISCV_TIMER
> - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
> + imply SIFIVE_CLINT if RISCV_MMODE
> + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
> imply CMD_CPU
> imply SPL_CPU_SUPPORT
> imply SPL_OPENSBI
> diff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig
> index b2cb155d6d..62fcadd710 100644
> --- a/arch/riscv/cpu/generic/Kconfig
> +++ b/arch/riscv/cpu/generic/Kconfig
> @@ -8,7 +8,8 @@ config GENERIC_RISCV
> imply CPU
> imply CPU_RISCV
> imply RISCV_TIMER
> - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
> + imply SIFIVE_CLINT if RISCV_MMODE
> + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
> imply CMD_CPU
> imply SPL_CPU_SUPPORT
> imply SPL_OPENSBI
> diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> index 2eb14815bc..b89b469d41 100644
> --- a/arch/riscv/include/asm/global_data.h
> +++ b/arch/riscv/include/asm/global_data.h
> @@ -18,7 +18,7 @@
> struct arch_global_data {
> long boot_hart; /* boot hart id */
> phys_addr_t firmware_fdt_addr;
> -#ifdef CONFIG_SIFIVE_CLINT
> +#if CONFIG_IS_ENABLED(SIFIVE_CLINT)
> void __iomem *clint; /* clint base address */
> #endif
> #ifdef CONFIG_ANDES_PLIC
> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> index 6c503ff2b2..861d7b489f 100644
> --- a/arch/riscv/lib/Makefile
> +++ b/arch/riscv/lib/Makefile
> @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
> obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
> obj-$(CONFIG_CMD_GO) += boot.o
> obj-y += cache.o
> +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o
> ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y)
> -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
> obj-$(CONFIG_ANDES_PLIC) += andes_plic.o
> obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o
> else
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-14 17:52 ` Anup Patel
@ 2020-08-14 17:59 ` Heinrich Schuchardt
2020-08-14 18:38 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2020-08-14 17:59 UTC (permalink / raw)
To: u-boot
On 14.08.20 19:52, Anup Patel wrote:
> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we
>> have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
>
> Can you elaborate why ?
>
> The rdtime instruction should generate an illegal instruction fault which
> OpenSBI will emulate.
The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11
has incompatible changes relative to version 1.9.1
mtval on the Kendryte K210 delivers the bad virtual address and not the
instruction:
lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
insn 0x4114121602, epc 0x8058c168.
The illegal instruction being
c01027f3 rdtime a5
In the long run it may make sense to provide backwards compatibility in
OpenSBI.
Without this patch I observe a crash in the loady command. So I cannot
load a file over the UART.
Best regards
Heinrich
>
> Regards,
> Anup
>
>>
>> Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in
>> SMODE.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> arch/riscv/Kconfig | 16 +++++++++++-----
>> arch/riscv/cpu/fu540/Kconfig | 3 ++-
>> arch/riscv/cpu/generic/Kconfig | 3 ++-
>> arch/riscv/include/asm/global_data.h | 2 +-
>> arch/riscv/lib/Makefile | 2 +-
>> 5 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 009a545fcf..96c386225b 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -153,12 +153,18 @@ config 64BIT
>> bool
>>
>> config SIFIVE_CLINT
>> - bool
>> - depends on RISCV_MMODE || SPL_RISCV_MMODE
>> + bool "SiFive's Core Local Interruptor (CLINT) driver"
>> select REGMAP
>> select SYSCON
>> - select SPL_REGMAP if SPL
>> - select SPL_SYSCON if SPL
>> + help
>> + The SiFive CLINT block holds memory-mapped control and status registers
>> + associated with software and timer interrupts.
>> +
>> +config SPL_SIFIVE_CLINT
>> + bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"
>> + depends on SPL
>> + select SPL_REGMAP
>> + select SPL_SYSCON
>> help
>> The SiFive CLINT block holds memory-mapped control and status registers
>> associated with software and timer interrupts.
>> @@ -186,7 +192,7 @@ config ANDES_PLMT
>> associated with timer tick.
>>
>> config RISCV_RDTIME
>> - bool
>> + bool "Timer API via rdtime instruction"
>> default y if RISCV_SMODE || SPL_RISCV_SMODE
>> help
>> The provides the riscv_get_time() API that is implemented using the
>> diff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig
>> index 2dcad8e27f..8b2c83039f 100644
>> --- a/arch/riscv/cpu/fu540/Kconfig
>> +++ b/arch/riscv/cpu/fu540/Kconfig
>> @@ -8,7 +8,8 @@ config SIFIVE_FU540
>> imply CPU
>> imply CPU_RISCV
>> imply RISCV_TIMER
>> - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
>> + imply SIFIVE_CLINT if RISCV_MMODE
>> + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
>> imply CMD_CPU
>> imply SPL_CPU_SUPPORT
>> imply SPL_OPENSBI
>> diff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig
>> index b2cb155d6d..62fcadd710 100644
>> --- a/arch/riscv/cpu/generic/Kconfig
>> +++ b/arch/riscv/cpu/generic/Kconfig
>> @@ -8,7 +8,8 @@ config GENERIC_RISCV
>> imply CPU
>> imply CPU_RISCV
>> imply RISCV_TIMER
>> - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
>> + imply SIFIVE_CLINT if RISCV_MMODE
>> + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
>> imply CMD_CPU
>> imply SPL_CPU_SUPPORT
>> imply SPL_OPENSBI
>> diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
>> index 2eb14815bc..b89b469d41 100644
>> --- a/arch/riscv/include/asm/global_data.h
>> +++ b/arch/riscv/include/asm/global_data.h
>> @@ -18,7 +18,7 @@
>> struct arch_global_data {
>> long boot_hart; /* boot hart id */
>> phys_addr_t firmware_fdt_addr;
>> -#ifdef CONFIG_SIFIVE_CLINT
>> +#if CONFIG_IS_ENABLED(SIFIVE_CLINT)
>> void __iomem *clint; /* clint base address */
>> #endif
>> #ifdef CONFIG_ANDES_PLIC
>> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
>> index 6c503ff2b2..861d7b489f 100644
>> --- a/arch/riscv/lib/Makefile
>> +++ b/arch/riscv/lib/Makefile
>> @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
>> obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
>> obj-$(CONFIG_CMD_GO) += boot.o
>> obj-y += cache.o
>> +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o
>> ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y)
>> -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
>> obj-$(CONFIG_ANDES_PLIC) += andes_plic.o
>> obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o
>> else
>> --
>> 2.28.0
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-14 17:59 ` Heinrich Schuchardt
@ 2020-08-14 18:38 ` Anup Patel
2020-08-14 19:26 ` Heinrich Schuchardt
0 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-08-14 18:38 UTC (permalink / raw)
To: u-boot
On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 14.08.20 19:52, Anup Patel wrote:
> > On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we
> >> have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
> >
> > Can you elaborate why ?
> >
> > The rdtime instruction should generate an illegal instruction fault which
> > OpenSBI will emulate.
>
> The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11
> has incompatible changes relative to version 1.9.1
>
> mtval on the Kendryte K210 delivers the bad virtual address and not the
> instruction:
Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
CSR.
The S-mode software always has working rdtime instruction for all
RISC-V systems. If HW does not implement TIME CSR then OpenSBI
emulates it. Please don't break this convention for U-Boot S-mode
because we do have RISC-V systems where TIME CSR is implemeted
in HW so this will patch will break U-Boot S-mode system because
on those system we are supposed to use TIME CSR from S-mode.
>
> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
> insn 0x4114121602, epc 0x8058c168.
>
> The illegal instruction being
> c01027f3 rdtime a5
>
> In the long run it may make sense to provide backwards compatibility in
> OpenSBI.
Yes, let's try to explore possible fixes in OpenSBI.
Instead of this patch, try the following change in OpenSBI ...
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
index 0e5523f..c8f2e4a 100644
--- a/lib/sbi/sbi_illegal_insn.c
+++ b/lib/sbi/sbi_illegal_insn.c
@@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn, struct
sbi_trap_regs *regs)
struct sbi_trap_info uptrap;
if (unlikely((insn & 3) != 3)) {
- if (insn == 0) {
- insn = sbi_get_insn(regs->mepc, &uptrap);
- if (uptrap.cause) {
- uptrap.epc = regs->mepc;
- return sbi_trap_redirect(regs, &uptrap);
- }
+ insn = sbi_get_insn(regs->mepc, &uptrap);
+ if (uptrap.cause) {
+ uptrap.epc = regs->mepc;
+ return sbi_trap_redirect(regs, &uptrap);
}
if ((insn & 3) != 3)
return truly_illegal_insn(insn, regs);
Regards,
Anup
>
> Without this patch I observe a crash in the loady command. So I cannot
> load a file over the UART.
>
> Best regards
>
> Heinrich
>
> >
> > Regards,
> > Anup
> >
> >>
> >> Adjust Kconfig and Makefile to allow compiling the Sifive CLINT driver in
> >> SMODE.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> arch/riscv/Kconfig | 16 +++++++++++-----
> >> arch/riscv/cpu/fu540/Kconfig | 3 ++-
> >> arch/riscv/cpu/generic/Kconfig | 3 ++-
> >> arch/riscv/include/asm/global_data.h | 2 +-
> >> arch/riscv/lib/Makefile | 2 +-
> >> 5 files changed, 17 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> >> index 009a545fcf..96c386225b 100644
> >> --- a/arch/riscv/Kconfig
> >> +++ b/arch/riscv/Kconfig
> >> @@ -153,12 +153,18 @@ config 64BIT
> >> bool
> >>
> >> config SIFIVE_CLINT
> >> - bool
> >> - depends on RISCV_MMODE || SPL_RISCV_MMODE
> >> + bool "SiFive's Core Local Interruptor (CLINT) driver"
> >> select REGMAP
> >> select SYSCON
> >> - select SPL_REGMAP if SPL
> >> - select SPL_SYSCON if SPL
> >> + help
> >> + The SiFive CLINT block holds memory-mapped control and status registers
> >> + associated with software and timer interrupts.
> >> +
> >> +config SPL_SIFIVE_CLINT
> >> + bool "SiFive's Core Local Interruptor (CLINT) driver in SPL"
> >> + depends on SPL
> >> + select SPL_REGMAP
> >> + select SPL_SYSCON
> >> help
> >> The SiFive CLINT block holds memory-mapped control and status registers
> >> associated with software and timer interrupts.
> >> @@ -186,7 +192,7 @@ config ANDES_PLMT
> >> associated with timer tick.
> >>
> >> config RISCV_RDTIME
> >> - bool
> >> + bool "Timer API via rdtime instruction"
> >> default y if RISCV_SMODE || SPL_RISCV_SMODE
> >> help
> >> The provides the riscv_get_time() API that is implemented using the
> >> diff --git a/arch/riscv/cpu/fu540/Kconfig b/arch/riscv/cpu/fu540/Kconfig
> >> index 2dcad8e27f..8b2c83039f 100644
> >> --- a/arch/riscv/cpu/fu540/Kconfig
> >> +++ b/arch/riscv/cpu/fu540/Kconfig
> >> @@ -8,7 +8,8 @@ config SIFIVE_FU540
> >> imply CPU
> >> imply CPU_RISCV
> >> imply RISCV_TIMER
> >> - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
> >> + imply SIFIVE_CLINT if RISCV_MMODE
> >> + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
> >> imply CMD_CPU
> >> imply SPL_CPU_SUPPORT
> >> imply SPL_OPENSBI
> >> diff --git a/arch/riscv/cpu/generic/Kconfig b/arch/riscv/cpu/generic/Kconfig
> >> index b2cb155d6d..62fcadd710 100644
> >> --- a/arch/riscv/cpu/generic/Kconfig
> >> +++ b/arch/riscv/cpu/generic/Kconfig
> >> @@ -8,7 +8,8 @@ config GENERIC_RISCV
> >> imply CPU
> >> imply CPU_RISCV
> >> imply RISCV_TIMER
> >> - imply SIFIVE_CLINT if (RISCV_MMODE || SPL_RISCV_MMODE)
> >> + imply SIFIVE_CLINT if RISCV_MMODE
> >> + imply SPL_SIFIVE_CLINT if SPL_RISCV_MMODE
> >> imply CMD_CPU
> >> imply SPL_CPU_SUPPORT
> >> imply SPL_OPENSBI
> >> diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> >> index 2eb14815bc..b89b469d41 100644
> >> --- a/arch/riscv/include/asm/global_data.h
> >> +++ b/arch/riscv/include/asm/global_data.h
> >> @@ -18,7 +18,7 @@
> >> struct arch_global_data {
> >> long boot_hart; /* boot hart id */
> >> phys_addr_t firmware_fdt_addr;
> >> -#ifdef CONFIG_SIFIVE_CLINT
> >> +#if CONFIG_IS_ENABLED(SIFIVE_CLINT)
> >> void __iomem *clint; /* clint base address */
> >> #endif
> >> #ifdef CONFIG_ANDES_PLIC
> >> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
> >> index 6c503ff2b2..861d7b489f 100644
> >> --- a/arch/riscv/lib/Makefile
> >> +++ b/arch/riscv/lib/Makefile
> >> @@ -10,8 +10,8 @@ obj-$(CONFIG_CMD_BOOTM) += bootm.o
> >> obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
> >> obj-$(CONFIG_CMD_GO) += boot.o
> >> obj-y += cache.o
> >> +obj-$(CONFIG_$(SPL_)SIFIVE_CLINT) += sifive_clint.o
> >> ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y)
> >> -obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
> >> obj-$(CONFIG_ANDES_PLIC) += andes_plic.o
> >> obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o
> >> else
> >> --
> >> 2.28.0
> >>
>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-14 18:38 ` Anup Patel
@ 2020-08-14 19:26 ` Heinrich Schuchardt
2020-08-15 14:06 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2020-08-14 19:26 UTC (permalink / raw)
To: u-boot
On 8/14/20 8:38 PM, Anup Patel wrote:
> On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 14.08.20 19:52, Anup Patel wrote:
>>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we
>>>> have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
>>>
>>> Can you elaborate why ?
>>>
>>> The rdtime instruction should generate an illegal instruction fault which
>>> OpenSBI will emulate.
>>
>> The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11
>> has incompatible changes relative to version 1.9.1
>>
>> mtval on the Kendryte K210 delivers the bad virtual address and not the
>> instruction:
>
> Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
> CSR.
>
> The S-mode software always has working rdtime instruction for all
> RISC-V systems. If HW does not implement TIME CSR then OpenSBI
> emulates it. Please don't break this convention for U-Boot S-mode
> because we do have RISC-V systems where TIME CSR is implemeted
> in HW so this will patch will break U-Boot S-mode system because
> on those system we are supposed to use TIME CSR from S-mode.
This patch does not change anything for existing systems. It only allows
me to customize U-Boot for the K210 differently. I understand that
fixing OpenSBI is a better approach.
>
>>
>> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
>> insn 0x4114121602, epc 0x8058c168.
>>
>> The illegal instruction being
>> c01027f3 rdtime a5
>>
>> In the long run it may make sense to provide backwards compatibility in
>> OpenSBI.
>
> Yes, let's try to explore possible fixes in OpenSBI.
>
> Instead of this patch, try the following change in OpenSBI ...
>
> diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
> index 0e5523f..c8f2e4a 100644
> --- a/lib/sbi/sbi_illegal_insn.c
> +++ b/lib/sbi/sbi_illegal_insn.c
> @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn, struct
> sbi_trap_regs *regs)
> struct sbi_trap_info uptrap;
>
> if (unlikely((insn & 3) != 3)) {
Why do put sbi_get_insn() behind this if and not before it?
> - if (insn == 0) {
> - insn = sbi_get_insn(regs->mepc, &uptrap);
> - if (uptrap.cause) {
> - uptrap.epc = regs->mepc;
> - return sbi_trap_redirect(regs, &uptrap);
> - }
> + insn = sbi_get_insn(regs->mepc, &uptrap);
> + if (uptrap.cause) {
> + uptrap.epc = regs->mepc;
> + return sbi_trap_redirect(regs, &uptrap);
> }
> if ((insn & 3) != 3)
> return truly_illegal_insn(insn, regs);
>
For this illegal instruction exception the fix works. But I think the
change is in the wrong place.
We have to cover all exceptions, e.g. unaligned access. So we better
should determine the instruction in sbi_trap_handler().
Best regards
Heinrich
> Regards,
> Anup
>
>>
>> Without this patch I observe a crash in the loady command. So I cannot
>> load a file over the UART.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Regards,
>>> Anup
>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-14 19:26 ` Heinrich Schuchardt
@ 2020-08-15 14:06 ` Anup Patel
2020-08-15 15:06 ` Heinrich Schuchardt
0 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-08-15 14:06 UTC (permalink / raw)
To: u-boot
On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/14/20 8:38 PM, Anup Patel wrote:
> > On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 14.08.20 19:52, Anup Patel wrote:
> >>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On the Kendryte K210 OpenBSI cannot emulate the rdtime instruction. So we
> >>>> have to use the Sifive CLINT driver to provide riscv_get_time() in SMODE.
> >>>
> >>> Can you elaborate why ?
> >>>
> >>> The rdtime instruction should generate an illegal instruction fault which
> >>> OpenSBI will emulate.
> >>
> >> The RISC-V Instruction Set Manual Volume II Privileged architectur 1.11
> >> has incompatible changes relative to version 1.9.1
> >>
> >> mtval on the Kendryte K210 delivers the bad virtual address and not the
> >> instruction:
> >
> > Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
> > CSR.
> >
> > The S-mode software always has working rdtime instruction for all
> > RISC-V systems. If HW does not implement TIME CSR then OpenSBI
> > emulates it. Please don't break this convention for U-Boot S-mode
> > because we do have RISC-V systems where TIME CSR is implemeted
> > in HW so this will patch will break U-Boot S-mode system because
> > on those system we are supposed to use TIME CSR from S-mode.
>
> This patch does not change anything for existing systems. It only allows
> me to customize U-Boot for the K210 differently. I understand that
> fixing OpenSBI is a better approach.
Currently, on most RISC-V systems the CLINT timer interrupts and IPI
interrupts are hard-wired to M-mode timer and software interrupt lines.
In other words, the CLINT is integrated as M-mode only device on most
RISC-V systems.
Due to above reason, CLINT driver is M-mode only driver for Linux kernel
The Linux S-mode will use:
1. TIME CSR as free running counter
2. SBI calls for timer interrupts
3. SBI calls for injecting IPIs
For #1 above, the M-mode firmware will trap-n-emulate TIME CSR
for S-mode if HW does not implement TIME CSR.
Based on above mentioned convention, the U-Boot CLINT driver
should be M-mode only and U-Boot S-mode should use TIME CSR
as a free running counter.
>
> >
> >>
> >> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
> >> insn 0x4114121602, epc 0x8058c168.
> >>
> >> The illegal instruction being
> >> c01027f3 rdtime a5
> >>
> >> In the long run it may make sense to provide backwards compatibility in
> >> OpenSBI.
> >
> > Yes, let's try to explore possible fixes in OpenSBI.
> >
> > Instead of this patch, try the following change in OpenSBI ...
> >
> > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
> > index 0e5523f..c8f2e4a 100644
> > --- a/lib/sbi/sbi_illegal_insn.c
> > +++ b/lib/sbi/sbi_illegal_insn.c
> > @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn, struct
> > sbi_trap_regs *regs)
> > struct sbi_trap_info uptrap;
> >
> > if (unlikely((insn & 3) != 3)) {
>
> Why do put sbi_get_insn() behind this if and not before it?
Currently, OpenSBI only deals with 32bit (or longer) illegal
instructions. If we see insn == 0 OR insn is 16bit then we
double-check instruction encoding using unprivileged access.
The PC in RISC-V is always 2-byte aligned address so if MTVAL
has fault instruction address instead of instruction encoding then
"(insn & 3) != 3" will be TRUE and we will be forced to double-check.
The "insn == 0" check below is causing problem RISC-V v1.9 spec
(i.e. MTVAL having instruction address) and it is totally harmless to
remove the "insn == 0" check for RISC-V v1.10 (or higher) hence
my suggestion to remove the check.
>
> > - if (insn == 0) {
> > - insn = sbi_get_insn(regs->mepc, &uptrap);
> > - if (uptrap.cause) {
> > - uptrap.epc = regs->mepc;
> > - return sbi_trap_redirect(regs, &uptrap);
> > - }
> > + insn = sbi_get_insn(regs->mepc, &uptrap);
> > + if (uptrap.cause) {
> > + uptrap.epc = regs->mepc;
> > + return sbi_trap_redirect(regs, &uptrap);
> > }
> > if ((insn & 3) != 3)
> > return truly_illegal_insn(insn, regs);
> >
>
> For this illegal instruction exception the fix works. But I think the
> change is in the wrong place.
>
> We have to cover all exceptions, e.g. unaligned access. So we better
> should determine the instruction in sbi_trap_handler().
That's incorrect.
For RISC-V v1.10 (or higher), the MTVAL contains:
1. Instruction encoding for illegal instruction trap
2. Fault address for unaligned traps, page faults, and access faults
For RISC-V v1.10 (or higher), the unaligned trap does not provide
fault instruction encoding so we end-up doing unpriv access.
Base on above, it's best to work-around your issue in
sbi_illegal_insn_handler() instead of sbi_trap_handler()
Regards,
Anup
>
> Best regards
>
> Heinrich
>
> > Regards,
> > Anup
> >
> >>
> >> Without this patch I observe a crash in the loady command. So I cannot
> >> load a file over the UART.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Regards,
> >>> Anup
> >>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-15 14:06 ` Anup Patel
@ 2020-08-15 15:06 ` Heinrich Schuchardt
2020-08-15 15:55 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2020-08-15 15:06 UTC (permalink / raw)
To: u-boot
Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <anup@brainfault.org>:
>On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>>
>> On 8/14/20 8:38 PM, Anup Patel wrote:
>> > On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>> >>
>> >> On 14.08.20 19:52, Anup Patel wrote:
>> >>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>> >>>>
>> >>>> On the Kendryte K210 OpenBSI cannot emulate the rdtime
>instruction. So we
>> >>>> have to use the Sifive CLINT driver to provide riscv_get_time()
>in SMODE.
>> >>>
>> >>> Can you elaborate why ?
>> >>>
>> >>> The rdtime instruction should generate an illegal instruction
>fault which
>> >>> OpenSBI will emulate.
>> >>
>> >> The RISC-V Instruction Set Manual Volume II Privileged architectur
>1.11
>> >> has incompatible changes relative to version 1.9.1
>> >>
>> >> mtval on the Kendryte K210 delivers the bad virtual address and
>not the
>> >> instruction:
>> >
>> > Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
>> > CSR.
>> >
>> > The S-mode software always has working rdtime instruction for all
>> > RISC-V systems. If HW does not implement TIME CSR then OpenSBI
>> > emulates it. Please don't break this convention for U-Boot S-mode
>> > because we do have RISC-V systems where TIME CSR is implemeted
>> > in HW so this will patch will break U-Boot S-mode system because
>> > on those system we are supposed to use TIME CSR from S-mode.
>>
>> This patch does not change anything for existing systems. It only
>allows
>> me to customize U-Boot for the K210 differently. I understand that
>> fixing OpenSBI is a better approach.
>
>Currently, on most RISC-V systems the CLINT timer interrupts and IPI
>interrupts are hard-wired to M-mode timer and software interrupt lines.
>In other words, the CLINT is integrated as M-mode only device on most
>RISC-V systems.
>
>Due to above reason, CLINT driver is M-mode only driver for Linux
>kernel
>
>The Linux S-mode will use:
>1. TIME CSR as free running counter
>2. SBI calls for timer interrupts
>3. SBI calls for injecting IPIs
>
>For #1 above, the M-mode firmware will trap-n-emulate TIME CSR
>for S-mode if HW does not implement TIME CSR.
>
>Based on above mentioned convention, the U-Boot CLINT driver
>should be M-mode only and U-Boot S-mode should use TIME CSR
>as a free running counter.
>
>>
>> >
>> >>
>> >> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
>> >> insn 0x4114121602, epc 0x8058c168.
>> >>
>> >> The illegal instruction being
>> >> c01027f3 rdtime a5
>> >>
>> >> In the long run it may make sense to provide backwards
>compatibility in
>> >> OpenSBI.
>> >
>> > Yes, let's try to explore possible fixes in OpenSBI.
>> >
>> > Instead of this patch, try the following change in OpenSBI ...
>> >
>> > diff --git a/lib/sbi/sbi_illegal_insn.c
>b/lib/sbi/sbi_illegal_insn.c
>> > index 0e5523f..c8f2e4a 100644
>> > --- a/lib/sbi/sbi_illegal_insn.c
>> > +++ b/lib/sbi/sbi_illegal_insn.c
>> > @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
>struct
>> > sbi_trap_regs *regs)
>> > struct sbi_trap_info uptrap;
>> >
>> > if (unlikely((insn & 3) != 3)) {
>>
>> Why do put sbi_get_insn() behind this if and not before it?
>
>Currently, OpenSBI only deals with 32bit (or longer) illegal
>instructions. If we see insn == 0 OR insn is 16bit then we
>double-check instruction encoding using unprivileged access.
>
>The PC in RISC-V is always 2-byte aligned address so if MTVAL
>has fault instruction address instead of instruction encoding then
>"(insn & 3) != 3" will be TRUE and we will be forced to double-check.
>The "insn == 0" check below is causing problem RISC-V v1.9 spec
>(i.e. MTVAL having instruction address) and it is totally harmless to
>remove the "insn == 0" check for RISC-V v1.10 (or higher) hence
>my suggestion to remove the check.
>
Thank you for your detailed explanation. Maybe you can add a comment to the code.
>>
>> > - if (insn == 0) {
>> > - insn = sbi_get_insn(regs->mepc, &uptrap);
>> > - if (uptrap.cause) {
>> > - uptrap.epc = regs->mepc;
>> > - return sbi_trap_redirect(regs,
>&uptrap);
>> > - }
>> > + insn = sbi_get_insn(regs->mepc, &uptrap);
>> > + if (uptrap.cause) {
>> > + uptrap.epc = regs->mepc;
>> > + return sbi_trap_redirect(regs, &uptrap);
>> > }
>> > if ((insn & 3) != 3)
>> > return truly_illegal_insn(insn, regs);
>> >
>>
>> For this illegal instruction exception the fix works. But I think the
>> change is in the wrong place.
>>
>> We have to cover all exceptions, e.g. unaligned access. So we better
>> should determine the instruction in sbi_trap_handler().
>
>That's incorrect.
>
>For RISC-V v1.10 (or higher), the MTVAL contains:
>1. Instruction encoding for illegal instruction trap
>2. Fault address for unaligned traps, page faults, and access faults
>
>For RISC-V v1.10 (or higher), the unaligned trap does not provide
>fault instruction encoding so we end-up doing unpriv access.
>
>Base on above, it's best to work-around your issue in
>sbi_illegal_insn_handler() instead of sbi_trap_handler()
>
That clarifies it.
For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
Best regards
Heinrich
>Regards,
>Anup
>
>>
>> Best regards
>>
>> Heinrich
>>
>> > Regards,
>> > Anup
>> >
>> >>
>> >> Without this patch I observe a crash in the loady command. So I
>cannot
>> >> load a file over the UART.
>> >>
>> >> Best regards
>> >>
>> >> Heinrich
>> >>
>> >>>
>> >>> Regards,
>> >>> Anup
>> >>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-15 15:06 ` Heinrich Schuchardt
@ 2020-08-15 15:55 ` Anup Patel
2020-08-16 10:14 ` Heinrich Schuchardt
0 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-08-15 15:55 UTC (permalink / raw)
To: u-boot
On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <anup@brainfault.org>:
> >On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt
> ><xypron.glpk@gmx.de> wrote:
> >>
> >> On 8/14/20 8:38 PM, Anup Patel wrote:
> >> > On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
> ><xypron.glpk@gmx.de> wrote:
> >> >>
> >> >> On 14.08.20 19:52, Anup Patel wrote:
> >> >>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
> ><xypron.glpk@gmx.de> wrote:
> >> >>>>
> >> >>>> On the Kendryte K210 OpenBSI cannot emulate the rdtime
> >instruction. So we
> >> >>>> have to use the Sifive CLINT driver to provide riscv_get_time()
> >in SMODE.
> >> >>>
> >> >>> Can you elaborate why ?
> >> >>>
> >> >>> The rdtime instruction should generate an illegal instruction
> >fault which
> >> >>> OpenSBI will emulate.
> >> >>
> >> >> The RISC-V Instruction Set Manual Volume II Privileged architectur
> >1.11
> >> >> has incompatible changes relative to version 1.9.1
> >> >>
> >> >> mtval on the Kendryte K210 delivers the bad virtual address and
> >not the
> >> >> instruction:
> >> >
> >> > Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
> >> > CSR.
> >> >
> >> > The S-mode software always has working rdtime instruction for all
> >> > RISC-V systems. If HW does not implement TIME CSR then OpenSBI
> >> > emulates it. Please don't break this convention for U-Boot S-mode
> >> > because we do have RISC-V systems where TIME CSR is implemeted
> >> > in HW so this will patch will break U-Boot S-mode system because
> >> > on those system we are supposed to use TIME CSR from S-mode.
> >>
> >> This patch does not change anything for existing systems. It only
> >allows
> >> me to customize U-Boot for the K210 differently. I understand that
> >> fixing OpenSBI is a better approach.
> >
> >Currently, on most RISC-V systems the CLINT timer interrupts and IPI
> >interrupts are hard-wired to M-mode timer and software interrupt lines.
> >In other words, the CLINT is integrated as M-mode only device on most
> >RISC-V systems.
> >
> >Due to above reason, CLINT driver is M-mode only driver for Linux
> >kernel
> >
> >The Linux S-mode will use:
> >1. TIME CSR as free running counter
> >2. SBI calls for timer interrupts
> >3. SBI calls for injecting IPIs
> >
> >For #1 above, the M-mode firmware will trap-n-emulate TIME CSR
> >for S-mode if HW does not implement TIME CSR.
> >
> >Based on above mentioned convention, the U-Boot CLINT driver
> >should be M-mode only and U-Boot S-mode should use TIME CSR
> >as a free running counter.
> >
> >>
> >> >
> >> >>
> >> >> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
> >> >> insn 0x4114121602, epc 0x8058c168.
> >> >>
> >> >> The illegal instruction being
> >> >> c01027f3 rdtime a5
> >> >>
> >> >> In the long run it may make sense to provide backwards
> >compatibility in
> >> >> OpenSBI.
> >> >
> >> > Yes, let's try to explore possible fixes in OpenSBI.
> >> >
> >> > Instead of this patch, try the following change in OpenSBI ...
> >> >
> >> > diff --git a/lib/sbi/sbi_illegal_insn.c
> >b/lib/sbi/sbi_illegal_insn.c
> >> > index 0e5523f..c8f2e4a 100644
> >> > --- a/lib/sbi/sbi_illegal_insn.c
> >> > +++ b/lib/sbi/sbi_illegal_insn.c
> >> > @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
> >struct
> >> > sbi_trap_regs *regs)
> >> > struct sbi_trap_info uptrap;
> >> >
> >> > if (unlikely((insn & 3) != 3)) {
> >>
> >> Why do put sbi_get_insn() behind this if and not before it?
> >
> >Currently, OpenSBI only deals with 32bit (or longer) illegal
> >instructions. If we see insn == 0 OR insn is 16bit then we
> >double-check instruction encoding using unprivileged access.
> >
> >The PC in RISC-V is always 2-byte aligned address so if MTVAL
> >has fault instruction address instead of instruction encoding then
> >"(insn & 3) != 3" will be TRUE and we will be forced to double-check.
> >The "insn == 0" check below is causing problem RISC-V v1.9 spec
> >(i.e. MTVAL having instruction address) and it is totally harmless to
> >remove the "insn == 0" check for RISC-V v1.10 (or higher) hence
> >my suggestion to remove the check.
> >
>
> Thank you for your detailed explanation. Maybe you can add a comment to the code.
Sure will do.
>
> >>
> >> > - if (insn == 0) {
> >> > - insn = sbi_get_insn(regs->mepc, &uptrap);
> >> > - if (uptrap.cause) {
> >> > - uptrap.epc = regs->mepc;
> >> > - return sbi_trap_redirect(regs,
> >&uptrap);
> >> > - }
> >> > + insn = sbi_get_insn(regs->mepc, &uptrap);
> >> > + if (uptrap.cause) {
> >> > + uptrap.epc = regs->mepc;
> >> > + return sbi_trap_redirect(regs, &uptrap);
> >> > }
> >> > if ((insn & 3) != 3)
> >> > return truly_illegal_insn(insn, regs);
> >> >
> >>
> >> For this illegal instruction exception the fix works. But I think the
> >> change is in the wrong place.
> >>
> >> We have to cover all exceptions, e.g. unaligned access. So we better
> >> should determine the instruction in sbi_trap_handler().
> >
> >That's incorrect.
> >
> >For RISC-V v1.10 (or higher), the MTVAL contains:
> >1. Instruction encoding for illegal instruction trap
> >2. Fault address for unaligned traps, page faults, and access faults
> >
> >For RISC-V v1.10 (or higher), the unaligned trap does not provide
> >fault instruction encoding so we end-up doing unpriv access.
> >
> >Base on above, it's best to work-around your issue in
> >sbi_illegal_insn_handler() instead of sbi_trap_handler()
> >
>
> That clarifies it.
>
> For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
Yes, already doing that.
>
> Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
Sure, let us know if you find any other issue.
Regards,
Anup
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-15 15:55 ` Anup Patel
@ 2020-08-16 10:14 ` Heinrich Schuchardt
2020-08-16 13:38 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2020-08-16 10:14 UTC (permalink / raw)
To: u-boot
On 8/15/20 5:55 PM, Anup Patel wrote:
> On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <anup@brainfault.org>:
>>> On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 8/14/20 8:38 PM, Anup Patel wrote:
>>>>> On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 14.08.20 19:52, Anup Patel wrote:
>>>>>>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>>>>>>
>>>>>>>> On the Kendryte K210 OpenBSI cannot emulate the rdtime
>>> instruction. So we
>>>>>>>> have to use the Sifive CLINT driver to provide riscv_get_time()
>>> in SMODE.
>>>>>>>
>>>>>>> Can you elaborate why ?
>>>>>>>
>>>>>>> The rdtime instruction should generate an illegal instruction
>>> fault which
>>>>>>> OpenSBI will emulate.
>>>>>>
>>>>>> The RISC-V Instruction Set Manual Volume II Privileged architectur
>>> 1.11
>>>>>> has incompatible changes relative to version 1.9.1
>>>>>>
>>>>>> mtval on the Kendryte K210 delivers the bad virtual address and
>>> not the
>>>>>> instruction:
>>>>>
>>>>> Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
>>>>> CSR.
>>>>>
>>>>> The S-mode software always has working rdtime instruction for all
>>>>> RISC-V systems. If HW does not implement TIME CSR then OpenSBI
>>>>> emulates it. Please don't break this convention for U-Boot S-mode
>>>>> because we do have RISC-V systems where TIME CSR is implemeted
>>>>> in HW so this will patch will break U-Boot S-mode system because
>>>>> on those system we are supposed to use TIME CSR from S-mode.
>>>>
>>>> This patch does not change anything for existing systems. It only
>>> allows
>>>> me to customize U-Boot for the K210 differently. I understand that
>>>> fixing OpenSBI is a better approach.
>>>
>>> Currently, on most RISC-V systems the CLINT timer interrupts and IPI
>>> interrupts are hard-wired to M-mode timer and software interrupt lines.
>>> In other words, the CLINT is integrated as M-mode only device on most
>>> RISC-V systems.
>>>
>>> Due to above reason, CLINT driver is M-mode only driver for Linux
>>> kernel
>>>
>>> The Linux S-mode will use:
>>> 1. TIME CSR as free running counter
>>> 2. SBI calls for timer interrupts
>>> 3. SBI calls for injecting IPIs
>>>
>>> For #1 above, the M-mode firmware will trap-n-emulate TIME CSR
>>> for S-mode if HW does not implement TIME CSR.
>>>
>>> Based on above mentioned convention, the U-Boot CLINT driver
>>> should be M-mode only and U-Boot S-mode should use TIME CSR
>>> as a free running counter.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
>>>>>> insn 0x4114121602, epc 0x8058c168.
>>>>>>
>>>>>> The illegal instruction being
>>>>>> c01027f3 rdtime a5
>>>>>>
>>>>>> In the long run it may make sense to provide backwards
>>> compatibility in
>>>>>> OpenSBI.
>>>>>
>>>>> Yes, let's try to explore possible fixes in OpenSBI.
>>>>>
>>>>> Instead of this patch, try the following change in OpenSBI ...
>>>>>
>>>>> diff --git a/lib/sbi/sbi_illegal_insn.c
>>> b/lib/sbi/sbi_illegal_insn.c
>>>>> index 0e5523f..c8f2e4a 100644
>>>>> --- a/lib/sbi/sbi_illegal_insn.c
>>>>> +++ b/lib/sbi/sbi_illegal_insn.c
>>>>> @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
>>> struct
>>>>> sbi_trap_regs *regs)
>>>>> struct sbi_trap_info uptrap;
>>>>>
>>>>> if (unlikely((insn & 3) != 3)) {
>>>>
>>>> Why do put sbi_get_insn() behind this if and not before it?
>>>
>>> Currently, OpenSBI only deals with 32bit (or longer) illegal
>>> instructions. If we see insn == 0 OR insn is 16bit then we
>>> double-check instruction encoding using unprivileged access.
>>>
>>> The PC in RISC-V is always 2-byte aligned address so if MTVAL
>>> has fault instruction address instead of instruction encoding then
>>> "(insn & 3) != 3" will be TRUE and we will be forced to double-check.
>>> The "insn == 0" check below is causing problem RISC-V v1.9 spec
>>> (i.e. MTVAL having instruction address) and it is totally harmless to
>>> remove the "insn == 0" check for RISC-V v1.10 (or higher) hence
>>> my suggestion to remove the check.
>>>
>>
>> Thank you for your detailed explanation. Maybe you can add a comment to the code.
>
> Sure will do.
>
>>
>>>>
>>>>> - if (insn == 0) {
>>>>> - insn = sbi_get_insn(regs->mepc, &uptrap);
>>>>> - if (uptrap.cause) {
>>>>> - uptrap.epc = regs->mepc;
>>>>> - return sbi_trap_redirect(regs,
>>> &uptrap);
>>>>> - }
>>>>> + insn = sbi_get_insn(regs->mepc, &uptrap);
>>>>> + if (uptrap.cause) {
>>>>> + uptrap.epc = regs->mepc;
>>>>> + return sbi_trap_redirect(regs, &uptrap);
>>>>> }
>>>>> if ((insn & 3) != 3)
>>>>> return truly_illegal_insn(insn, regs);
>>>>>
>>>>
>>>> For this illegal instruction exception the fix works. But I think the
>>>> change is in the wrong place.
>>>>
>>>> We have to cover all exceptions, e.g. unaligned access. So we better
>>>> should determine the instruction in sbi_trap_handler().
>>>
>>> That's incorrect.
>>>
>>> For RISC-V v1.10 (or higher), the MTVAL contains:
>>> 1. Instruction encoding for illegal instruction trap
>>> 2. Fault address for unaligned traps, page faults, and access faults
>>>
>>> For RISC-V v1.10 (or higher), the unaligned trap does not provide
>>> fault instruction encoding so we end-up doing unpriv access.
>>>
>>> Base on above, it's best to work-around your issue in
>>> sbi_illegal_insn_handler() instead of sbi_trap_handler()
>>>
>>
>> That clarifies it.
>>
>> For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
>
> Yes, already doing that.
>
>>
>> Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
grubriscv64.efi with too many modules loaded runs out of memory on the
Maixduino board.
With
./grub-mkimage -O riscv64-efi -o grubriscv64.efi --prefix= -d \
grub-core lsefisystab minicmd normal reboot
I get a GRUB binary containing the minimum required for the U-Boot unit
tests.
After applying the patches
lib: sbi_init: Avoid thundering hurd problem with coldbook_lock -
http://lists.infradead.org/pipermail/opensbi/2020-August/000107.html
lib: sbi: Handle the case were MTVAL has illegal instruction address
http://lists.infradead.org/pipermail/opensbi/2020-August/000108.html
to OpenSBI I can start GRUB and execute the commands used by the U-Boot
unit tests successfully on the Maixduino board.
Best regards
Heinrich
>
> Sure, let us know if you find any other issue.
>
> Regards,
> Anup
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE
2020-08-16 10:14 ` Heinrich Schuchardt
@ 2020-08-16 13:38 ` Anup Patel
0 siblings, 0 replies; 10+ messages in thread
From: Anup Patel @ 2020-08-16 13:38 UTC (permalink / raw)
To: u-boot
On Sun, Aug 16, 2020 at 3:49 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/15/20 5:55 PM, Anup Patel wrote:
> > On Sat, Aug 15, 2020 at 8:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Am 15. August 2020 16:06:41 MESZ schrieb Anup Patel <anup@brainfault.org>:
> >>> On Sat, Aug 15, 2020 at 12:57 AM Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 8/14/20 8:38 PM, Anup Patel wrote:
> >>>>> On Fri, Aug 14, 2020 at 11:35 PM Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>
> >>>>>> On 14.08.20 19:52, Anup Patel wrote:
> >>>>>>> On Fri, Aug 14, 2020 at 11:15 PM Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>>>
> >>>>>>>> On the Kendryte K210 OpenBSI cannot emulate the rdtime
> >>> instruction. So we
> >>>>>>>> have to use the Sifive CLINT driver to provide riscv_get_time()
> >>> in SMODE.
> >>>>>>>
> >>>>>>> Can you elaborate why ?
> >>>>>>>
> >>>>>>> The rdtime instruction should generate an illegal instruction
> >>> fault which
> >>>>>>> OpenSBI will emulate.
> >>>>>>
> >>>>>> The RISC-V Instruction Set Manual Volume II Privileged architectur
> >>> 1.11
> >>>>>> has incompatible changes relative to version 1.9.1
> >>>>>>
> >>>>>> mtval on the Kendryte K210 delivers the bad virtual address and
> >>> not the
> >>>>>> instruction:
> >>>>>
> >>>>> Ahh, I see. The MTVAL CSR is actually legacy MBADADDR CSR and this
> >>>>> CSR.
> >>>>>
> >>>>> The S-mode software always has working rdtime instruction for all
> >>>>> RISC-V systems. If HW does not implement TIME CSR then OpenSBI
> >>>>> emulates it. Please don't break this convention for U-Boot S-mode
> >>>>> because we do have RISC-V systems where TIME CSR is implemeted
> >>>>> in HW so this will patch will break U-Boot S-mode system because
> >>>>> on those system we are supposed to use TIME CSR from S-mode.
> >>>>
> >>>> This patch does not change anything for existing systems. It only
> >>> allows
> >>>> me to customize U-Boot for the K210 differently. I understand that
> >>>> fixing OpenSBI is a better approach.
> >>>
> >>> Currently, on most RISC-V systems the CLINT timer interrupts and IPI
> >>> interrupts are hard-wired to M-mode timer and software interrupt lines.
> >>> In other words, the CLINT is integrated as M-mode only device on most
> >>> RISC-V systems.
> >>>
> >>> Due to above reason, CLINT driver is M-mode only driver for Linux
> >>> kernel
> >>>
> >>> The Linux S-mode will use:
> >>> 1. TIME CSR as free running counter
> >>> 2. SBI calls for timer interrupts
> >>> 3. SBI calls for injecting IPIs
> >>>
> >>> For #1 above, the M-mode firmware will trap-n-emulate TIME CSR
> >>> for S-mode if HW does not implement TIME CSR.
> >>>
> >>> Based on above mentioned convention, the U-Boot CLINT driver
> >>> should be M-mode only and U-Boot S-mode should use TIME CSR
> >>> as a free running counter.
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> lib/sbi/sbi_illegal_insn.c(123) sbi_illegal_insn_handler:
> >>>>>> insn 0x4114121602, epc 0x8058c168.
> >>>>>>
> >>>>>> The illegal instruction being
> >>>>>> c01027f3 rdtime a5
> >>>>>>
> >>>>>> In the long run it may make sense to provide backwards
> >>> compatibility in
> >>>>>> OpenSBI.
> >>>>>
> >>>>> Yes, let's try to explore possible fixes in OpenSBI.
> >>>>>
> >>>>> Instead of this patch, try the following change in OpenSBI ...
> >>>>>
> >>>>> diff --git a/lib/sbi/sbi_illegal_insn.c
> >>> b/lib/sbi/sbi_illegal_insn.c
> >>>>> index 0e5523f..c8f2e4a 100644
> >>>>> --- a/lib/sbi/sbi_illegal_insn.c
> >>>>> +++ b/lib/sbi/sbi_illegal_insn.c
> >>>>> @@ -119,12 +119,10 @@ int sbi_illegal_insn_handler(ulong insn,
> >>> struct
> >>>>> sbi_trap_regs *regs)
> >>>>> struct sbi_trap_info uptrap;
> >>>>>
> >>>>> if (unlikely((insn & 3) != 3)) {
> >>>>
> >>>> Why do put sbi_get_insn() behind this if and not before it?
> >>>
> >>> Currently, OpenSBI only deals with 32bit (or longer) illegal
> >>> instructions. If we see insn == 0 OR insn is 16bit then we
> >>> double-check instruction encoding using unprivileged access.
> >>>
> >>> The PC in RISC-V is always 2-byte aligned address so if MTVAL
> >>> has fault instruction address instead of instruction encoding then
> >>> "(insn & 3) != 3" will be TRUE and we will be forced to double-check.
> >>> The "insn == 0" check below is causing problem RISC-V v1.9 spec
> >>> (i.e. MTVAL having instruction address) and it is totally harmless to
> >>> remove the "insn == 0" check for RISC-V v1.10 (or higher) hence
> >>> my suggestion to remove the check.
> >>>
> >>
> >> Thank you for your detailed explanation. Maybe you can add a comment to the code.
> >
> > Sure will do.
> >
> >>
> >>>>
> >>>>> - if (insn == 0) {
> >>>>> - insn = sbi_get_insn(regs->mepc, &uptrap);
> >>>>> - if (uptrap.cause) {
> >>>>> - uptrap.epc = regs->mepc;
> >>>>> - return sbi_trap_redirect(regs,
> >>> &uptrap);
> >>>>> - }
> >>>>> + insn = sbi_get_insn(regs->mepc, &uptrap);
> >>>>> + if (uptrap.cause) {
> >>>>> + uptrap.epc = regs->mepc;
> >>>>> + return sbi_trap_redirect(regs, &uptrap);
> >>>>> }
> >>>>> if ((insn & 3) != 3)
> >>>>> return truly_illegal_insn(insn, regs);
> >>>>>
> >>>>
> >>>> For this illegal instruction exception the fix works. But I think the
> >>>> change is in the wrong place.
> >>>>
> >>>> We have to cover all exceptions, e.g. unaligned access. So we better
> >>>> should determine the instruction in sbi_trap_handler().
> >>>
> >>> That's incorrect.
> >>>
> >>> For RISC-V v1.10 (or higher), the MTVAL contains:
> >>> 1. Instruction encoding for illegal instruction trap
> >>> 2. Fault address for unaligned traps, page faults, and access faults
> >>>
> >>> For RISC-V v1.10 (or higher), the unaligned trap does not provide
> >>> fault instruction encoding so we end-up doing unpriv access.
> >>>
> >>> Base on above, it's best to work-around your issue in
> >>> sbi_illegal_insn_handler() instead of sbi_trap_handler()
> >>>
> >>
> >> That clarifies it.
> >>
> >> For rdtime the change solves the problem and I can send files to the K210 via the UART using U-Boot's loady command. Will you turn the suggested change into a patch?
> >
> > Yes, already doing that.
> >
> >>
> >> Even with this change grubriscv64.efi is failing on the K210 by acessing incorrect addresses. It runs without failure on QEMU. I will continue analyzing the situation.
>
> grubriscv64.efi with too many modules loaded runs out of memory on the
> Maixduino board.
>
> With
>
> ./grub-mkimage -O riscv64-efi -o grubriscv64.efi --prefix= -d \
> grub-core lsefisystab minicmd normal reboot
>
> I get a GRUB binary containing the minimum required for the U-Boot unit
> tests.
>
> After applying the patches
>
> lib: sbi_init: Avoid thundering hurd problem with coldbook_lock -
> http://lists.infradead.org/pipermail/opensbi/2020-August/000107.html
>
> lib: sbi: Handle the case were MTVAL has illegal instruction address
> http://lists.infradead.org/pipermail/opensbi/2020-August/000108.html
>
> to OpenSBI I can start GRUB and execute the commands used by the U-Boot
> unit tests successfully on the Maixduino board.
If possible please add:
1. defconfig for U-Boot S-mode on Kendryte K210
2. documentation about how to run U-Boot S-mode on Kendryte K210
This will be very helpful to users who want to run some RTOS in S-mode
using U-Boot S-mode.
Regards,
Anup
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-16 13:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14 17:45 [PATCH 1/1] riscv: riscv_get_time() implementation for SMODE Heinrich Schuchardt
2020-08-14 17:52 ` Anup Patel
2020-08-14 17:59 ` Heinrich Schuchardt
2020-08-14 18:38 ` Anup Patel
2020-08-14 19:26 ` Heinrich Schuchardt
2020-08-15 14:06 ` Anup Patel
2020-08-15 15:06 ` Heinrich Schuchardt
2020-08-15 15:55 ` Anup Patel
2020-08-16 10:14 ` Heinrich Schuchardt
2020-08-16 13:38 ` Anup Patel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.