soc: arm: atmel_sam: Rework clock initialization

[pdgendt]

Start the clock initialization with resetting first to the MAIN clock, same as the reset value.
This fixes the issue of re-initialising the fast clock, for example after the bootloader.

[attie-argentum]

Please could you clarify the issue and how this resolves it?

I'm not keen on the "internal oscillator is disabled, therefore everything is configured correctly and as desired" assumption here... it may work for you, but it'll very probably bite someone else in subtle ways.

I'd much prefer a "reset to internal RC oscillator, and reconfigure the external crystal oscillator" approach, if possible.

[pdgendt]

I'd much prefer a "reset to internal RC oscillator, and reconfigure the external crystal oscillator" approach, if possible.

I've modified the code to switch to the MAIN clock (reset value) before doing the initialization.

CC: @nandojve

Two follow-up questions:

• Should the same be done for sam3x/sam4e/same70/samv71?
• The same70 and samv71 actually have a check, is this preferred over resetting to the MAIN clock:

	/*
	 * Setup main external crystal oscillator if not already done
	 * by a previous program i.e. bootloader
	 */

	if (!(PMC->CKGR_MOR & CKGR_MOR_MOSCSEL_Msk)) {
		// ...
	}

[bjarki-trackunit]

Do it for all of them! Just tested it on ATSAM4E16C with MCUBoot, works perfectly :)

[nandojve]

Hi @pdgendt ,

If you want to proper fix this you need to take care of more things. All this code was implemented before MCUboot to any SAM device. In fact, the support was added not to long ago but officially we still not support on Zephyr, besides MCUboot already have all necessary details in place to make it available.

In regards of your PR, you need to setup the whole chain of registers to make sure that we can support correctly any bootloader. In the future, LPM could disable the on-chip RC oscillator and then when you select the MCK from it you stuck the device if the system do a call to the reset vector.

That said, you must assume that everything was complete reconfigured and system made a call to the reset vector:

1- Enable the internal OSC with 12MHz
2- Select MOSCSEL to internal OSC
3- Set MCK to MAINCK
4- Set EFC0->EEFC_FMR = EEFC_FMR_FWS(0);
5- Enable Clock Failure Detector
6- Configure all other clocks (except disabling the internal OSC)
7- Set EFC0->EEFC_FMR = EEFC_FMR_FWS(5); /* 120MHz */
8- Set MCK to PPLACK
9- Disable internal OSC to save power

It will be nice if you can update all the series.

[pdgendt]

Hey @nandojve,

If you want to proper fix this you need to take care of more things. All this code was implemented before MCUboot to any SAM device. In fact, the support was added not to long ago but officially we still not support on Zephyr, besides MCUboot already have all necessary details in place to make it available.

Understood, I will try to do the changes and test on sam4s, for me MCUboot is the needed use case, so official support would be nice. I do need the changes from #64215 for my board.

It will be nice if you can update all the series.

It would even be nicer to convert the Kconfig options to device tree devices and properties, and share common code between the SoCs. But this might be too big of a step to take at once.

[pdgendt]

5- Enable Clock Failure Detector

@nandojve, the datasheet has the following:

Are we guaranteed the slow RC is active?

Section 29.13 Main Clock Failure Detector however states it will be activated.

Anyway, is there an example on how the failure detection should be done as I'm not entirely sure how this works.

[pdgendt]

@bjarki-trackunit would you mind verifying the current state for the SAM4E if this still works?

@nandojve I think the only missing part is the clock failure detection now.
I've simply enabled the register.

[nandojve]

Hi @pdgendt ,

Any particular reason to no update same7x samv7x?

[pdgendt]

Any particular reason to no update same7x samv7x?

I can try, but the code was already a bit different.

[nandojve]

Any particular reason to no update same7x samv7x?

I can try, but the code was already a bit different.

If you can incorporate the equal parts is already fine. Then we have same sequence for all series.

[pdgendt]

@nandojve I think I've made all requested changes, except the set_source with prescaler. The datasheet states that the prescaler should be set depending on the source:

For this reason I think it makes sense to keep these separate so we have this freedom in the calling code.

I can only test using SAM4S SoC, please validate the other too.

@bjarki-trackunit Can you test for the SAM4E SoC?

[nandojve]

Hi @pdgendt ,
Needs rebase : )

[nandojve]

Hi @pdgendt ,

Sorry about my delay on this. I can test this weekend on SAM4S/4E/V7.

[nandojve]

Everything working!

This introduces a regression on samv71 for MCUboot. Checked with #64215

[pdgendt]

@nandojve I don't have a samv71 to test, can you suggest a fix?

[nandojve]

@nandojve I don't have a samv71 to test, can you suggest a fix?

Yes, I'll check it soon.

[nandojve]

Hi @pdgendt ,

Just apply this patch : )

diff --git a/soc/arm/atmel_sam/common/soc_pmc.h b/soc/arm/atmel_sam/common/soc_pmc.h
index 719d728848..3be251a3d0 100644
--- a/soc/arm/atmel_sam/common/soc_pmc.h
+++ b/soc/arm/atmel_sam/common/soc_pmc.h
@@ -316,6 +316,8 @@ static ALWAYS_INLINE bool soc_pmc_osc_is_ready_main_xtal(void)
  */
 static ALWAYS_INLINE void soc_pmc_switch_mainck_to_xtal(bool bypass, uint32_t xtal_startup_time)
 {
+       soc_pmc_osc_enable_main_xtal(xtal_startup_time);
+
        /* Enable Main Xtal oscillator */
        if (bypass) {
                PMC->CKGR_MOR = (PMC->CKGR_MOR & ~CKGR_MOR_MOSCXTEN)
diff --git a/soc/arm/atmel_sam/samv71/soc.c b/soc/arm/atmel_sam/samv71/soc.c
index 6e82b9fe2f..5b446a8549 100644
--- a/soc/arm/atmel_sam/samv71/soc.c
+++ b/soc/arm/atmel_sam/samv71/soc.c
@@ -45,7 +45,6 @@ static ALWAYS_INLINE void clock_init(void)
                soc_supc_slow_clock_select_crystal_osc();
        }
 
-
        if (IS_ENABLED(CONFIG_SOC_ATMEL_SAMV71_EXT_MAINCK)) {
                /*
                 * Setup main external crystal oscillator.

[pdgendt]

@nandojve applied

[bjarki-trackunit]

Nice work!

[nandojve]

I made another round with SAM4S, SAM4E and SAMV71. Everything working!
Thank you so much for this work @pdgendt : )