Skip to content

cpu: efm32: generalize support for EFM32/EZR32/EFR32#8139

Merged
basilfx merged 3 commits intoRIOT-OS:masterfrom
basilfx:feature/efm32
Dec 19, 2017
Merged

cpu: efm32: generalize support for EFM32/EZR32/EFR32#8139
basilfx merged 3 commits intoRIOT-OS:masterfrom
basilfx:feature/efm32

Conversation

@basilfx
Copy link
Copy Markdown
Member

@basilfx basilfx commented Nov 24, 2017

In addition to the Kinetis and STM32 MCUs, this PR should summarize support for all EFM32/EZR32/EFR32 into a single folder.

I have taken a different route: in cpu/efm32/families, one folder per family exists, with family-specific (vendor) files (example of a family is the EFR32MG1P). I have a cpus.txt that contains information that is used for conditionals in the Makefiles, the linker script etc. It is parsed by this script. Above approach is IMHO a good trade-off between maintainability and code duplication. I know that cpus.txt contains more CPUs than the number of vendor files included, but it is unlikely that this file changes as long as Silicon Labs doesn't add new CPUs to an existing family. Currently, adding support for a new CPU is just adding the vendor header to cpus/family/<family-name>/includes/vendor.

The (generic) drivers in cpu/efm32/periph are the same as in current master (ok, few changes to support more UARTs and I2Cs).

The emlib package has been replaced with the superset gecko_sdk. We need the latter to support the radio library, which is a vendor-provided blob. Thanks to @kaibeckmann for initial work!

There are a few changes to review, but the most important ones are 03d1e55 and 4ffc8e9 (the others are very similar or vendor files).

@basilfx basilfx added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: boards Area: Board ports Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Nov 24, 2017
@PeterKietzmann PeterKietzmann mentioned this pull request Nov 27, 2017
Closed
@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Nov 28, 2017

Rebase + subtle reminder :-)

kYc0o
kYc0o previously requested changes Nov 28, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

The changes are good in general, although some are less in the scope of the PR, maybe it's just the title. I don't know if it was intentional to introduce changes to the files while moving them, but maybe they belong to another PR.

boards/ikea-tradfri/include/periph_conf.h Outdated
#endif
#ifndef CMU_LFXOINIT
#define CMU_LFXOINIT CMU_LFXOINIT_DEFAULT
#endif
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o Nov 28, 2017

Choose a reason for hiding this comment

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

Is this change part of this PR?

Copy link
Copy Markdown
Member Author

@basilfx basilfx Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes and no :-)

I saw in @kaibeckmann his port repository that he deviated from the default values, so I generalized this to be part of periph_conf.h. It is true that the board he used isn't part of RIOT-OS (yet), but I thought it was a good addition to take into account.

Copy link
Copy Markdown
Member Author

@basilfx basilfx Nov 28, 2017

Choose a reason for hiding this comment

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

AFAIK you need to be able to override the defaults to setup the radio driver.

Copy link
Copy Markdown
Contributor

@kYc0o kYc0o Nov 28, 2017

Choose a reason for hiding this comment

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

Thus, all the introduced changes about the clocking and peripherals in general can be considered improvements? If so (and if it's not a pain to take them off this PR), I'd like to have another PR with them.

Copy link
Copy Markdown
Member Author

@basilfx basilfx Nov 28, 2017

Choose a reason for hiding this comment

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

OK, fair enough. I'll open another PR for these changes, then I'll update this PR after that's fixed :-)

Copy link
Copy Markdown
Contributor

@kYc0o kYc0o Nov 28, 2017

Choose a reason for hiding this comment

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

I think this PR will get in quite fast since the additions for new families and the relocation of some files doesn't change anything of the current working code. I will spend more time reviewing and testing the changes, which is better if they are separated. Thanks!

boards/ikea-tradfri/include/periph_conf.h Outdated
#endif
#ifndef EMU_EM4INIT
#define EMU_EM4INIT EMU_EM4INIT_DEFAULT
#endif
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o Nov 28, 2017

Choose a reason for hiding this comment

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

Same here and below.

Copy link
Copy Markdown
Member Author

@basilfx basilfx Nov 28, 2017

Choose a reason for hiding this comment

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

See comment above :-)

This was referenced Nov 28, 2017
Merged
Merged
@basilfx basilfx added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 28, 2017
@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Nov 28, 2017

Waiting for #8165 and #8166.

@basilfx basilfx removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 29, 2017
@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Nov 29, 2017

#8165 and #8166 are merged, and I have updates this PR accordingly.

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 30, 2017

Nice, changes look all valid to me. Don't have an EFM board available here right now, so will give this a test run tomorrow. pre-ACK :-)

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 30, 2017

does this PR still need squashing? If yes, just re-set the label...

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Nov 30, 2017
@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Nov 30, 2017

It needs squashing. I'll squahs the cpu/efm32_common and cpu/efm32 into one, so you can see the renames.

@basilfx basilfx added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 30, 2017
@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Dec 2, 2017

@haukepetersen Did you test it yesterday? Am I allowed to squash?

@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Dec 5, 2017

Subtle ping :-)

@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Dec 11, 2017

@haukepetersen @kYc0o can you take a look? I would like to continue with this :-)

@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Dec 18, 2017

Rebased.

haukepetersen
haukepetersen approved these changes Dec 19, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen 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. Except the code size increase introduced by the emlib, I actually think this CPU turned out quite nice! Can't wait for the radio support :-)

Tested on 'sltb001a` and works as expected -> ACK

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Dec 19, 2017

@kYc0o: It seems to me like your issues are addressed, would you mind to dismiss them? Thx!

@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 CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 19, 2017
@basilfx basilfx dismissed kYc0o’s stale review December 19, 2017 19:33

PR #8165 and #8166 fixes the issues that have been mentioned in the review.

@basilfx basilfx merged commit 90c81f6 into RIOT-OS:master Dec 19, 2017
This was referenced Dec 19, 2017
Closed
Closed
Closed
Closed
Closed
Closed
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
@basilfx basilfx mentioned this pull request Feb 5, 2018
Merged
@basilfx basilfx deleted the feature/efm32 branch February 24, 2018 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kYc0o kYc0o kYc0o left review comments

@haukepetersen haukepetersen haukepetersen approved these changes

Assignees

@haukepetersen haukepetersen

@kYc0o kYc0o

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.01

Development

Successfully merging this pull request may close these issues.

4 participants

@basilfx @haukepetersen @kYc0o @aabadie