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

STM32F7: Add bootloader support (new trial) #5972

Merged
merged 6 commits into from Feb 2, 2018

Conversation

Projects
None yet
7 participants
@bcostm
Contributor

bcostm commented Jan 30, 2018

Description

  • First trial done in PR #5821 but reverted in PR #5932 due to a sync issue with CI tests on ARM side. Not reproduce on my side.
  • The only thing I changed compared to the previous PR is the Cache initialization which is done first in the mbed_sdk_init function.
  • Fixes Issue #5718

Status

IN DEVELOPMENT

Waiting ci-test result to see if sync issue is still present or not.

bcostm added some commits Dec 19, 2017

Check cache before enabling it
The mbed_sdk_init can be called either during cold boot or during
application boot after bootloader has been executed.
In case the bootloader has already enabled the cache,
is is needed to not enable it again.
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 30, 2018

First trial done in PR #5821 but reverted in PR #5932 due to a sync issue with CI tests on ARM side. Not reproduce on my side.

As I understood this was related to cache settings as pinpointed in the PR earlier, and confirmed by few other users (their applications broke). Therefore I would like to understand what broke it (here you can see the confirmation it was just one commit #5912 (comment) ), you should be able to reproduce this. 4 lines changes regarding Cache? that from the commit looks like it can affect an app?

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jan 30, 2018

Yes I know it is related to this cache settings but I have no clue why this causes sync issue on your side. I have no problem when I run the tests. No clue either how to solve it. I just want to see if the few lines I have modified change something in your tests.

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jan 31, 2018

Hi @MikeDK
Would it be possible for you to check this PR and see if you still have a problem with your application as you have noticed in PR #5912 ? On my side I have tested a basic example on a NUCLEO_F746ZG board with ARM, GCC and IAR compilers and all is ok.

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Jan 31, 2018

Hi @bcostm
I just did a test with my application (which basically uses EventQueue, SDBlockDevice, FATFileSystem and some own classes), and it seems to work.

The interesting thing is: it even works now without commit 6e71398

I guess I will have to try harder to reproduce the error on my side ;)

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Jan 31, 2018

Ok some new info:

When I checkout 7ff1cf5, which is the commit before the revert PR from @0xc0170, build and flash - it is working as long as I do not unpower the board. If I plug out the USB power and replug, then the board is hanging - so error appears only if cold booted ... If only flashing via copy .bin on mass storage device, the problem does not appear.

So this means the error happens somewhere between power up and st-link getting ready I guess.

Now I just checked this PR again ... If I apply all patches but the last one (6e71398), then the same - works as long I do not unpower the board.

If I then apply patch 6e71398, everything works again.

So it seems that last commit really fixes the problem ;)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 31, 2018

@MikeDK appreciate the extensive testing for this one!

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jan 31, 2018

Thanks @MikeDK I appreciate too 👍

When you say

If I then apply patch 6e71398, everything works again.

Is it working when you unplug/replug the board ?

Another question: are you on Linux or Windows ?

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Jan 31, 2018

@bcostm yes, with the latest commit, the board is also starting up correctly when unplugged from power and replugged again.

I work under Linux (Debian Stretch) 🥇

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jan 31, 2018

OK because I am on Windows and I can't test it on Linux. @0xc0170 if I am not wrong the ci-test which had the sync issue are ran on Linux right ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 31, 2018

OK because I am on Windows and I can't test it on Linux. @0xc0170 if I am not wrong the ci-test which had the sync issue are ran on Linux right ?

I believe on both. I could see aborted on linux, but also windows with IAR had issues (what I am not certain is the sequence it has happened if linux was first and started having those issues).

@MikeDK

This comment has been minimized.

Contributor

MikeDK commented Jan 31, 2018

Maybe I can test this also on Windows ... I'll have to check with my coworkers - most of them use Windows

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 31, 2018

@studavekar Pinging you just to let you know about the PR.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jan 31, 2018

OK because I am on Windows and I can't test it on Linux. @0xc0170 if I am not wrong the ci-test which had the sync issue are ran on Linux right ?

Interesting if its host OS causing this bug. would be good to know more about what about OS causing this bug ST-link ?

@cmonr cmonr added needs: work and removed needs: review labels Feb 1, 2018

@bcostm

This comment has been minimized.

Contributor

bcostm commented Feb 1, 2018

If I then apply patch 6e71398, everything works again.
So it seems that last commit really fixes the problem ;)

Can you please launch the ci-morph tests to see how it is now ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 1, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

/morph uvisor-test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 1, 2018

/morph uvisor-test

1 similar comment
@orenc17

This comment has been minimized.

Contributor

orenc17 commented Feb 1, 2018

/morph uvisor-test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 1, 2018

Looks like things are still stable. Going to merge this in first thing tomorrow to keep a controlled eye on it.

Interesting that the only code change that I could tell was the positioning of code in mbed_overrides.c

@cmonr cmonr added ready for merge and removed needs: work labels Feb 1, 2018

@bcostm

This comment has been minimized.

Contributor

bcostm commented Feb 2, 2018

OK. I don't know exactly what causes the error previously. For the story, I looked in ST CubeF7 examples and found that the cache initialization was done BEFORE the clock setting. This is why I changed it also.

But I also remind the remark from @LMESTM in the previous PR:

As of now, cache is enabled in targets/TARGET_STM/mbed_overrides.c. But shouldn't this be generic ?
Also in case of bootloader, shoudn't mbed_start_application handle the cache cleaning / disabling before jumping into new application ?

@cmonr cmonr merged commit 1e79439 into ARMmbed:master Feb 2, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter 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/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 3, 2018

@bcostm Interesting. I'm not sure mbed_start_application would be a good place to put the cache-handling code, since it sounds like the function is called after all peripherals are setup (at least, that's the impression I get).

Care to take this comversation to a new issue? I suspect we might have more M7 issues down the line, and it would be good to continue discussion in an issue instead of a completed PR :)

@bcostm

This comment has been minimized.

Contributor

bcostm commented Feb 5, 2018

OK I'll open a new issue on this subject to follow-up this.

@bcostm bcostm referenced this pull request Feb 5, 2018

Open

Cortex-M7 cache setting #6010

@mbed-ci

This comment has been minimized.

@bcostm bcostm deleted the bcostm:dev_BL_STM32F7 branch Mar 21, 2018

everhardt added a commit to withthegrid/mbed-os that referenced this pull request May 31, 2018

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