Skip to content
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

Support mbed_start_application for Cortex-M0+ #9791

Merged
merged 4 commits into from Feb 25, 2019

Conversation

Projects
None yet
10 participants
@sarahmarshy
Copy link
Contributor

commented Feb 21, 2019

Description

Add support for mbed_start_application for Cortex M0+ targets. This will enable M0+ targets to support bootloader applications.

Pull request type

[X ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@c1728p9

@donatieng
Copy link
Member

left a comment

LGTM

@@ -170,7 +177,11 @@ __asm static void start_new_application(void *sp, void *pc)
void start_new_application(void *sp, void *pc)
{
__asm volatile(
"movw r2, #0 \n" // Fail to compile "mov r2, #0" with ARMC6. Replace with MOVW.
#if defined(__CORTEX_M0PLUS)
"mov r2, #0 \n" // No MOVW instruction on Cortex-M0+

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 21, 2019

Member

interesting note that ARMC6 fails, does this compile for m0+ ?

This comment has been minimized.

Copy link
@sarahmarshy

sarahmarshy Feb 21, 2019

Author Contributor

I don't have armc6 to test. This was a comment from @ccli8. I also thought it was interesting, because this is part of the code path for IAR and GCC, not Arm compiler, so I don't know the significance of the comment. I left it as is in this PR.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 21, 2019

Member

I see, @ccli8 should that comment be removed or ?

This comment has been minimized.

Copy link
@ccli8

ccli8 Feb 22, 2019

Contributor

@sarahmarshy ARMC6 (not ARM) toolchain doesn't define __CC_ARM, but define __GNUC__, so ARMC6 will compile this code path.

@0xc0170 I re-compile this code path with ARMC6 (6.10) and GCC_ARM (2018/Q2), mov r2, #0 compiles failed only on ARMC6/M23 combination. So if the comment is to be left, it can change to more accurate like Fail to compile "mov r2, #0" with ARMC6/M23. Replace with MOVW.

#elif defined (__GNUC__) || defined (__ICCARM__)

void start_new_application(void *sp, void *pc)
{
    __asm volatile(
        "movw   r2, #0      \n" // Fail to compile "mov r2, #0" with ARMC6. Replace with MOVW.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 25, 2019

Member

Thanks @ccli8 . The latest update took care of it

@ciarmcom ciarmcom requested review from c1728p9 and ARMmbed/mbed-os-maintainers Feb 21, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

@sarahmarshy, thank you for your changes.
@c1728p9 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-core Feb 21, 2019

for (i = 0; i < 8; i++) {
NVIC->IP[i] = 0x00000000;
}
#else

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Feb 21, 2019

Contributor

As an alternative to having this as a separate block could you set isr_groups_32 = 1 for m0+ and reuse the code below?

This comment has been minimized.

Copy link
@sarahmarshy

sarahmarshy Feb 21, 2019

Author Contributor

Good point. I'll do that.

This comment has been minimized.

Copy link
@sarahmarshy

sarahmarshy Feb 21, 2019

Author Contributor

Fixed in latest commit 👍

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Feb 21, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 21, 2019

Test run: FAILED

Summary: 4 of 8 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@sarahmarshy Please review the build failure.

Was this changeset tested in IAR and ARM compilers?

@sarahmarshy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@cmonr, no, but it has been now. Should be fixed by latest commit 👍

@@ -158,7 +161,11 @@ static void powerdown_scb(uint32_t vtor)

__asm static void start_new_application(void *sp, void *pc)
{
#if defined(__CORTEX_M0PLUS)

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Feb 22, 2019

Contributor

It's best to just use MOVS unconditionally here - it's a 16-bit instruction instead of 32-bit (which is why the M0+ supports it). So you'll save ROM on all Cortex-Ms.

This comment has been minimized.

Copy link
@sarahmarshy

sarahmarshy Feb 22, 2019

Author Contributor

Good call. I've added this in my most recent commit.

platform/mbed_application.c Outdated
@@ -170,7 +173,7 @@ __asm static void start_new_application(void *sp, void *pc)
void start_new_application(void *sp, void *pc)
{
__asm volatile(
"movw r2, #0 \n" // Fail to compile "mov r2, #0" with ARMC6. Replace with MOVW.
"movs r2, #0 \n"

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Feb 22, 2019

Contributor

Glad that worked. (It's so fiddly getting inline assembler to go smoothly across all toolchains - especially as GCC refuses to use unified syntax for Thumb-1 devices. Actually a bit surprised it is accepting MOVS...)

The MOVT comment is no longer relevant with the MOVW gone.

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:m0-start-app branch Feb 22, 2019

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:m0-start-app branch to 3b98ebc Feb 22, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 22, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

started CI

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 24, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 2
Build artifacts

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

@0xc0170 does this need any other reviews before merge?

@0xc0170 0xc0170 merged commit 5f38878 into ARMmbed:master Feb 25, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9271 cycles (-936 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.