On Tue, Oct 31, 2023 at 6:35 AM ~lbryndza <lbryn...@git.sr.ht> wrote:
>
> From: Lucjan Bryndza <lbryndza....@icloud.com>
>
> The current implementation of timers does not work properly
> even in basic functionality. A counter configured to report
> an interrupt every 10ms reports the first interrupts after a
> few seconds. There are also no properly implemented count up and
> count down modes. This commit fixes bugs with interrupt
> reporting and implements the basic modes of the counter's
> time-base block.
>
> Signed-off-by: Lucjan Bryndza <lbryndza....@icloud.com>
Thanks. This is already easier to follow than the first version. It
still helps to split this up, it's relatively long and difficult to
follow the changes.
You have done a great job of explaining what the current problem is in
the commit message, but do you mind explaining a bit more details
about your fix?
I know it might seem like a pain, but I am pretty much reviewing the
STM32 work in my spare time and I haven't worked on it directly in
awhile. The simpler you can make the changes the easier and quicker it
is to review. As you have noticed, timers are tricky to get right, so
we want to try and make sure it's correct this time :)
> ---
> hw/arm/stm32f405_soc.c | 2 +-
> hw/timer/stm32f2xx_timer.c | 262 +++++++++++++++++++----------
> include/hw/timer/stm32f2xx_timer.h | 23 ++-
> 3 files changed, 189 insertions(+), 98 deletions(-)
>
> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
> index cef23d7ee4..69316181b3 100644
> --- a/hw/arm/stm32f405_soc.c
> +++ b/hw/arm/stm32f405_soc.c
> @@ -183,7 +183,7 @@ static void stm32f405_soc_realize(DeviceState *dev_soc,
> Error **errp)
> /* Timer 2 to 5 */
> for (i = 0; i < STM_NUM_TIMERS; i++) {
> dev = DEVICE(&(s->timer[i]));
> - qdev_prop_set_uint64(dev, "clock-frequency", 1000000000);
> + qdev_prop_set_uint64(dev, "clock-frequency", 48000000);
> if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer[i]), errp)) {
> return;
> }
> diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
> index ba8694dcd3..9a992231fa 100644
> --- a/hw/timer/stm32f2xx_timer.c
> +++ b/hw/timer/stm32f2xx_timer.c
> @@ -23,12 +23,17 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "hw/irq.h"
> #include "hw/qdev-properties.h"
> #include "hw/timer/stm32f2xx_timer.h"
> #include "migration/vmstate.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> +#include "qemu/typedefs.h"
> +#include "qemu/timer.h"
> +#include "qemu/main-loop.h"
> +#include "sysemu/dma.h"
>
> #ifndef STM_TIMER_ERR_DEBUG
> #define STM_TIMER_ERR_DEBUG 0
> @@ -42,63 +47,87 @@
>
> #define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>
> -static void stm32f2xx_timer_set_alarm(STM32F2XXTimerState *s, int64_t now);
>
> -static void stm32f2xx_timer_interrupt(void *opaque)
> +static uint32_t stm32f2xx_timer_get_count(STM32F2XXTimerState *s)
> {
> - STM32F2XXTimerState *s = opaque;
> -
> - DB_PRINT("Interrupt\n");
> -
> - if (s->tim_dier & TIM_DIER_UIE && s->tim_cr1 & TIM_CR1_CEN) {
> - s->tim_sr |= 1;
> - qemu_irq_pulse(s->irq);
> - stm32f2xx_timer_set_alarm(s, s->hit_time);
> - }
> -
> - if (s->tim_ccmr1 & (TIM_CCMR1_OC2M2 | TIM_CCMR1_OC2M1) &&
> - !(s->tim_ccmr1 & TIM_CCMR1_OC2M0) &&
> - s->tim_ccmr1 & TIM_CCMR1_OC2PE &&
> - s->tim_ccer & TIM_CCER_CC2E) {
> - /* PWM 2 - Mode 1 */
> - DB_PRINT("PWM2 Duty Cycle: %d%%\n",
> - s->tim_ccr2 / (100 * (s->tim_psc + 1)));
> + uint64_t cnt = ptimer_get_count(s->timer);
> + if (s->count_mode == TIMER_UP_COUNT) {
> + return s->tim_arr - (cnt & 0xffff);
> + } else {
> + return cnt & 0xffff;
> }
> }
>
> -static inline int64_t stm32f2xx_ns_to_ticks(STM32F2XXTimerState *s, int64_t
> t)
> +
> +static void stm32f2xx_timer_set_count(STM32F2XXTimerState *s, uint32_t cnt)
> {
> - return muldiv64(t, s->freq_hz, 1000000000ULL) / (s->tim_psc + 1);
> + if (s->count_mode == TIMER_UP_COUNT) {
> + ptimer_set_count(s->timer, s->tim_arr - (cnt & 0xffff));
> + } else {
> + ptimer_set_count(s->timer, cnt & 0xffff);
> + }
> }
>
> -static void stm32f2xx_timer_set_alarm(STM32F2XXTimerState *s, int64_t now)
> +static void stm32f2xx_timer_update(STM32F2XXTimerState *s)
> {
> - uint64_t ticks;
> - int64_t now_ticks;
> + if (s->tim_cr1 & TIM_CR1_DIR) {
> + s->count_mode = TIMER_DOWN_COUNT;
> + } else {
> + s->count_mode = TIMER_UP_COUNT;
> + }
>
> - if (s->tim_arr == 0) {
> - return;
> + if (s->tim_cr1 & TIM_CR1_CMS) {
> + s->count_mode = TIMER_UP_COUNT;
> }
>
> - DB_PRINT("Alarm set at: 0x%x\n", s->tim_cr1);
> + if (s->tim_cr1 & TIM_CR1_CEN) {
> + DB_PRINT("Enabling timer\n");
> + ptimer_set_freq(s->timer, s->freq_hz);
> + ptimer_run(s->timer, !(s->tim_cr1 & 0x04));
> + } else {
> + DB_PRINT("Disabling timer\n");
> + ptimer_stop(s->timer);
> + }
> +}
>
> - now_ticks = stm32f2xx_ns_to_ticks(s, now);
> - ticks = s->tim_arr - (now_ticks - s->tick_offset);
> +static void stm32f2xx_timer_update_uif(STM32F2XXTimerState *s, uint8_t value)
> +{
> + s->tim_sr &= ~TIM_SR1_UIF;
> + s->tim_sr |= (value & TIM_SR1_UIF);
> + qemu_set_irq(s->irq, value);
> +}
>
> - DB_PRINT("Alarm set in %d ticks\n", (int) ticks);
> +static void stm32f2xx_timer_tick(void *opaque)
> +{
> + STM32F2XXTimerState *s = (STM32F2XXTimerState *)opaque;
> + DB_PRINT("Alarm raised\n");
> + stm32f2xx_timer_update_uif(s, 1);
> +
> + if (s->count_mode == TIMER_UP_COUNT) {
> + stm32f2xx_timer_set_count(s, 0);
> + } else {
> + stm32f2xx_timer_set_count(s, s->tim_arr);
> + }
>
> - s->hit_time = muldiv64((ticks + (uint64_t) now_ticks) * (s->tim_psc + 1),
> - 1000000000ULL, s->freq_hz);
> + if (s->tim_cr1 & TIM_CR1_CMS) {
> + if (s->count_mode == TIMER_UP_COUNT) {
> + s->count_mode = TIMER_DOWN_COUNT;
> + } else {
> + s->count_mode = TIMER_UP_COUNT;
> + }
> + }
>
> - timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->hit_time);
> - DB_PRINT("Wait Time: %" PRId64 " ticks\n", s->hit_time);
> + if (s->tim_cr1 & TIM_CR1_OPM) {
> + s->tim_cr1 &= ~TIM_CR1_CEN;
> + } else {
> + stm32f2xx_timer_update(s);
> + }
> }
>
> +
> static void stm32f2xx_timer_reset(DeviceState *dev)
> {
> STM32F2XXTimerState *s = STM32F2XXTIMER(dev);
> - int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -
> s->tim_cr1 = 0;
> s->tim_cr2 = 0;
> s->tim_smcr = 0;
> @@ -117,8 +146,6 @@ static void stm32f2xx_timer_reset(DeviceState *dev)
> s->tim_dcr = 0;
> s->tim_dmar = 0;
> s->tim_or = 0;
> -
> - s->tick_offset = stm32f2xx_ns_to_ticks(s, now);
> }
>
> static uint64_t stm32f2xx_timer_read(void *opaque, hwaddr offset,
> @@ -132,15 +159,18 @@ static uint64_t stm32f2xx_timer_read(void *opaque,
> hwaddr offset,
> case TIM_CR1:
> return s->tim_cr1;
> case TIM_CR2:
> - return s->tim_cr2;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: CR2 not supported");
> + return 0;
These type of smaller changes can be split into a separate patch
> case TIM_SMCR:
> - return s->tim_smcr;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: SMCR not supported");
> + return 0;
> case TIM_DIER:
> return s->tim_dier;
> case TIM_SR:
> return s->tim_sr;
> case TIM_EGR:
> - return s->tim_egr;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: EGR write only");
> + return 0;
> case TIM_CCMR1:
> return s->tim_ccmr1;
> case TIM_CCMR2:
> @@ -148,8 +178,7 @@ static uint64_t stm32f2xx_timer_read(void *opaque, hwaddr
> offset,
> case TIM_CCER:
> return s->tim_ccer;
> case TIM_CNT:
> - return stm32f2xx_ns_to_ticks(s,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) -
> - s->tick_offset;
> + return stm32f2xx_timer_get_count(s);
> case TIM_PSC:
> return s->tim_psc;
> case TIM_ARR:
> @@ -163,105 +192,152 @@ static uint64_t stm32f2xx_timer_read(void *opaque,
> hwaddr offset,
> case TIM_CCR4:
> return s->tim_ccr4;
> case TIM_DCR:
> - return s->tim_dcr;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: DCR not supported");
> + return 0;
> case TIM_DMAR:
> - return s->tim_dmar;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: CR2 not supported");
> + return 0;
> case TIM_OR:
> return s->tim_or;
> default:
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
> }
> -
> return 0;
> }
>
> +static void stm32f2xx_update_cr1(STM32F2XXTimerState *s, uint64_t value)
> +{
> + s->tim_cr1 = value & 0x3FF;
> + ptimer_transaction_begin(s->timer);
> + stm32f2xx_timer_update(s);
> + ptimer_transaction_commit(s->timer);
> + DB_PRINT("write cr1 = %x\n", s->tim_cr1);
> +}
> +
> +static void stm32f2xx_update_sr(STM32F2XXTimerState *s, uint64_t value)
> +{
> + s->tim_sr ^= (value ^ 0xFFFF);
> + s->tim_sr &= 0x1eFF;
> + ptimer_transaction_begin(s->timer);
> + stm32f2xx_timer_update_uif(s, s->tim_sr & 0x1);
> + ptimer_transaction_commit(s->timer);
> + DB_PRINT("write sr = %x\n", s->tim_sr);
> +}
> +
> +static void stm32f2xx_update_psc(STM32F2XXTimerState *s, uint64_t value)
> +{
> + s->tim_psc = value & 0xffff;
> + ptimer_transaction_begin(s->timer);
> + ptimer_set_freq(s->timer, s->freq_hz);
> + ptimer_transaction_commit(s->timer);
> + DB_PRINT("write psc = %x\n", s->tim_psc);
> +}
> +
> +static void stm32f2xx_update_egr(STM32F2XXTimerState *s, uint64_t value)
> +{
> + s->tim_egr = value & 0x1E;
> + if (value & TIM_EGR_TG) {
> + s->tim_sr |= TIM_EGR_TG;
> + }
> + if (value & TIM_EGR_UG) {
> + /* UG bit - reload */
> + ptimer_transaction_begin(s->timer);
> + ptimer_set_limit(s->timer, s->tim_arr, 1);
> + ptimer_transaction_commit(s->timer);
> + }
> + DB_PRINT("write EGR = %x\n", s->tim_egr);
> +}
> +
> +static void stm32f2xx_update_cnt(STM32F2XXTimerState *s, uint64_t value)
> +{
> + ptimer_transaction_begin(s->timer);
> + stm32f2xx_timer_set_count(s, value & 0xffff);
> + ptimer_transaction_commit(s->timer);
> + DB_PRINT("write cnt = %x\n", stm32f2xx_timer_get_count(s));
> +}
> +
> +static void stm32f2xx_update_arr(STM32F2XXTimerState *s, uint64_t value)
> +{
> + s->tim_arr = value & 0xffff;
> + ptimer_transaction_begin(s->timer);
> + ptimer_set_limit(s->timer, s->tim_arr, 1);
> + ptimer_transaction_commit(s->timer);
> + DB_PRINT("write arr = %x\n", s->tim_arr);
> +}
> +
> static void stm32f2xx_timer_write(void *opaque, hwaddr offset,
> - uint64_t val64, unsigned size)
> + uint64_t value, unsigned size)
> {
> STM32F2XXTimerState *s = opaque;
> - uint32_t value = val64;
> - int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - uint32_t timer_val = 0;
> -
> - DB_PRINT("Write 0x%x, 0x%"HWADDR_PRIx"\n", value, offset);
Why remove this?
>
> switch (offset) {
> case TIM_CR1:
> - s->tim_cr1 = value;
> + stm32f2xx_update_cr1(s, value);
> return;
> case TIM_CR2:
> - s->tim_cr2 = value;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: CR2 not supported");
> return;
> case TIM_SMCR:
> - s->tim_smcr = value;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: SCMR not supported");
> return;
> case TIM_DIER:
> - s->tim_dier = value;
> + s->tim_dier = value & 0x5F5F;
> + DB_PRINT("write dier = %x\n", s->tim_dier);
> return;
> case TIM_SR:
> - /* This is set by hardware and cleared by software */
> - s->tim_sr &= value;
> + stm32f2xx_update_sr(s, value);
> return;
> case TIM_EGR:
> - s->tim_egr = value;
> - if (s->tim_egr & TIM_EGR_UG) {
> - timer_val = 0;
> - break;
> - }
> + stm32f2xx_update_egr(s, value);
> return;
> case TIM_CCMR1:
> - s->tim_ccmr1 = value;
> + s->tim_ccmr1 = value & 0xffff;
> + DB_PRINT("write ccmr1 = %x\n", s->tim_ccmr1);
I don't think we need this print
> return;
> case TIM_CCMR2:
> - s->tim_ccmr2 = value;
> + s->tim_ccmr2 = value & 0xffff;
> + DB_PRINT("write ccmr2 = %x\n", s->tim_ccmr2);
> return;
> case TIM_CCER:
> - s->tim_ccer = value;
> + s->tim_ccer = value & 0x3333;
> + DB_PRINT("write ccer = %x\n", s->tim_ccer);
> return;
> case TIM_PSC:
> - timer_val = stm32f2xx_ns_to_ticks(s, now) - s->tick_offset;
> - s->tim_psc = value & 0xFFFF;
> - break;
> + stm32f2xx_update_psc(s, value);
> + return;
> case TIM_CNT:
> - timer_val = value;
> - break;
> + stm32f2xx_update_cnt(s, value);
> + return;
> case TIM_ARR:
> - s->tim_arr = value;
> - stm32f2xx_timer_set_alarm(s, now);
> + stm32f2xx_update_arr(s, value);
> return;
> case TIM_CCR1:
> - s->tim_ccr1 = value;
> + s->tim_ccr1 = value & 0xffff;
> return;
> case TIM_CCR2:
> - s->tim_ccr2 = value;
> + s->tim_ccr2 = value & 0xffff;
These small changes can also be split into a separate patch
> return;
> case TIM_CCR3:
> - s->tim_ccr3 = value;
> + s->tim_ccr3 = value & 0xffff;
> return;
> case TIM_CCR4:
> - s->tim_ccr4 = value;
> + s->tim_ccr4 = value & 0xffff;
> return;
> case TIM_DCR:
> - s->tim_dcr = value;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: DCR not supported");
> return;
> case TIM_DMAR:
> - s->tim_dmar = value;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: DMAR not supported");
> return;
> case TIM_OR:
> - s->tim_or = value;
> + qemu_log_mask(LOG_GUEST_ERROR, "stm32_timer: OR not supported");
> return;
> default:
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
> return;
> }
> -
> - /* This means that a register write has affected the timer in a way that
> - * requires a refresh of both tick_offset and the alarm.
> - */
> - s->tick_offset = stm32f2xx_ns_to_ticks(s, now) - timer_val;
> - stm32f2xx_timer_set_alarm(s, now);
> }
>
> static const MemoryRegionOps stm32f2xx_timer_ops = {
> @@ -272,10 +348,10 @@ static const MemoryRegionOps stm32f2xx_timer_ops = {
>
> static const VMStateDescription vmstate_stm32f2xx_timer = {
> .name = TYPE_STM32F2XX_TIMER,
> - .version_id = 1,
> - .minimum_version_id = 1,
> + .version_id = 2,
> + .minimum_version_id = 2,
> .fields = (VMStateField[]) {
> - VMSTATE_INT64(tick_offset, STM32F2XXTimerState),
> + VMSTATE_INT32(count_mode, STM32F2XXTimerState),
> VMSTATE_UINT32(tim_cr1, STM32F2XXTimerState),
> VMSTATE_UINT32(tim_cr2, STM32F2XXTimerState),
> VMSTATE_UINT32(tim_smcr, STM32F2XXTimerState),
> @@ -300,7 +376,7 @@ static const VMStateDescription vmstate_stm32f2xx_timer =
> {
>
> static Property stm32f2xx_timer_properties[] = {
> DEFINE_PROP_UINT64("clock-frequency", struct STM32F2XXTimerState,
> - freq_hz, 1000000000),
> + freq_hz, 0),
Why remove the default?
Alistair
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -318,7 +394,11 @@ static void stm32f2xx_timer_init(Object *obj)
> static void stm32f2xx_timer_realize(DeviceState *dev, Error **errp)
> {
> STM32F2XXTimerState *s = STM32F2XXTIMER(dev);
> - s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f2xx_timer_interrupt,
> s);
> + if (s->freq_hz == 0) {
> + error_setg(errp, "stm32f2xx_timer: Timer clock not defined");
> + return;
> + }
> + s->timer = ptimer_init(stm32f2xx_timer_tick, s, PTIMER_POLICY_LEGACY);
> }
>
> static void stm32f2xx_timer_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/timer/stm32f2xx_timer.h
> b/include/hw/timer/stm32f2xx_timer.h
> index 90f40f1746..c83f7b0d6f 100644
> --- a/include/hw/timer/stm32f2xx_timer.h
> +++ b/include/hw/timer/stm32f2xx_timer.h
> @@ -28,6 +28,7 @@
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "qom/object.h"
> +#include "hw/ptimer.h"
>
> #define TIM_CR1 0x00
> #define TIM_CR2 0x04
> @@ -49,9 +50,15 @@
> #define TIM_DMAR 0x4C
> #define TIM_OR 0x50
>
> -#define TIM_CR1_CEN 1
> +#define TIM_CR1_CEN 0x0001
> +#define TIM_CR1_DIR 0x0010
> +#define TIM_CR1_CMS 0x0060
> +#define TIM_CR1_OPM 0x0008
>
> -#define TIM_EGR_UG 1
> +#define TIM_SR1_UIF 0x0001
> +
> +#define TIM_EGR_UG 0x0001
> +#define TIM_EGR_TG 0x0040
>
> #define TIM_CCER_CC2E (1 << 4)
> #define TIM_CCMR1_OC2M2 (1 << 14)
> @@ -61,6 +68,7 @@
>
> #define TIM_DIER_UIE 1
>
> +
> #define TYPE_STM32F2XX_TIMER "stm32f2xx-timer"
> typedef struct STM32F2XXTimerState STM32F2XXTimerState;
> DECLARE_INSTANCE_CHECKER(STM32F2XXTimerState, STM32F2XXTIMER,
> @@ -72,12 +80,10 @@ struct STM32F2XXTimerState {
>
> /* <public> */
> MemoryRegion iomem;
> - QEMUTimer *timer;
> + ptimer_state *timer;
> qemu_irq irq;
> -
> - int64_t tick_offset;
> - uint64_t hit_time;
> uint64_t freq_hz;
> + int count_mode;
>
> uint32_t tim_cr1;
> uint32_t tim_cr2;
> @@ -99,4 +105,9 @@ struct STM32F2XXTimerState {
> uint32_t tim_or;
> };
>
> +enum {
> + TIMER_UP_COUNT = 0,
> + TIMER_DOWN_COUNT = 1
> +};
> +
> #endif /* HW_STM32F2XX_TIMER_H */
> --
> 2.38.5
>
