cpu/sam0: provide interface to query GCLK frequency #12969

benpicco · 2019-12-16T18:58:22Z

Contribution description

The sam0 family offers multiple GCLKs that can be sourced from different timers.
However, all peripherals were effectively always clocked by the hard-coded CLOCK_CORECLOCK.

This causes several issues:

on high CPU frequencies (> 51 MHz) the available I2C divider is too small to produce the required I2C baud rate
on low CPU frequencies the available frequency is too low to produce high UART baudrates.

This introduces a sam0_gclk_freq() function to query the frequency of the individual GCLKs, so other GCLKs than 0 can be used for peripherals.

This would also allow to change the CPU frequency at run-time without affecting peripherals, unless they use GCLKs sourced from GCLK0.

The first commit in this series also changes the .gclk_src config struct from a bit mask to the GCLK ID.
While not strictly necessary, it cleans up the board configurations and removes the need to differentiate between samd2x and later MCUs.

Testing procedure

In only tested the changes on samr21-xpro and same54-xpro

On same54-xpro you will find that i2c_scan (and I2C in general) now works (USEMODULE += i2c_scan), before it would get stuck due to a wrong clock frequency.

Issues/PRs references

fixes #12037

cpu/samd5x/cpu.c

boards/sodaq-autonomo/include/periph_conf.h

boards/samd21-xpro/include/periph_conf.h

keestux · 2020-01-16T19:27:43Z

Let me mention it once more. There are now a lot of places where the code has 0, 1, 5, etc, but these numbers mean nothing unless you know the (undocumented) values. For example, 0 is for the core clock, 1 is a 1MHz clock, etc.

In the mean time a define was added in periph_cpu.h, namely SAM0_GCLK_32KHZ. Why don't we extend this section and have defines for the other GCLKs? We can add (a brief) description what the GCLK is (frequency, power down mode, etc). Maybe just as a start. And if you're adventurous you could change all these

    .gclk_src       = 0,
...
    .gclk_src       = 1,

into

    .gclk_src       = SAM0_GCLK_MAIN,
...
    .gclk_src       = SAM0_GCLK_1MHZ,

benpicco · 2020-01-17T09:09:36Z

Alright, that makes sense!
I'll do a separate commit.

cpu/samd21/cpu.c

cpu/samd21/include/periph_cpu.h

cpu/samd5x/cpu.c

cpu/saml21/cpu.c

cpu/sam0_common/periph/rtc.c

cpu/samd21/cpu.c

dylad · 2020-01-31T10:17:04Z

I ran dist/tools/compile_and_test_for_board/compile_and_test_for_board.py for SAML10, SAML11, SAML21 and SAME54, so far so good.
If @keestux 's comments were addressed and if SAMD21 is also happy, we should be good to go.

dylad

two small changes.
@keestux @aabadie could you check if samd21 is still happy with this PR please ?
We should be good to go.

cpu/samd5x/cpu.c

keestux · 2020-02-04T19:04:54Z

It looks good to me.
ACK

dylad · 2020-02-04T19:15:50Z

It looks good to me.
ACK

Nice !

@benpicco Please squash !

To simplify board definitions and for unification between samd2x and newer models, don't use the GCLK bitmask in board definitions. Instead use the GCLK index and generate the bitmask when needed.

Instead of hard-coding the peripheral clocks to CLOCK_CORECLOCK introduce helper functions to return the frequency of the individual GCLKs and use those for baud-rate calculations. This requires the GCLK to be part of the peripheral's config struct. While this is already the case for most peripherals, this also adds it for those where it wasn't used before. As it defaults to 0 (CLOCK_CORECLOCK) no change is to be expected.

We can't run I2C off the 120 MHz main clock as the availiable dividers are too small. Use the 48 MHz GCLK 6 instead which offers an appropriate frequency. fixes RIOT-OS#12037

Use the same 32 kHz GCLK to feed the PLL and the RTT, etc.

Give the clocks explicit names to better identify their meaning.

benpicco · 2020-02-04T21:14:55Z

Squashed & rebased to the master.

dylad

ACK.
Thanks for this PR @benpicco !

dylad · 2020-02-05T08:29:43Z

Here we go.

benpicco · 2020-02-05T09:18:12Z

Thank you all for reviewing & testing this!

fjmolinas · 2020-02-07T15:08:31Z

cpu/samd21/cpu.c

@@ -72,19 +94,35 @@ static void clk_init(void)
    while (!(SYSCTRL->PCLKSR.reg & SYSCTRL_PCLKSR_OSC8MRDY)) {}
 #endif

+    /* Setup GCLK2 with divider 1 (32.768kHz) */


@benpicco why did you move this up? GCLK->CTRL.reg = GCLK_CTRL_SWRSTresets what is done here, can I move it back down in #13306? Iget the same drift as before otherwise.

I see you use it for the dfll, I'll move GCLK->CTRL.reg = GCLK_CTRL_SWRS up.

Hm previously this was in the #if CLOCK_USE_PLL block, but a 32kHz clock is always needed so I moved it here.
I must have missed the GCLK_CTRL_SWRS bit.

benpicco requested review from bergzand, dylad and keestux December 16, 2019 18:58

benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 17, 2019

keestux reviewed Dec 18, 2019

View reviewed changes

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved

benpicco force-pushed the sam0-gclk branch from c1ba3af to 723d156 Compare January 16, 2020 15:10

SodaqMoja reviewed Jan 16, 2020

View reviewed changes

boards/sodaq-autonomo/include/periph_conf.h Outdated Show resolved Hide resolved

SodaqMoja reviewed Jan 16, 2020

View reviewed changes

boards/samd21-xpro/include/periph_conf.h Outdated Show resolved Hide resolved

fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 17, 2020

keestux reviewed Jan 17, 2020

View reviewed changes

cpu/samd21/cpu.c Outdated Show resolved Hide resolved

keestux reviewed Jan 17, 2020

View reviewed changes

cpu/samd21/include/periph_cpu.h Show resolved Hide resolved

keestux reviewed Jan 17, 2020

View reviewed changes

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved

keestux reviewed Jan 17, 2020

View reviewed changes

cpu/saml21/cpu.c Outdated Show resolved Hide resolved

keestux reviewed Jan 17, 2020

View reviewed changes

cpu/sam0_common/periph/rtc.c Outdated Show resolved Hide resolved

benpicco force-pushed the sam0-gclk branch from 9f5b40f to 8afaf6b Compare January 18, 2020 16:29

benpicco removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 22, 2020

keestux reviewed Jan 22, 2020

View reviewed changes

cpu/samd21/cpu.c Outdated Show resolved Hide resolved

keestux reviewed Jan 22, 2020

View reviewed changes

cpu/samd21/cpu.c Outdated Show resolved Hide resolved

keestux reviewed Jan 22, 2020

View reviewed changes

cpu/samd21/cpu.c Outdated Show resolved Hide resolved

benpicco force-pushed the sam0-gclk branch 2 times, most recently from 89fcbb8 to e48dde5 Compare January 24, 2020 09:07

keestux reviewed Jan 25, 2020

View reviewed changes

cpu/samd21/cpu.c Outdated Show resolved Hide resolved

benpicco mentioned this pull request Jan 30, 2020

pkg: add library for Microchip CryptoAuth devices as package #13014

Merged

dylad reviewed Feb 4, 2020

View reviewed changes

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved

benpicco added 2 commits February 4, 2020 21:06

cpu/sam0: use GCLK ID instead of bitmask

a51d167

To simplify board definitions and for unification between samd2x and newer models, don't use the GCLK bitmask in board definitions. Instead use the GCLK index and generate the bitmask when needed.

benpicco force-pushed the sam0-gclk branch from 5ccde61 to 894e490 Compare February 4, 2020 20:07

benpicco added 4 commits February 4, 2020 21:16

boards/same54-xpro: use 48 MHz GCLK6 for I2C

4fbca25

We can't run I2C off the 120 MHz main clock as the availiable dividers are too small. Use the 48 MHz GCLK 6 instead which offers an appropriate frequency. fixes RIOT-OS#12037

cpu/samd21: only configure one 32kHz GCLK

df33ffd

Use the same 32 kHz GCLK to feed the PLL and the RTT, etc.

cpu/sam0: use defines for GCLK IDs

38b6ee5

Give the clocks explicit names to better identify their meaning.

boards: use explicit GCLK_SRC defines

c4e0ed4

benpicco force-pushed the sam0-gclk branch from 894e490 to c4e0ed4 Compare February 4, 2020 20:18

dylad approved these changes Feb 5, 2020

View reviewed changes

dylad merged commit 83604db into RIOT-OS:master Feb 5, 2020

benpicco deleted the sam0-gclk branch February 5, 2020 09:12

fjmolinas reviewed Feb 7, 2020

View reviewed changes

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/sam0: provide interface to query GCLK frequency #12969

cpu/sam0: provide interface to query GCLK frequency #12969

benpicco commented Dec 16, 2019

keestux commented Jan 16, 2020

benpicco commented Jan 17, 2020

dylad commented Jan 31, 2020

dylad left a comment

keestux commented Feb 4, 2020

dylad commented Feb 4, 2020

benpicco commented Feb 4, 2020

dylad left a comment

dylad commented Feb 5, 2020

benpicco commented Feb 5, 2020

fjmolinas Feb 7, 2020

fjmolinas Feb 7, 2020

benpicco Feb 7, 2020

cpu/sam0: provide interface to query GCLK frequency #12969

cpu/sam0: provide interface to query GCLK frequency #12969

Conversation

benpicco commented Dec 16, 2019

Contribution description

Testing procedure

Issues/PRs references

keestux commented Jan 16, 2020

benpicco commented Jan 17, 2020

dylad commented Jan 31, 2020

dylad left a comment

Choose a reason for hiding this comment

keestux commented Feb 4, 2020

dylad commented Feb 4, 2020

benpicco commented Feb 4, 2020

dylad left a comment

Choose a reason for hiding this comment

dylad commented Feb 5, 2020

benpicco commented Feb 5, 2020

fjmolinas Feb 7, 2020

Choose a reason for hiding this comment

fjmolinas Feb 7, 2020

Choose a reason for hiding this comment

benpicco Feb 7, 2020

Choose a reason for hiding this comment