samd21:setup all clock generators in clock.c #7315

photonthunder · 2017-07-04T20:13:35Z

Goal is to have all clock generators setup in clock.c and then turned on/off in periph_conf.c. Does contain some cleanup as well.

kaspar030 · 2017-07-04T20:21:16Z

boards/samd21-xpro/include/periph_conf.h

@@ -272,6 +277,12 @@ static const spi_conf_t spi_config[] = {
 #define RTT_MAX_VALUE       (0xffffffff)
 #define RTT_FREQUENCY       (32768U)    /* in Hz. For changes see `rtt.c` */
 #define RTT_RUNSTDBY        (1)         /* Keep RTT running in sleep states */
+
+#if RTT_NUMOF || RTC_NUMOF
+#ifndef XOSC32_RUNSTBY


seems like a D is missing

kaspar030 · 2017-07-04T20:21:35Z

boards/samd21-xpro/include/periph_conf.h

-#define GEN2_ULP32K         (1)
-#else
+#define CLOCK_CORECLOCK     (48000000UL)
+#elif CLOCK_USE_8MHZ_DEFAULT
 /* edit this value to your needs */
 #define CLOCK_DIV           (1U)
 /* generate the actual core clock frequency */
 #define CLOCK_CORECLOCK     (8000000 / CLOCK_DIV)
 #endif


Does it make sense to add an error case here?

Added, I wish there was a better way to set this up to make it cleaner...

dylad · 2017-07-04T20:40:35Z

I'll try to take a deeper look soon but what I can already said : there are some broken boards with the current changes

sodaq autonomo
arduino zero
arduino mkr1000

photonthunder · 2017-07-05T15:20:06Z

@dylad we could move the #ifndef statements from cpu.c to a more global header, but I didn't know which would be appropriate. We can of course tweak the appropriate periph_conf.h files. @kaspar030 thanks. I am interested in feedback on a better method for handling the clocks in periph_conf.h, something easier to read and tweak?

biboc · 2017-07-10T07:23:48Z

That's a good idea to move everything in clock.c
Do it for SAML21 as well, it is very similar.
Should you move clock.c in sam0_common folder?

photonthunder · 2017-07-10T15:54:07Z

@biboc, @dylad had made the same request. I was thinking we would get this approved and then expand to the SAML21, but we can expand it now to help get it moving. Mostly waiting for feedback on where to put the #ifndef so that you don't have to include all the options in every periph_conf.h.

photonthunder · 2017-08-15T16:04:27Z

Any feedback on these generic clock generators? Need to clean this up since both RTT and the internal 32 kHz oscillator try to use generator 2 you can end up with a very difficult bug to find. Almost need a periph_clock.h to go along with periph_conf.h. You put the default settings (#ifndef) in periph_clock.h and place that after periph_conf.h in the appropriate files. I will experiment but would greatly appreciate some ideas.

photonthunder · 2017-08-16T17:45:04Z

@biboc, @dylad I was just looking at the saml21 board and I don't see any code referring to the different generic clock generators? Don't even setup the external oscillator for rtt.c? So I think I will try and get this working for the samd21 only for now.

photonthunder · 2017-08-16T18:53:44Z

@kaspar030 I moved the ifndef to an include file (periph_clock_config.h) and added it to the appropriate samd21 files. All boards now compile. Rebased and Squashed as well.

dylad · 2017-08-17T10:49:04Z

So I think I will try and get this working for the samd21 only for now

Ok, once your PR will be merged, I'll expand your work to SAML21 family.

haukepetersen

Nice one, I like the direction!

Looking at the clock configuration code, I am wondering if we can't be more 'user friendly' here. In the end, we want to define only the values we absolutely have to. So thinking out loud: could we cut the config down to something like this:

/* select the main clock source:
 *  0: PLL fed by internal 8MHz oscillator
 *  1: PLL fed by external 32.768kHz oscillator
 *  2: internal 8MHz oscillator
 */
#define CLOCK_SRC      (0)
/* define core clock frequency [in Hz] */
#define CLOCK_CORECLOCK   (48000000U)
/* is an external 32.768 oscillator available? */
#define CLOCK_HAS_XOSC32  (1)

The rest of the parameters can simply be computed by the given data:
case 0: compute MUL and DIV
case 1: compute data from CLOCK_CORECLOCK
case 2: compute N from CLOCK_CORECLOCK

What do you think?

haukepetersen · 2017-08-28T15:36:37Z

boards/arduino-mkr1000/include/periph_conf.h

@@ -34,16 +34,17 @@ extern "C" {
 #endif

 /**
- * @brief   External oscillator and clock configuration
+ * @name   External oscillator and clock configuration


space missing

haukepetersen · 2017-08-28T15:39:17Z

boards/arduino-mkr1000/include/periph_conf.h


 #if CLOCK_USE_PLL
-/* edit these values to adjust the PLL output frequency */
+    /* edit these values to adjust the PLL output frequency */


these indention seems to be off

haukepetersen · 2017-08-28T15:42:35Z

cpu/samd21/include/periph_clock_config.h

+
+/**
+ * @brief   Default Clock configurations
+ */


I think doxygen will be more happy if you use

/** * @name ... * @{ */ /** @} */

here.

haukepetersen · 2017-08-28T15:43:13Z

cpu/samd21/include/periph_clock_config.h

+ * @brief   Default Clock configurations
+ */
+
+/* If RTC or RTT is on Need XOSC32 */


is in need of?

haukepetersen · 2017-08-28T15:45:05Z

cpu/samd21/include/periph_clock_config.h

+#ifndef GEN2_XOSC32
+#define GEN2_XOSC32         (1)
+#endif
+#endif


I must say I am not so happy with the RTC/RTT dependency here. I would prefer that the cpu clock module merely provides functions to enable/disable the low frequency clock, which then in term can be used by the RTT/RTC. This is how we recently refactored the clock code for the stm32 family...

So more precise: IMHO the board config should only define if an external 32khz oscillator is available, and the RTC/RTT modules should then simply request enabling that clock from this module.

Or of course the XOSC32K clock is enabled during system initialization in case it is configured to be feeding the PLL.

photonthunder · 2017-08-28T16:43:42Z

@haukepetersen thanks for looking at this. I do like your comments about core clock configuration since I imagine more options will be introduce in the future. I also like the idea of doing an enable/disable function, I will look at the stm32 code and try to mimic.

photonthunder · 2017-08-29T21:51:20Z

In this version there is just two parameters visible by default for core clock configuration (CLOCK_SRC and CLOCK_CORECLOCK). Interesting thing is that I found what could be a bug in gpio.c where it uses the generic clock generator 2. Well in the old code clock generator 2 was either the ultra low power 32 kHz or the external oscillator. All three modes do appear to work for the samd21-xpro. I also rebased before making these changes.

travisgriggs · 2017-09-11T23:45:44Z

Hi @kaspar030 and @haukepetersen. Are we on the right direction with this still here? We would love to see this go in. It would make our own maintenance with RIOT a lot simpler since we use this in production.

photonthunder · 2017-09-13T03:23:49Z

updated external interrupt gpio so that it uses XOSC32 if it is available. Gives better response on interrupt.

dylad · 2018-07-31T07:13:36Z

@photonthunder looks like Travis is complaining again. Still some whitespaces in the wild.

photonthunder · 2018-08-02T01:30:13Z

Fingers crossed this time...

dylad · 2018-08-02T11:37:34Z

Murdock approves. I'll try the PR on a samd21 ASAP just to make sure boards still run.

dylad

@photonthunder I ran some tests on arduino-zero-like board this looks good except RTC which is broken.

Here some outputs with your branch vs master branch :

2018-08-03 09:32:46,521 - INFO # main(): This is RIOT! (Version: 2018.07-devel-1052-g5c57-dylan-meso-samd21_clock)
2018-08-03 09:32:46,522 - INFO # 
2018-08-03 09:32:46,524 - INFO # RIOT RTC low-level driver test
2018-08-03 09:32:46,529 - INFO # This test will display 'Alarm!' every 5 seconds for 4 times
2018-08-03 09:32:46,533 - INFO #   Setting clock to   2011-12-13 14:15:15
2018-08-03 09:32:46,538 - INFO # Clock value is now   2011-12-13 14:15:15
2018-08-03 09:32:46,542 - INFO #   Setting alarm to   2011-12-13 14:15:20
2018-08-03 09:32:46,546 - INFO #    Alarm is set to   2011-12-13 14:15:20
2018-08-03 09:32:46,547 - INFO # 
2018-08-03 09:32:46,702 - INFO # Alarm!
2018-08-03 09:32:46,858 - INFO # Alarm!
2018-08-03 09:32:47,015 - INFO # Alarm!
2018-08-03 09:32:47,171 - INFO # Alarm!

2018-08-03 09:34:01,465 - INFO # main(): This is RIOT! (Version: 2018.07-devel-907-g446f-dylan-meso)
2018-08-03 09:34:01,466 - INFO # 
2018-08-03 09:34:01,468 - INFO # RIOT RTC low-level driver test
2018-08-03 09:34:01,473 - INFO # This test will display 'Alarm!' every 5 seconds for 4 times
2018-08-03 09:34:01,477 - INFO #   Setting clock to   2011-12-13 14:15:15
2018-08-03 09:34:01,522 - INFO # Clock value is now   2011-12-13 14:15:15
2018-08-03 09:34:01,526 - INFO #   Setting alarm to   2011-12-13 14:15:20
2018-08-03 09:34:01,541 - INFO #    Alarm is set to   2011-12-13 14:15:20
2018-08-03 09:34:01,541 - INFO # 
2018-08-03 09:34:07,478 - INFO # Alarm!
2018-08-03 09:34:12,478 - INFO # Alarm!
2018-08-03 09:34:17,478 - INFO # Alarm!
2018-08-03 09:34:22,477 - INFO # Alarm!

I just changed PERIOD from 2 to 5 in tests/periph_rtc (but same behaviour with PERIOD = 2)

photonthunder · 2018-08-04T19:43:51Z

@dylad, I know this is going to sound crazy but I think that is correct since the default option is running without standby on, so RTC doesn't run in deep sleep mode. This is why I started Issue #7913 to try and come up with a better approach. Will you try the same test but change setup_gen2_xosc32(false); to true and check the result?

dylad · 2018-08-05T21:37:26Z

THB, I don't think its a good idea to disable RTC by default. Users may want to use calendar, or trigger an event at specific time.
As long as MODULE_PERIPH_RTC is present, RTC must be enable for user.

photonthunder · 2018-08-05T22:45:53Z

So the existing code for RTC has standby off by default. That doesn't mean RTC is off it just means that it doesn't run in sleep. I think this is the larger issue that needs to be addressed with these chips is a method for indicating that whether or not you want things running in standby mode, which needs to occur when you setup the initial clock generators. Thus you need a whole bunch of defines which quickly gets overwhelming for users.

dylad · 2018-08-08T18:27:59Z

@photonthunder I agree with you. We need a way to enable/disable standby mode efficiently. But in the current state, tests/periph_rtc doesn't work with this PR (standby mode or not) this need to be fix. We can then think about how to enable/disable standby features later.

photonthunder · 2018-08-08T19:41:57Z

@dylad sounds good, so did you try it with with standby enabled? setup_gen2_xosc32(true); Did the test pass?

dylad · 2018-08-08T20:05:03Z

I cannot run any tests as I am on vacation right now.
If someone can test it would be great.
Anyway, If this works with setup_gen2_xosc32(true) then this PR need to be adapted as well

photonthunder · 2018-08-13T17:12:34Z

Updated to true, but I am on the road and don't have hardware to test...

dylad · 2018-09-21T19:51:03Z

I just retested this PR, (Sorry for the delay, I had to order another board because the previous died)
tests/periph_rtc still don't have the required behaviour.

keestux · 2019-04-30T18:47:51Z

What's the status of this PR?

Oh, and by the way, the commit message has "...all clock generators in clock.c", but there is no clock.c. Is that an old commit message?

photonthunder · 2019-04-30T19:20:30Z

The whole thing is very old. I explored other methods to try and handle the challenge that each peripheral is tied back to a GCLK and how to make that interactive without so many options (see #7913) but I haven't touched this in a long time so it probably needs a clean look.. probably just close this and someone can start a new PR if they desire...

keestux · 2019-04-30T19:26:13Z

Testing with Sodaq ONE (with a modified periph_conf.h, because it is missing in the patch) gives incorrect timing of tests/periph_rtc. All Alarm messages are printed at once. There should be a 2s delay between them.

tests/periph_rtt seems to behave properly.

photonthunder · 2019-04-30T19:31:25Z

Feel free to hack away if you have time and interest. I may be able to look at this later this week or next week. I just figured most people where just hacking what they needed for each specific board?

keestux · 2019-04-30T19:34:20Z

I should have shown more interest long time ago. Sorry about that.

stale · 2019-11-01T19:36:20Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

photonthunder mentioned this pull request Jul 4, 2017

samd21: pull all generic clock generators into clock.c #7208

Closed

kaspar030 previously requested changes Jul 4, 2017

View reviewed changes

aabadie modified the milestone: Release 2017.10 Jul 12, 2017

photonthunder force-pushed the samd21_clock branch from 7889368 to 3f316f3 Compare August 16, 2017 17:34

photonthunder force-pushed the samd21_clock branch from 46b3071 to 68aa812 Compare August 16, 2017 18:52

photonthunder force-pushed the samd21_clock branch 3 times, most recently from aa5b980 to 11774e6 Compare August 16, 2017 21:26

photonthunder mentioned this pull request Aug 28, 2017

boards/samd21-based: fix xtimer configuration #7522

Merged

haukepetersen requested changes Aug 28, 2017

View reviewed changes

photonthunder force-pushed the samd21_clock branch from 11774e6 to 82d692b Compare August 29, 2017 20:21

photonthunder mentioned this pull request Sep 4, 2017

cpu/sam0_common: merge GPIO driver #7559

Merged

photonthunder force-pushed the samd21_clock branch from 67fc8ac to d17d1cd Compare September 5, 2017 19:47

photonthunder force-pushed the samd21_clock branch 2 times, most recently from 41d84c1 to 66e08f3 Compare September 12, 2017 19:49

miri64 assigned kaspar030 Sep 19, 2017

photonthunder force-pushed the samd21_clock branch from 8fe6c10 to f98eca7 Compare September 26, 2017 18:10

photonthunder force-pushed the samd21_clock branch 2 times, most recently from f248dd9 to 5c57481 Compare August 1, 2018 19:36

dylad 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 Aug 2, 2018

dylad requested changes Aug 3, 2018

View reviewed changes

photonthunder force-pushed the samd21_clock branch 2 times, most recently from e69b715 to 346bda1 Compare August 13, 2018 17:12

samd21:setup all clock generators in clock.c

346bda1

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 1, 2019

stale bot closed this Dec 2, 2019

samd21:setup all clock generators in clock.c #7315

samd21:setup all clock generators in clock.c #7315

Conversation

photonthunder commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylad commented Jul 4, 2017

photonthunder commented Jul 5, 2017

biboc commented Jul 10, 2017

photonthunder commented Jul 10, 2017

photonthunder commented Aug 15, 2017

photonthunder commented Aug 16, 2017

photonthunder commented Aug 16, 2017

dylad commented Aug 17, 2017

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

photonthunder commented Aug 28, 2017

photonthunder commented Aug 29, 2017 • edited Loading

travisgriggs commented Sep 11, 2017

photonthunder commented Sep 13, 2017

dylad commented Jul 31, 2018

photonthunder commented Aug 2, 2018

dylad commented Aug 2, 2018

dylad left a comment

Choose a reason for hiding this comment

photonthunder commented Aug 4, 2018

dylad commented Aug 5, 2018

photonthunder commented Aug 5, 2018

dylad commented Aug 8, 2018

photonthunder commented Aug 8, 2018

dylad commented Aug 8, 2018

photonthunder commented Aug 13, 2018 • edited Loading

dylad commented Sep 21, 2018

keestux commented Apr 30, 2019

photonthunder commented Apr 30, 2019

keestux commented Apr 30, 2019

photonthunder commented Apr 30, 2019

keestux commented Apr 30, 2019

stale bot commented Nov 1, 2019

photonthunder commented Aug 29, 2017 •

edited

Loading

photonthunder commented Aug 13, 2018 •

edited

Loading