RISC-V interrupts in HiFive1

Wed Aug 16 12:58:22 UTC 2017

2017-08-16 10:27 GMT+02:00 Denis Obrezkov <denisobrezkov at gmail.com>:

> 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/cpuk
>> it/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/cpuk
>> it/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/cpuk
>> it/score/cpu/riscv32/rtems/score/cpu.h#L554.
>> >>> [2]
>> >>> https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpuk
>> it/score/cpu/riscv32/rtems/score/cpu.h#L574
>> >>> > Context initialize:
>> >>> >
>> >>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpuk
>> it/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
>

I've tried your solution Hesham, but I found out that level value was lost
after several ticks.
So, I turned back to my initial solution.

-- 
Regards, Denis Obrezkov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20170816/9f5013b5/attachment.html>