Skip to content

Support mbed_start_application for Cortex-M23 #6949

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

Merged
merged 4 commits into from
Jun 7, 2018

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented May 18, 2018

Description

This PR implements mbed_start_application on Cortex-M23 for bootloader related application.

Pull request type

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

Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

With "mov r2, #0", compile OK with GCC_ARM, but failed with ARMC6.
With "ldr r2, =0", compile OK with ARMC6, but failed with GCC_ARM.
Finally, with "movw r2, #0"/"movt r2, #0", compile OK with both ARMC6 and GCC_ARM.
@ccli8
Copy link
Contributor Author

ccli8 commented May 21, 2018

In 646f614, I fix start_new_application compile error with different toolchains:

  1. With mov r2, #0, compile OK with GCC_ARM, but failed with ARMC6.
  2. With ldr r2, =0, compile OK with ARMC6, but failed with GCC_ARM.
  3. Finally, with movw r2, #0/movt r2, #0, compile OK with both ARMC6 and GCC_ARM.

@cmonr cmonr requested a review from a team May 24, 2018 03:28
@cmonr
Copy link
Contributor

cmonr commented May 24, 2018

@dreemkiller Mind taking a look at this? Not sure who else would be familiar with the M23 aside for you and @deepikabhavnani

@cmonr cmonr requested review from dreemkiller and removed request for a team May 24, 2018 16:25
@dreemkiller
Copy link
Contributor

@TacoGrandeTX, take a look

@@ -48,12 +48,20 @@ static void powerdown_nvic()
int i;
int j;

#if defined(__CORTEX_M23)
isr_groups_32 = 16;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16 is not a good number... and more than even a M33 could support. The max number of external interrupts a M23 can support is 240. It's sad that M23 doesn't have the ICTR so the max value we could use here is 7 (or 8... depending on how it's used elsewhere). Rule SGCR in v8-M ARM states: "When a particular NVIC interrupt line is not implemented, the registers that are associated with it are reserved." Yet Rule GLNG states "Privileged accesses to unimplemented registers are RES0" so accessing non-existing registers should be harmless but are wasteful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TacoGrandeTX Thanks for your detailed information. I fix it in 3c14cb6.

@@ -69,17 +77,20 @@ static void powerdown_scb(uint32_t vtor)
SCB->SCR = 0x00000000;
// SCB->CCR - Implementation defined value
for (i = 0; i < 12; i++) {
#if defined(__CORTEX_M7)
#if defined(__CORTEX_M7) || defined(__CORTEX_M23)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not good - M23 has only 2 SHPR registers (the first is reserved). Looks like we have mixed up the size of our register definitions... core_cm23.h is using uint32_t (x2) while core_cm7.h is using uint8_t (x12). We need two for loops or should change SCB_Type in core_cm23.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TacoGrandeTX I fix it in 3c14cb6.

@@ -107,7 +118,12 @@ __asm static void start_new_application(void *sp, void *pc)
void start_new_application(void *sp, void *pc)
{
__asm volatile (
#if defined(__CORTEX_M23)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This two-instruction sequence is not required... "mov r2, #0" will put 0 (zero-extended) into r2 on the M23. We can remove the entire #if block. MOVW/MOVT is useful for loading a full 32-value into a register without a literal pool... not the case here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - missed the commit comments! Clearly AC6 is not accepting the inline assembly properly (I ran a local test) so perhaps we could use "movw r2, #0" for both conditions and still remove the #if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TacoGrandeTX I replace with movw/movt and remove #if block in 3c14cb6.

@cmonr
Copy link
Contributor

cmonr commented May 25, 2018

@ccli8 ^^^ ?

@cmonr
Copy link
Contributor

cmonr commented May 25, 2018

Taking another look at this PR, this can come in with a patch release.

1. M23 doesn't support ICTR and supports up to 240 external interrupts.
2. Fix reset of SHPR
3. Fix inline assembly compile error with ARMC6
@cmonr
Copy link
Contributor

cmonr commented May 29, 2018

@TacoGrandeTX @deepikabhavnani Mind re-reviewing?

Copy link
Contributor

@TacoGrandeTX TacoGrandeTX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MOVT is superfluous - recommend removing it.

@@ -118,12 +125,8 @@ __asm static void start_new_application(void *sp, void *pc)
void start_new_application(void *sp, void *pc)
{
__asm volatile (
#if defined(__CORTEX_M23)
"movw r2, #0 \n"
"movw r2, #0 \n" // Fail to compile "mov r2, #0" with ARMC6. Replace with movw/movt.
"movt r2, #0 \n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - my previous comments weren't very clear! MOVT is superfluous here as MOVW will zero-extend. So we can remove the MOVT instruction. "MOVW r2, #0" will put 0x0 into all 32-bits of r2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TacoGrandeTX Yes, MOVT is superfluous. I remove it in 23dcd82.

@cmonr
Copy link
Contributor

cmonr commented May 31, 2018

Looks like all of the review comments have been addressed.
@TacoGrandeTX Mind confirming?

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 1, 2018

Build : SUCCESS

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

Triggering tests

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

@adbridge
Copy link
Contributor

adbridge commented Jun 1, 2018

Restarted pr-head

@mbed-ci
Copy link

mbed-ci commented Jun 2, 2018

@mbed-ci
Copy link

mbed-ci commented Jun 2, 2018

@cmonr
Copy link
Contributor

cmonr commented Jun 2, 2018

Interesting... Don't think the test failure is the PR's fault.
/morph test

@mbed-ci
Copy link

mbed-ci commented Jun 2, 2018

@cmonr
Copy link
Contributor

cmonr commented Jun 3, 2018

@TacoGrandeTX Could you re-approve if you're alright with the changes?

Copy link
Contributor

@TacoGrandeTX TacoGrandeTX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good!

@cmonr cmonr added the needs: CI label Jun 6, 2018
@cmonr
Copy link
Contributor

cmonr commented Jun 6, 2018

/morph build

@cmonr
Copy link
Contributor

cmonr commented Jun 6, 2018

Oh...
Me and my reflexes.

@mbed-ci
Copy link

mbed-ci commented Jun 6, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Jun 7, 2018

@mbed-ci
Copy link

mbed-ci commented Jun 7, 2018

@cmonr cmonr merged commit 38f9519 into ARMmbed:master Jun 7, 2018
@ccli8 ccli8 deleted the nuvoton_m23_bootloader branch June 8, 2018 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants