RISC-V timer incorrectly written

[atthecodeface]

I don't know if this is handled by Andy Ross' updates to the timer stuff, so rather than it getting dropped:
riscv_machine_timer_irq_handler in drivers/timer/riscv_machine_timer.c
is presumably the interrupt handler for the timer. Its process is to do its stuff then rearm the timer.

The RISC-V privileged spec does not guarantee spurious timer interrupts on timer rearming; there is a chance that a timer interrupt will occur after the timer is armed again, immediately as the loop delay from write-time-comparator to interrupt-disable is arbitrarily long (there was a mail thread on this on RISC-V ISA dev a couple of months back). Hence the code as is can continuously loop, and certainly have system clock ticks occur far more frequently than they should.

Also, the mechanism for writing the timer comparator does not match the spec: it writes val_low then val_high. The 'correct' approach is to set val_high to 0xffffffff, then write val_low, then write val_high; this is in figure 3.15 of the RISC-V privileged ISA specification (although I think there isa bug in that code too as the first write ought to be to timecmp+4, not timecmp, and this has also been noted on the mailing list)

Finally the spec does not actually indicate that a timer value should be low-then-hig, ii.e. matching the structure:

typedef struct {
u32_t val_low;
u32_t val_high;
} riscv_machine_timer_t;

The spec actually has no requirement of little or big endianness here. I regard it as a bug in the spec, not the code, though.

[andyross]

Cool, thanks. I was literally just this morning starting the rework on this driver, and indeed had copied that "low then high" mechanism (and even added a comment explaining it) verbatim. That kind of latching is a pretty common trick for multi-word counters, so it didn't look weird to me.

What is the correct reference for this device? The only docs I could find publicly were the SiFive manual for the SiFive E310. And I suppose I could check the qemu source...

As far as the spurious interrupt problem: that should be handled. The ISR (you can see similar logic in the HPET and -- though it's hairy there -- SysTick drivers) checks the current time vs. last and announces ticks for true expired time and not just a count of interrupts.

[nategraff-sifive]

@andyross The Freedom E310-G000 Manual and the RISC-V Privileged Architecture Specification are the references I use for the machine timer. You can also check out the Freedom U540-C000 Manual which has its own implementation of the RISC-V machine timer.

I should also mention I've been working on getting your Xtensa SMP support ported to RISC-V, so as long as we're talking about the machine timer driver another change the driver needs is to be aware of which hart's timer it's rearming.

That has its own share of specification vagaries, though, because the hart IDs need not be contiguously assigned, but it really sucks to support if they're not. So far I've been working on the assumption that they are contiguous and that the hart ID can function as an index to an array of mtimecmp values, which has been enough to get things running on QEMU.

[andyross]

Driver that should address this up at #10556