RISC-V interrupts in HiFive1

[Hesham Almatary]

/

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.

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

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

> Interrupt context save:
> https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L200
>
> Could have a look?
>
>
>
>
> --
> Regards, Denis Obrezkov



-- 
Hesham