[v3,3/4] hw/timer/bcm2835: Support the timer COMPARE registers

This peripheral has 1 free-running timer and 4 compare registers.

Only the free-running timer is implemented. Add support the
COMPARE registers (each register is wired to an IRQ).

Reference: "BCM2835 ARM Peripherals" datasheet [*]
            chapter 12 "System Timer":

  The System Timer peripheral provides four 32-bit timer channels
  and a single 64-bit free running counter. Each channel has an
  output compare register, which is compared against the 32 least
  significant bits of the free running counter values. When the
  two values match, the system timer peripheral generates a signal
  to indicate a match for the appropriate channel. The match signal
  is then fed into the interrupt controller.

This peripheral is used since Linux 3.7, commit ee4af5696720
("ARM: bcm2835: add system timer").

[*] https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v3:
- Only compare 32 least significant bits of the free running
  counter values (Luc)
---
 include/hw/timer/bcm2835_systmr.h | 11 ++++++--
 hw/timer/bcm2835_systmr.c         | 45 ++++++++++++++++++++-----------
 hw/timer/trace-events             |  4 ++-
 3 files changed, 41 insertions(+), 19 deletions(-)

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~

On 10/3/20 7:17 PM, Richard Henderson wrote:
> 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

Thanks for the example of wrong behavior...

> 
> 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.

... and the correct one :)

I'll correct as suggested.

Thanks!

Phil.