Re: [PATCH v4 3/4] hw/timer/bcm2835: Support the timer COMPARE registers

[Luc Michel]

On 22:37 Sat 10 Oct     , Philippe Mathieu-Daudé wrote:
> 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>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> v4:
> - Fix arithmetic to correctly get value in future (Richard)
> - Use 32-bit argument value (Richard)
> 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         | 48 ++++++++++++++++++++-----------
>  hw/timer/trace-events             |  6 ++--
>  3 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/timer/bcm2835_systmr.h 
> b/include/hw/timer/bcm2835_systmr.h
> index f15788a78d8..bd3097d746b 100644
> --- a/include/hw/timer/bcm2835_systmr.h
> +++ b/include/hw/timer/bcm2835_systmr.h
> @@ -11,6 +11,7 @@
>  
>  #include "hw/sysbus.h"
>  #include "hw/irq.h"
> +#include "qemu/timer.h"
>  #include "qom/object.h"
>  
>  #define TYPE_BCM2835_SYSTIMER "bcm2835-sys-timer"
> @@ -18,18 +19,24 @@ OBJECT_DECLARE_SIMPLE_TYPE(BCM2835SystemTimerState, 
> BCM2835_SYSTIMER)
>  
>  #define BCM2835_SYSTIMER_COUNT 4
>  
> +typedef struct {
> +    unsigned id;
> +    QEMUTimer timer;
> +    qemu_irq irq;
> +    BCM2835SystemTimerState *state;
> +} BCM2835SystemTimerCompare;
> +
>  struct BCM2835SystemTimerState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>  
>      /*< public >*/
>      MemoryRegion iomem;
> -    qemu_irq irq;
> -
>      struct {
>          uint32_t ctrl_status;
>          uint32_t compare[BCM2835_SYSTIMER_COUNT];
>      } reg;
> +    BCM2835SystemTimerCompare tmr[BCM2835_SYSTIMER_COUNT];
>  };
>  
>  #endif
> diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c
> index b234e83824f..67669a57ff3 100644
> --- a/hw/timer/bcm2835_systmr.c
> +++ b/hw/timer/bcm2835_systmr.c
> @@ -28,20 +28,13 @@ REG32(COMPARE1,     0x10)
>  REG32(COMPARE2,     0x14)
>  REG32(COMPARE3,     0x18)
>  
> -static void bcm2835_systmr_update_irq(BCM2835SystemTimerState *s)
> +static void bcm2835_systmr_timer_expire(void *opaque)
>  {
> -    bool enable = !!s->reg.ctrl_status;
> +    BCM2835SystemTimerCompare *tmr = opaque;
>  
> -    trace_bcm2835_systmr_irq(enable);
> -    qemu_set_irq(s->irq, enable);
> -}
> -
> -static void bcm2835_systmr_update_compare(BCM2835SystemTimerState *s,
> -                                          unsigned timer_index)
> -{
> -    /* TODO fow now, since neither Linux nor U-boot use these timers. */
> -    qemu_log_mask(LOG_UNIMP, "COMPARE register %u not implemented\n",
> -                  timer_index);
> +    trace_bcm2835_systmr_timer_expired(tmr->id);
> +    tmr->state->reg.ctrl_status |= 1 << tmr->id;
> +    qemu_set_irq(tmr->irq, 1);
>  }
>  
>  static uint64_t bcm2835_systmr_read(void *opaque, hwaddr offset,
> @@ -75,19 +68,33 @@ static uint64_t bcm2835_systmr_read(void *opaque, hwaddr 
> offset,
>  }
>  
>  static void bcm2835_systmr_write(void *opaque, hwaddr offset,
> -                                 uint64_t value, unsigned size)
> +                                 uint64_t value64, unsigned size)
>  {
>      BCM2835SystemTimerState *s = BCM2835_SYSTIMER(opaque);
> +    int index;
> +    uint32_t value = value64;
> +    uint32_t triggers_delay_us;
> +    uint64_t now;
>  
>      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);
> +            }
> +        }
>          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;
> +        trace_bcm2835_systmr_run(index, triggers_delay_us);
> +        timer_mod(&s->tmr[index].timer, now + triggers_delay_us);
>          break;
>      case A_COUNTER_LOW:
>      case A_COUNTER_HIGH:
> @@ -125,7 +132,14 @@ static void bcm2835_systmr_realize(DeviceState *dev, 
> Error **errp)
>      memory_region_init_io(&s->iomem, OBJECT(dev), &bcm2835_systmr_ops,
>                            s, "bcm2835-sys-timer", 0x20);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> -    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(s->tmr); i++) {
> +        s->tmr[i].id = i;
> +        s->tmr[i].state = s;
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->tmr[i].irq);
> +        timer_init_us(&s->tmr[i].timer, QEMU_CLOCK_VIRTUAL,
> +                      bcm2835_systmr_timer_expire, &s->tmr[i]);
> +    }
>  }
>  
>  static const VMStateDescription bcm2835_systmr_vmstate = {
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index b996d992000..7a4326d9566 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -77,9 +77,11 @@ nrf51_timer_write(uint8_t timer_id, uint64_t addr, 
> uint32_t value, unsigned size
>  nrf51_timer_set_count(uint8_t timer_id, uint8_t counter_id, uint32_t value) 
> "timer %u counter %u count 0x%" PRIx32
>  
>  # bcm2835_systmr.c
> -bcm2835_systmr_irq(bool enable) "timer irq state %u"
> +bcm2835_systmr_timer_expired(unsigned id) "timer #%u expired"
> +bcm2835_systmr_irq_ack(unsigned id) "timer #%u acked"
>  bcm2835_systmr_read(uint64_t offset, uint64_t data) "timer read: offset 0x%" 
> PRIx64 " data 0x%" PRIx64
> -bcm2835_systmr_write(uint64_t offset, uint64_t data) "timer write: offset 
> 0x%" PRIx64 " data 0x%" PRIx64
> +bcm2835_systmr_write(uint64_t offset, uint32_t data) "timer write: offset 
> 0x%" PRIx64 " data 0x%" PRIx32
> +bcm2835_systmr_run(unsigned id, uint64_t delay_us) "timer #%u expiring in 
> %"PRIu64" us"
>  
>  # avr_timer16.c
>  avr_timer16_read(uint8_t addr, uint8_t value) "timer16 read addr:%u value:%u"
> -- 
> 2.26.2
> 

--