2017-08-16 10:27 GMT+02:00 Denis Obrezkov <denisobrez...@gmail.com>: > 2017-08-16 3:09 GMT+02:00 Hesham Almatary <heshamelmat...@gmail.com>: > >> On Wed, Aug 16, 2017 at 10:57 AM, Joel Sherrill <j...@rtems.org> wrote: >> > >> > >> > 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/cpuk >> it/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/cpuk >> it/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/cpuk >> it/score/cpu/riscv32/rtems/score/cpu.h#L554. >> >>> [2] >> >>> https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpuk >> it/score/cpu/riscv32/rtems/score/cpu.h#L574 >> >>> > Context initialize: >> >>> > >> >>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpuk >> it/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). >> > >> +1 mstatus encodes some other read only bits that might affect how >> your program behaves of they changed. For example, writing 0s to some >> RO (or hardwired ones) fields might trigger an underfined behaviour, >> depending on the micoarchitecure implementation. >> >> > 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. >> > >> +1 >> >> > 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 >> > >> > >> >> >> >> -- >> Hesham >> > Ok, though I am more concerned about setting CONFIGURE_MICROSECONDS_PER_ > TICK. > It seems it can't be set, even if I pass it through -D option. > -- > Regards, Denis Obrezkov >
I've tried your solution Hesham, but I found out that level value was lost after several ticks. So, I turned back to my initial solution. -- Regards, Denis Obrezkov
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
