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é <f4...@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.
>
