Skip to content

kinetis: Refactor clock initialization code#8928

Merged
kYc0o merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-clock-init
Apr 17, 2018
Merged

kinetis: Refactor clock initialization code#8928
kYc0o merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-clock-init

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Apr 11, 2018 •
edited
Loading

Contribution description

  • Move RTC oscillator configuration to the clock_config struct and handle it from kinetis_mcg_init
  • Modularize kinetis_mcg_init to improve readability
  • Merge boolean settings in clock_config_t into a single clock_flags field

Issues/PRs references

Used by #8814, #8930, #8933

@jnohlgard jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 11, 2018
@jnohlgard jnohlgard force-pushed the pr/kinetis-clock-init branch from 6635d12 to 308c828 Compare April 11, 2018 18:32
@jnohlgard jnohlgard mentioned this pull request Apr 11, 2018
Merged
@jnohlgard jnohlgard force-pushed the pr/kinetis-clock-init branch from 308c828 to c6478ac Compare April 12, 2018 06:09
This was referenced Apr 12, 2018
Merged
Merged
@jnohlgard jnohlgard added this to the Release 2018.07 milestone Apr 12, 2018
@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 12, 2018 •
edited
Loading

Can you rebase? I'm testing this PR and so far I found:

  • frdm-kw41z (broken also in master) works!
  • pba-d-01-kw2x works!
  • mulle works!
  • frdm-k64f works!

@jnohlgard jnohlgard force-pushed the pr/kinetis-clock-init branch from c6478ac to 06cbdf4 Compare April 12, 2018 14:12
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 12, 2018

@kYc0o
Rebased.
What is the issue you are experiencing with frdm-kw41z? I tested with my frdm-kw41z board right now and it works fine with the examples/default application

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 12, 2018

Sorry, I forgot to say that I was testing with tests/periph_rtc.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 12, 2018

Ah, I just realised that the RTC oscillator is not started... need to examine

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 12, 2018

found the source of the issue on frdm-kw41z. Will post update

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 12, 2018

@kYc0o
Try again on FRDM-KW41Z

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 12, 2018

Perfect! Works like a charm.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 12, 2018

Please squash.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 12, 2018

All in one commit?

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 12, 2018

I'd say one for the boards and one for the cpu. Though @cladmi has preference to have working commits... But well, here there are quite some changes to make them all in one commit.

@jnohlgard jnohlgard force-pushed the pr/kinetis-clock-init branch from 233ac5c to 38f2a26 Compare April 13, 2018 10:07
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 13, 2018

@kYc0o I tend to agree with @cladmi, it is nice if all commits are atomic and compilable. It certainly makes bisecting easier since you don't have to skip over commits which are not compiling.

I squashed it all to one, I will split it if you still want one for the CPU and one for the boards

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 13, 2018

Ok, I agree. All in one is ok! Thanks for the explanation.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 13, 2018

Murdock found some problems. Can you take a look @gebart ?

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 13, 2018

Will address. I forgot that C++ won't accept ints as enums.

@jnohlgard jnohlgard force-pushed the pr/kinetis-clock-init branch from 38f2a26 to aa15147 Compare April 16, 2018 11:19
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 16, 2018

fixed the c++ errors. OK to squash?

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Apr 16, 2018

Yes! Please go ahead!

@jnohlgard jnohlgard force-pushed the pr/kinetis-clock-init branch from 1a1b446 to c54f6b4 Compare April 17, 2018 04:59
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 17, 2018

squashed

@jnohlgard jnohlgard mentioned this pull request Apr 17, 2018
Closed
5 tasks
kYc0o
kYc0o approved these changes Apr 17, 2018
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.

ACK.

@kYc0o kYc0o merged commit 689333f into RIOT-OS:master Apr 17, 2018
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Apr 17, 2018

Thanks for the review

@jnohlgard jnohlgard deleted the pr/kinetis-clock-init branch April 17, 2018 10:19
maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018
@kYc0o @maxvankessel
Merge pull request RIOT-OS#8928 from gebart/pr/kinetis-clock-init
af40b37 
kinetis: Refactor clock initialization code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kYc0o kYc0o kYc0o approved these changes

@astralien3000 astralien3000 Awaiting requested review from astralien3000

Assignees

No one assigned

Labels

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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

2 participants

@jnohlgard @kYc0o