Enable new HAL us_ticker API on fast model MPS2 platform #7175

jamesbeyond · 2018-06-08T10:34:57Z

Description

Fast Model targets didn't implement the new HAL us_ticker API.

This PR is about modify fast models ticker implementation to match with recently merged HAL ticker branch #7009

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

CC @mprse @c1728p9 @bulislaw

0xc0170

LGTM but from the code changes, us ticker and drivers changes do not seem to be related , or are they?

How the PRESCALE Msk is related to enablement? I would do two commits, one in the driver and then in HAL implementation

0xc0170 · 2018-06-08T11:07:07Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/TARGET_FVP_MPS2_M0/device/CMSDK_CM0.h

@@ -270,7 +270,7 @@ typedef struct {
 #define CMSDK_DUALTIMER1_CTRL_INTEN_Msk      (0x1ul << CMSDK_DUALTIMER1_CTRL_INTEN_Pos)       /* CMSDK_DUALTIMER1 CTRL_INTEN: CTRL Int Enable Mask */

 #define CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos   2                                                /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Position */
-#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk   (0x3ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos)    /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */
+#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk   (0x1ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos)    /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */


In the review comment , I was referring to this line. I can see it is needed for us ticker but still looks like 2 step action . Editing sdk (for whatever reason - wrong definitions, updating to the latest or something that is not clear from this commit) and second updating us ticker implementation based on the SDK update (header files here).

OK, I kind of agree it is may treat as 2 step action.
I would revert the changes about SDK, which mostly about comments not accurate.
I'll raise a separate PR if required.

ithinuel

Can you provide some clarification on these points please ?

ithinuel · 2018-06-08T11:09:14Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/TARGET_FVP_MPS2_M0/device/CMSDK_CM0.h

@@ -270,7 +270,7 @@ typedef struct {
 #define CMSDK_DUALTIMER1_CTRL_INTEN_Msk      (0x1ul << CMSDK_DUALTIMER1_CTRL_INTEN_Pos)       /* CMSDK_DUALTIMER1 CTRL_INTEN: CTRL Int Enable Mask */

 #define CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos   2                                                /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Position */
-#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk   (0x3ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos)    /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */
+#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk   (0x1ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos)    /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */


This reduces the field size from 2 bits to 1.
What are doing the bits in position 3 and 4 ?

bit 4 is an undefined bit, bit 3 is another prescale bit, maybe I should keep the SDK definition as it was. Anyway, I will revert it based on Martin's comments. I will raise another PR to change SDK.

Indeed, if the field is 2 bits wide this mask should stay at 0x3ul.
To set the value in the register you should write

US_TICKER_TIMER1->TimerControl &= ~CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk; US_TICKER_TIMER1->TimerControl |= 0x1 << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos; // you can replace 0x1 by any value in [0..3]

ithinuel · 2018-06-08T11:10:42Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

-    US_TICKER_TIMER2->TimerControl |= 0x80; // enable counter
+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_MODE_Msk;     // set TIMER2 periodic mode
+
+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_ONESHOOT_Msk; // set TIMER2 one-shot mode


CMSDK_DUALTIMER2_CTRL_ONESHOOT_Msk should probably be CMSDK_DUALTIMER2_CTRL_ONESHOT_Msk

Yes, that's a typo in SDK, will change that

ithinuel · 2018-06-08T11:11:52Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c


-    US_TICKER_TIMER1->TimerControl |= 0x80; // enable counter
-    US_TICKER_TIMER2->TimerControl |= 0x80; // enable counter
+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_MODE_Msk;     // set TIMER2 periodic mode


Is configuring the US_TICKER_TIMER2 as periodic & one-shot intended ? It sound a bit opposite.

@jamesbeyond explained that periodic actually means that the timer will come back to the TimerLoad value when it reaches 0 after triggering an interrupt else the mode is said free running.
One-shot means that the timer will get disabled after reaching 0 and/or triggering the interrupt.

mprse · 2018-06-08T11:36:34Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_ONESHOOT_Msk; // set TIMER2 one-shot mode
+
+    US_TICKER_TIMER1->TimerControl |= CMSDK_DUALTIMER1_CTRL_EN_Msk; // enable TIMER1 counter
+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_EN_Msk; // enable TIMER2 counter


According to the ticker requirements interrupts should be disabled after init, so I suggest to disable US_TICKER_TIMER2 (I assume that it is used to generating interrupts) and disable interrupts instead of enabling them:
NVIC_DisableIRQ(US_TICKER_TIMER_IRQn);

Ticker interrupts are now disabled, but I think there is no need to enable TIMER2 here. It should be enabled while setting the interrupt.

The interrupts and counters ticking controlled by different bits.
Since the interrupts control bit is off. and let the TIMER2 ticking going not having any bad impacts to the HAL ticker behavior in the current implementation.
I can set TIMER2 to NOT ticking by default, reduce the ambiguity.

mprse · 2018-06-08T11:38:19Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

+    us_ticker_inited = 1;
+}
+
+void us_ticker_free(void)


We don't have requirements for us_ticker_free but I think it should also disable ticker interrupt.

mprse · 2018-06-08T11:38:53Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

 }

 uint32_t us_ticker_read()
 {
-    uint32_t return_value = 0;
    if (!us_ticker_inited) {


This code is redundant and should be removed.

mprse · 2018-06-08T11:39:29Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

@@ -61,12 +87,10 @@ void us_ticker_set_interrupt(timestamp_t timestamp)
        us_ticker_init();


This code is redundant and should be removed.

So you mean it is not necessary to check the flag, we just need to call us_ticker_init() directly?

No, I was thinking about entire block. Upper layer handles initialization, so it will call init only once on first ticker usage.

I can see HAL API claimed that as undefined behavior, I will remove the function of checking flags

mprse · 2018-06-08T11:45:22Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

-    US_TICKER_TIMER1->TimerLoad = (delta) * 25; //initialise the timer value
-    US_TICKER_TIMER1->TimerControl |= 0x80; //enable timer
+    uint32_t delta =  timestamp - us_ticker_read();
+    US_TICKER_TIMER2->TimerLoad = delta;                               // Set TIMER2 load value


Can we safety change timer value while timer is running?

I think we should also consider case when delta is equal to 0. Will interrupt be fired?

In the current implementation, if 0 is set, interrupt will be fired.
I didn't find clear definition HAL API what happen if current_time is set to interrupt. but seems This line indicates a interrupt will need to be fired

mprse · 2018-06-08T11:47:06Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

-    US_TICKER_TIMER1->TimerControl &= 0xDF;
-    US_TICKER_TIMER2->TimerControl &= 0xDF;
-
+    US_TICKER_TIMER2->TimerControl &= ~CMSDK_DUALTIMER2_CTRL_INTEN_Msk;


I think we should also disable ticker interrupt, not only TIMER2:
NVIC_DisableIRQ(US_TICKER_TIMER_IRQn);

NVIC_DisableIRQ() added

jamesbeyond · 2018-06-11T11:05:01Z

Hi all, the code had been changed based on your comments, please review again @0xc0170 @ithinuel @mprse

0xc0170 · 2018-06-12T08:15:00Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

 int us_ticker_inited = 0;

 void us_ticker_init(void)
 {
    if (us_ticker_inited) {
+        us_ticker_disable_interrupt();


Is this what targets do? I dont recall seeing it before. init should disable interrupt , but in this case, why only if it was already initialized?

why us_ticker_inited variable is not static?

Code implements the following ticker requirement:

mbed-os/hal/us_ticker_api.h

Line 64 in ed9a1f1

* * The function ticker_init allows the ticker to keep counting and disables the ticker interrupt - Verified by test ::ticker_init_test

Yes, calling us_ticker_disable_interrupt() on an uninitialized ticker is an undefined behavior in HAL
I re-checked, upper level dirvers are not use this variable, it can be staticed, good catch.

mprse · 2018-06-15T10:05:26Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c


-    US_TICKER_TIMER1->TimerControl |= 0x80; // enable counter
-    US_TICKER_TIMER2->TimerControl |= 0x80; // enable counter
+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_MODE_Msk;     // set TIMER2 periodic mode


If periodic mode means that counter after reaching 0 is reloaded and continue counting, then it looks like this mode should be used for TIMER1 which is dedicated to count elapsed time.

As I previously mentioned in Wilfried's comment:
Based on SSE-200 TRM
the timer can be set in 2 main modes:
Free-running mode: which not triggers interrupts, and wraps counter form the MAX.
Periodic mode: generates interrupts when it reaches 0, and reload from TIMERLOAD value.

So this case, we use TIMER1 in the free-running mode for monitoring time eclipsing, and TIME2 in periodic mode, and reset TIMERLOAD when we set the interrupt.

maybe I should be put these as comments in the code, seem just reading from the mode name, it is a bit misleading.

mprse

Minor changes requested.

mprse · 2018-06-15T10:10:07Z

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/us_ticker.c

+    uint32_t delta =  timestamp - us_ticker_read();
+    US_TICKER_TIMER2->TimerControl  &= ~CMSDK_DUALTIMER2_CTRL_EN_Msk;  // disable TIMER2
+    US_TICKER_TIMER2->TimerLoad = delta;                               // Set TIMER2 load value
+    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_INTEN_Msk; // enable TIMER2 interrupt


I suggest to enable timer interrupts only if they are disabled.

So you mean calling NVIC_EnableIRQ() before Enable TIMER2 counter?

Something like:

if (US_TICKER_TIMER2->TimerControl & CMSDK_DUALTIMER2_CTRL_INTEN_Msk== 0) { US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_INTEN_Msk; NVIC_EnableIRQ(US_TICKER_TIMER_IRQn); }

So is there any particular reasons for this?
what are the drawbacks for setting interrupt regardless of the current status?

I was thinking about the case where an interrupt been set, but not timeout yet, another interrupt got set again. Or where NVIC got enabled for the TIMER_IRQ, but TimerControl interrupts BIT was not set.

I was just thinking about optimisation (increase performance). NVIC_EnableIRQ() doesn't need to be called every time the interrupt is set.

A write only operation would be probably faster than a "read/check" or "read/check/write".

I think there is no guarantee that CMSDK_DUALTIMER2_CTRL_INTERUPT bit and NVIC_EnableIRQ() are in the same states (outside this driver code), so set NVIC_EnableIRQ() regardless CMSDK_DUALTIMER2_CTRL_INTERUPT would be "safer"?

A write only operation would be probably faster than a "read/check" or "read/check/write".

This is only suggestion and I didn't checked that. Maybe you are right, but please note there is an extra call to NVIC_EnableIRQ() function. Additionally there is no point to do things which are already done. This function might be called very often when we have Ticker objects with small periods (us).

jamesbeyond · 2018-06-15T15:30:33Z

Hi @mprse, your first two comments were addressed and waiting for your feedback on the third one.

mprse

Looks good!

cmonr · 2018-06-19T01:11:28Z

@ithinuel @0xc0170 All good with the changes?

0xc0170 · 2018-06-20T12:50:51Z

/morph build

mbed-ci · 2018-06-20T14:35:21Z

Build : SUCCESS

Build number : 2400
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7175/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-06-20T16:17:42Z

Exporter Build : SUCCESS

Build number : 2035
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7175/

mbed-ci · 2018-06-20T17:29:57Z

Test : SUCCESS

Build number : 2186
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7175/2186

Enable new HAL us_ticker API on fast model MPS2 platform

ad668a7

jamesbeyond force-pushed the fm_mbed_hal branch from 210fa2b to ad668a7 Compare June 8, 2018 10:39

0xc0170 requested a review from a team June 8, 2018 11:02

0xc0170 added the needs: review label Jun 8, 2018

0xc0170 previously approved these changes Jun 8, 2018

View reviewed changes

0xc0170 reviewed Jun 8, 2018

View reviewed changes

ithinuel reviewed Jun 8, 2018

View reviewed changes

mprse reviewed Jun 8, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jun 8, 2018

update us_ticker driver and revert CMSDK headers changes

810d534

jamesbeyond dismissed 0xc0170’s stale review via 810d534 June 11, 2018 10:49

add disable interrupt function in us_ticker_free()

0c8527f

cmonr added needs: review and removed needs: work labels Jun 11, 2018

0xc0170 reviewed Jun 12, 2018

View reviewed changes

change us_ticker_inited to static

3ba635d

mprse reviewed Jun 15, 2018

View reviewed changes

mprse suggested changes Jun 15, 2018

View reviewed changes

add comments about TIMER modes and set TIMER2 off by default

4324258

mprse approved these changes Jun 18, 2018

View reviewed changes

0xc0170 approved these changes Jun 20, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jun 20, 2018

cmonr added the release-version: 5.9.2 label Jun 20, 2018

cmonr merged commit c4113ae into ARMmbed:master Jun 20, 2018

0xc0170 removed the needs: CI label Jun 20, 2018

		@@ -61,12 +87,10 @@ void us_ticker_set_interrupt(timestamp_t timestamp)
		us_ticker_init();

Enable new HAL us_ticker API on fast model MPS2 platform #7175

Enable new HAL us_ticker API on fast model MPS2 platform #7175

Uh oh!

Conversation

jamesbeyond commented Jun 8, 2018

Description

Pull request type

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ithinuel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesbeyond commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse left a comment

Choose a reason for hiding this comment

mprse Jun 8, 2018 •

edited

Loading

mprse Jun 18, 2018 •

edited

Loading