RISC-V interrupts in HiFive1

Wed Aug 16 00:57:03 UTC 2017

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).

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20170815/a2610d79/attachment-0002.html>