RISC-V interrupts in HiFive1

Wed Aug 16 08:27:24 UTC 2017

2017-08-16 3:09 GMT+02:00 Hesham Almatary <heshamelmatary at gmail.com>:

> On Wed, Aug 16, 2017 at 10:57 AM, Joel Sherrill <joel at rtems.org> wrote:
> >
> >
> > On Tue, Aug 15, 2017 at 7:50 PM, Denis Obrezkov <denisobrezkov at gmail.com
> >
> > wrote:
> >>
> >> 2017-08-16 2:06 GMT+02:00 Hesham Almatary <heshamelmatary at gmail.com>:
> >>>
> >>> /
> >>>
> >>> On Wed, Aug 16, 2017 at 3:03 AM, Denis Obrezkov <
> denisobrezkov at gmail.com>
> >>> wrote:
> >>> > 2017-08-15 14:57 GMT+02:00 Joel Sherrill <joel at rtems.org>:
> >>> >>
> >>> >>
> >>> >>
> >>> >> On Aug 15, 2017 4:32 AM, "Denis Obrezkov" <denisobrezkov at gmail.com>
> >>> >> wrote:
> >>> >>
> >>> >> 2017-08-15 5:44 GMT+02:00 Hesham Almatary <heshamelmatary at gmail.com
> >:
> >>> >>>
> >>> >>> Hi Denis,
> >>> >>>
> >>> >>> You just need to modify riscv_interrupt_disable(). Read the
> priv-spec
> >>> >>> manual for your RISC-V version, and determine which bit should be
> >>> >>> cleared (it's called MIE in priv-1.10, but you mentioned you work
> >>> >>> with
> >>> >>> priv-1.9).
> >>> >>
> >>> >>
> >>> >> Is the bit set correctly for the idle thread?
> >>> >>
> >>> >> I assume this is with a real clock driver so is it maintained
> properly
> >>> >> across an ISR?
> >>> >>
> >>> >> It is either screwed up by the thread context initialization,
> context
> >>> >> switch, or ISR code. ISR code has two exit paths. It could be the
> >>> >> preemption
> >>> >> path. That would occur about 5 seconds into the execution of ticker
> >>> >> and TA1
> >>> >> will preemption idle.
> >>> >>>
> >>> >>>
> >>> >>
> >>> > I understand this but still can find a mistake.
> >>> > This is what I get:
> >>> >
> >>> >
> >>> > *** LOW MEMORY CLOCK TICK TEST ***
> >>> >
> >>> > TA1 - rtems_clock_get_tod - 09:00:00 12/31/1988
> >>> >
> >>> > TA2 - rtems_clock_get_tod - 09:00:00 12/31/1988
> >>> >
> >>> > TA3 - rtems_clock_get_tod - 09:00:00 12/31/1988
> >>> >
> >>> > TA1 - rtems_clock_get_tod - 09:00:04 12/31/1988
> >>> >
> >>> > TA2 - rtems_clock_get_tod - 09:00:09 12/31/1988
> >>> >
> >>> > TA1 - rtems_clock_get_tod - 09:00:09 12/31/1988
> >>> >
> >>> >
> >>> > After that I can examine: p Clock_driver_ticks
> >>> >
> >>> > $8 = 1001
> >>> >
> >>> >
> >>> > mstatus value is 0x0001800 (interrupts disabled)
> >>> >
> >>> > Instruction for enabling global interrupts is:
> >>> > csrsi mstatus, 0x8
> >>> > for disabling:
> >>> > csrci mstatus, 0x8
> >>> > it atomically changes the fourth bit of mstatus (ie bit).
> >>> >
> >>> > I think that something wrong happens in context switches between TA1,
> >>> > TA2
> >>> > and Idle.
> >>> >
> >>> > This is my context switch file:
> >>> >
> >>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/
> cpukit/score/cpu/riscv32/riscv-context-switch.S
> >>> You added new fields (e.g. mepc, mstatus) to the _CPU_Context_switch
> >>> function without reserving an area for them in the corresponding
> >>> riscv32 Context_Control structure.
> >>
> >> Yes, I found this mistake already, and decided not to save mstatus,
> >> because there is no point
> >> in saving it at the current stage of development.
> >>>
> >>>
> >>> > Interrupt disabling:
> >>> >
> >>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/
> cpukit/score/cpu/riscv32/rtems/score/cpu.h#L542
> >>> That's not right [1]. You should return the previous mstatus content
> >>> (with interrupts enabled i.e. before disabling interrupts). Also, if I
> >>> understand correctly, this [2] is also wrong. You should simply set
> >>> mstatus with the level variable (as my code does). That is supposed to
> >>> get this level value by a previous call to riscv_interrupt_disable(),
> >>> assuming it's implemented correctly. This way you're enabling
> >>> interrupts back.
> >>
> >> As I said above, there is no need to save mstatus content since we don't
> >> change it in other parts of the program. mstatus is used only for
> >> interrupt
> >> enabling/disabling, thus I simple enable and disable interrupts using
> >> appropriate instructions.
> >>>
> >>>
> >>> [1]
> >>> https://github.com/embeddedden/rtems-riscv/blob/hifive1/
> cpukit/score/cpu/riscv32/rtems/score/cpu.h#L554.
> >>> [2]
> >>> https://github.com/embeddedden/rtems-riscv/blob/hifive1/
> cpukit/score/cpu/riscv32/rtems/score/cpu.h#L574
> >>> > Context initialize:
> >>> >
> >>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/
> cpukit/score/cpu/riscv32/riscv-context-initialize.c
> >>> Now that you use interrupts, it makes sense to add mstatus to
> >>> Context_Control structure for riscv32, and initialize mstatus context
> >>> in this function to enable interrupts (after addressing the above
> >>> comment).
> >>
> >> I agree that it should be done, but I think I will have no time for this
> >> before evaluation.
> >
> >
> > As Hesham pointed out, interrupt disable needs to return a status value.
> > It is usually just the contents of the status register with the interrupt
> > enable/disable bit(s).
> >
> +1 mstatus encodes some other read only bits that might affect how
> your program behaves of they changed. For example, writing 0s to some
> RO  (or hardwired ones) fields might trigger an underfined behaviour,
> depending on the micoarchitecure implementation.
>
> > The context should include the status register. It may no longer be
> > technically necessary but most every other port does it so I wouldn't
> > bet on not needing this. For sure, as long as setting the interrupt
> > disable level in rtems_task_mode works, you have to support this.
> >
> +1
>
> > Not having enough memory in the context area would cause weirdness.
> >
> > I would expect interrupts to be manually disabled after you return from
> the
> > C ISR handler and then **restored** at the end of the ISR. I know this
> > is redundant and I am talking in the abstract but there are two paths
> > out of the ISR and it is easy to mess one up.
> >
> > It is also easy to blow a stack in an ISR handler.
> >
> > --joel
> >
> >>>
> >>>
> >>
> >> I also think that after GSoC we should refresh our toolchain.
> >> --
> >>
> >> Regards, Denis Obrezkov
> >
> >
>
>
>
> --
> Hesham
>
Ok, though I am more concerned about setting
CONFIGURE_MICROSECONDS_PER_TICK.
It seems it can't be set, even if I pass it through -D option.
-- 
Regards, Denis Obrezkov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20170816/5e676722/attachment-0002.html>