Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: timer: Improve the accuracy of the MCUX OS Timer #84938

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −21
Open

drivers: timer: Improve the accuracy of the MCUX OS Timer #84938

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 30 additions & 21 deletions drivers/timer/mcux_os_timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

#define CYC_PER_TICK ((uint32_t)((uint64_t)sys_clock_hw_cycles_per_sec() \
/ (uint64_t)CONFIG_SYS_CLOCK_TICKS_PER_SEC))
#define CYC_PER_US ((uint32_t)((uint64_t)sys_clock_hw_cycles_per_sec() \
/ (uint64_t)USEC_PER_SEC))
#define MAX_CYC INT_MAX
#define MAX_TICKS ((MAX_CYC - CYC_PER_TICK) / CYC_PER_TICK)
#define MIN_DELAY 1000
Expand All @@ -37,7 +39,12 @@ static OSTIMER_Type *base;
*/
static uint64_t cyc_sys_compensated;
#if DT_NODE_HAS_STATUS_OKAY(DT_NODELABEL(standby)) && CONFIG_PM
/* This is the counter device used when OS timer is not available in
* standby mode.
*/
static const struct device *counter_dev;
/* Indicates if the counter is running. */
bool counter_running;
#endif

static uint64_t mcux_lpc_ostick_get_compensated_timer_value(void)
Expand Down Expand Up @@ -86,8 +93,7 @@ static uint32_t mcux_lpc_ostick_set_counter_timeout(int32_t curr_timeout)
uint32_t timeout;
int32_t ticks;
struct counter_top_cfg top_cfg = { 0 };

timeout = k_ticks_to_us_ceil32(curr_timeout);
Comment on lines -89 to -90
Copy link
Member

@decsny decsny Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change from ceil to near ? can it be in the commit message

timeout = k_ticks_to_us_near32(curr_timeout);

ticks = counter_us_to_ticks(counter_dev, timeout);
ticks = CLAMP(ticks, 1, counter_get_max_top_value(counter_dev));
Expand All @@ -111,17 +117,17 @@ static uint32_t mcux_lpc_ostick_set_counter_timeout(int32_t curr_timeout)
}
}

/* Counter is set to wakeup the system after the requested time */
if (counter_start(counter_dev) != 0) {
ret = 1;
}
counter_running = true;
Comment on lines +120 to +124
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if counter_start fails, shouldn't counter_running be set to false ?

#if CONFIG_MCUX_OS_TIMER_PM_POWERED_OFF
/* Capture the current timer value for cases where it loses its state
* in low power modes.
*/
cyc_sys_compensated += OSTIMER_GetCurrentTimerValue(base);
#endif

/* Counter is set to wakeup the system after the requested time */
if (counter_start(counter_dev) != 0) {
ret = 1;
}
Comment on lines -120 to -124
Copy link
Member

@decsny decsny Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this moved before getting the timer value ? I didn't understand from the commit message

} else {
ret = 1;
}
Expand All @@ -143,23 +149,26 @@ static uint32_t mcux_lpc_ostick_compensate_system_timer(void)
uint32_t slept_time_ticks;
uint32_t slept_time_us;

counter_stop(counter_dev);

counter_get_value(counter_dev, &slept_time_ticks);

if (!(counter_is_counting_up(counter_dev))) {
slept_time_ticks = counter_get_top_value(counter_dev) - slept_time_ticks;
}
slept_time_us = counter_ticks_to_us(counter_dev, slept_time_ticks);
cyc_sys_compensated += (k_us_to_ticks_floor32(slept_time_us) * CYC_PER_TICK);
if (counter_running) {
counter_stop(counter_dev);
counter_running = false;
counter_get_value(counter_dev, &slept_time_ticks);

if (!(counter_is_counting_up(counter_dev))) {
slept_time_ticks = counter_get_top_value(counter_dev) -
slept_time_ticks;
}
slept_time_us = counter_ticks_to_us(counter_dev, slept_time_ticks);
cyc_sys_compensated += CYC_PER_US * slept_time_us;
#if CONFIG_MCUX_OS_TIMER_PM_POWERED_OFF
/* Reactivate os_timer for cases where it loses its state */
OSTIMER_Init(base);
/* Reset the OS Timer to a known state */
RESET_PeripheralReset(kOSEVENT_TIMER_RST_SHIFT_RSTn);
/* Reactivate os_timer for cases where it loses its state */
OSTIMER_Init(base);
#endif

/* Announce the time slept to the kernel*/
mcux_lpc_ostick_isr(NULL);
/* Announce the time slept to the kernel*/
mcux_lpc_ostick_isr(NULL);
}
} else {
ret = 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole function is confusing to read, it can be cleaned up by inverting the conditionals, like

if (!counter_dev) {
   return 1;
}

if (!counter_running) {
   return 0;
}

/* rest of the code */

return 0;

Expand Down
Loading