On Fri, Nov 30, 2018 at 12:34 PM Rick Chen <rickche...@gmail.com> wrote: > > Anup Patel <a...@brainfault.org> 於 2018年11月27日 週二 下午8:38寫道： > > > > On Tue, Nov 27, 2018 at 4:17 PM Alexander Graf <ag...@suse.de> wrote: > > > > > > > > > > > > On 27.11.18 07:52, Anup Patel wrote: > > > > On Tue, Nov 27, 2018 at 12:09 PM Rick Chen <rickche...@gmail.com> wrote: > > > >> > > > >>>> Subject: [PATCH v5 1/4] riscv: Add kconfig option to run U-Boot in > > > >>>> S-mode > > > >>>> > > > >>>> 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 <a...@brainfault.org> > > > >>>> Reviewed-by: Bin Meng <bmeng...@gmail.com> > > > >>>> Tested-by: Bin Meng <bmeng...@gmail.com> > > > >>>> Reviewed-by: Lukas Auer <lukas.a...@aisec.fraunhofer.de> > > > >>>> --- > > > >>>> arch/riscv/Kconfig | 5 +++++ > > > >>>> arch/riscv/cpu/start.S | 33 ++++++++++++++++++++++++++++ > > > >>>> arch/riscv/include/asm/encoding.h | 2 ++ > > > >>>> arch/riscv/lib/interrupts.c | 36 > > > >>>> +++++++++++++++++++++++-------- > > > >>>> 4 files changed, 67 insertions(+), 9 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..704190f946 100644 > > > >>>> --- a/arch/riscv/cpu/start.S > > > >>>> +++ b/arch/riscv/cpu/start.S > > > >>>> @@ -41,10 +41,18 @@ _start: > > > >>>> li t0, CONFIG_SYS_SDRAM_BASE > > > >>>> SREG a2, 0(t0) > > > >>>> la t0, trap_entry > > > >>>> +#ifdef CONFIG_RISCV_SMODE > > > >>>> + csrw stvec, t0 > > > >>>> +#else > > > >>>> csrw mtvec, t0 > > > >>>> +#endif > > > >>>> > > > >>>> /* mask all interrupts */ > > > >>>> +#ifdef CONFIG_RISCV_SMODE > > > >>>> + csrw sie, zero > > > >>>> +#else > > > >>>> csrw mie, zero > > > >>>> +#endif > > > >>>> > > > >>>> /* Enable cache */ > > > >>>> jal icache_enable > > > >>>> @@ -166,7 +174,11 @@ fix_rela_dyn: > > > >>>> */ > > > >>>> la t0, trap_entry > > > >>>> add t0, t0, t6 > > > >>>> +#ifdef CONFIG_RISCV_SMODE > > > >>>> + csrw stvec, t0 > > > >>>> +#else > > > >>>> csrw mtvec, t0 > > > >>>> +#endif > > > >>>> > > > >>>> clear_bss: > > > >>>> la t0, __bss_start /* t0 <- rel __bss_start in > > > >>>> FLASH */ > > > >>>> @@ -238,17 +250,34 @@ trap_entry: > > > >>>> SREG x29, 29*REGBYTES(sp) > > > >>>> SREG x30, 30*REGBYTES(sp) > > > >>>> SREG x31, 31*REGBYTES(sp) > > > >>>> +#ifdef CONFIG_RISCV_SMODE > > > >>>> + csrr a0, scause > > > >>>> + csrr a1, sepc > > > >>>> +#else > > > >>>> csrr a0, mcause > > > >>>> csrr a1, mepc > > > >>>> +#endif > > > >>>> mv a2, sp > > > >>>> jal handle_trap > > > >>>> +#ifdef CONFIG_RISCV_SMODE > > > >>>> + csrw sepc, a0 > > > >>>> +#else > > > >>>> csrw mepc, a0 > > > >>>> +#endif > > > >>>> > > > >>>> +#ifdef CONFIG_RISCV_SMODE > > > >>>> +/* > > > >>>> + * Remain in S-mode after sret > > > >>>> + */ > > > >>>> + li t0, SSTATUS_SPP > > > >>>> + csrs sstatus, t0 > > > >>>> +#else > > > >>>> /* > > > >>>> * Remain in M-mode after mret > > > >>>> */ > > > >>>> li t0, MSTATUS_MPP > > > >>>> csrs mstatus, t0 > > > >>>> +#endif > > > >> > > > >> It only the difference between s and m in csr. > > > >> The code seem to be duplicate too much. > > > >> Can you implement it in macro ? > > > >> or consider to separate it in different .S file > > > >> > > > >> It may too complex to maintain in the future. > > > >> > > > > > > > > Here are few reasons why not to do this: > > > > 1. We don't have same set of CSRs for M-mode and S-mode. For > > > > certain, M-mode CSRs we don't have any corresponding S-mode > > > > CSRs. > > > > 2. Number of CSRs for each mode are really few so there won't be > > > > much #ifdef in code. Having separate .S will be total overkill and > > > > too much code duplication. > > > > 3. Using macro would mean we use incomplete CSR names > > > > examples: "status" for "mstatus"/"sstatus", "epc" for "mepc"/"sepc", > > > > etc. This means we cannot grep code for use of a CSR. I think this > > > > is less readable compared to using #ifdef > > > > > > > > Despite above reasons, above can always be attempted as > > > > separate patch. > > > > > Hi Anup > > I mean it is not a good way to switch s and m mode like that > #ifdef CONFIG_RISCV_SMODE > csrw sepc, a0 > #else > csrw mepc, a0 > #endif > > > You can use ## to get stvec ot mtvec in different CONFIG > It can be refered to some kernel code as below: > glue.h > #define ____glue(name,fn) name##fn > #define __glue(name,fn) ____glue(name,fn) > > glue-proc.h > #ifdef CONFIG_CPU_ARM720T > #define CPU_NAME cpu_arm720 > #else > #define CPU_NAME cpu_arm7tdmi > #endif > > #define cpu_proc_init __glue(CPU_NAME,_proc_init) > > Then It will compile as cpu_arm720_proc_init or cpu_arm7tdmi_proc_init > in different configuration. > stvec and mtvce can be implement by the same way.
I had understood your suggestion previously. Though I am not fan of this approach, I will change it anyway. Thanks, Anup _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
