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

efm32: add support for riotboot #11940

Merged
merged 3 commits into from
Oct 1, 2019

Conversation

basilfx
Copy link
Member

@basilfx basilfx commented Jul 31, 2019

Contribution description

This PR adds support for riotboot for EFM32 and all boards.

RFC

There is still one part that needs discussion (and should move to a different PR). 8ea2d1d exposes a global define that can be used to skip certain parts of the bootloader, when building the bootloader.

I need that for several reasons:

  • reducing the size of the bootloader (EFM32 cpu_init() does a lot more)
  • skipping parts in cpu_init() that are not relevant during boot
    • configuring clock sources
      • example: I could have an updated firmware that uses another source/configuration
    • e.g. chip applying errata
    • configuring DC-DC parameters
      • example: I decide to update DC-DC parameters to make my board more efficient or stable
    • peripheral initialization

My goal is to boot as quick as possible to the actual firmware on the defaults, and then do proper initialization.

I don't know if there is a better way to detect if I'm building the firmware, and I am happy to adapt if there is a way.

Resolved by #12297.

Testing procedure

cd tests/riotboot && BOARD=ikea-tradfri make test should work (or other EFM32 board, e.g. SLTB001a).

Issues/PRs references

#8902 (comment)

@basilfx basilfx added Platform: ARM Platform: This PR/issue effects ARM-based platforms Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: boards Area: Board ports labels Jul 31, 2019
@kYc0o kYc0o self-assigned this Jul 31, 2019
@aabadie aabadie requested a review from fjmolinas August 6, 2019 14:43
@fjmolinas
Copy link
Contributor

@cladmi might want to take a look at the BOOTLOADER define.

@aabadie
Copy link
Contributor

aabadie commented Aug 6, 2019

Otherwise I confirm this is working on the Pearl Gecko board. Nice!

@basilfx basilfx force-pushed the feature/efm32_riotboot branch from ca6f640 to 08ecaa6 Compare September 24, 2019 11:57
@basilfx
Copy link
Member Author

basilfx commented Sep 24, 2019

@cladmi @aabadie I have rebased this PR.

@cladmi Do you have an opinion on this approach, or know a better way? Would like to have something like this merged.

@kaspar030
Copy link
Contributor

8ea2d1d exposes a global define that can be used to skip certain parts of the bootloader, when building the bootloader.

I think that's alright. The bootloader cpu_init() just has to initialize what's needed in order to successfully execute the bootloader's kernel_init and jump to the actual image.

@@ -8,7 +8,7 @@ BOARD ?= samr21-xpro
FEATURES_REQUIRED += riotboot

# Disable unused modules
CFLAGS += -DNDEBUG -DLOG_LEVEL=LOG_NONE
CFLAGS += -DNDEBUG -DLOG_LEVEL=LOG_NONE -DBOOTLOADER
Copy link
Contributor

Choose a reason for hiding this comment

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

(bikeshadding) maybe call the define "RIOTBOOT"?

Copy link
Contributor

@fjmolinas fjmolinas Sep 24, 2019

Choose a reason for hiding this comment

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

I would keep the term BOOTLOADER or use RIOT_BOOTLOADER instead of RIOTBOOT. riotboot is associated with a module that any application can use so I think we should try and keep it [edit:semantically] separate from stuff only the bootloader needs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't care. I think that RIOT_BOOTLOADER reduces chances on a conflicting define.

I was planning to split this commit from this PR, so I'll change it over there!

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, "RIOT_BOOTLOADER" is just an arbitrary but new name of RIOT's bootloader, "riotboot". Why a new name?

"BOOTLOADER" is quite general, but in this case, we only mean riotboot. Why imply that this means any more than riotboot?

For referencing whether the support code is available, "MODULE_RIOTBOOT" can already be used.

How can people not be of the opinion that "RIOTBOOT" is the right name for this define? 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care. I think that RIOT_BOOTLOADER reduces chances on a conflicting define.

I do a little. Please keep BOOTLOADER over RIOT_BOOTLOADER if RIOTBOOT doesn't resonate.

Copy link
Contributor

Choose a reason for hiding this comment

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

How can people not be of the opinion that "RIOTBOOT" is the right name for this define? wink

I would think something that isn't RIOTBOOT is better since this isn't necessarily a riotboot boot-loader issue, but for any bootloader, e.g: for a case where e.g. mcuboot or similar would be supported, I imagine this initialization would need to be skipped. But that could be changed if and when the need arises.

Well, "RIOT_BOOTLOADER" is just an arbitrary but new name of RIOT's bootloader, "riotboot". Why a new name?

Ok, you are right.

@basilfx
Copy link
Member Author

basilfx commented Sep 24, 2019

I created #12297 to address the riotboot part. Will update this PR once that gets merged.

@basilfx basilfx force-pushed the feature/efm32_riotboot branch from 08ecaa6 to dcd6ac5 Compare September 24, 2019 16:20
@basilfx
Copy link
Member Author

basilfx commented Sep 24, 2019

Cherry-picked #12297 on top of this one so it can be tested. Will remove this if #12297 gets merged.

@basilfx
Copy link
Member Author

basilfx commented Sep 26, 2019

I've update this PR now that #12297 got merged. I've also disabled most parts of board_init(). Especially on the sltb001a, this reduces the size by almost 2kiB.

Due to #12311, this doesn't work on the sltb001a directly, but it is not related to this PR. I've tested this on the stk3600, slstk3401a, slstk3402a and sltb001a (with changes).

@basilfx basilfx added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Oct 1, 2019
@basilfx basilfx force-pushed the feature/efm32_riotboot branch from 73dee9e to 0336eff Compare October 1, 2019 10:02
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

This is very straightforward.

Only adds definition for bootloader size and skips certain board init parts if RIOT is used as a bootloader.

@benpicco
Copy link
Contributor

benpicco commented Oct 1, 2019

Another rebase is in order.

@basilfx basilfx force-pushed the feature/efm32_riotboot branch from 0336eff to 08bc67c Compare October 1, 2019 16:08
@basilfx
Copy link
Member Author

basilfx commented Oct 1, 2019

Rebased.

@benpicco benpicco merged commit c4fa084 into RIOT-OS:master Oct 1, 2019
@basilfx basilfx deleted the feature/efm32_riotboot branch January 14, 2020 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants