Skip to content

drivers: timer: nrf: Optimize k_cycle_get_32(). #9275

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

Merged
merged 2 commits into from
Aug 13, 2018
Merged

drivers: timer: nrf: Optimize k_cycle_get_32(). #9275

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2747916
drivers: timer: nrf: Optimize k_cycle_get_32().
pizi-nordic Aug 3, 2018
481011a
driver: timer: nrf: Remove redundant RTC_COUNTER reads.
pizi-nordic Aug 6, 2018
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
96 changes: 46 additions & 50 deletions drivers/timer/nrf_rtc_timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,23 +266,23 @@ u32_t _get_remaining_program_time(void)

u32_t _get_elapsed_program_time(void)
{
u32_t rtc_now, rtc_prev, rtc_elapsed;
u32_t rtc_elapsed, rtc_past_copy;

if (!expected_sys_ticks) {
return 0;
}

rtc_now = RTC_COUNTER;
/* Read rtc_past before RTC_COUNTER */
rtc_past_copy = rtc_past;

/* Make sure that compiler will not reverse access to RTC and
* rtc_past.
*/
compiler_barrier();

/* Discard value of RTC_COUNTER read at LFCLK transition */
do {
/* Number of RTC cycles passed since last RTC Programing*/
rtc_elapsed = (rtc_now - rtc_past) & RTC_MASK;
rtc_prev = rtc_now;
rtc_now = RTC_COUNTER;
} while (rtc_prev != rtc_now);
rtc_elapsed = (RTC_COUNTER - rtc_past_copy) & RTC_MASK;

/*Convert number of Machine cycles to SYS TICS*/
/* Convert number of Machine cycles to SYS_TICKS */
return (rtc_elapsed / sys_clock_hw_cycles_per_tick);
}

Expand Down Expand Up @@ -329,22 +329,17 @@ void _set_time(u32_t time)
*/
s32_t _get_max_clock_time(void)
{
u32_t rtc_now, rtc_prev, rtc_away, sys_away = 0;
u32_t rtc_away, sys_away = 0;

rtc_now = RTC_COUNTER;
/* Discard value of RTC_COUNTER read at LFCLK transition */
do {
rtc_away = (RTC_MASK - rtc_now);
rtc_away = rtc_away > RTC_HALF ? RTC_HALF : rtc_away;
rtc_prev = rtc_now;
/* Calculate time to programe into RTC*/
rtc_now = RTC_COUNTER;
} while (rtc_now != rtc_prev);
/* Calculate time to program into RTC */
rtc_away = (RTC_MASK - RTC_COUNTER);
rtc_away = rtc_away > RTC_HALF ? RTC_HALF : rtc_away;

/* Convert RTC Ticks to SYS TICKS*/
if (rtc_away >= sys_clock_hw_cycles_per_tick) {
sys_away = rtc_away / sys_clock_hw_cycles_per_tick;
}

return sys_away;
}

Expand All @@ -371,25 +366,23 @@ void _enable_sys_clock(void)
u64_t _get_elapsed_clock_time(void)
{
u64_t elapsed;
u32_t rtc_now, rtc_elapsed, rtc_prev, sys_elapsed;
u32_t rtc_elapsed, rtc_past_copy;

rtc_now = RTC_COUNTER;
/* Read _sys_clock_tick_count and rtc_past before RTC_COUNTER */
elapsed = _sys_clock_tick_count;
rtc_past_copy = rtc_past;

/* Discard value of RTC_COUNTER read at LFCLK transition */
do {
/* Calculate number of rtc cycles elapsed since RTC programing*/
rtc_elapsed = (rtc_now - rtc_past) & RTC_MASK;
elapsed = _sys_clock_tick_count;
rtc_prev = rtc_now;
rtc_now = RTC_COUNTER;
} while (rtc_now != rtc_prev);
/* Make sure that compiler will not reverse access to RTC and
* variables above.
*/
compiler_barrier();

rtc_elapsed = (RTC_COUNTER - rtc_past_copy) & RTC_MASK;
if (rtc_elapsed >= sys_clock_hw_cycles_per_tick) {
/* Convert RTC cycles to SYS TICKS*/
sys_elapsed = rtc_elapsed / sys_clock_hw_cycles_per_tick;
/* Update total number of SYS_TICKS passed*/
elapsed += sys_elapsed;
/* Update total number of SYS_TICKS passed */
elapsed += (rtc_elapsed / sys_clock_hw_cycles_per_tick);
}

return elapsed;
}
#endif
Expand Down Expand Up @@ -550,24 +543,27 @@ int _sys_clock_driver_init(struct device *device)

u32_t _timer_cycle_get_32(void)
{
u32_t ticked_cycles;
u32_t elapsed_cycles;
u32_t sys_clock_tick_count;
u32_t rtc_prev;
u32_t rtc_now;

rtc_now = RTC_COUNTER;
/* Discard value of RTC_COUNTER read at LFCLK transition */
do {
sys_clock_tick_count = _sys_clock_tick_count;
elapsed_cycles = (rtc_now - (sys_clock_tick_count *
sys_clock_hw_cycles_per_tick)) &
RTC_MASK;
rtc_prev = rtc_now;
rtc_now = RTC_COUNTER;
} while (rtc_now != rtc_prev);
Copy link
Contributor

@cvinayak cvinayak Aug 4, 2018

Choose a reason for hiding this comment

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

This loop ensures that at any realtime the returned value is not off by 1 system clock tick.

Copy link
Contributor Author

@pizi-nordic pizi-nordic Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think that such "protection" does not give any value:

int get_time()
{
    int time;

/*
 * If RTC_REGISTER changes here, code below will get updated value.
 */
    do {
        time = RTC_REGISTER;
        /*
         * If RTC_REGISTER changes here, the loop will actually delay
         * return from the function and returned value will include
         * effect of the delay.
         */
     } while (time != RTC_REGISTER)

/*
 * If RTC_REGISTER changes here, value returned by this function will
 *  be off by 1 system clock cycle.
 */

return time;
}

In both versions, the one with loop and the one without, there is risk that a "stale" value will be returned. It is unavoidable, as even return from the function takes time and RTC counter could be updated during that period. I personally do not see any benefit from this loop (especially as in this case access to RTC is atomic and there is no risk that we get inconsistent value of the register).

Copy link
Contributor

@cvinayak cvinayak Aug 6, 2018

Choose a reason for hiding this comment

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

What you are not considering is the one clock tick that elapses, and the kernel cannot tolerate a stale clock tick. You can use your implementation on nRF52 boards and run a typical peripheral sample, it will stall eventually when the system clock ticks between the reading of the kernel variable and reading of the RTC counter.

Copy link
Contributor Author

@pizi-nordic pizi-nordic Aug 6, 2018

Choose a reason for hiding this comment

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

Are you mentioning here problem you pointed there: #9275 (comment) ? If so, please look on my comment posted here: #9275 (comment)

Copy link
Contributor

@cvinayak cvinayak Aug 6, 2018

Choose a reason for hiding this comment

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

I understand your explanation. except for:
/*

  • If RTC_REGISTER changes here, value returned by this function will
  • be off by 1 system clock tick.
    */

Here the returned value is only off by 1 cycle, not 1 system clock tick.

But as long as, tests cases pass as before, I am ok with the changes (because I am not very well informed on the kernel's use of this function).

Copy link
Contributor Author

@pizi-nordic pizi-nordic Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That is true. I had 1 clock cycle in my mind, not tick. Example updated.


return (sys_clock_tick_count * sys_clock_hw_cycles_per_tick) +
elapsed_cycles;
/* Number of timer cycles announced as ticks so far. */
ticked_cycles = _sys_clock_tick_count * sys_clock_hw_cycles_per_tick;
Copy link
Member

@anangl anangl Aug 7, 2018

Choose a reason for hiding this comment

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

Now I realized that this information was stored in rtc_past, so actually there's no need to do this multiplication here.

Copy link
Contributor Author

@pizi-nordic pizi-nordic Aug 7, 2018

Choose a reason for hiding this comment

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

There is subtle difference:

rtc_past = (_sys_clock_tick_count * sys_clock_hw_cycles_per_tick) & RTC_MASK;
ticked_cycles = (_sys_clock_tick_count * sys_clock_hw_cycles_per_tick);

We are doing this multiplication because we want 32-bit timestamp which will not wrap up when RTC do so.

Copy link
Member

@anangl anangl Aug 7, 2018

Choose a reason for hiding this comment

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

Right. I missed it.

Copy link
Member

@anangl anangl Aug 8, 2018

Choose a reason for hiding this comment

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

However, ;-)
it seems that rtc_past could be stored without prior masking (then it could be used instead of the multiplication mentioned above), as in all places where it is later used the masking would be done anyway (either by using RTC_MASK or in hardware when rtc_compare_set is called).

But I guess this can be left as a field for future optimizations.


/* Make sure that compiler will not reverse access to RTC and
* _sys_clock_tick_count.
*/
compiler_barrier();

/* Number of timer cycles since last announced tick we know about.
*
* The value of RTC_COUNTER is not reset on tick, so it will
* compensate potentialy missed update of _sys_clock_tick_count
* which could have happen between the ticked_cycles calculation
* and the code below.
*/
elapsed_cycles = (RTC_COUNTER - ticked_cycles) & RTC_MASK;

return ticked_cycles + elapsed_cycles;
}

#ifdef CONFIG_SYSTEM_CLOCK_DISABLE
Expand Down