Re: [PATCH 15/15] HiFive1: disable/enable interrupts during context switch
On Fri, Aug 18, 2017 at 11:30 AM, Denis Obrezkov wrote: > 2017-08-17 23:58 GMT+02:00 Gedare Bloom : >> >> On Thu, Aug 17, 2017 at 4:17 PM, Denis Obrezkov >> wrote: >> > 2017-08-17 22:07 GMT+02:00 Gedare Bloom : >> >> >> >> On Thu, Aug 17, 2017 at 1:48 PM, Denis Obrezkov >> >> >> >> wrote: >> >> > 2017-08-17 17:25 GMT+02:00 Gedare Bloom : >> >> >> >> >> >> On Wed, Aug 16, 2017 at 11:13 AM, Denis Obrezkov >> >> >> wrote: >> >> >> > --- >> >> >> > cpukit/score/cpu/riscv32/riscv-context-switch.S | 12 ++-- >> >> >> > 1 file changed, 10 insertions(+), 2 deletions(-) >> >> >> > >> >> >> > diff --git a/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> > b/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> > index a199596..bcdfe0e 100644 >> >> >> > --- a/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> > +++ b/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> > @@ -46,6 +46,7 @@ PUBLIC(restore) >> >> >> > >> >> >> > SYM(_CPU_Context_switch): >> >> >> >/* Disable interrupts and store all registers */ >> >> >> > + csrci mstatus, 0x8 >> >> >> Why is this necessary? >> >> >> >> >> >> >SREG x1, 4(a0) >> >> >> >SREG x2, 8(a0) >> >> >> >SREG x3, 12(a0) >> >> >> > @@ -78,8 +79,9 @@ SYM(_CPU_Context_switch): >> >> >> >SREG x30, 120(a0) >> >> >> >SREG x31, 124(a0) >> >> >> > >> >> >> > -SYM(restore): >> >> >> > >> >> >> > +SYM(restore): >> >> >> > + >> >> >> >LREG x1, 4(a1) >> >> >> >LREG x2, 8(a1) >> >> >> >LREG x3, 12(a1) >> >> >> > @@ -111,9 +113,15 @@ SYM(restore): >> >> >> >LREG x29, 116(a1) >> >> >> >LREG x30, 120(a1) >> >> >> >LREG x31, 124(a1) >> >> >> > - ret >> >> >> > + >> >> >> > + >> >> >> > + csrsi mstatus, 0x8 >> >> >> > + nop >> >> >> > + nop >> >> >> Why the nops? >> >> >> >> >> >> > + ret >> >> >> > >> >> >> > SYM(_CPU_Context_restore): >> >> >> > + csrci mstatus, 0x8 >> >> >> >mv a1, a0 >> >> >> >j restore >> >> >> >nop >> >> >> > -- >> >> >> > 2.1.4 >> >> >> > >> >> >> > ___ >> >> >> > devel mailing list >> >> >> > devel@rtems.org >> >> >> > http://lists.rtems.org/mailman/listinfo/devel >> >> > >> >> > So, don't we turn off interrupts during the context switch? >> >> >> >> Nope, and turning them back on unconditionally is wrong too. >> >> >> >> > Yes, nops are unnecessary. >> >> > >> >> > >> >> > -- >> >> > Regards, Denis Obrezkov >> > >> > Ok, I was confused by this obsolete comment: >> > /* Disable interrupts and store all registers */ >> > Will remove all that enabling/disabling. >> > >> > Is the same true for start.S file with interrupted task's stack saving? >> > >> I'm not sure exactly what you're referring to, but usually start.S >> will, among other things, turn off and ack pending interrupts to >> quiesce the hardware before handing off the rest of system init to >> boot_card. Eventually RTEMS will enable interrupts again using the >> cpukit's interrupt_enable layer. >> >> > >> > >> > -- >> > Regards, Denis Obrezkov > > I mean when an interrupt occurs, the execution flow jumps to the trap > address. > Then, in my case all process'es registers are saved in a common stack > (I know that dedicated interrupt stack should be implemented, but I haven't > time to do that) > and then we jump to IRQ dispatching routine. After interrupt handling we > restore saved > registers. So, my question is - should we disable/enable interrupts during > interrupt handling? > Disabling: > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L201 > Jump to dispatching routine: > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L241 > Enabling interrupts back: > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L285 > This interrupt handling is incomplete/broken I think. Keeping the interrupts turned off for the duration of RTEMS + User ISR handling is not "real-time". What other CPU did you look at to understand how to deal with interrupts? Usually, the BSP will support installing interrupt handlers that jump to ISR_Handler(), which then sets up a safe "C" environment for the user handler to use. The ISR_Handler() should enable interrupts before invoking the user code. From what I can tell, your implementation of ISR_Handler doesn't do anything. Gedare > > -- > Regards, Denis Obrezkov ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 15/15] HiFive1: disable/enable interrupts during context switch
2017-08-19 14:33 GMT+02:00 Gedare Bloom : > On Fri, Aug 18, 2017 at 11:30 AM, Denis Obrezkov > wrote: > > 2017-08-17 23:58 GMT+02:00 Gedare Bloom : > >> > >> On Thu, Aug 17, 2017 at 4:17 PM, Denis Obrezkov < > denisobrez...@gmail.com> > >> wrote: > >> > 2017-08-17 22:07 GMT+02:00 Gedare Bloom : > >> >> > >> >> On Thu, Aug 17, 2017 at 1:48 PM, Denis Obrezkov > >> >> > >> >> wrote: > >> >> > 2017-08-17 17:25 GMT+02:00 Gedare Bloom : > >> >> >> > >> >> >> On Wed, Aug 16, 2017 at 11:13 AM, Denis Obrezkov > >> >> >> wrote: > >> >> >> > --- > >> >> >> > cpukit/score/cpu/riscv32/riscv-context-switch.S | 12 > ++-- > >> >> >> > 1 file changed, 10 insertions(+), 2 deletions(-) > >> >> >> > > >> >> >> > diff --git a/cpukit/score/cpu/riscv32/riscv-context-switch.S > >> >> >> > b/cpukit/score/cpu/riscv32/riscv-context-switch.S > >> >> >> > index a199596..bcdfe0e 100644 > >> >> >> > --- a/cpukit/score/cpu/riscv32/riscv-context-switch.S > >> >> >> > +++ b/cpukit/score/cpu/riscv32/riscv-context-switch.S > >> >> >> > @@ -46,6 +46,7 @@ PUBLIC(restore) > >> >> >> > > >> >> >> > SYM(_CPU_Context_switch): > >> >> >> >/* Disable interrupts and store all registers */ > >> >> >> > + csrci mstatus, 0x8 > >> >> >> Why is this necessary? > >> >> >> > >> >> >> >SREG x1, 4(a0) > >> >> >> >SREG x2, 8(a0) > >> >> >> >SREG x3, 12(a0) > >> >> >> > @@ -78,8 +79,9 @@ SYM(_CPU_Context_switch): > >> >> >> >SREG x30, 120(a0) > >> >> >> >SREG x31, 124(a0) > >> >> >> > > >> >> >> > -SYM(restore): > >> >> >> > > >> >> >> > +SYM(restore): > >> >> >> > + > >> >> >> >LREG x1, 4(a1) > >> >> >> >LREG x2, 8(a1) > >> >> >> >LREG x3, 12(a1) > >> >> >> > @@ -111,9 +113,15 @@ SYM(restore): > >> >> >> >LREG x29, 116(a1) > >> >> >> >LREG x30, 120(a1) > >> >> >> >LREG x31, 124(a1) > >> >> >> > - ret > >> >> >> > + > >> >> >> > + > >> >> >> > + csrsi mstatus, 0x8 > >> >> >> > + nop > >> >> >> > + nop > >> >> >> Why the nops? > >> >> >> > >> >> >> > + ret > >> >> >> > > >> >> >> > SYM(_CPU_Context_restore): > >> >> >> > + csrci mstatus, 0x8 > >> >> >> >mv a1, a0 > >> >> >> >j restore > >> >> >> >nop > >> >> >> > -- > >> >> >> > 2.1.4 > >> >> >> > > >> >> >> > ___ > >> >> >> > devel mailing list > >> >> >> > devel@rtems.org > >> >> >> > http://lists.rtems.org/mailman/listinfo/devel > >> >> > > >> >> > So, don't we turn off interrupts during the context switch? > >> >> > >> >> Nope, and turning them back on unconditionally is wrong too. > >> >> > >> >> > Yes, nops are unnecessary. > >> >> > > >> >> > > >> >> > -- > >> >> > Regards, Denis Obrezkov > >> > > >> > Ok, I was confused by this obsolete comment: > >> > /* Disable interrupts and store all registers */ > >> > Will remove all that enabling/disabling. > >> > > >> > Is the same true for start.S file with interrupted task's stack > saving? > >> > > >> I'm not sure exactly what you're referring to, but usually start.S > >> will, among other things, turn off and ack pending interrupts to > >> quiesce the hardware before handing off the rest of system init to > >> boot_card. Eventually RTEMS will enable interrupts again using the > >> cpukit's interrupt_enable layer. > >> > >> > > >> > > >> > -- > >> > Regards, Denis Obrezkov > > > > I mean when an interrupt occurs, the execution flow jumps to the trap > > address. > > Then, in my case all process'es registers are saved in a common stack > > (I know that dedicated interrupt stack should be implemented, but I > haven't > > time to do that) > > and then we jump to IRQ dispatching routine. After interrupt handling we > > restore saved > > registers. So, my question is - should we disable/enable interrupts > during > > interrupt handling? > > Disabling: > > https://github.com/embeddedden/rtems-riscv/blob/ > hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L201 > > Jump to dispatching routine: > > https://github.com/embeddedden/rtems-riscv/blob/ > hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L241 > > Enabling interrupts back: > > https://github.com/embeddedden/rtems-riscv/blob/ > hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L285 > > > This interrupt handling is incomplete/broken I think. Keeping the > interrupts turned off for the duration of RTEMS + User ISR handling is > not "real-time". What other CPU did you look at to understand how to > deal with interrupts? > I'v kept Hasham's RISC-V generic interrupt code. I thought that we don't want to have nested interrupts. I also tried to turn off interrupts only during register saving/restoring. > > Usually, the BSP will support installing interrupt handlers that jump > to ISR_Handler(), which then sets up a safe "C" environment for the > user handler to use. The ISR_Handler() should enable interrupts before > invoking the user code. From what I can tell, your implementation of > ISR_Handler doesn't do anything. > Ok, I am going to figure out how to imple
Re: [PATCH v2 1/4] waf_generator: Copy headers if necessary.
On 17/8/17 11:46 am, Sichen Zhao wrote: >>> On 17 Aug 2017, at 1:37 am, Sichen Zhao wrote: >>> >>> Hi Chris, >>> Just for remind: can the openssl patch get merged? >> Yes they can. I am sorry I had some pressing issues locally and a few >> patches queued. I am on the road today and will see if I can get to them >> remotely. It is top of my list. > Np, and thank you. Pushed. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 02/15] HiFive1: add linker file
You can try to share this file (with RISC-V based BSPs and other
shared linkcmds if any that are arch-independent) and avoid
duplications, and only include the relevant parts of your HiFive
boards, similar to ARM-based BSPs.
It's important you try to address comments/feedback; students should
go through the code review process, etc it's one of the goals of
Google Summer of Code and open-source involvement.
On Fri, Aug 18, 2017 at 2:59 AM, Denis Obrezkov wrote:
> 2017-08-17 16:57 GMT+02:00 Gedare Bloom :
>>
>> On Wed, Aug 16, 2017 at 11:12 AM, Denis Obrezkov
>> wrote:
>> > ---
>> > c/src/lib/libbsp/riscv32/hifive1/startup/linkcmds | 379
>> > ++
>> > 1 file changed, 379 insertions(+)
>> > create mode 100644 c/src/lib/libbsp/riscv32/hifive1/startup/linkcmds
>> >
>> > diff --git a/c/src/lib/libbsp/riscv32/hifive1/startup/linkcmds
>> > b/c/src/lib/libbsp/riscv32/hifive1/startup/linkcmds
>> > new file mode 100644
>> > index 000..8bf56a0
>> > --- /dev/null
>> > +++ b/c/src/lib/libbsp/riscv32/hifive1/startup/linkcmds
>> > @@ -0,0 +1,379 @@
>> > +/**
>> > + * @file
>> > + *
>> > + * @ingroup bsp_linker
>> > + *
>> > + * @brief Memory map
>> > + */
>> > +
>> > +/*
>> > + *
>> > + * Copyright (c) 2015 University of York.
>> > + * Hesham ALMatary
>> > + *
>> > + * Redistribution and use in source and binary forms, with or without
>> > + * modification, are permitted provided that the following conditions
>> > + * are met:
>> > + * 1. Redistributions of source code must retain the above copyright
>> > + *notice, this list of conditions and the following disclaimer.
>> > + * 2. Redistributions in binary form must reproduce the above copyright
>> > + *notice, this list of conditions and the following disclaimer in
>> > the
>> > + *documentation and/or other materials provided with the
>> > distribution.
>> > + *
>> > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS''
>> > AND
>> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>> > THE
>> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> > PURPOSE
>> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE
>> > LIABLE
>> > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> > CONSEQUENTIAL
>> > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE
>> > GOODS
>> > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
>> > INTERRUPTION)
>> > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
>> > STRICT
>> > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
>> > ANY WAY
>> > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY
>> > OF
>> > + * SUCH DAMAGE.
>> > + */
>> > +
>> > +OUTPUT_FORMAT("elf32-littleriscv", "elf32-littleriscv",
>> > "elf32-littleriscv")
>> > +OUTPUT_ARCH (riscv)
>> > +
>> > +ENTRY (_start)
>> > +
>> > +MEMORY
>> > +{
>> > +ROM : ORIGIN = 0x2040, LENGTH = 16M
>> > +RAM : ORIGIN = 0x8000, LENGTH = 16K /* 64KiB external RAM,
>> > but gdb knows only about 16 KiB */
>> Is there actually 64K ram?
>>
>> I don't have the knowledge to review the rest of this for correctness.
>> It would be good to have a shared linkcmds approach though, like ARM.
>>
>> > +ITIM : ORIGIN = 0x0800, LENGTH = 8K
>> > +}
>> > +
>> > +REGION_ALIAS ("REGION_VECTOR", ROM);
>> > +REGION_ALIAS ("REGION_START", ROM);
>> > +REGION_ALIAS ("REGION_TEXT", ROM);
>> > +REGION_ALIAS ("REGION_TEXT_LOAD", ROM);
>> > +REGION_ALIAS ("REGION_RODATA", ROM);
>> > +REGION_ALIAS ("REGION_RODATA_LOAD", ROM);
>> > +REGION_ALIAS ("REGION_DATA", RAM);
>> > +REGION_ALIAS ("REGION_DATA_LOAD", ROM);
>> > +REGION_ALIAS ("REGION_FAST_DATA", RAM);
>> > +REGION_ALIAS ("REGION_FAST_DATA_LOAD", ROM);
>> > +REGION_ALIAS ("REGION_BSS", RAM);
>> > +REGION_ALIAS ("REGION_WORK", RAM);
>> > +REGION_ALIAS ("REGION_STACK", RAM);
>> > +
>> > +/* The following address is used for text output */
>> > +/* bsp_section_outbut_buffer = 0x8F80; */
>> > + bsp_section_vector_begin = 0x2040;
>> > +
>> > +
>> > +STACK_LENGTH = 0;
>> > +/*
>> > + * Global symbols that may be defined externally
>> > + */
>> > +bsp_vector_table_size = DEFINED (bsp_vector_table_size) ?
>> > bsp_vector_table_size : 64;
>> > +
>> > +bsp_section_xbarrier_align = DEFINED (bsp_section_xbarrier_align) ?
>> > bsp_section_xbarrier_align : 1;
>> > +bsp_section_robarrier_align = DEFINED (bsp_section_robarrier_align) ?
>> > bsp_section_robarrier_align : 1;
>> > +bsp_section_rwbarrier_align = DEFINED (bsp_section_rwbarrier_align) ?
>> > bsp_section_rwbarrier_align : 1;
>> > +
>> > +bsp_stack_align = DEFINED (bsp_stack_align) ? bsp_stack_align : 8;
>> > +
>> > +bsp_stack_main_size = DEFINED (bsp_stack_main_size) ?
>> > bsp_stack_main_size : 1024;
>> > +bsp_stack_main_size = ALIGN (bsp_stack_main_size, bsp_stack_align);
>> > +
>> > +_bsp_processor_count = DEFINED (_bsp_processor_count) ?
>> > _bsp_pro
Re: [PATCH 04/15] HiFive1: add start.S file with basic initialization
On Thu, Aug 17, 2017 at 1:12 AM, Denis Obrezkov wrote: > --- > c/src/lib/libbsp/riscv32/hifive1/start/start.S | 290 > + > 1 file changed, 290 insertions(+) > create mode 100644 c/src/lib/libbsp/riscv32/hifive1/start/start.S > > diff --git a/c/src/lib/libbsp/riscv32/hifive1/start/start.S > b/c/src/lib/libbsp/riscv32/hifive1/start/start.S > new file mode 100644 > index 000..9531998 > --- /dev/null > +++ b/c/src/lib/libbsp/riscv32/hifive1/start/start.S > @@ -0,0 +1,290 @@ > +/* > + * Copyright (c) 2015 University of York. > + * Hesham ALMatary > + * You should add your name/copyrights. > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + *notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + *notice, this list of conditions and the following disclaimer in the > + *documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > +#include > +#include > + > +# define LREG lw > +# define SREG sw > + > +#define CLINT_BASE 0x0200 > +#define MTIME_OFFSET 0xBFF8 > +#define MTIMECMP_OFFSET 0x4000 > +#define MTIME_REG CLINT_BASE+MTIME_OFFSET > +#define MTIMECMP CLINT_BASE+MTIMECMP_OFFSET > + > +#define REGBYTES 4 > + > +EXTERN(bsp_section_bss_begin) > +EXTERN(bsp_section_bss_end) > +EXTERN(ISR_Handler) > +EXTERN(bsp_start_vector_table_end) > +EXTERN(bsp_start_vector_table_size) > +EXTERN(bsp_vector_table_size) > +EXTERN(bsp_section_stack_begin) > + > + > +.balign 4 > +PUBLIC(RISCV_Exception_default) > +/* PUBLIC(bsp_start_vector_table_begin) */ > +PUBLIC(_start) > + > +.section .vector, "ax" > + No use of .vector here anymore, so should be deleted. Also commented code is not encouraged. > + > +.section .start, "wax" > +TYPE_FUNC(_start) > +SYM(_start): x1? > + li x2, 0 > + li x3, 0 > + li x4, 0 > + li x5, 0 > + li x6, 0 > + li x7, 0 > + li x8, 0 > + li x9, 0 > + li x10,0 > + li x11,0 > + li x12,0 > + li x13,0 > + li x14,0 > + li x15,0 > + li x16,0 > + li x17,0 > + li x18,0 > + li x19,0 > + li x20,0 > + li x21,0 > + li x22,0 > + li x23,0 > + li x24,0 > + li x25,0 > + li x26,0 > + li x27,0 > + li x28,0 > + li x29,0 > + li x30,0 > + li x31,0 > + > + /* load stack and frame pointers */ > + la sp, bsp_section_stack_begin > + > + > + One newline only. > +/* Clearing .bss */ > + la t0, bsp_section_bss_begin > + la t1, bsp_section_bss_end > + > +_loop_clear_bss: > + bge t0, t1, _end_clear_bss > + addi t0, t0, 4 > + swx0, 0(t0) > + j _loop_clear_bss > + nop No need for nop > +_end_clear_bss: > + > + > +/* Copy data */ > + la t0, _copy_start > + la t1, _copy_end > + la t3, __data_start_rom > + > +_loop_copy_data: > + bge t0, t1, _end_copy_data > + lb t4, 0(t3) > + sb t4, 0(t0) Any reason why you use bytes instead of words? > + addi t0, t0, 1 > + addi t3, t3, 1 > + j_loop_copy_data > + nop ditto > +_end_copy_data: > + > + > + > +/* > + * Examples of generating clock and software interrupts. > + * interrupts won't be generated because they are turned off > + */ > +irq_gen: > +/* sw t0, 0(t1) */ > + li t2, MTIME_REG > + li t4, 0x30 Try not to use hard-coded value, or use comments where they've to be used. > + lw t0, 0(t2) > + add t0, t0, t4 > + li t3, MTIMECMP > + sw t0, 0(t3) > + > +/* copy MSB of mtime */ > + addi t3, t3, 4 > + addi t2, t2, 4 > +/* race condition may arise here */ > + lw t0, 0(t2) > + sw t0, 0(t3) > + > + li t3, 0x0200 > + li t4, 0x1 > + /* sw t4, 0(t3) */ > + nop > + > + > +/* Enable interrupts in mie register (not enabled in mstatus yet) */ I will aruge interrupts should not be enabled at start, rather, they should be enabled when you're going to a new thread (init context, discussed in a previous post). > + la t0, RISCV_Exception_default > + csrw mtvec, t0 > + li t0, 0x088 I don't know what 0x88 is; for readability, always use macros with descriptive names to make it easier for code reviewe
Re: [PATCH 08/15] HiFive1: add irq dispatching function
On Fri, Aug 18, 2017 at 1:11 AM, Gedare Bloom wrote:
> On Wed, Aug 16, 2017 at 11:12 AM, Denis Obrezkov
> wrote:
>> ---
>> c/src/lib/libbsp/riscv32/hifive1/irq/irq.c | 90
>> ++
>> 1 file changed, 90 insertions(+)
>> create mode 100644 c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>>
>> diff --git a/c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>> b/c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>> new file mode 100644
>> index 000..6fd4e75
>> --- /dev/null
>> +++ b/c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>> @@ -0,0 +1,90 @@
>> +/**
>> + * @file
>> + *
>> + * @ingroup riscv_interrupt
>> + *
>> + * @brief Interrupt support.
>> + */
>> +
>> +/*
>> + * RISCV CPU Dependent Source
>> + *
>> + * Copyright (c) 2015 University of York.
>> + * Hesham ALMatary
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *notice, this list of conditions and the following disclaimer in the
>> + *documentation and/or other materials provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> PURPOSE
>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
>> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
>> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
>> STRICT
>> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +/* Almost all of the jobs that the following functions should
>> + * do are implemented in cpukit
>> + */
>> +
>> +void bsp_interrupt_handler_default(rtems_vector_number vector)
>> +{
>> +printk("spurious interrupt: %u\n", vector);
>> +}
>> +
>> +rtems_status_code bsp_interrupt_facility_initialize()
>> +{
>> + return 0;
>> +}
>> +
>> +rtems_status_code bsp_interrupt_vector_enable(rtems_vector_number vector)
>> +{
>> + return 0;
>> +}
>> +
>> +rtems_status_code bsp_interrupt_vector_disable(rtems_vector_number vector)
>> +{
>> + return 0;
>> +}
>> +
>> +static uint32_t cause = 0;
>> +volatile uint64_t * mtimecmp = (volatile uint64_t *)0x02004000;
>> +volatile uint64_t * mtime = (volatile uint64_t *)0x0200bff8;
>> +
Avoid hard-coded values whenever possible, same for the rest of the patches.
> What are these global variables used for?
>
>> +void handle_trap_new ()
> What is the purpose of this function?
>
>> +{
>> +asm volatile ("csrr %0, mcause": "=r" (cause));
> It would be good to have a static inline function for reading this register.
>
+1 Consider adding a utility .h file to read/write commonly used
registers (e.g. mstatus)
>> +if (cause & MCAUSE_INT) {
>> + /* an interrupt occurred */
>> + if ((cause & MCAUSE_MTIME) == MCAUSE_MTIME) {
I believe you don't need the " == MCAUSE_MTIME"
>> + /* Timer interrupt */
>> + (*mtimecmp) = (*mtime) + FE310_CLOCK_PERIOD;
>> +
>> bsp_interrupt_handler_table[1].handler(bsp_interrupt_handler_table[1].arg);
>> + } else if ((cause & MCAUSE_MEXT) == MCAUSE_MEXT) {
>> + /*External interrupt */
>> + asm volatile ("csrci mie, 0x800");
I assume you're only handling timer interrupts. It would be useful to
tell the user (printk?) that this interrupt type is not handled
proprely yet, and add a FIXME/TODO comment here.
>> + } else if ((cause & MCAUSE_MSWI) == MCAUSE_MSWI) {
>> + /* Software interrupt */
>> + volatile uint32_t * msip_reg = (volatile uint32_t *)
>> 0x0200;
>> + *msip_reg = 0;
>> + }
>> +} else {
>> + /* an exception occurred */
>> +}
>> +
>> +}
>> --
>> 2.1.4
>>
>> ___
>> devel mailing list
>> devel@rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
--
Hesham
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 13/15] HiFive1: add interrupts enable/disable support
On Thu, Aug 17, 2017 at 1:13 AM, Denis Obrezkov wrote:
> ---
> cpukit/score/cpu/riscv32/rtems/score/cpu.h | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/cpukit/score/cpu/riscv32/rtems/score/cpu.h
> b/cpukit/score/cpu/riscv32/rtems/score/cpu.h
> index 8f8a864..9b484e6 100644
> --- a/cpukit/score/cpu/riscv32/rtems/score/cpu.h
> +++ b/cpukit/score/cpu/riscv32/rtems/score/cpu.h
> @@ -455,7 +455,7 @@ Context_Control_fp _CPU_Null_fp_context;
> *
> */
>
> -#define CPU_STACK_MINIMUM_SIZE 4096
> +#define CPU_STACK_MINIMUM_SIZE 700
>
> /*
> * CPU's worst alignment requirement for data types on a byte boundary.
> This
> @@ -538,7 +538,6 @@ Context_Control_fp _CPU_Null_fp_context;
> * level is returned in _level.
> *
> */
> -
> static inline uint32_t riscv_interrupt_disable( void )
> {
>register uint32_t sstatus;
> @@ -549,13 +548,15 @@ static inline uint32_t riscv_interrupt_disable( void )
> "csrw sstatus, %[temp]; \t"
> : [temp] "=r" (temp) : [sstatus] "r" (sstatus):
> );
> +#elif FE3XX
> + __asm__ volatile ("csrci mstatus, 0x8");
> + (void)temp;
Consider implementing this proprely as we discussed before. Also,
there's no need for the #elif FE3XX; just modify the #else below.
Enabling/disabling global interrupts in RISC-V (and this file) is
ISA/RISC-V specific, not BSP specific.
> #else
>__asm__ __volatile__ ("csrr %[sstatus], mstatus; \t"
> "andi %[temp], %[sstatus], -2; \t"
> "csrw mstatus, %[temp]; \t"
> : [temp] "=r" (temp) : [sstatus] "r" (sstatus):
> );
> -
> #endif
>return sstatus;
> }
> @@ -565,15 +566,18 @@ static inline void riscv_interrupt_enable(uint32_t
> level)
> #ifdef SEL4
>__asm__ __volatile__ ("csrw sstatus, %[level];"
> :: [level] "r" (level):);
> +#elif FE3XX
> + __asm__ volatile ("csrsi mstatus, 0x8");
ditto
> #else
>__asm__ __volatile__ ("csrw mstatus, %[level];"
> :: [level] "r" (level):);
> -
> #endif
> }
>
> #define _CPU_ISR_Disable( _level ) \
> -_level = riscv_interrupt_disable()
> +do { \
> +_level = riscv_interrupt_disable();\
> +} while(0)
>
> /*
> * Enable interrupts to the previous level (returned by _CPU_ISR_Disable).
> @@ -583,7 +587,9 @@ static inline void riscv_interrupt_enable(uint32_t level)
> */
>
> #define _CPU_ISR_Enable( _level ) \
> - riscv_interrupt_enable( _level )
> +do {\
> + riscv_interrupt_enable( _level ); \
> +} while(0)
>
> /*
> * This temporarily restores the interrupt to _level before immediately
> --
> 2.1.4
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
--
Hesham
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 15/15] HiFive1: disable/enable interrupts during context switch
On Sat, Aug 19, 2017 at 10:56 PM, Denis Obrezkov wrote: > 2017-08-19 14:33 GMT+02:00 Gedare Bloom : >> >> On Fri, Aug 18, 2017 at 11:30 AM, Denis Obrezkov >> wrote: >> > 2017-08-17 23:58 GMT+02:00 Gedare Bloom : >> >> >> >> On Thu, Aug 17, 2017 at 4:17 PM, Denis Obrezkov >> >> >> >> wrote: >> >> > 2017-08-17 22:07 GMT+02:00 Gedare Bloom : >> >> >> >> >> >> On Thu, Aug 17, 2017 at 1:48 PM, Denis Obrezkov >> >> >> >> >> >> wrote: >> >> >> > 2017-08-17 17:25 GMT+02:00 Gedare Bloom : >> >> >> >> >> >> >> >> On Wed, Aug 16, 2017 at 11:13 AM, Denis Obrezkov >> >> >> >> wrote: >> >> >> >> > --- >> >> >> >> > cpukit/score/cpu/riscv32/riscv-context-switch.S | 12 >> >> >> >> > ++-- >> >> >> >> > 1 file changed, 10 insertions(+), 2 deletions(-) >> >> >> >> > >> >> >> >> > diff --git a/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> >> > b/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> >> > index a199596..bcdfe0e 100644 >> >> >> >> > --- a/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> >> > +++ b/cpukit/score/cpu/riscv32/riscv-context-switch.S >> >> >> >> > @@ -46,6 +46,7 @@ PUBLIC(restore) >> >> >> >> > >> >> >> >> > SYM(_CPU_Context_switch): >> >> >> >> >/* Disable interrupts and store all registers */ >> >> >> >> > + csrci mstatus, 0x8 >> >> >> >> Why is this necessary? >> >> >> >> >> >> >> >> >SREG x1, 4(a0) >> >> >> >> >SREG x2, 8(a0) >> >> >> >> >SREG x3, 12(a0) >> >> >> >> > @@ -78,8 +79,9 @@ SYM(_CPU_Context_switch): >> >> >> >> >SREG x30, 120(a0) >> >> >> >> >SREG x31, 124(a0) >> >> >> >> > >> >> >> >> > -SYM(restore): >> >> >> >> > >> >> >> >> > +SYM(restore): >> >> >> >> > + >> >> >> >> >LREG x1, 4(a1) >> >> >> >> >LREG x2, 8(a1) >> >> >> >> >LREG x3, 12(a1) >> >> >> >> > @@ -111,9 +113,15 @@ SYM(restore): >> >> >> >> >LREG x29, 116(a1) >> >> >> >> >LREG x30, 120(a1) >> >> >> >> >LREG x31, 124(a1) >> >> >> >> > - ret >> >> >> >> > + >> >> >> >> > + >> >> >> >> > + csrsi mstatus, 0x8 >> >> >> >> > + nop >> >> >> >> > + nop >> >> >> >> Why the nops? >> >> >> >> >> >> >> >> > + ret >> >> >> >> > >> >> >> >> > SYM(_CPU_Context_restore): >> >> >> >> > + csrci mstatus, 0x8 >> >> >> >> >mv a1, a0 >> >> >> >> >j restore >> >> >> >> >nop >> >> >> >> > -- >> >> >> >> > 2.1.4 >> >> >> >> > >> >> >> >> > ___ >> >> >> >> > devel mailing list >> >> >> >> > devel@rtems.org >> >> >> >> > http://lists.rtems.org/mailman/listinfo/devel >> >> >> > >> >> >> > So, don't we turn off interrupts during the context switch? >> >> >> >> >> >> Nope, and turning them back on unconditionally is wrong too. >> >> >> >> >> >> > Yes, nops are unnecessary. >> >> >> > >> >> >> > >> >> >> > -- >> >> >> > Regards, Denis Obrezkov >> >> > >> >> > Ok, I was confused by this obsolete comment: >> >> > /* Disable interrupts and store all registers */ >> >> > Will remove all that enabling/disabling. >> >> > >> >> > Is the same true for start.S file with interrupted task's stack >> >> > saving? >> >> > >> >> I'm not sure exactly what you're referring to, but usually start.S >> >> will, among other things, turn off and ack pending interrupts to >> >> quiesce the hardware before handing off the rest of system init to >> >> boot_card. Eventually RTEMS will enable interrupts again using the >> >> cpukit's interrupt_enable layer. >> >> >> >> > >> >> > >> >> > -- >> >> > Regards, Denis Obrezkov >> > >> > I mean when an interrupt occurs, the execution flow jumps to the trap >> > address. >> > Then, in my case all process'es registers are saved in a common stack >> > (I know that dedicated interrupt stack should be implemented, but I >> > haven't >> > time to do that) >> > and then we jump to IRQ dispatching routine. After interrupt handling we >> > restore saved >> > registers. So, my question is - should we disable/enable interrupts >> > during >> > interrupt handling? >> > Disabling: >> > >> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L201 >> > Jump to dispatching routine: >> > >> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L241 >> > Enabling interrupts back: >> > >> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L285 >> > >> This interrupt handling is incomplete/broken I think. Keeping the >> interrupts turned off for the duration of RTEMS + User ISR handling is >> not "real-time". What other CPU did you look at to understand how to >> deal with interrupts? > > I'v kept Hasham's RISC-V generic interrupt code. I thought that we don't > want to have nested interrupts. I also tried to turn off interrupts only > during > register saving/restoring. As I mentioned before, there is no interrupt (handling) at all in my port/BSP (simulator-based); if there's any, they are just stubs. I'd suggest you look at other BSPs an
