Re: Patch to fix malfunctioning of T2-T5 timers on the STM32 platform

[Lucjan Bryndza]

Hi Philippe 


> Wiadomość napisana przez Philippe Mathieu-Daudé <philmd@linaro.org> w dniu 
> 30.10.2023, o godz. 05:18:
> 
> Hi Lucjan,
> 
> On 27/10/23 21:37, Lucjan Bryndza wrote:
>> Current implementation of T2 - T5 times on the STM32 platform does not work 
>> properly.
>> After configuring the timer-counter circuit to report interrupts every 10ms, 
>> in reality the first interrupt is reported
>> only once after a few seconds, while subsequent interrupts do not come.
>> The current code also does not properly emulate the operation of even the 
>> basic functions of the TIME-BASE unit.
>> This patch contains fixes that reimplements the basic functionality
>> of the time base unit such as up-counting down-counting , and alternate-mode 
>> up-down counting.
>> The ptimer() API is used to emulate timers.
>> After applying the patch, STM32 timer works correctly in its basic 
>> functionality.
>> The ISIX-RTOS test unit was used to test the patch.
>> Links and instructions can be found below:
>> https://github.com/lucckb/isixrtos/blob/master/tests/libisix/01_basic_primitives.cpp
>>  
>> <https://github.com/lucckb/isixrtos/blob/master/tests/libisix/01_basic_primitives.cpp>
>> https://github.com/lucckb/isixrtos/blob/master/extras/doc/unit_test_qemu.md 
>> <https://github.com/lucckb/isixrtos/blob/master/extras/doc/unit_test_qemu.md>
>> qemu-system-arm -M olimex-stm32-h405  -kernel 
>> build/tests/libisix/isixunittests.binary -nographic
>> unittests_entry.cpp:146|ISIX VERSION pub/ep0319-157-gb239b35f-dirty
>> unittests_entry.cpp:83|Exceptions pretest. OK
>> 51 selected tests
>> [   RUN    ] 01_base_00 TimeBase timer vs systick
>> [  1001ms  ] ...
>> [   RUN    ] 01_base_01 Basic heap allocator
>> [   1ms    ] ...
>> Best Regards
>> Lucjan Bryndza
>> From 3ccfe70979d1b263d4fa22104ecf42ac5a628554 Mon Sep 17 00:00:00 2001
>> From: Lucjan Bryndza <lbryndza.oss@icloud.com>
>> Date: Thu, 26 Oct 2023 22:45:26 +0200
>> Subject: [PATCH] Fixing the basic functionality of STM32 timers
>> The current implementation of timers does not work properly
>> even in the basic functionality. A counter configured to report
>> an interrupt 10ms every reports the first interrupts after a
>> few seconds.   Count up and
>> count down modes are also not properly implemented. 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.oss@icloud.com>
>> ---
>>  hw/arm/stm32f405_soc.c             |   2 +-
>>  hw/timer/stm32f2xx_timer.c         | 291 ++++++++++++++++++-----------
>>  include/hw/timer/stm32f2xx_timer.h |  23 ++-
>>  3 files changed, 202 insertions(+), 114 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);
> 
> Correct, this is for the 405 SoC.
> 
>>          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..65f3287125 100644
>> --- a/hw/timer/stm32f2xx_timer.c
>> +++ b/hw/timer/stm32f2xx_timer.c
>> @@ -29,11 +29,18 @@
>>  #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
>>  #endif
>> +/* PCLK /4 */
>> +#define CLOCK_FREQUENCY 48000000ULL
> 
> This timer is generic, we shouldn't enforce a frequency from a
> particular SoC.

Ok I’ve been removed this line 
> 
> 
>>  static const MemoryRegionOps stm32f2xx_timer_ops = {
>> @@ -275,7 +353,7 @@ static const VMStateDescription vmstate_stm32f2xx_timer 
>> = {
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>>      .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,25 +378,24 @@ static const VMStateDescription 
>> vmstate_stm32f2xx_timer = {
>>  static Property stm32f2xx_timer_properties[] = {
>>      DEFINE_PROP_UINT64("clock-frequency", struct STM32F2XXTimerState,
>> -                       freq_hz, 1000000000),
>> +                       freq_hz, CLOCK_FREQUENCY),
> 
> So here I suggest using '0', and in stm32f2xx_timer_realize() propagate
> an error if the frequency is still 0.

Ok.I have changed as you suggested



> 
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
> 
> Regards,
> 
> Phil.


Best Regards
Lucjan