Skip to content

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

Merged
dylad merged 6 commits intoRIOT-OS:masterfrom
benpicco:sam0-gclk
Feb 5, 2020
Merged

cpu/sam0: provide interface to query GCLK frequency#12969
dylad merged 6 commits intoRIOT-OS:masterfrom
benpicco:sam0-gclk

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Dec 16, 2019

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

@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances 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 Area: cpu Area: CPU/MCU ports labels Dec 16, 2019
@benpicco 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
keestux reviewed Dec 18, 2019
View reviewed changes
@benpicco benpicco added State: waiting for other PR State: The PR requires another PR to be merged first State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 20, 2019
SodaqMoja
SodaqMoja reviewed Jan 16, 2020
View reviewed changes
SodaqMoja
SodaqMoja reviewed Jan 16, 2020
View reviewed changes
@keestux
Copy link
Copy Markdown
Contributor

keestux commented Jan 16, 2020

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,

@fjmolinas 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
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jan 17, 2020

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

keestux
keestux reviewed Jan 17, 2020
View reviewed changes
keestux
keestux reviewed Jan 17, 2020
View reviewed changes
keestux
keestux reviewed Jan 17, 2020
View reviewed changes
keestux
keestux reviewed Jan 17, 2020
View reviewed changes
keestux
keestux reviewed Jan 17, 2020
View reviewed changes
@benpicco 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
keestux reviewed Jan 22, 2020
View reviewed changes
keestux
keestux reviewed Jan 22, 2020
View reviewed changes
keestux
keestux reviewed Jan 22, 2020
View reviewed changes
@benpicco benpicco force-pushed the sam0-gclk branch 2 times, most recently from 89fcbb8 to e48dde5 Compare January 24, 2020 09:07
keestux
keestux reviewed Jan 25, 2020
View reviewed changes
@benpicco benpicco mentioned this pull request Jan 30, 2020
Merged
@dylad
Copy link
Copy Markdown
Member

dylad commented Jan 31, 2020

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
dylad reviewed Feb 4, 2020
View reviewed changes
Copy link
Copy Markdown
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.

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

@keestux
Copy link
Copy Markdown
Contributor

keestux commented Feb 4, 2020

It looks good to me.
ACK

@dylad
Copy link
Copy Markdown
Member

dylad commented Feb 4, 2020

It looks good to me.
ACK

Nice !

@benpicco Please squash !

@benpicco
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
cpu/sam0: don't hard-code peripheral clocks
1496149 
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.
@benpicco
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
@benpicco
cpu/samd21: only configure one 32kHz GCLK
df33ffd 
Use the same 32 kHz GCLK to feed the PLL and the RTT, etc.
@benpicco
cpu/sam0: use defines for GCLK IDs
38b6ee5 
Give the clocks explicit names to better identify their meaning.
@benpicco
boards: use explicit GCLK_SRC defines
c4e0ed4
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Feb 4, 2020

Squashed & rebased to the master.

dylad
dylad approved these changes Feb 5, 2020
View reviewed changes
Copy link
Copy Markdown
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.
Thanks for this PR @benpicco !

@dylad
Copy link
Copy Markdown
Member

dylad commented Feb 5, 2020

Here we go.

@dylad dylad merged commit 83604db into RIOT-OS:master Feb 5, 2020
@benpicco benpicco deleted the sam0-gclk branch February 5, 2020 09:12
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Feb 5, 2020

Thank you all for reviewing & testing this!

fjmolinas
fjmolinas reviewed Feb 7, 2020
View reviewed changes
cpu/samd21/cpu.c
while (!(SYSCTRL->PCLKSR.reg & SYSCTRL_PCLKSR_OSC8MRDY)) {}
#endif

/* Setup GCLK2 with divider 1 (32.768kHz) */
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Feb 7, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Feb 7, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Feb 7, 2020

Choose a reason for hiding this comment

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

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.

@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keestux keestux keestux left review comments

@fjmolinas fjmolinas fjmolinas left review comments

@dylad dylad dylad approved these changes

@bergzand bergzand Awaiting requested review from bergzand

+1 more reviewer

@SodaqMoja SodaqMoja SodaqMoja left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.04

Development

Successfully merging this pull request may close these issues.

cpu/sam0_common: i2c baudrate calculation fails if CLOCK_CORECLOCK > 51 MHz

6 participants

@benpicco @keestux @dylad @SodaqMoja @fjmolinas @leandrolanzieri