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

[Philippe Mathieu-Daudé]

On 9/21/20 9:39 PM, Luc Michel wrote:
> 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".

Yes, you are correct. The reduced datasheet is not easy to understand.

>> +        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);

This matches the datasheet description, thanks!

>            }
> 
> 
> The rest is OK to me.
>