<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2017-08-16 3:09 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"><div class="m_-8411380679682076787HOEnZb"><div class="m_-8411380679682076787h5">On Wed, Aug 16, 2017 at 10:57 AM, Joel Sherrill <<a href="mailto:joel@rtems.org" target="_blank">joel@rtems.org</a>> wrote:<br>
><br>
><br>
> On Tue, Aug 15, 2017 at 7:50 PM, Denis Obrezkov <<a href="mailto:denisobrezkov@gmail.com" target="_blank">denisobrezkov@gmail.com</a>><br>
> wrote:<br>
>><br>
>> 2017-08-16 2:06 GMT+02:00 Hesham Almatary <<a href="mailto:heshamelmatary@gmail.com" target="_blank">heshamelmatary@gmail.com</a>>:<br>
>>><br>
>>> /<br>
>>><br>
>>> On Wed, Aug 16, 2017 at 3:03 AM, Denis Obrezkov <<a href="mailto:denisobrezkov@gmail.com" target="_blank">denisobrezkov@gmail.com</a>><br>
>>> 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>><br>
>>> >> 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<br>
>>> >>> 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<br>
>>> >> preemption<br>
>>> >> path. That would occur about 5 seconds into the execution of ticker<br>
>>> >> 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,<br>
>>> > TA2<br>
>>> > and Idle.<br>
>>> ><br>
>>> > This is my context switch file:<br>
>>> ><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>
>>> 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>
>><br>
>> Yes, I found this mistake already, and decided not to save mstatus,<br>
>> because there is no point<br>
>> in saving it at the current stage of development.<br>
>>><br>
>>><br>
>>> > Interrupt disabling:<br>
>>> ><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>
>><br>
>> As I said above, there is no need to save mstatus content since we don't<br>
>> change it in other parts of the program. mstatus is used only for<br>
>> interrupt<br>
>> enabling/disabling, thus I simple enable and disable interrupts using<br>
>> appropriate instructions.<br>
>>><br>
>>><br>
>>> [1]<br>
>>> <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]<br>
>>> <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>
>>> ><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>
>><br>
>> I agree that it should be done, but I think I will have no time for this<br>
>> before evaluation.<br>
><br>
><br>
> As Hesham pointed out, interrupt disable needs to return a status value.<br>
> It is usually just the contents of the status register with the interrupt<br>
> enable/disable bit(s).<br>
><br>
</div></div>+1 mstatus encodes some other read only bits that might affect how<br>
your program behaves of they changed. For example, writing 0s to some<br>
RO (or hardwired ones) fields might trigger an underfined behaviour,<br>
depending on the micoarchitecure implementation.<br>
<span><br>
> The context should include the status register. It may no longer be<br>
> technically necessary but most every other port does it so I wouldn't<br>
> bet on not needing this. For sure, as long as setting the interrupt<br>
> disable level in rtems_task_mode works, you have to support this.<br>
><br>
</span>+1<br>
<div class="m_-8411380679682076787HOEnZb"><div class="m_-8411380679682076787h5"><br>
> Not having enough memory in the context area would cause weirdness.<br>
><br>
> I would expect interrupts to be manually disabled after you return from the<br>
> C ISR handler and then **restored** at the end of the ISR. I know this<br>
> is redundant and I am talking in the abstract but there are two paths<br>
> out of the ISR and it is easy to mess one up.<br>
><br>
> It is also easy to blow a stack in an ISR handler.<br>
><br>
> --joel<br>
><br>
>>><br>
>>><br>
>><br>
>> I also think that after GSoC we should refresh our toolchain.<br>
>> --<br>
>><br>
>> Regards, Denis Obrezkov<br>
><br>
><br>
<br>
<br>
<br>
</div></div><span class="m_-8411380679682076787HOEnZb"><font color="#888888">--<br>
Hesham<br></font></span></blockquote><div>Ok, though I am more concerned about setting CONFIGURE_MICROSECONDS_PER_TICK.</div><div>It seems it can't be set, even if I pass it through -D option.</div></div></div><div class="gmail_extra">-- <br><div class="m_-8411380679682076787gmail_signature" data-smartmail="gmail_signature">Regards, Denis Obrezkov</div>
</div></div>