drivers: counter: lptmr: Fix get_pending_int wrong implementation #99784
Conversation
zephyrbot
requested review from
Holt-Sun,
dbaluta,
decsny,
dleach02,
manuargue,
mmahadevan108 and
nordic-krch
drivers/counter/counter_mcux_lptmr.c
Outdated
| .mode = DT_INST_PROP(n, timer_mode_sel), \ | ||
| .pin = DT_INST_PROP_OR(n, input_pin, 0), \ | ||
| .polarity = DT_INST_PROP(n, active_low), \ | ||
| .free_running = DT_INST_PROP(n, free_running), \ |
There was a problem hiding this comment.
You used DT_INST_PROP(n, free_running) which requires the property to exist, but binding does not mark free-running as required, builds will fail if older DTS files omit it.
| .free_running = DT_INST_PROP(n, free_running), \ | |
| .free_running = DT_INST_PROP_OR(n, free_running, 0), \ |
There was a problem hiding this comment.
boolean property default false, so there is no build issue if this property is not set.
ZhaoxiangJin
force-pushed
the
add-dt-property-for-free-running
branch
from
865840c to
b596705
Compare
|
Hi, I am not sure about the purpose, from the counter driver model, my understanding is the peripheral counter should be reset when reached the top value. What is the purpose of free-running mode? |
When reaching the top, the API expects "can be reset and optional callback is periodically called", which doesn't seem to mandate counter reset. Enabling free-running mode is one of the solutions. The fundamental solution is to fix the defect in the systick driver. Although I will ultimately use the latter, the former is still an improvement for the lptmr driver. |
Hi, I would like to ask @nordic-krch 's help to understand the expected behavior. Thanks |
|
similar as #95143? |
Seems he is doing something in timer/lptmr, I am not intend to use lptmr for system timer, using lptmr as system timer in all power modes means we should use FRO16K as clock source, but the FRO16K is inaccurate. |
drivers/counter/counter_mcux_lptmr.c
Outdated
| flags = LPTMR_GetStatusFlags(config->base); | ||
|
|
||
| return ((flags & mask) == mask); | ||
| return return (uint32_t)!!LPTMR_GetStatusFlags(config->base); |
There was a problem hiding this comment.
TCF(Timer Compare Flag) is set when counter equals compared value even if TIE(Timer Interrupt Enable) is not set.
Since this API is to get pending interrupt, so I think it's necessary to check TIE:
return (config->base->CSR & mask) == mask;
There was a problem hiding this comment.
you are correct, i have updated the patch
ZhaoxiangJin
force-pushed
the
add-dt-property-for-free-running
branch
from
349308c to
3a3a9f8
Compare
|
Hello @Holt-Sun, @FelixWang47831, any comments regarding this topic? Thanks. |
See my comment: #100081 (comment) |
Yes, top feature allows to set lower value at which counter will reset. In implementations that i did i actually use one of the compare channels to set that. If not set then counter resets after reaching it's HW top value. |
So in reality, whether to reset after reaching top is decided by the driver itself rather than being mandated by the API. |
|
Add Holt as the PR assignee since he is the NXP counter expert and this PR only changes the NXP driver part. |
|
Whether the counter will reset or not after reaching top is an important behavior, generally the behaviors should be defined clearly in spec, otherwise when application migrate from one platform to another, the function will be broken.
|
| flags = LPTMR_GetStatusFlags(config->base); | ||
|
|
||
| return ((flags & mask) == mask); | ||
| return (uint32_t)!!((config->base->CSR & mask) == mask); |
There was a problem hiding this comment.
this doesn't match the commit message description:
The correct behavior is to check only the TCF
(Timer Compare Flag) to see if an interrupt is pending,
regardless of the TIE (Timer Interrupt Enable) status.
This commit updates the function to return the TCF flag
directly.
There was a problem hiding this comment.
commit message updated.
ZhaoxiangJin
force-pushed
the
add-dt-property-for-free-running
branch
from
3a3a9f8 to
a2f16a0
Compare
I think it's strange to assume that software should restrict counter hardware from performing a hardware wrap when reaching top. Do all counter hardware implementations even have this feature? I think what we need to define is: counting is free-running modulo the top value, and boundary events should be represented through callbacks/guard periods. |
I don't recall such assumption. Counter API assumes that counter always wraps after reaching top. There is the flag |
Originally, function 'mcux_lptmr_get_pending_int' checks if both TCF and TIE flags are set to determine if an interrupt is pending. But function 'LPTMR_GetStatusFlags' only returns the status flags, that is we will never get TIE flag from it. then the function always returns false. The correct approach is to read the CSR register directly and check if both TIE and TCF bits are 1. If yes, there's a pending interrupt; if not, there isn't. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
ZhaoxiangJin
force-pushed
the
add-dt-property-for-free-running
branch
from
a2f16a0 to
636fca7
Compare
ZhaoxiangJin
changed the title
|
9 participants
Originally, function 'mcux_lptmr_get_pending_int' checks if both
TCF and TIE flags are set to determine if an interrupt is pending.
But function 'LPTMR_GetStatusFlags' only returns the status flags,
that is we will never get TIE flag from it. then the function
always returns false.
The correct approach is to read the CSR register directly and check
if both TIE and TCF bits are 1. If yes, there's a pending interrupt;
if not, there isn't.