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