On 10/2/20 11:42 AM, Philippe Mathieu-Daudé wrote:
> @@ -78,16 +71,29 @@ static void bcm2835_systmr_write(void *opaque, hwaddr
> offset,
> uint64_t value, unsigned size)
> {
> BCM2835SystemTimerState *s = BCM2835_SYSTIMER(opaque);
> + int index;
> + uint64_t now;
> + uint64_t triggers_delay_us;
>
> trace_bcm2835_systmr_write(offset, value);
> switch (offset) {
> case A_CTRL_STATUS:
> s->reg.ctrl_status &= ~value; /* Ack */
> - bcm2835_systmr_update_irq(s);
> + for (index = 0; index < ARRAY_SIZE(s->tmr); index++) {
> + if (extract32(value, index, 1)) {
> + trace_bcm2835_systmr_irq_ack(index);
> + qemu_set_irq(s->tmr[index].irq, 0);
> + }
I think it might be instructive to have the parameter be uint64_t value64, and
the immediately do
uint32_t value = value64;
That matches up better with extract32, the trace arguments...
> + }
> break;
> case A_COMPARE0 ... A_COMPARE3:
> - s->reg.compare[(offset - A_COMPARE0) >> 2] = value;
> - bcm2835_systmr_update_compare(s, (offset - A_COMPARE0) >> 2);
> + index = (offset - A_COMPARE0) >> 2;
> + s->reg.compare[index] = value;
> + now = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL);
> + /* Compare lower 32-bits of the free-running counter. */
> + triggers_delay_us = value - (now & UINT32_MAX);
> + trace_bcm2835_systmr_run(index, triggers_delay_us);
> + timer_mod(&s->tmr[index].timer, now + triggers_delay_us);
... and here.
Also, the arithmetic looks off.
Consider when you want a long timeout, and pass in a value slightly below now.
So, e.g.
now = 0xabcdffffffff;
value = 0x0000fffffffe;
since triggers_delay_us is uint64_t, that comparison becomes
triggers_delay_us = 0x0000fffffffe - 0xffffffff;
= 0xffffffffffffffff;
Then you add back in now, and do *not* get a value in the future:
now + triggers_delay_us
= 0xabcdffffffff + 0xffffffffffffffff
= 0xabcdfffffffe
What I think you want is
uint32_t triggers_delay_us = value - now
= 0xffffffff;
which then zero-extends when you add back to now to get
now + triggers_delay_us
= 0xabcdffffffff + 0xffffffff
= 0xabcefffffffe
which is indeed in the future.
r~
