Nuvoton: Adhere to reworked ticker spec to release with Mbed OS 5.9 #7029
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Greentea test (NUC472/ARM) |
Greentea test (M453/ARM) |
Greentea test (M487/ARM) |
Greentea test (NANO130/ARM) |
|
On all Nuvoton targets, void lp_ticker_disable_interrupt(void)
{
TIMER_DisableInt((TIMER_T *) NU_MODBASE(TIMER_MODINIT.modname));
wait_us((NU_US_PER_SEC / NU_TMRCLK_PER_SEC) * 3);
}
|
ccli8
changed the title
|
Same as for RTC branch (my latest comment there) , this will be redirected to master once feature branch is integrated |
|
@0xc0170 I've done rebase onto master and change merge target branch to master. |
|
@bulislaw Another HAL PR to check. |
|
@bulislaw #6536 (LPTICKER_DELAY_TICKS) doesn't help. Nuvoton's situation gets worse. In Nuvoton's |
|
@c1728p9 how suitable would be to extend what we have to cover all cases? |
These APIs have write access to Timer registers. Anyway, I will seek another means to make these APIs non-blocking. Besides, I have questions about these ticker APIs:
|
This is up to you @ccli8. Whichever is more straight forward for your to implement. The only requirement is that once
The NVIC interrupt should get cleared automatically when you return from the given interrupt handler, so you should only need to clear the timer flag. |
NUC472/ARM |
M453/ARM |
M487/ARM |
NANO130/ARM |
1. Introduce S/W interrupt enable/disable to reduce calls to TIMER_EnableInt/TIMER_DisableInt. 2. Allow dummy interrupt because clear interrupt flag is not synchronized. 3. Enable LPTICKER_DELAY_TICKS to make lp_ticker_set_interrupt non-blocking.
This is to make us_ticker/lp_ticker code consistent.
This is to make us_ticker/lp_ticker code consistent.
ccli8
force-pushed
the
nuvoton_5.9_ticker
branch
from
966040e
to
310a1fe
Compare
|
Do rebase on master to fix conflict |
|
Hi @ccli8 I took an in-depth look into this on the NUMAKER_PFM_M453. I was unable to reproduce the problems you describe with requiring a delay to clear the interrupt. I used GPIO to record the timing of A screenshots of the timing can be seen below. None of the functions take more than 10us to execute. Test code used for this screenshot and for the testing above can be found on the lp_timer_test_code branch of my mbed-os repo.
Further I looked through the Reference Manual but didn't see any information about synchronization delays when using the TMR module. Could this already be handled by hardware? Summary - @ccli8 would it be possible to remove the special handling needed for |
It is because dummy interrupt is very rare or pending time caused by it is very short.
This is to make us_ticker/lp_ticker code consistent.
|
@c1728p9 Many thanks for your in-depth look. Re-checking with our designer, I have to fix my point. For LXT/LIRC-clocked Timer (lp_ticker), there are synchronization issues with register control. Before that, I have to clarify two clock domains:
|
c1728p9
reviewed
Jun 28, 2018
| TIMER_ClearIntFlag((TIMER_T *) NU_MODBASE(TIMER_MODINIT.modname)); | ||
| wait_us((NU_US_PER_SEC / NU_TMRCLK_PER_SEC) * 3); | ||
| /* To avoid sync issue, we clear TIF/TWKF simultaneously rather than call separate | ||
| * driver API: |
There was a problem hiding this comment.
Since this is on the PCLK domain does writing these separately cause issues? If not you could revert this change.
There was a problem hiding this comment.
@c1728p9 TIMER_ClearIntFlag/TIMER_ClearWakeupFlag are so close. It is safer to clear these interrupt flags altogether.
|
|
||
| /* Disable wakeup */ | ||
| TIMER_DisableWakeup(timer_base); | ||
| wait_us((NU_US_PER_SEC / NU_TMRCLK_PER_SEC) * 3); |
There was a problem hiding this comment.
Is this wait_us needed? Same question for the wait_us below.
There was a problem hiding this comment.
@c1728p9 All these wait_us are necessary.
| * It is very rare that we would meet dummy interrupt and get stuck in ISR until | ||
| * 'clear interrupt flag' takes effect. The issue is ignorable because the pending | ||
| * time is very short (at most 1 dummy interrupt). We won't take special handling for it. | ||
| */ |
There was a problem hiding this comment.
@c1728p9 Re-thanks for your detailed review.
c1728p9
approved these changes
Jun 29, 2018
|
/morph build |
cmonr
reviewed
Jun 30, 2018
| @@ -52,6 +52,7 @@ unsigned int ticker_overflow_delta; | |||
|
|
|||
| /* Auxiliary function to count ticker ticks elapsed during execution of N cycles of empty while loop. | |||
| * Parameter <step> is used to disable compiler optimisation. */ | |||
| MBED_NOINLINE | |||
There was a problem hiding this comment.
Never mind. Commit b18f690 explained it perfectly.
|
/morph build |
Build : SUCCESSBuild number : 2485 Triggering tests/morph test |
1 similar comment
Build : SUCCESSBuild number : 2485 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2124 |
Test : SUCCESSBuild number : 2260 |
0xc0170
approved these changes
Jul 2, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR tries to adhere to reworked ticker spec to release with Mbed OS 5.9.
Targets
Pull request type