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

boards/cpu: add support for efm32lg and stk3600 #4824

Closed
wants to merge 9 commits into from

Conversation

basilfx
Copy link
Member

@basilfx basilfx commented Feb 15, 2016

This PR supersedes #4722 and only contains five commits to add:

  • The common EFM32 peripheral drivers.
  • The EFM32LG CPU.
  • The STK3600 board.

I remove all other EFM32LG CPUs except the one that is on the STK3600. Eventually, I would like to add them again.

The comments of #4722 still apply. Compared to the previous PR, I have improved I2C, fixed timer issues, corrected comments and adapted peripheral driver changes.

You may want to skip commit e671c47 and d7d7bfb, as they contain a lot of vendor files. The other commits should be browser-compatible.

All files
Benchmarks

@OlegHahm OlegHahm added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 15, 2016
@haukepetersen
Copy link
Contributor

Looking manageable now on first sight :-)

Just did quick compile run, and I have to say I stay with my opinion about vendor libs:

hello-world - slwstk6220a (without emlib):
   text    data     bss     dec     hex filename
   7788     128    2744   10660    29a4

hello-world - stk3600 (with emlib):
   text    data     bss     dec     hex filename
  11772     136    2760   14668    394c

default - slwstk6220a (without emlib):
   text    data     bss     dec     hex filename
  12084     164    2748   14996    3a94

default - stk3600 (with emlib):
   text    data     bss     dec     hex filename
  16076     172    2764   19012    4a44

So for hello world this is 150% and for default still 133% the code size of the direct implementation.

Randomly picked looking at the code for the GPIO and the TIMER periph drivers show, that the implementations without emlib are 20-40 lines shorter and IMHO easier to read - so if using the emlib is not even making the code shorter or easier to read I really don't see the point...

But again, these are quick and not in-depth observations. Looking for example at the gnrc_networking example, the emlib implementation uses only 7.5% ROM, at a code size of ~65k this is still a lot, but could be tolerated if it really brings other advantages...

To conclude, I am not at all convinced of using the emlib, but I would like to hear some other opinions on this. @kaspar030, @gebart, @thomaseichinger, what do you think?

@basilfx
Copy link
Member Author

basilfx commented Feb 15, 2016

Thank you for looking into this so quickly :-)

Some explanation of the code size:

  • By default, some features are enabled that increase the code size (~500 bytes), for example AEM and LEUART.
  • Peripherals are explicitly reset before use (~200 bytes)
  • Clock management unit code (~2500 bytes) is shared by multiple peripherals.

Using LTO also shrinks the size (~4000 bytes for examples/default), but it fails to run :-)

Have you also looked at the benchmarks I did? Some boards have similar code sizes. You can click the numbers in the text/data/bss column to see which archive/library is using so how much space (sorry for hidden links/bad UI).

I'll see if I can further reduce the code size. What would be acceptable, given the features provided by emlib (proper clock setup, resetting peripherals, low-power support, chip errata, supported chips/generations)?

@basilfx
Copy link
Member Author

basilfx commented Feb 16, 2016

For examples/default I did a comparison that should better fit the features of the current EZR32WG target:

Plain:
   text    data     bss     dec     hex filename
  16188     172    2764   19124    4ab4 default.elf

Low-power disabled:
   text    data     bss     dec     hex filename
  15840     172    2764   18776    4958 default.elf

The above, and AEM disabled:
   text    data     bss     dec     hex filename
  15624     172    2764   18560    4880 default.elf

The above, and no reset:
   text    data     bss     dec     hex filename
  15548     172    2764   18484    4834 default.elf

The above, and no chip errata:
   text    data     bss     dec     hex filename
  15448     172    2764   18384    47d0 default.elf

The above, and no LPM:
   text    data     bss     dec     hex filename
  15132     172    2764   18068    4694 default.elf

The above, and no other clock setup:
   text    data     bss     dec     hex filename
  15108     172    2764   18044    467c default.elf

The above, and LTO=yes (broken binary!):
   text    data     bss     dec     hex filename
  12864     124    2636   15624    3d08 default.elf

I noticed that GCC 5.2 (which I have installed here) produces slightly bigger binaries compared to GCC 4.9.

@haukepetersen
Copy link
Contributor

Let's take a look at the features:

proper clock setup

What proper is can be argued about, but this can be done writing some registers directly without any trouble

resetting peripherals

When programming peripherals directly on register level, this comes for free

low-power support

again, by just picking the power states we need and just use them (instead of everything), we can save code size

chip errata

this is indeed something useful, but we could just copy that part out of the emlib

supported chips/generations

When only implementing a sub-set of features that we actually need in RIOT, we are also only effected by a sub-set of the diffs between chip generations and family members. I think this can actually be handled

But to make sure: I think importing the board(s) using the emlib is still better than not supporting them at all, and I would be okay in using it for now but keeping a note that it (might) make sense to optimize it out some point in the future.

@basilfx
Copy link
Member Author

basilfx commented Feb 18, 2016

Sorry for late reply. Some comments:

proper clock setup

What proper is can be argued about, but this can be done writing some registers directly without any trouble

I consider proper as the way the developers of emlib have written it. They know about the hardware details, so I assume they know the ins-and-outs.

low-power support

again, by just picking the power states we need and just use them (instead of everything), we can save code size

I was referring to the low-power peripherals, e.g. LEUART or LETIMER. I could change it to include defines to pick which peripherals to support. For instance, the STK3200 has it's STDIO connected to a LEUART. In this case, disabling USART would be better for the code size if it is not used.

supported chips/generations

When only implementing a sub-set of features that we actually need in RIOT, we are also only effected by a sub-set of the diffs between chip generations and family members. I think this can actually be handled

Using emlib, it was rather easy to support the new Gemstone chips, without it costing too much time. I see that as an advantage.

But to make sure: I think importing the board(s) using the emlib is still better than not supporting them at all, and I would be okay in using it for now but keeping a note that it (might) make sense to optimize it out some point in the future.

Agreed.

I'm currently revising the code to optimize code size a bit and improve readability. I'll update this PR whenever I think it's ready again :-)

@basilfx basilfx force-pushed the feature/efm32_stk3600 branch from 17fc36c to 891b971 Compare February 22, 2016 18:56
@basilfx
Copy link
Member Author

basilfx commented Feb 22, 2016

With the new commits (work in progress, somewhat inspired by/depending on #4862 and #4780), I have the numbers below. I stripped my port to resemble the existing SLWSTK6220A board as much as possible.

Baseline (SLWSTK6220A):
   text    data     bss     dec     hex filename
  12348     164    2748   15260    3b9c default.elf
   8092     128    2744   10964    2ad4 hello-world.elf
   5116     128    2744    7988    1f34 minimal.elf

The above, and LTO=yes (broken binary!):
   text    data     bss     dec     hex filename
  11200     124    2620   13944    3678 default.elf
   7400     120    2616   10136    2798 hello-world.elf
   4388     120    2616    7124    1bd4 minimal.elf

------

Old (STK3600, without additional commits):
   text    data     bss     dec     hex filename
  16188     172    2764   19124    4ab4 default.elf
  11920     136    2760   14816    39e0 hello-world.elf
   9104     136    2760   12000    2ee0 minimal.elf

------

New (STK3600, with additional commits):
   text    data     bss     dec     hex filename
   16084    172    2764   19020    4a4c default.elf
   11816    136    2760   14712    3978 hello-world.elf
   8984     136    2760   11880    2e68 minimal.elf

Low-power disabled:
   text    data     bss     dec     hex filename
  15736     172    2764   18672    48f0 default.elf
  11468     136    2760   14364    381c hello-world.elf
   8636     136    2760   11532    2d0c minimal.elf

The above, and AEM disabled:
   text    data     bss     dec     hex filename
   15520    172    2764   18456    4818 default.elf
   11252    136    2760   14148    3744 hello-world.elf
   8420     136    2760   11316    2c34 minimal.elf

The above, and no reset:
   text    data     bss     dec     hex filename
  15444     172    2764   18380    47cc default.elf
  11176     136    2760   14072    36f8 hello-world.elf
   8344     136    2760   11240    2be8 minimal.elf

The above, and no chip errata:
   text    data     bss     dec     hex filename
  15344     172    2764   18280    4768 default.elf
  11076     136    2760   13972    3694 hello-world.elf
   8244     136    2760   11140    2b84 minimal.elf

The above, and no LPM:
   text    data     bss     dec     hex filename
  15028     172    2764   17964    462c default.elf
  10760     136    2760   13656    3558 hello-world.elf
   7928     136    2760   10824    2a48 minimal.elf

The above, and no other clock setup:
   text    data     bss     dec     hex filename
  15004     172    2764   17940    4614 default.elf
  10736     136    2760   13632    3540 hello-world.elf
   7904     136    2760   10800    2a30 minimal.elf

The above, and equal number of UARTs:
   text    data     bss     dec     hex filename
  14888     172    2748   17808    4590 default.elf
  10620     136    2744   13500    34bc hello-world.elf
   7788     136    2744   10668    29ac minimal.elf

The above, without LFXO support (not used in SLWSTK6220A):
   text    data     bss     dec     hex filename
  14872     168    2752   17792    4580 default.elf
  10604     132    2748   13484    34ac hello-world.elf
   7772     132    2748   10652    299c minimal.elf

The above, and LTO=yes (broken binary!):
   text    data     bss     dec     hex filename
  12712     124    2620   15456    3c60 default.elf
   8872     120    2616   11608    2d58 hello-world.elf
   5952     120    2616    8688    21f0 minimal.elf

Baseline versus old:

  • default.elf: +25%
  • hello-world.elf: +35%
  • minimal.elf: +50%

Baseline versus new:

  • default.elf: +25%
  • hello-world.elf: +34%
  • minimal.elf: +49%

Baseline versus equal:

  • default.elf: +17%
  • hello-world.elf: +23%
  • minimal.elf: +33%

@basilfx
Copy link
Member Author

basilfx commented Feb 22, 2016

Inspired by #4866:

But again, these are quick and not in-depth observations. Looking for example at the gnrc_networking example, the emlib implementation uses only 7.5% ROM, at a code size of ~65k this is still a lot, but could be tolerated if it really brings other advantages...

emlib uses bit banding, where applicable. I see it is used in GPIO, Timer, USART, RTC and other abstractions. Now I understand why, I see this as an advantage that also increases code size a bit.

@basilfx basilfx force-pushed the feature/efm32_stk3600 branch from 891b971 to 45406af Compare February 24, 2016 21:29
@haukepetersen
Copy link
Contributor

The numbers look indeed slightly better. I won't oppose using the emlib, though I have to say (and looking at the code), I still don't like it at all. I won't have the time to review this in depth this or next week.

@thomaseichinger: would you mind to take a look?

@basilfx basilfx force-pushed the feature/efm32_stk3600 branch 2 times, most recently from b290a9d to f472b5a Compare February 29, 2016 17:14
@basilfx
Copy link
Member Author

basilfx commented Mar 16, 2016

I will update this PR once all the peripheral-changing PRs are merged.

@basilfx basilfx force-pushed the feature/efm32_stk3600 branch 2 times, most recently from 4e7efd0 to a9215e8 Compare March 22, 2016 17:19
@miri64 miri64 modified the milestone: Release 2016.07 Mar 29, 2016
@basilfx basilfx force-pushed the feature/efm32_stk3600 branch from a9215e8 to 125b393 Compare May 11, 2016 22:57
@basilfx
Copy link
Member Author

basilfx commented May 11, 2016

I have updated this PR to work with the API changes proposed in the last few months.

For the record, emlib now supports 20 different EFM32 families, for which I generate code.

@chrysn
Copy link
Member

chrysn commented May 18, 2016

The branch works well for me (I'm new to RIOT, building own examples based on hello-world example, using a Giant Gecko STK instead of a Leopard Gecko; didn't test jlink for programming, but it works with OpenOCD out of the box).

When I find bugs or shortcomings (eg. wrong pin direction in usart RX pin setup, baud rates not achieved for lack of CMU_ClockDivSet), do you prefer me to report them here, or to open dedicated bugs against RIOT once this is merged?

@basilfx
Copy link
Member Author

basilfx commented May 18, 2016

You can fork my branch, make the fix and I will pull it in :-)

I use a generator to bootstrap this port as long as it not merged yet. I do this because it creates the files for all the different EFM32 families automatically. You can also make a PR over there: https://github.com/basilfx/EFM2Riot

@basilfx basilfx force-pushed the feature/efm32_stk3600 branch from 125b393 to 3e61266 Compare June 15, 2016 22:26
@basilfx
Copy link
Member Author

basilfx commented Jun 15, 2016

I know it's a big PR, but I'd like to receive some feedback sometime :-)

@OlegHahm
Copy link
Member

Sorry for the delay - the assigned maintainer (@haukepetersen) has been offline for a loooong time. Maybe another maintainer is willing to take over? @kaspar030, @gebart, @thomaseichinger, @PeterKietzmann?

@kYc0o
Copy link
Contributor

kYc0o commented Jul 25, 2016

Feature freeze -> postponed.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016
@basilfx
Copy link
Member Author

basilfx commented Sep 4, 2016

We could close this PR and continue with #5652. Most of this code is part of efm32_common, which is also in the other PR. If that one is accepted, then adding all of the other boards is little work.

OK?

@OlegHahm
Copy link
Member

OlegHahm commented Sep 4, 2016

Makes sense to me.

@basilfx
Copy link
Member Author

basilfx commented Sep 5, 2016

Ok, let's continue in #5652.

@basilfx basilfx closed this Sep 5, 2016
@basilfx basilfx deleted the feature/efm32_stk3600 branch November 19, 2017 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants