Re: [PATCH v3 1/3] hw/timer: add DesignWare APB timer model

Hi raoyi,

On 11/8/26 08:46, raoyi wrote:
> Add generic DesignWare APB timer device model.
> The timer uses the Clock framework for frequency input
> and supports a configurable number of timer channels
> via the num-timers property (max 8, default 6).

Scratch "uses the Clock framework for frequency input and",
this is an irrelevant implementation detail.

> 
> Signed-off-by: raoyi <[email protected]>
> ---
>   hw/timer/Kconfig                |   4 +
>   hw/timer/dw-apb-timer.c         | 341 ++++++++++++++++++++++++++++++++
>   hw/timer/meson.build            |   1 +
>   hw/timer/trace-events           |   8 +
>   include/hw/timer/dw-apb-timer.h |  65 ++++++
>   5 files changed, 419 insertions(+)
>   create mode 100644 hw/timer/dw-apb-timer.c
>   create mode 100644 include/hw/timer/dw-apb-timer.h

Please cover this device in MAINTAINERS.

> 
> diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig
> index b3d823ce2c..b533cf1ba9 100644
> --- a/hw/timer/Kconfig
> +++ b/hw/timer/Kconfig
> @@ -65,3 +65,7 @@ config STELLARIS_GPTM
>   
>   config AVR_TIMER16
>       bool
> +
> +config DW_APB_TIMER
> +    bool
> +    select PTIMER
> diff --git a/hw/timer/dw-apb-timer.c b/hw/timer/dw-apb-timer.c
> new file mode 100644
> index 0000000000..385eff3ba4
> --- /dev/null
> +++ b/hw/timer/dw-apb-timer.c
> @@ -0,0 +1,341 @@
> +/*
> + * Synopsys DesignWare APB timer
> + *
> + * Copyright (c) 2026 raoyi <[email protected]>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bitops.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "hw/core/ptimer.h"
> +#include "hw/core/qdev-clock.h"
> +#include "hw/core/sysbus.h"
> +#include "hw/core/qdev-properties.h"
> +#include "hw/timer/dw-apb-timer.h"
> +#include "trace.h"
> +
> +static void dw_apb_timer_update_irq(DWAPBTimerChannel *t)
> +{
> +    qemu_set_irq(t->irq, t->int_status &&
> +                 !(t->control & DW_APB_TIMER_CONTROL_INT));
> +}
> +
> +static void dw_apb_timer_clk_update(void *opaque, ClockEvent event)
> +{
> +    DWAPBTimerChannel *t = opaque;
> +
> +    if (!t->ptimer) {
> +        return;
> +    }
> +    ptimer_transaction_begin(t->ptimer);
> +    ptimer_set_period_from_clock(t->ptimer, t->clk, 1);
> +    ptimer_transaction_commit(t->ptimer);
> +}
> +
> +static void dw_apb_timer_enable(DWAPBTimerChannel *t)
> +{
> +    ptimer_transaction_begin(t->ptimer);
> +    ptimer_set_limit(t->ptimer, t->load ? t->load : 1, 1);
> +    ptimer_run(t->ptimer, 1);
> +    ptimer_transaction_commit(t->ptimer);
> +
> +    trace_dw_apb_timer_enable(t->id, t->load);

We usually trace on entry. If you want to notify the realized
action, then rename as trace_dw_apb_timer_enabled() for clarity?

> +}
> +
> +static void dw_apb_timer_disable(DWAPBTimerChannel *t)
> +{
> +    ptimer_transaction_begin(t->ptimer);
> +    ptimer_stop(t->ptimer);
> +    ptimer_transaction_commit(t->ptimer);
> +
> +    t->int_status = 0;
> +    dw_apb_timer_update_irq(t);
> +
> +    trace_dw_apb_timer_disable(t->id);

Ditto trace_dw_apb_timer_disabled().

> +}
> +
> +static void dw_apb_timer_tick(void *opaque)
> +{
> +    DWAPBTimerChannel *t = opaque;
> +    uint32_t reload;
> +
> +    trace_dw_apb_timer_tick(t->id);
> +
> +    t->int_status = 1;
> +    dw_apb_timer_update_irq(t);
> +
> +    if (t->control & DW_APB_TIMER_CONTROL_MODE_PERIODIC) {
> +        reload = t->load ? t->load : 1;
> +    } else {
> +        reload = UINT32_MAX;
> +    }
> +
> +    ptimer_set_limit(t->ptimer, reload, 1);
> +    ptimer_run(t->ptimer, 1);
> +}
> +
> +static uint64_t dw_apb_timer_read(void *opaque, hwaddr addr,
> +                                  unsigned int size)
> +{
> +    DWAPBTimerState *s = DW_APB_TIMER(opaque);
> +    uint32_t value = 0;
> +
> +    addr &= 0xfff;

Masking address like that is dubious. Users should map aliased / banked
regions with memory_region_init_alias() / memory_region_add_subregion().

> +
> +    if (addr < DW_APB_TIMER_INT_STATUS) {
> +        unsigned int idx = addr / DW_APB_TIMER_STRIDE;
> +        hwaddr reg = addr % DW_APB_TIMER_STRIDE;
> +
> +        if (idx < s->num_timers) {
> +            DWAPBTimerChannel *t = &s->timers[idx];
> +
> +            switch (reg) {
> +            case DW_APB_TIMER_N_LOAD_COUNT:
> +                value = t->load;
> +                break;
> +            case DW_APB_TIMER_N_CURRENT_VALUE:
> +                if (t->control & DW_APB_TIMER_CONTROL_ENABLE) {
> +                    value = ptimer_get_count(t->ptimer);
> +                }
> +                break;
> +            case DW_APB_TIMER_N_CONTROL:
> +                value = t->control;
> +                break;
> +            case DW_APB_TIMER_N_EOI:
> +                t->int_status = 0;
> +                dw_apb_timer_update_irq(t);
> +                trace_dw_apb_timer_irq_clear(t->id);
> +                break;
> +            case DW_APB_TIMER_N_INT_STATUS:
> +                value = t->int_status &&
> +                        !(t->control & DW_APB_TIMER_CONTROL_INT);
> +                break;
> +            default:
> +                break;
> +            }
> +        }
> +    } else {
> +        switch (addr) {
> +        case DW_APB_TIMER_INT_STATUS:
> +            for (int i = 0; i < s->num_timers; i++) {
> +                DWAPBTimerChannel *t = &s->timers[i];
> +
> +                value |= (t->int_status &&
> +                          !(t->control & DW_APB_TIMER_CONTROL_INT)) << i;
> +            }
> +            break;
> +        case DW_APB_TIMER_EOI:
> +            for (int i = 0; i < s->num_timers; i++) {
> +                DWAPBTimerChannel *t = &s->timers[i];
> +
> +                t->int_status = 0;
> +                dw_apb_timer_update_irq(t);
> +            }
> +            break;
> +        case DW_APB_TIMER_RAW_INT_STATUS:
> +            for (int i = 0; i < s->num_timers; i++) {
> +                value |= s->timers[i].int_status << i;
> +            }
> +            break;
> +        case DW_APB_TIMER_COMP_VERSION:
> +            value = s->comp_version;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +
> +    trace_dw_apb_timer_read(addr, value);
> +    return value;
> +}
> +
> +static void dw_apb_timer_write(void *opaque, hwaddr addr,
> +                               uint64_t value, unsigned int size)
> +{
> +    DWAPBTimerState *s = DW_APB_TIMER(opaque);
> +
> +    addr &= 0xfff;
> +
> +    if (addr < DW_APB_TIMER_INT_STATUS) {
> +        unsigned int idx = addr / DW_APB_TIMER_STRIDE;
> +        hwaddr reg = addr % DW_APB_TIMER_STRIDE;
> +
> +        if (idx < s->num_timers) {
> +            DWAPBTimerChannel *t = &s->timers[idx];
> +
> +            switch (reg) {
> +            case DW_APB_TIMER_N_LOAD_COUNT:
> +                t->load = value;
> +                break;
> +            case DW_APB_TIMER_N_CONTROL: {
> +                uint32_t old_control = t->control;
> +                uint32_t new_control = value & DW_APB_TIMER_CONTROL_RW_MASK;
> +
> +                t->control = new_control;
> +                if ((new_control ^ old_control) &
> +                    DW_APB_TIMER_CONTROL_ENABLE) {
> +                    if (new_control & DW_APB_TIMER_CONTROL_ENABLE) {
> +                        dw_apb_timer_enable(t);
> +                    } else {
> +                        dw_apb_timer_disable(t);
> +                    }
> +                }
> +                if ((new_control ^ old_control) &
> +                    DW_APB_TIMER_CONTROL_INT) {
> +                    dw_apb_timer_update_irq(t);
> +                }
> +                break;
> +            }
> +            default:
> +                break;
> +            }
> +        }
> +    }
> +
> +    trace_dw_apb_timer_write(addr, value);
> +}
> +
> +static const MemoryRegionOps dw_apb_timer_ops = {
> +    .read = dw_apb_timer_read,
> +    .write = dw_apb_timer_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static void dw_apb_timer_realize(DeviceState *dev, Error **errp)
> +{
> +    DWAPBTimerState *s = DW_APB_TIMER(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    if (s->num_timers == 0 || s->num_timers > DW_APB_TIMER_MAX_TIMERS) {
> +        error_setg(errp, "dw-apb-timer: num-timers must be between 1 and %u",
> +                   DW_APB_TIMER_MAX_TIMERS);
> +        return;
> +    }
> +
> +    for (int i = 0; i < s->num_timers; i++) {
> +        DWAPBTimerChannel *t = &s->timers[i];
> +
> +        if (!clock_has_source(t->clk)) {
> +            error_setg(errp, "dw-apb-timer: timer[%u] clock must be connected",
> +                       i);
> +            return;
> +        }
> +    }
> +
> +    for (int i = 0; i < s->num_timers; i++) {
> +        DWAPBTimerChannel *t = &s->timers[i];
> +
> +        t->ptimer = ptimer_init(dw_apb_timer_tick, t,
> +            PTIMER_POLICY_NO_IMMEDIATE_TRIGGER |
> +            PTIMER_POLICY_NO_IMMEDIATE_RELOAD |
> +            PTIMER_POLICY_NO_COUNTER_ROUND_DOWN);
> +        ptimer_transaction_begin(t->ptimer);
> +        ptimer_set_limit(t->ptimer, UINT32_MAX, 1);
> +        ptimer_transaction_commit(t->ptimer);
> +        sysbus_init_irq(sbd, &t->irq);
> +
> +        /* The source may have been connected before the ptimer existed. */
> +        dw_apb_timer_clk_update(t, ClockUpdate);
> +    }
> +
> +    memory_region_init_io(&s->mmio, OBJECT(dev), &dw_apb_timer_ops,
> +                          s, TYPE_DW_APB_TIMER, DW_APB_TIMER_MMIO_SIZE);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +}
> +
> +static void dw_apb_timer_reset(DeviceState *dev)
> +{
> +    DWAPBTimerState *s = DW_APB_TIMER(dev);
> +
> +    for (int i = 0; i < s->num_timers; i++) {
> +        DWAPBTimerChannel *t = &s->timers[i];
> +
> +        ptimer_transaction_begin(t->ptimer);
> +        ptimer_stop(t->ptimer);
> +        ptimer_transaction_commit(t->ptimer);
> +
> +        t->load = 0;
> +        t->control = 0;
> +        t->int_status = 0;
> +        dw_apb_timer_update_irq(t);
> +    }
> +}
> +
> +static const VMStateDescription vmstate_dw_apb_timer_channel = {
> +    .name = "dw-apb-timer-channel",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_PTIMER(ptimer, DWAPBTimerChannel),
> +        VMSTATE_CLOCK(clk, DWAPBTimerChannel),
> +        VMSTATE_UINT32(load, DWAPBTimerChannel),
> +        VMSTATE_UINT32(control, DWAPBTimerChannel),
> +        VMSTATE_UINT32(int_status, DWAPBTimerChannel),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_dw_apb_timer = {
> +    .name = "dw-apb-timer",
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_STRUCT_VARRAY_UINT32(timers, DWAPBTimerState,
> +                                     num_timers, 0,
> +                                     vmstate_dw_apb_timer_channel,
> +                                     DWAPBTimerChannel),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const Property dw_apb_timer_properties[] = {
> +    DEFINE_PROP_UINT32("num-timers", DWAPBTimerState, num_timers, 1),
> +    DEFINE_PROP_UINT32("comp-version", DWAPBTimerState, comp_version,
> +                       0x3231312A),

Why expose "comp-version"? What is this default magic value?

> +};
> +
> +static void dw_apb_timer_init(Object *obj)
> +{
> +    DWAPBTimerState *s = DW_APB_TIMER(obj);
> +
> +    for (int i = 0; i < DW_APB_TIMER_MAX_TIMERS; i++) {
> +        DWAPBTimerChannel *t = &s->timers[i];
> +        g_autofree char *name = g_strdup_printf("timer[%d]", i);
> +
> +        t->id = i;
> +        t->clk = qdev_init_clock_in(DEVICE(obj), name,
> +                                    dw_apb_timer_clk_update, t,
> +                                    ClockUpdate);
> +    }
> +}
> +
> +static void dw_apb_timer_class_init(ObjectClass *klass, const void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    dc->realize = dw_apb_timer_realize;
> +    dc->vmsd = &vmstate_dw_apb_timer;
> +    dc->desc = "Synopsys DesignWare APB timer";
> +    device_class_set_legacy_reset(dc, dw_apb_timer_reset);

Please do not use device_class_set_legacy_reset().

> +    device_class_set_props(dc, dw_apb_timer_properties);
> +}
> +
> +static const TypeInfo dw_apb_timer_info = {
> +    .name          = TYPE_DW_APB_TIMER,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(DWAPBTimerState),
> +    .instance_init = dw_apb_timer_init,

Missing deallocation:

        .instance_finalize = dw_apb_timer_init,

(Adding dw_apb_timer_unrealize would also be cleaner).

> +    .class_init    = dw_apb_timer_class_init,
> +};
> +
> +static void dw_apb_timer_register_type(void)
> +{
> +    type_register_static(&dw_apb_timer_info);
> +}
> +
> +type_init(dw_apb_timer_register_type)
> diff --git a/hw/timer/meson.build b/hw/timer/meson.build
> index 201b5d8316..fccc36540c 100644
> --- a/hw/timer/meson.build
> +++ b/hw/timer/meson.build
> @@ -34,3 +34,4 @@ specific_ss.add(when: 'CONFIG_IBEX', if_true: files('ibex_timer.c'))
>   system_ss.add(when: 'CONFIG_SIFIVE_PWM', if_true: files('sifive_pwm.c'))
>   
>   system_ss.add(when: 'CONFIG_AVR_TIMER16', if_true: files('avr_timer16.c'))
> +system_ss.add(when: 'CONFIG_DW_APB_TIMER', if_true: files('dw-apb-timer.c'))
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index 634ba1da27..7f592a96c8 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -1,5 +1,13 @@
>   # See docs/devel/tracing.rst for syntax documentation.
>   
> +# dw-apb-timer.c
> +dw_apb_timer_read(uint64_t addr, uint32_t val) "DW APB timer read: [0x%" PRIx64 "] -> 0x%" PRIx32
> +dw_apb_timer_write(uint64_t addr, uint64_t val) "DW APB timer write: [0x%" PRIx64 "] <- 0x%" PRIx64
> +dw_apb_timer_tick(int idx) "DW APB timer %d tick"
> +dw_apb_timer_irq_clear(int idx) "DW APB timer %d IRQ cleared"
> +dw_apb_timer_enable(int idx, uint32_t load) "DW APB timer %d enabled load=0x%" PRIx32
> +dw_apb_timer_disable(int idx) "DW APB timer %d disabled"
> +
>   # slavio_timer.c
>   slavio_timer_get_out(uint64_t limit, uint32_t counthigh, uint32_t count) "limit 0x%"PRIx64" count 0x%x0x%08x"
>   slavio_timer_irq(uint32_t counthigh, uint32_t count) "callback: count 0x%x0x%08x"
> diff --git a/include/hw/timer/dw-apb-timer.h b/include/hw/timer/dw-apb-timer.h
> new file mode 100644
> index 0000000000..b08b713fa3
> --- /dev/null
> +++ b/include/hw/timer/dw-apb-timer.h
> @@ -0,0 +1,65 @@
> +/*
> + * Synopsys DesignWare APB timer
> + *
> + * Copyright (c) 2026 raoyi <[email protected]>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef DW_APB_TIMER_H
> +#define DW_APB_TIMER_H
> +
> +#include "qemu/bitops.h"
> +#include "hw/core/sysbus.h"
> +#include "hw/core/irq.h"
> +#include "hw/core/clock.h"
> +#include "qom/object.h"
> +
> +#define TYPE_DW_APB_TIMER "dw-apb-timer"
> +OBJECT_DECLARE_SIMPLE_TYPE(DWAPBTimerState, DW_APB_TIMER)
> +
> +#define DW_APB_TIMER_MMIO_SIZE    0x100
> +#define DW_APB_TIMER_MAX_TIMERS   8
> +#define DW_APB_TIMER_STRIDE       0x14
> +
> +/* Per-timer register offsets */
> +#define DW_APB_TIMER_N_LOAD_COUNT    0x00
> +#define DW_APB_TIMER_N_CURRENT_VALUE 0x04
> +#define DW_APB_TIMER_N_CONTROL       0x08
> +#define DW_APB_TIMER_N_EOI           0x0c
> +#define DW_APB_TIMER_N_INT_STATUS    0x10
> +/* Global register offsets */
> +#define DW_APB_TIMER_INT_STATUS        0xa0
> +#define DW_APB_TIMER_EOI               0xa4
> +#define DW_APB_TIMER_RAW_INT_STATUS    0xa8
> +#define DW_APB_TIMER_COMP_VERSION      0xac
> +
> +/* Control register bits */
> +#define DW_APB_TIMER_CONTROL_ENABLE        BIT(0)
> +/* 1: periodic, 0:free running. */
> +#define DW_APB_TIMER_CONTROL_MODE_PERIODIC BIT(1)
> +#define DW_APB_TIMER_CONTROL_INT           BIT(2)
> +#define DW_APB_TIMER_CONTROL_RW_MASK       0x7

Please keep offsets / bits definitions within the C file,
no need to expose them.

> +
> +typedef struct DWAPBTimerChannel {
> +    struct ptimer_state *ptimer;
> +    Clock *clk;
> +    qemu_irq irq;
> +    unsigned int id;
> +    uint32_t load;
> +    uint32_t control;
> +    uint32_t int_status;
> +} DWAPBTimerChannel;
> +
> +struct DWAPBTimerState {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */

Please drop those private/public comments.

> +    MemoryRegion mmio;
> +    uint32_t num_timers;
> +    uint32_t comp_version;
> +    DWAPBTimerChannel timers[DW_APB_TIMER_MAX_TIMERS];
> +};
> +
> +#endif

Besides the minor comments, the overall patch LGTM!

Regards,

Phil.