[PATCH 13/15] HiFive1: add interrupts enable/disable support

[Denis Obrezkov]

2017-08-21 19:39 GMT+02:00 Gedare Bloom <gedare at rtems.org>:

> On Mon, Aug 21, 2017 at 11:33 AM, Denis Obrezkov
> <denisobrezkov at gmail.com> wrote:
> > 2017-08-20 5:18 GMT+02:00 Hesham Almatary <heshamelmatary at gmail.com>:
> >>
> >> On Thu, Aug 17, 2017 at 1:13 AM, Denis Obrezkov <
> denisobrezkov at gmail.com>
> >> 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 at rtems.org
> >> > http://lists.rtems.org/mailman/listinfo/devel
> >>
> >>
> >>
> >> --
> >> Hesham
> >
> >
> > As I answered you earlier I tried your solution and it just didn't work.
> > I can try it again on the next iteration of the reviewing process, but
> as I
> > said
> > the _level value just isn't preserved between interrupts
> enabling/disabling.
> >
> Lack of variable preservation indicates a bug somewhere, probably
> either in assembly code or (interrupt) context switching.
>
I agree, but I still wasn't able to identify it, I'll submit Patch v2
today,
and start search for the bug.

-- 
Regards, Denis Obrezkov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20170822/4af074dd/attachment-0002.html>