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

fix for issue #3715: correction in startup files for ARM and IAR, alignment of system_stm32f429xx.c files #3716

Merged
merged 1 commit into from Mar 6, 2017

Conversation

Projects
None yet
7 participants
@adustm
Member

adustm commented Feb 7, 2017

Description

Startup file have been modified for GCC_ARM since the integration of uvisor.
This modification needs to be ported also for ARM and IAR, even if uvisor is not present on those toolchains.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

#3397

Steps to test or reproduce

Before this PR, any DISCO_F429ZI test was running into timeout with ARM and IAR
Now it works

@adustm

This comment has been minimized.

Member

adustm commented Feb 7, 2017

@jeromecoutant it should fix mbed release test errors that you've seen.
Cheers

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 8, 2017

/morph test-nightly

@0xc0170 0xc0170 added the needs: CI label Feb 8, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Feb 8, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1526

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 8, 2017

@sg- sg- added needs: review and removed ready for merge labels Feb 9, 2017

@sg- sg- requested a review from Patater Feb 9, 2017

@Patater

Patater approved these changes Feb 9, 2017

@sg- @adustm @theotherjimmy

Tested working with uVisor on DISCO_F429ZI using mbed OS cd55625.

Build fails with latest mbed OS master (29cfee4) because of a missing /BUILD/DISCO_F429ZI/GCC_ARM/.link_script.ld file., but I don't think that error is related to this PR. Looks like the error may have been introduced with af4d848. Is this a known issue?

@sg-

This comment has been minimized.

Member

sg- commented Feb 13, 2017

@c1728p9 @theotherjimmy Can you have a look?

/* FPU settings ------------------------------------------------------------*/
#if (__FPU_PRESENT == 1) && (__FPU_USED == 1)
SCB->CPACR |= ((3UL << 10*2)|(3UL << 11*2)); /* set CP10 and CP11 Full Access */
#endif

This comment has been minimized.

@c1728p9

c1728p9 Feb 14, 2017

Contributor

Why was this removed?

This comment has been minimized.

@adustm

adustm Feb 16, 2017

Member

Hi, NUCLEO_F429ZI shares some files with DISCO_F429ZI, including startup files.
Since integration of uvisor for DISCO_F429ZI, this line of the SystemInit is copied into a new function (System_InitPre) that is called before the uvisor_init (if present) and system_init in the startup files.
Then this line is not removed but moved to https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/device/system_init_pre.c

targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/system_stm32f4xx.c Outdated
#else
SCB->VTOR = FLASH_BASE | VECT_TAB_OFFSET; /* Vector Table Relocation in Internal FLASH */
#endif

This comment has been minimized.

@c1728p9

c1728p9 Feb 14, 2017

Contributor

why was this removed?

This comment has been minimized.

@adustm

adustm Feb 16, 2017

Member

Hi, NUCLEO_F429ZI shares some files with DISCO_F429ZI, including startup files.
Since integration of uvisor for DISCO_F429ZI, this line of the SystemInit is copied into a new function (System_InitPre) that is called before the uvisor_init (if present) and system_init in the startup files.
Then this line is not removed but moved to https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/device/system_init_pre.c

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Feb 14, 2017

I had a few questions, but the PR looks fine to me. Also, @AlessandroA should probably take a look, since it was the uVisor changes that caused this problem.

@adustm

This comment has been minimized.

Member

adustm commented Feb 16, 2017

Hello @c1728p9
Thank you for the review.
I've answered your comments.
NUCLEO_F429ZI should soon be added uvisor, like DISCO_F429ZI does. So it makes sense to integrate modifications that will be required for uvisor later on.
kind regards
Armelle

@AlessandroA

This comment has been minimized.

Contributor

AlessandroA commented Feb 16, 2017

I will re-issue the uVisor tests as I realized that the DISCO_F429ZI target was not tested automatically (this is now fixed).

@AlessandroA

👍

@sg-

This comment has been minimized.

Member

sg- commented Feb 21, 2017

Looks like I missed this in PR #3655 but if there are uVisor specific startup hooks seems the functions should be named uvisor... to avoid confusion.

https://github.com/ARMmbed/mbed-os/search?utf8=%E2%9C%93&q=SystemInitPre

@sg- sg- added needs: work and removed ready for merge labels Feb 22, 2017

@adustm

This comment has been minimized.

Member

adustm commented Feb 23, 2017

Hello @sg-
SystemInitPre is not specific to uvisor. It is the common part of system init that must be done before uvisor init.
Kind regards

@adustm

This comment has been minimized.

Member

adustm commented Feb 27, 2017

Hi,
This issue is blocking several users on developper.mbed.org and github. Any news ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 27, 2017

This issue is blocking several users on developper.mbed.org and github. Any news ?

Can you please resolve the conflict ? We will rerun CI right after

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 27, 2017

SystemInitPre is not specific to uvisor. It is the common part of system init that must be done before uvisor init.

@sg- Please review this

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 28, 2017

bump

@adustm adustm force-pushed the adustm:disco_f429zi_debug branch to d69c5ed Feb 28, 2017

@adustm

This comment has been minimized.

Member

adustm commented Feb 28, 2017

Hello @0xc0170
I've rebased and fixed the conflict.
This issue is blocking people willing to use NUCLEO_F429ZI with ARM and IAR toolchains, I hope it will be merged.
Cheers

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 28, 2017

/morph test-nightly

@sg- sg- added needs: CI and removed needs: work labels Feb 28, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Feb 28, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1607

All builds and test passed!

@0xc0170 0xc0170 merged commit f168f62 into ARMmbed:master Mar 6, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has started
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adustm adustm deleted the adustm:disco_f429zi_debug branch Mar 7, 2017

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