cpu/samd5x: enable FDPLL1 at 200MHz #19581

dylad · 2023-05-12T07:52:47Z

Contribution description

This PR allows to use the second FDPLL (the first one is used to generated the 120MHz frequency used by the core and some peripherals). The second FDPLL is setup to run at 200MHz which is the maximum allowed by this MCU.
In fact, I reused the existing function which setup FDPLL0 so it can be used in a generic way for both PLL (since they are the same IP).

I change the way the computation offset (left shift by 5) is done because 200MHz << 5 wouldn't fit inside an uint32_t and I wanted to avoid using an uint64_t here

Two additional commits are present for a small cleanup and a fix.

This is currently unused in our codebase, so it shouldn't impact this platform too much as the ONDEMAND bit is set. the FDPLL will not be running out of the box. But @gschorcht might need it pretty soon.

Testing procedure

This PR can be tested on a same54-xpro and an oscilloscope using the following the patch:

From 76490845ec72387b24116bdd364a61365c186aa1 Mon Sep 17 00:00:00 2001
From: Dylan Laduranty <[email protected]>
Date: Thu, 11 May 2023 17:42:16 +0200
Subject: [PATCH] removeme! for debug purpose

Signed-off-by: Dylan Laduranty <[email protected]>
---
 cpu/samd5x/cpu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/cpu/samd5x/cpu.c b/cpu/samd5x/cpu.c
index f778991a5b..2866c8c9e5 100644
--- a/cpu/samd5x/cpu.c
+++ b/cpu/samd5x/cpu.c
@@ -220,7 +220,7 @@ static void fdpll_init(uint8_t idx, uint32_t f_cpu)
 }
 
 static void gclk_connect(uint8_t id, uint8_t src, uint32_t flags) {
-    GCLK->GENCTRL[id].reg = GCLK_GENCTRL_SRC(src) | GCLK_GENCTRL_GENEN | flags | GCLK_GENCTRL_IDC;
+    GCLK->GENCTRL[id].reg = GCLK_GENCTRL_SRC(src) | GCLK_GENCTRL_GENEN | flags | GCLK_GENCTRL_OE | GCLK_GENCTRL_IDC;
     while (GCLK->SYNCBUSY.reg & GCLK_SYNCBUSY_GENCTRL(id)) {}
 }
 
@@ -384,6 +384,12 @@ void cpu_init(void)
     dma_init();
 #endif
 
+    sam0_gclk_enable(SAM0_GCLK_200MHZ);
+    /* output both FDPLL (GCLK0 and GCLK4) to gpios */
+    gpio_init_mux(GPIO_PIN(PB, 14), GPIO_MUX_M);
+    gpio_init_mux(GPIO_PIN(PB, 10), GPIO_MUX_M);
+    /* PB14 -> EXT2    PB10 -> QSPI SCK */
+
     /* initialize stdio prior to periph_init() to allow use of DEBUG() there */
     early_init();
 
-- 
2.35.3

It will output both FDPLLs to PB14 and PB10. Their frequency can then be measured using an oscilloscope.

Issues/PRs references

None.

dylad · 2023-05-12T07:53:17Z

Actual results:

riot-ci · 2023-05-12T08:03:37Z

Murdock results

✔️ PASSED

6607ed1 cpu/samd5x: add support for FDPLL1 running at 200MHz

Success	Failures	Total	Runtime
6931	0	6931	10m:41s

Artifacts

Documentation preview

cpu/samd5x/cpu.c

gschorcht · 2023-05-12T09:51:47Z

cpu/samd5x/cpu.c

 return;
 }

 /* We source the DPLL from 32kHz GCLK1 */
- const uint32_t LDR = ((f_cpu << 5) / 32768);
+ const uint32_t LDR = (f_cpu / 32768);


Alternatively you could have used

Suggested change

const uint32_t LDR = (f_cpu / 32768);

const uint32_t LDR = (f_cpu >> 10); /* LDR = ((f_cpu << 5) / 32768) */

Should be the same as ((f_cpu << 5) / 32768). This might me more reproducible together with the former code for DPLLRATIO.

I'll give it a try.

dylad · 2023-05-12T11:05:28Z

@gschorcht I've applied your changes and squashed.
FDPLL1 looks good:

benpicco · 2023-05-12T12:58:52Z

This is currently unused in our codebase, so it shouldn't impact this platform too much as the ONDEMAND bit is set. the FDPLL will not be running out of the box.

Hm, are you sure about that? With examples/timer_periodic_wakeup I see an increase in current consumption from 3.16 mA to 4.98 mA on same54-xpro.

benpicco · 2023-05-12T13:00:14Z

cpu/samd5x/cpu.c

@@ -354,7 +353,8 @@ void cpu_init(void)

 xosc_init(0);
 xosc_init(1);
- fdpll0_init(CLOCK_CORECLOCK * DPLL_DIV);
+ fdpll_init(0, CLOCK_CORECLOCK * DPLL_DIV);
+ fdpll_init(1, MHZ(200));


Current consumption is back to normal when I remove this line.

Strange, the ONDEMAND bit is set and since nobody is connected yet to the associated GCLK it shouldn't impact the overall consumption...
I'll investigate.

Okay, I've found the reason why. ONDEMAND bit is "enable-protected".
I'll provide a fix later.
This also mean that the FDPLL0 is not running with ONDEMAND set which is a bug.

gschorcht · 2023-05-12T13:31:25Z

I don't have a scope at hand, but I can confirm that it seems to work. Independent on that the manual seems to be wrong regarding the description of the calculation of DIV as discussed with @dylad already, I get now the following SDCLK frequencies:

200 kHz during the card initialization and identification
12.5 MHz when the card is switch to the default speed of 25 MHz and
25 MHz after the HighSpeed capability test when the card is switched to the high speed mode of 50 MHz.

All speeds should be exactly the double.

Some debug output from _set_speed in sdhc.c:

data dir: /sd0
_set_speed fsdhc=400000 clk=200000000 div=250
_set_speed fsdhc=25000000 clk=200000000 div=4
_set_speed fsdhc=50000000 clk=200000000 div=2
_set_speed fsdhc=400000 clk=200000000 div=250
_set_speed fsdhc=25000000 clk=200000000 div=4
_set_speed fsdhc=50000000 clk=200000000 div=2
main(): This is RIOT! (Version: 2023.07-devel-286-gf2c28-cpu/samd5x/enable_fdpll1_test)

According to the manual, F_SDCLK = F_BASECLK / (2 × DIV). This should give for example for clk=200000000 and div=250 exactly the expected fsdhc=400000. But the result is a clock speed of only 200 kHz.

dylad · 2023-05-13T20:02:34Z

@benpicco I've slightly change the fdpll_init() function again.
The consumption should be back to normal.
I didn't change the behavior for FDPLL0. I'd like to have a deeper look at the consumption optimization. So this should belongs to another PR.

gschorcht · 2023-05-15T07:16:02Z

@benpicco Can you confirm that there is no change in power consumption if FDPLL1 isn't used?

benpicco · 2023-05-15T15:49:07Z

Now it hangs on init:

0x00000d78 in fdpll_init (flags=128 '\200', f_cpu=200000000, idx=1 '\001') at /home/benpicco/dev/RIOT/cpu/samd5x/cpu.c:218
218	    while (!(OSCCTRL->Dpll[idx].DPLLSTATUS.bit.CLKRDY &&
                     OSCCTRL->Dpll[idx].DPLLSTATUS.bit.LOCK)) {}

It works when I remove the ONDEMAND bit, but then power consumption is of course up again

--- a/cpu/samd5x/cpu.c
+++ b/cpu/samd5x/cpu.c
@@ -354,7 +354,7 @@ void cpu_init(void)
     xosc_init(0);
     xosc_init(1);
     fdpll_init(0, CLOCK_CORECLOCK * DPLL_DIV, 0);
-    fdpll_init(1, MHZ(200), OSCCTRL_DPLLCTRLA_ONDEMAND);
+    fdpll_init(1, MHZ(200), 0);
 
     /* select the source of the main clock */
     if (USE_DPLL) {

dylad · 2023-05-15T17:23:49Z

I'll have my board back tomorrow. I'll give it a try.

Signed-off-by: Dylan Laduranty <[email protected]>

These functions can be used to set both FDPLL0 and FDPLL1 by using an extra argument 'idx' (index) and allow to set the ONDEMAND bit using the 'flags' argument Signed-off-by: Dylan Laduranty <[email protected]>

Signed-off-by: Dylan Laduranty <[email protected]>

dylad · 2023-05-16T14:35:07Z

FDPLL0 no more blocks with ONDEMAND and power consumption is back to normal.

benpicco · 2023-05-23T14:51:22Z

bors merge

bors · 2023-05-23T20:02:46Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

dylad · 2023-05-24T06:11:26Z

Thanks for the review & merge !

dylad requested a review from benpicco May 12, 2023 07:52

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels May 12, 2023

dylad added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 12, 2023

gschorcht reviewed May 12, 2023

View reviewed changes

cpu/samd5x/cpu.c Show resolved Hide resolved

gschorcht reviewed May 12, 2023

View reviewed changes

dylad force-pushed the cpu/samd5x/enable_fdpll1 branch from 98c1e4b to 81a3ed6 Compare May 12, 2023 11:04

benpicco reviewed May 12, 2023

View reviewed changes

dylad force-pushed the cpu/samd5x/enable_fdpll1 branch from 81a3ed6 to 8c76e71 Compare May 13, 2023 19:55

dylad added 4 commits May 16, 2023 15:53

cpu/samd5x: fix SAM0_DPLL_FREQ_MAX_HZ value

ba83e13

Signed-off-by: Dylan Laduranty <[email protected]>

cpu/samd5x: remove duplicate USE_VREG_BUCK

af2fa99

Signed-off-by: Dylan Laduranty <[email protected]>

cpu/samd5x: replace fdpll0_init by two generic functions

f790d9f

These functions can be used to set both FDPLL0 and FDPLL1 by using an extra argument 'idx' (index) and allow to set the ONDEMAND bit using the 'flags' argument Signed-off-by: Dylan Laduranty <[email protected]>

cpu/samd5x: add support for FDPLL1 running at 200MHz

6607ed1

Signed-off-by: Dylan Laduranty <[email protected]>

dylad force-pushed the cpu/samd5x/enable_fdpll1 branch from 8c76e71 to 6607ed1 Compare May 16, 2023 14:17

benpicco approved these changes May 23, 2023

View reviewed changes

bors bot merged commit 3469fce into RIOT-OS:master May 23, 2023

dylad deleted the cpu/samd5x/enable_fdpll1 branch May 24, 2023 06:11

benpicco added this to the Release 2023.07 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/samd5x: enable FDPLL1 at 200MHz #19581

cpu/samd5x: enable FDPLL1 at 200MHz #19581

dylad commented May 12, 2023

dylad commented May 12, 2023

riot-ci commented May 12, 2023 •

edited

Loading

gschorcht May 12, 2023 •

edited

Loading

dylad May 12, 2023

dylad commented May 12, 2023 •

edited

Loading

benpicco commented May 12, 2023

benpicco May 12, 2023

dylad May 12, 2023

dylad May 12, 2023

gschorcht commented May 12, 2023 •

edited

Loading

dylad commented May 13, 2023

gschorcht commented May 15, 2023

benpicco commented May 15, 2023 •

edited

Loading

dylad commented May 15, 2023

dylad commented May 16, 2023

benpicco commented May 23, 2023

bors bot commented May 23, 2023

dylad commented May 24, 2023

	const uint32_t LDR = (f_cpu / 32768);
	const uint32_t LDR = (f_cpu >> 10); /* LDR = ((f_cpu << 5) / 32768) */

cpu/samd5x: enable FDPLL1 at 200MHz #19581

cpu/samd5x: enable FDPLL1 at 200MHz #19581

Conversation

dylad commented May 12, 2023

Contribution description

Testing procedure

Issues/PRs references

dylad commented May 12, 2023

riot-ci commented May 12, 2023 • edited Loading

Murdock results

Artifacts

gschorcht May 12, 2023 • edited Loading

Choose a reason for hiding this comment

dylad May 12, 2023

Choose a reason for hiding this comment

dylad commented May 12, 2023 • edited Loading

benpicco commented May 12, 2023

benpicco May 12, 2023

Choose a reason for hiding this comment

dylad May 12, 2023

Choose a reason for hiding this comment

dylad May 12, 2023

Choose a reason for hiding this comment

gschorcht commented May 12, 2023 • edited Loading

dylad commented May 13, 2023

gschorcht commented May 15, 2023

benpicco commented May 15, 2023 • edited Loading

dylad commented May 15, 2023

dylad commented May 16, 2023

benpicco commented May 23, 2023

bors bot commented May 23, 2023

dylad commented May 24, 2023

riot-ci commented May 12, 2023 •

edited

Loading

gschorcht May 12, 2023 •

edited

Loading

dylad commented May 12, 2023 •

edited

Loading

gschorcht commented May 12, 2023 •

edited

Loading

benpicco commented May 15, 2023 •

edited

Loading