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

CPU SAML21: fix timers frequency when using slow clocks #16446

Merged
merged 4 commits into from
May 6, 2021

Conversation

ant9000
Copy link
Contributor

@ant9000 ant9000 commented May 5, 2021

My recently merged PR #16433 introduces a bug: when OSC16M is not running at 16MHz, the timer clock SAM0_GCLK_8MHZ is no longer correct.

I've renamed SAM0_GCLK_8MHZ as SAM0_GCLK_TIMER, and compute it as 4MHz or 8MHz depending on the frequency of the core clock - CLOCK_CORECLOCK at 4MHz or at 12MHz result in SAM0_GCLK_TIMER at 4MHz, all other available core clocks still produce the original 8MHz one.

@ant9000 ant9000 requested a review from dylad as a code owner May 5, 2021 08:47
@benpicco benpicco added Area: cpu Area: CPU/MCU ports Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels May 5, 2021
@ant9000 ant9000 force-pushed the pr_cpu_saml21_slow_clock_fix_timers branch from 8392e2f to 2b213b2 Compare May 5, 2021 08:54
@benpicco
Copy link
Contributor

benpicco commented May 5, 2021

Looks good to me - just a minor thing: please prefix the commits with the subsystem they are touching, so

  • cpu/saml21: fix timer skew for slow clocks
  • boards: use SAM0_GCLK_TIMER for saml21 boards

@ant9000 ant9000 force-pushed the pr_cpu_saml21_slow_clock_fix_timers branch from 2b213b2 to 7a253b4 Compare May 5, 2021 08:57
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 5, 2021
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

One comment otherwise looks good.

cpu/saml21/cpu.c Outdated Show resolved Hide resolved
cpu/saml21/cpu.c Outdated Show resolved Hide resolved
@ant9000 ant9000 force-pushed the pr_cpu_saml21_slow_clock_fix_timers branch 3 times, most recently from c72f429 to cd6912e Compare May 5, 2021 14:32
@ant9000 ant9000 force-pushed the pr_cpu_saml21_slow_clock_fix_timers branch from cd6912e to c03816f Compare May 5, 2021 14:34
@dylad
Copy link
Member

dylad commented May 5, 2021

LGTM, tested on saml21-xpro with tests/xtimer_usleep

@benpicco any last word ?

cpu/saml21/cpu.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

benpicco commented May 5, 2021

There is a case for using OSC16M instead of the DFLL - the buck converter can only be enabled if no DPLL/DFLL is used (see errata sheet).
But I can do that as a follow-up.

@ant9000
Copy link
Contributor Author

ant9000 commented May 5, 2021

There is a case for using OSC16M instead of the DFLL - the buck converter can only be enabled if no DPLL/DFLL is used (see errata sheet).
But I can do that as a follow-up.

If I am not mistaken, that errata should be already taken care of, in _set_active_mode_vreg; but if you really need a 48MHz clock (for USB, for instance), you just can't work around enabling DFLL48M (at present, DPLL96M is not supported at all in RIOT).

Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

ACK.
Re-tested on SAML21. Looks good so far.

@dylad
Copy link
Member

dylad commented May 6, 2021

Here we go.

@dylad dylad merged commit a1c3782 into RIOT-OS:master May 6, 2021
@ant9000 ant9000 deleted the pr_cpu_saml21_slow_clock_fix_timers branch May 6, 2021 22:30
@ant9000
Copy link
Contributor Author

ant9000 commented May 6, 2021

Thanks a lot!

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants