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.

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

MIMXRT1050EVK: Fix Sleep support #7419

Merged
merged 4 commits into from Aug 8, 2018

Conversation

Projects
None yet
6 participants
@mmahadevan108
Contributor

mmahadevan108 commented Jul 4, 2018

Description

Add support for SLEEP in MXRT1050EVK. Support for low power modes on MXRT requires
1. Updates to the CCM LPCR register to set the power mode
2. Enabling wakeup source in the GPC module
3. Ensuring the low power functions related to powering off
FLEXx SPI and SDRAM are executed from internal memory
4. Save PLL state before disabling them
5. Restore PLL state on resume

All tests passed under GCC_ARM and IAR toolchains.

With ARM toolchain, all tests passed except I see the below green-tea crash when running mbed-os-tests-mbed_hal-lp_ticker test. I have debugged this and the test is passing, however it crashes on exit. Also this happens only with ARM toolchain. Any ideas??


mbedhtrun: mbedhtrun -m MIMXRT1050_EVK -p COM50:9600 -f "BUILD/tests/MIMXRT1050_EVK/ARM/mbed-os/TESTS/mbed_hal/lp_ticker/lp_ticker.bin" -e "mbed-os\TESTS\host_tests" -d D: -c default -t 0227000041114e450004300ac2040020df91000097969900 -r default -C 4 --sync 5 -P 60
mbedgt: mbed-host-test-runner: started
[1530709453.47][HTST][INF] host test executor ver. 1.3.1
[1530709453.47][HTST][INF] copy image onto target...
[1530709453.47][COPY][INF] Waiting up to 60 sec for '0227000041114e450004300ac2040020df91000097969900' mount point (current is 'D:')...
1 file(s) copied.
[1530709463.43][HTST][INF] starting host test process...
[1530709465.57][CONN][INF] starting connection process...
[1530709465.57][CONN][INF] notify event queue about extra 60 sec timeout for serial port pooling
[1530709465.57][CONN][INF] initializing serial port listener...
[1530709465.57][PLGN][INF] Waiting up to 60 sec for '0227000041114e450004300ac2040020df91000097969900' serial port (current is 'COM50')...
[1530709465.57][HTST][INF] setting timeout to: 60 sec
[1530709465.66][SERI][INF] serial(port=COM50, baudrate=9600, read_timeout=0.01, write_timeout=5)
[1530709465.66][SERI][INF] reset device using 'default' plugin...
[1530709465.92][SERI][INF] waiting 1.00 sec after reset
[1530709466.92][SERI][INF] wait for it...
[1530709466.92][SERI][TXD] mbedmbedmbedmbedmbedmbedmbedmbedmbedmbed
[1530709466.92][CONN][INF] sending up to 5 __sync packets (specified with --sync=5)
[1530709466.92][CONN][INF] sending preamble 'c550c4f6-142d-41dd-8848-ac685a497706'
[1530709466.92][SERI][TXD] {{__sync;c550c4f6-142d-41dd-8848-ac685a497706}}
[1530709467.06][CONN][RXD] mbedmbedmbedmbedmbedmbedmbedmbed
[1530709467.10][CONN][INF] found SYNC in stream: {{__sync;c550c4f6-142d-41dd-8848-ac685a497706}} it is #0 sent, queued..
.
[1530709467.11][HTST][INF] sync KV found, uuid=c550c4f6-142d-41dd-8848-ac685a497706, timestamp=1530709467.102000
[1530709467.12][CONN][INF] found KV pair in stream: {{__version;1.3.0}}, queued...
[1530709467.13][HTST][INF] DUT greentea-client version: 1.3.0
[1530709467.15][CONN][INF] found KV pair in stream: {{__timeout;20}}, queued...
[1530709467.15][HTST][INF] setting timeout to: 20 sec
[1530709467.18][CONN][INF] found KV pair in stream: {{__host_test_name;default_auto}}, queued...
[1530709467.18][HTST][INF] host test class: '<class 'mbed_host_tests.host_tests.default_auto.DefaultAuto'>'
[1530709467.18][HTST][INF] host test setup() call...
[1530709467.18][HTST][INF] CALLBACKs updated
[1530709467.18][HTST][INF] host test detected: default_auto
[1530709467.21][CONN][RXD] >>> Running 3 test cases...
[1530709467.26][CONN][INF] found KV pair in stream: {{__testcase_name;lp ticker info test}}, queued...
[1530709467.30][CONN][INF] found KV pair in stream: {{__testcase_name;lp ticker sleep test}}, queued...
[1530709467.35][CONN][RXD]
[1530709467.35][CONN][INF] found KV pair in stream: {{__testcase_name;lp ticker glitch test}}, queued...
[1530709467.39][CONN][RXD] >>> Running case #1: 'lp ticker info test'...
[1530709467.43][CONN][INF] found KV pair in stream: {{__testcase_start;lp ticker info test}}, queued...
[1530709467.48][CONN][INF] found KV pair in stream: {{__testcase_finish;lp ticker info test;1;0}}, queued...
[1530709467.53][CONN][RXD] >>> 'lp ticker info test': 1 passed, 0 failed
[1530709467.53][CONN][RXD]
[1530709467.58][CONN][RXD] >>> Running case #2: 'lp ticker sleep test'...
Exception in thread Thread-3:
Traceback (most recent call last):
File "c:\python27\lib\threading.py", line 801, in __bootstrap_inner
self.run()
File "c:\python27\lib\threading.py", line 754, in run
self.__target(*self.__args, **self.__kwargs)
File "c:\python27\lib\site-packages\mbed_greentea\mbed_greentea_cli.py", line 489, in run_test_thread
verbose=verbose)
File "c:\python27\lib\site-packages\mbed_greentea\mbed_test_api.py", line 318, in run_host_test
returncode, htrun_output = run_htrun(cmd, verbose)
File "c:\python27\lib\site-packages\mbed_greentea\mbed_test_api.py", line 134, in run_htrun
htrun_output += line.decode('utf-8')
File "c:\python27\lib\encodings\utf_8.py", line 16, in decode
return codecs.utf_8_decode(input, errors, True)
UnicodeDecodeError: 'utf8' codec can't decode byte 0xa1 in position 28: invalid start byte

mbedgt: could not generate test report
mbedgt: completed in 20.36 sec
mbedgt: exited with code -1000
[mbed] ERROR: "mbedgt" returned error code -1000.
[mbed] ERROR: Command "mbedgt --test-spec C:\mymbed\BUILD\tests\MIMXRT1050_EVK\ARM\test_spec.json -n mbed-os-tests-mbed_
hal-lp_ticker -V" in "C:\mymbed"


Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Jul 4, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 4, 2018

With ARM toolchain, all tests passed except I see the below green-tea crash when running mbed-os-tests-mbed_hal-lp_ticker test. I have debugged this and the test is passing, however it crashes on exit. Also this happens only with ARM toolchain. Any ideas??

Is this connected to this changeset?

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal Jul 4, 2018

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Jul 4, 2018

I have looked closely at this changeset and cannot understand the cause of test framework crash. Can someone look at the green-tea failure log.

@mprse

This comment has been minimized.

Member

mprse commented Jul 5, 2018

It looks like deep-sleep test fails. In the test there is a special delay used before entering deep-sleep, so the green tea transmission can be successfully finished:

wait_cycles(400000);

Maybe it is not sufficient for your target on ARM compiler and needs to be increased. Can you try to increase the delay?

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Jul 5, 2018

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Jul 5, 2018

@mprse Thank you for your suggestion. I made the below change and don't see the crash anymore.

index 8c40e9c66..9b3f731bc 100644
--- a/TESTS/mbed_hal/lp_ticker/main.cpp
+++ b/TESTS/mbed_hal/lp_ticker/main.cpp
@@ -98,7 +98,7 @@ void lp_ticker_deepsleep_test()
     lp_ticker_init();

     /* Wait for green tea UART transmission before entering deep-sleep mode. */
-    wait_cycles(400000);
+    wait_cycles(1000000);

     overflow_protect();

@@ -128,6 +128,9 @@ void lp_ticker_glitch_test()
     uint32_t last = lp_ticker_read();
     const uint32_t start = last;

+    /* Give sometime for the counter to tick */
+    wait_cycles(1000000);
+
     while (last < (start + TICKER_GLITCH_TEST_TICKS)) {
         const uint32_t cur = lp_ticker_read();
         TEST_ASSERT(cur >= last);
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 9, 2018

Maybe it is not sufficient for your target on ARM compiler and needs to be increased. Can you try to increase the delay?

@mprse How shall we address this issue?

@mprse

This comment has been minimized.

Member

mprse commented Jul 9, 2018

@mprse How shall we address this issue?

This is workaround commonly used in tests. Since the required delay is target specific I had to increase the delay few times already.
Many tests use this approach and waits before entering deep-sleep mode (I can't see other option), but I think we should consider more elegant solution (some green tea functionality), so it can be handled properly.
But for the moment we should simply increase the delay. Shell I create the PR? @mmahadevan108 is 1000000 value sufficient?

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Jul 12, 2018

@mprse @mmahadevan108

I'm implementing LP Ticker feature for GR-PEACH and GR-LYCHEE, but I faced this issue(lp ticker sleep test is fail) when using ARMCC toolchain.
Same as the above, It seems that a special delay used before entering deep-sleep is short for my boards.

But for the moment we should simply increase the delay. Shell I create the PR? @mmahadevan108 is 1000000 value sufficient?

I will appreciate if you could create this PR.

@mprse

This comment has been minimized.

Member

mprse commented Jul 12, 2018

I will appreciate if you could create this PR.

Fix can be found here: PR #7494.

Please let me know if this fix solves the issue.

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Jul 13, 2018

@mprse

Fix can be found here: PR #7494.

Please let me know if this fix solves the issue

Thank you. Your PR solved my issue.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 13, 2018

Depends on #7494 .

@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Add_MXRT_Sleep_Support branch from 83e53f1 to aefae1b Aug 1, 2018

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Aug 1, 2018

Apologies for the delay, I have rebased this PR. I still see an issue with the glitch test, below change fixes it, could you please take a look.

--- a/TESTS/mbed_hal/lp_ticker/main.cpp
+++ b/TESTS/mbed_hal/lp_ticker/main.cpp
@@ -150,7 +150,12 @@ void lp_ticker_glitch_test()
     uint32_t last = lp_ticker_read();
     const uint32_t start = last;

     while (last < (start + TICKER_GLITCH_TEST_TICKS)) {
+        /* Wait a while - let the ticker to count. */
+        busy_wait_ms(SERIAL_FLUSH_TIME_MS);
+
         const uint32_t cur = lp_ticker_read();
         TEST_ASSERT(cur >= last);
         last = cur;
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 2, 2018

Apologies for the delay, I have rebased this PR. I still see an issue with the glitch test, below change fixes it, could you please take a look.

@mprse Please review

@mprse

This comment has been minimized.

Member

mprse commented Aug 2, 2018

@mmahadevan108 This test checks if lp ticker does not glitch backwards due to an incorrectly implemented ripple counter driver.
As far as I can see you added the delay in the while loop, so mainly you have reduced number of attempts when test is looking for the glitch. This is not the right way. The delay is only needed before going into deep-sleep. It looks like there is some problem with lp ticker driver. I see that lp ticker is based on 32 bit counter and operates at 32 kHz. So the rollover is likely not possible in this case. Could you please provide the test output with the failure?

Additionally please check the pseudo code implementation of lp_ticker_read() function which is checked by the failing test case:

mbed-os/hal/lp_ticker_api.h

Lines 131 to 158 in f9862b8

/** Read the current tick
*
* If no rollover has occurred, the seconds passed since lp_ticker_init()
* was called can be found by dividing the ticks returned by this function
* by the frequency returned by ::lp_ticker_get_info.
*
* @return The current timer's counter value in ticks
*
* Pseudo Code:
* @code
* uint32_t lp_ticker_read()
* {
* uint16_t count;
* uint16_t last_count;
*
* // Loop until the same tick is read twice since this
* // is ripple counter on a different clock domain.
* count = LPTMR_COUNT;
* do {
* last_count = count;
* count = LPTMR_COUNT;
* } while (last_count != count);
*
* return count;
* }
* @endcode
*/
uint32_t lp_ticker_read(void);

As you can see counter is read in loop until the same value is read twice. Looking into implementation of lp_ticker_read() it simply returns counter value. Not sure if this is the case, but I think it is worth to check:

uint32_t lp_ticker_read(void)
{
return GPT_GetCurrentTimerCount(GPT2);
}

@mprse

I see that this PR not only changes implementation of sleep functions and enables sleep support in targets.json, but also adds some files, changes linker scripts, fixing codding style, etc. This is all done in one commit and it is hard to understand the changes. Could you please split this commit and add some note to describe the purpose of the changes.

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Aug 2, 2018

All changes in this PR are related to adding sleep support on MRT1050

The sleep implementation on MXRT is a bit more involved than Kinetis and LPC devices. The changes to the linker script maps memory that stays active during DEEP SLEEP to hold certain functions.

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Aug 2, 2018

@mprse @0xc0170 . Apologies for the earlier report. I was testing on a wrong version of the board.

The lp ticker test passes with the current implementation.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 3, 2018

@mmahadevan108 Can you add that explanation to the commit ? How the commit fixes it (what problem there was and how it is addressed)

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 3, 2018

@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Add_MXRT_Sleep_Support branch from aefae1b to 33e6ac9 Aug 3, 2018

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Aug 3, 2018

@0xc0170 . Done, update commit message to include more information.

@0xc0170

0xc0170 approved these changes Aug 6, 2018 edited

Update below in the next review

@0xc0170

0xc0170 requested changes Aug 6, 2018 edited

It would be better if this is separated to more commits as @mprse requested earlier. One commit changes multiple files for multiple reasons with one common goal - fix sleep.

Now the commit adds more details for fixing, it's more clear to me what is being changed however took me a while to go through changes.

Commits as follows (as I understand changes from this pull request - for illustration how it could be done):

  • add lpm init (why are we adding lpm files, is this new driver update?)
  • update sdk init: add copy function (what is being copied ? this requires some linker script changes?)
  • SKIP_SYSCLK_INIT addition (why is this needed now ?)
  • sleep: add pre/post processing steps
  • enable sleep

Note: my previous review was supposed to be needs work rather than approval, as I can't change it, adding this new one

mmahadevan108 added some commits Aug 6, 2018

MIMXRT1050_EVK: Add Low Power Manager files
This is needed to support different Low-Power modes available
in MXRT1050

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
MXRT1050_EVK: Ensure certain low power function are linked to interna…
…l memory

Low power functions related to powering off FLEXSPI and SDRAM needs
to be copied to internal memory

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
MXRT1050_EVK: Sleep: add pre/post processing steps
Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
MXRT1050_EVK: Enable Sleep function and add SKIP_SYSCLK_INIT macro
SKIP_SYSCLK_INIT prevents reinitializing the SYS_CLK PLL used by SDRAM.
This PLL is setup during bootup by the ROM code.

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>

@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Add_MXRT_Sleep_Support branch from 33e6ac9 to 9cf2b76 Aug 6, 2018

@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Aug 6, 2018

The PR has been broken to smaller commits as suggested earlier.

@0xc0170

0xc0170 approved these changes Aug 7, 2018

@mmahadevan108 Thanks ! Looks good , much easier to review and understand changes

@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 7, 2018

@mprse

mprse approved these changes Aug 8, 2018

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 8, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 8, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 8, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 8, 2018

@cmonr cmonr merged commit d360da9 into ARMmbed:master Aug 8, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 595 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8971 cycles (-836 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Aug 8, 2018

@mmahadevan108 mmahadevan108 deleted the NXPmicro:Add_MXRT_Sleep_Support branch Aug 8, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment