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/ > 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). > > > +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
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
