drivers: timer: Add support for IFX Low power timer #89429
Conversation
github-actions
bot
added
area: Clock Control
platform: Infineon
github-actions
bot
requested review from
andyross,
ifyall,
nordic-krch,
npal-cy,
talih0 and
teburd
sreeramIfx
force-pushed
the
lptimer_support
branch
from
b79dfff to
d490ea3
Compare
sreeramIfx
force-pushed
the
lptimer_support
branch
from
d490ea3 to
a53c621
Compare
sreeramIfx
force-pushed
the
lptimer_support
branch
2 times, most recently
from
c3f9aa3 to
6e7559f
Compare
| /* passing ticks==1 means "announce the next tick", ticks value of zero (or even negative) | ||
| * is legal and treated identically: it simply indicates the kernel would like the next | ||
| * tick announcement as soon as possible. | ||
| */ | ||
| if (ticks < 1) { | ||
| ticks = 1; | ||
| } | ||
|
|
||
| uint32_t set_ticks = ((uint32_t)(ticks)*LPTIMER_FREQ) / CONFIG_SYS_CLOCK_TICKS_PER_SEC; |
There was a problem hiding this comment.
I suggest moving this calculation, particularly with the divide, before acquiring the lock to reduce the time spent holding the lock.
There was a problem hiding this comment.
This looks like a compile time constant to me unless LPTIMER_FREQ hides runtime code. There won't be a division at runtime.
| /* gives the value of LPTIM counter (ms) since the previous 'announce' */ | ||
| uint64_t ret = (((uint64_t)(lptimer_value - last_lptimer_value)) * | ||
| CONFIG_SYS_CLOCK_TICKS_PER_SEC) / | ||
| LPTIMER_FREQ; |
There was a problem hiding this comment.
I suggest moving this calculation after releasing the lock to reduce the time spend holding the lock.
| /* convert lptim count in a nb of hw cycles with precision */ | ||
| uint64_t ret = ((uint64_t)lp_time * sys_clock_hw_cycles_per_sec()) / LPTIMER_FREQ; |
There was a problem hiding this comment.
I suggest moving this calculation after releasing the lock to reduce the time spend holding the lock.
drivers/timer/ifx_cat1_lp_timer.c
Outdated
| uint32_t delta_ticks = | ||
| ((uint64_t)(lptimer_value - last_lptimer_value) * CONFIG_SYS_CLOCK_TICKS_PER_SEC) / | ||
| LPTIMER_FREQ; | ||
| sys_clock_announce(IS_ENABLED(CONFIG_TICKLESS_KERNEL) ? delta_ticks : (delta_ticks > 0)); | ||
| last_lptimer_value = lptimer_value; |
There was a problem hiding this comment.
If lock is not required for sys_clock_announce function, release lock before doing the delta_ticks computation to reduce time spent with resource locked. If it's possible to pre-compute the divide, we could also move that computation out of the interrupt handler.
There was a problem hiding this comment.
Unrelated to the comment above but about the same line: this is a precision bug. "last_lptimer_value" wants to be the last time announced, not the precise time of the last interrupt. Otherwise this will slip a little time every interrupt and if it happens to cross a tick boundary, that tick will be "lost" and never announced.
sreeramIfx
force-pushed
the
lptimer_support
branch
from
6e7559f to
d44cb03
Compare
drivers/timer/ifx_cat1_lp_timer.c
Outdated
| uint32_t delta_ticks = | ||
| ((uint64_t)(lptimer_value - last_lptimer_value) * CONFIG_SYS_CLOCK_TICKS_PER_SEC) / | ||
| LPTIMER_FREQ; | ||
| sys_clock_announce(IS_ENABLED(CONFIG_TICKLESS_KERNEL) ? delta_ticks : (delta_ticks > 0)); | ||
| last_lptimer_value = lptimer_value; |
There was a problem hiding this comment.
Unrelated to the comment above but about the same line: this is a precision bug. "last_lptimer_value" wants to be the last time announced, not the precise time of the last interrupt. Otherwise this will slip a little time every interrupt and if it happens to cross a tick boundary, that tick will be "lost" and never announced.
sreeramIfx
force-pushed
the
lptimer_support
branch
from
d44cb03 to
242a3fa
Compare
sreeramIfx
force-pushed
the
lptimer_support
branch
from
242a3fa to
6f65ebe
Compare
Adding support for low power timer to enable low power modes Signed-off-by: Sreeram Tatapudi <[email protected]>
sreeramIfx
force-pushed
the
lptimer_support
branch
from
6f65ebe to
0110bd1
Compare
|
Adding support for low power timer to enable low power modes