Re: [PATCH v2 4/5] hw/timer/bcm2835: Support the timer COMPARE registers

[Luc Michel <luc.michel@greensocs.com>]

Hi Phil,

On 9/21/20 5:52 AM, 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>
> ---
>   include/hw/timer/bcm2835_systmr.h | 11 +++++++--
>   hw/timer/bcm2835_systmr.c         | 41 +++++++++++++++++++------------
>   hw/timer/trace-events             |  4 ++-
>   3 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/timer/bcm2835_systmr.h b/include/hw/timer/bcm2835_systmr.h
> index e0db9e9e12b..17fdd9d67b2 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"
> @@ -20,18 +21,24 @@ DECLARE_INSTANCE_CHECKER(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..43e644f5e45 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,
> @@ -78,16 +71,25 @@ static void bcm2835_systmr_write(void *opaque, hwaddr offset,
>                                    uint64_t value, unsigned size)
>   {
>       BCM2835SystemTimerState *s = BCM2835_SYSTIMER(opaque);
> +    int index;
>   
>       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;
> +        timer_mod(&s->tmr[index].timer, value);
I think "value" is wrong here. timer_mod takes an absolute time value. 
Here "value" is a 32 bits truncated view of "current_time + some_time".
> +        trace_bcm2835_systmr_run(index,
> +                                 value - qemu_clock_get_us(QEMU_CLOCK_VIRTUAL));
Here also.

I think you can do something like (untested):
            {
                uint64_t now, triggers_at;
                uint32_t clo, triggers_in;

                index = (offset - A_COMPARE0) >> 2;
                s->reg.compare[index] = value;

                /* get the current clock and its truncated 32 bits view */
                now = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL);
                clo = now;

                /* will overflow in case clo > value. We still get the 
correct value on 32 bits (which is "UINT32_MAX - (clo - value)") */
                triggers_in = value - clo;

                /* timer_mod takes an absolute time value, go back to 64 
bits values */
                triggers_at = now + triggers_in;
                timer_mod(&s->tmr[index].timer, triggers_at);

                trace_bcm2835_systmr_run(index, triggers_in);
            }


The rest is OK to me.

-- 
Luc

>           break;
>       case A_COUNTER_LOW:
>       case A_COUNTER_HIGH:
> @@ -125,7 +127,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..f4ca31d4951 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_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"
>