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

STM32L1 Flash API and xDot bootloader support #4640

Merged
merged 13 commits into from Jul 17, 2017

Conversation

Projects
None yet
6 participants
@chrissnow
Contributor

chrissnow commented Jun 26, 2017

Description

Flash API for STM32L1
xDot support for bootloaders

Status

READY

in support of #4534

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Jun 27, 2017

@chrissnow thanks for pushing this support. I'll make a short review of it.
Could you run the test cases related to flash and share the results ?
tests-mbed_drivers-flashiap
tests-mbed_hal-flash

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 27, 2017

@LMESTM I will try and run those today. Clearing of flags in the erase routine I wasn't sure of. Other than sector size it's basically the L0 code.

@mfiore02 please could you review the xDot changes?

targets/TARGET_STM/TARGET_STM32L1/flash_api.c Outdated
return 0;
}
uint32_t flash_get_sector_size(const flash_t *obj, uint32_t address) {

This comment has been minimized.

@0xc0170

0xc0170 Jun 27, 2017

Member

Please fix the formatting for some functions, { for the body of the function starts on the new line

This comment has been minimized.

@chrissnow

chrissnow Jun 28, 2017

Contributor

@LMESTM this was taken from the L0 so should probably go there too.

This comment has been minimized.

@LMESTM

LMESTM Jun 28, 2017

Contributor

@chrissnow ok thanks !

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 27, 2017

@LMESTM results from tests

| target              | platform_name | test suite                  | test case                 | passed | failed | result | elapsed_time (sec) |
+---------------------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_drivers-flashiap | FlashIAP - init           | 1      | 0      | OK     | 0.05               |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_drivers-flashiap | FlashIAP - program        | 1      | 0      | OK     | 3.5                |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_drivers-flashiap | FlashIAP - program errors | 1      | 0      | OK     | 0.06               |
+---------------------+---------------+-----------------------------+---------------------------+--------+--------+--------+--------------------+
+---------------------+---------------+----------------------+-------------------------------+--------+--------+--------+--------------------+
| target              | platform_name | test suite           | test case                     | passed | failed | result | elapsed_time (sec) |
+---------------------+---------------+----------------------+-------------------------------+--------+--------+--------+--------------------+
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_hal-flash | Flash - buffer alignment test | 1      | 0      | OK     | 0.96               |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_hal-flash | Flash - clock and cache test  | 1      | 0      | OK     | 0.34               |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_hal-flash | Flash - erase sector          | 1      | 0      | OK     | 0.11               |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_hal-flash | Flash - init                  | 1      | 0      | OK     | 0.33               |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_hal-flash | Flash - mapping alignment     | 1      | 0      | OK     | 0.06               |
| XDOT_L151CC-GCC_ARM | XDOT_L151CC   | tests-mbed_hal-flash | Flash - program page          | 1      | 0      | OK     | 0.58               |
+---------------------+---------------+----------------------+-------------------------------+--------+--------+--------+--------------------+

@0xc0170 formatting has been tidied as requested.

@mfiore02

This comment has been minimized.

Contributor

mfiore02 commented Jun 27, 2017

I have a couple comments/questions:

  • Assuming normal builds for xDot will continue to behave as they have been (compiled for no bootloader). Is this correct?
  • If MTS desires to write our own bootloader in the future and include it by default in xDot compiles, how can we architect that so that either bootloader could be used? Is there a way to disable the hooks that offset the application (linker files & vector table address) and combine the bootloader (post build hooks)? So either bootloader (or no bootloader) could be used? This might be a nice change to backport to mDot and Dragonfly as well (our other boards with a bootloader). A bit of a rabbit trail, but I think it's worth considering.
  • I only see changes to the ARM linker file to support building the bootloader. Why not support it with GCC and/or IAR as well? Not everyone has an ARM license, so at least adding GCC support would be nice.
@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 27, 2017

@mfiore02
Normal builds are unaffected, the changes don't actually enable\merge a bootloader it just adds support for one, you need to define one in "mbed_app.json" such as

{
    "target_overrides": {
        "K64F": {
            "target.bootloader_img": "bootloader/K64F.bin"
        },
        "NUCLEO_F429ZI": {
            "target.bootloader_img": "bootloader/NUCLEO_F429ZI.bin"
        },
        "UBLOX_EVK_ODIN_W2": {
            "target.bootloader_img": "bootloader/UBLOX_EVK_ODIN_W2.bin"
        },
        "XDOT_L151CC": {
            "target.bootloader_img": "bootloader/xDot.bin"
        }
    }
}

if MTS make their own you would just point that to your bootloader bin, the build tools then seem to find its size, sort's out the linker etc then merge it at the end. without "target.bootloader_img" nothing changes.

MTS would just need to default that to your bin.

The bootloader build uses mbed_app.json again but just to fix the size of the bootloader so it can be padded.

    "target_overrides": {
        "K64F": {
            "target.restrict_size": "0x40000"
        },
        "NUCLEO_F429ZI": {
            "target.restrict_size": "0x40000"
        },
        "UBLOX_EVK_ODIN_W2": {
            "target.restrict_size": "0x40000"
        },
        "XDOT_L151CC": {
            "target.restrict_size": "0x10000"
        },
		"*": {
            "platform.stdio-flush-at-exit": false
        }
    },
    "config": {
        "update_file": "\"/disk/app.bin\""
    }, "macros": [ 
        "NDEBUG=1"
    ]
}

I forgot to sort out GCC + IAR linker, I will take a look tonight.

I hope that makes some sense!
#4534 might help explain things.

@mfiore02

This comment has been minimized.

Contributor

mfiore02 commented Jun 27, 2017

@chrissnow thanks, that answers my questions. I'm OK with these changes once GCC is added as a build option.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

@chrissnow I would like to see IAR supported as well, as supporting only ARM and GCC_ARM could lead to hard to debug failures when someone attempts to use this bootloader support on IAR.

@mfiore02

This comment has been minimized.

Contributor

mfiore02 commented Jun 27, 2017

LGTM!

@LMESTM

LMESTM approved these changes Jun 28, 2017

Only 2 comments in code to be removed or updated

targets/TARGET_STM/TARGET_STM32L1/flash_api.c Outdated
uint32_t flash_get_sector_size(const flash_t *obj, uint32_t address)
{
/* considering 1 sector = 1 page */

This comment has been minimized.

@LMESTM

LMESTM Jun 28, 2017

Contributor

This comment is not valid.
Tt actually comes from L4 where the assumption is true.
Then the code was copied and modified for L0 where all sectors are 32 pages - so the comment should have been removed in L0 - I will do it.
Now this is copied in L1 but is not true either - so needs to be removed

targets/TARGET_STM/TARGET_STM32L1/flash_api.c Outdated
uint32_t flash_get_page_size(const flash_t *obj)
{
/* considering 1 sector = 1 page */

This comment has been minimized.

@LMESTM

LMESTM Jun 28, 2017

Contributor

ditto

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

@LMESTM comments removed as requested.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

@chrissnow I merged #4610 and it has caused a merge conflict on this PR. Could you rebase?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

@chrissnow It looks like something went wrong with your rebase. We cannot backport to a release any PRs that have merge commits on them. Could you rebase to remove that merge commit?

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

@theotherjimmy yes that didn't do what I was expecting!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

I recommend marking the current branch with another branch in case things don't go as planned again.

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

@theotherjimmy any suggestions for what git commands to run? I seems to be digging a bigger hole at the moment!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

I'd try

git branch xDot-Bootloader-bak
git fetch origin
git rebase origin/master

Assuming that origin points to ARMmbed/mbed-os.

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

thoughts on this?

Auto-merging features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h
CONFLICT (content): Merge conflict in features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h
error: Failed to merge in the changes.
Patch failed at 0095 NUCLEO_F429ZI/mbedtls: add SHA1 hw_acceleration
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

image

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

@chrissnow Something looks wrong. You should not need to apply that patch; it's already on master.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

I'm going to give it a go real quick. I'll link my branch if I succeed

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

@chrissnow I think that's related to doing git rebase master and your master branch being very behind. I just ran into the same merge conflict, and that was why.

I corrected it by aborting the rebase (git rebase --abort) and then updating my master branch (git checkout master, git pull origin master). Then I retried the rebase (git checkout xDot-Bootloader, git rebase master) and got a merge conflict on your first commit in targets.json, which I expected to happen.

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

@theotherjimmy I hope that is now sorted out, Thanks for the assistance.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

@chrissnow It looks like you pulled and merged instead of force pushing.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

You end up with a merge commit, and duplicate commits that way!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

Supposing that the log up to 4b59838 is correct:

git checkout 4d59838774ff28c480f84b9ebfe86946e9cca423

checkout out the repo just before the merge commit, and after the rebase

git branch -D xDot-Bootloader

Kill the old branch

git checkout -b xDot-Bootloader

recreate the branch using the correct head

git push -f -u chrissnow

Push the new history to this PR (supposing that chrissnow is the remote for chrissnow/mbed-os)

@chrissnow chrissnow force-pushed the chrissnow:xDot-Bootloader branch Jun 28, 2017

@chrissnow chrissnow force-pushed the chrissnow:xDot-Bootloader branch to 2a2d904 Jun 30, 2017

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jul 13, 2017

@theotherjimmy this should be good to go now #4666 is merged.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 13, 2017

@chrissnow Agreed. Removing preceding PR tag

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 14, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 810

All builds and test passed!

@theotherjimmy theotherjimmy merged commit 6593d5b into ARMmbed:master Jul 17, 2017

4 checks passed

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

janjongboom added a commit to janjongboom/mbed-os that referenced this pull request Aug 8, 2017

Merge pull request ARMmbed#4640 from chrissnow/xDot-Bootloader
STM32L1 Flash API and xDot bootloader support

janjongboom added a commit to janjongboom/mbed-os that referenced this pull request Aug 8, 2017

Merge pull request ARMmbed#4640 from chrissnow/xDot-Bootloader
STM32L1 Flash API and xDot bootloader support

@chrissnow chrissnow referenced this pull request Oct 17, 2017

Merged

LPC1769 port #5025

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