On Tue, Aug 15, 2017 at 7:50 PM, Denis Obrezkov <denisobrez...@gmail.com> wrote:
> 2017-08-16 2:06 GMT+02:00 Hesham Almatary <heshamelmat...@gmail.com>: > >> / >> >> On Wed, Aug 16, 2017 at 3:03 AM, Denis Obrezkov <denisobrez...@gmail.com> >> wrote: >> > 2017-08-15 14:57 GMT+02:00 Joel Sherrill <j...@rtems.org>: >> >> >> >> >> >> >> >> On Aug 15, 2017 4:32 AM, "Denis Obrezkov" <denisobrez...@gmail.com> >> wrote: >> >> >> >> 2017-08-15 5:44 GMT+02:00 Hesham Almatary <heshamelmat...@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 >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
