cpu/samd21: fix NVM wait states

[dylad]

While looking in cpu/samd21 I found a small bug. See commit.
Moreover, I've noticed another bug but I don't know how to fix it. I opened an issue because it also related to Atmel CMSIS see #6874
Cheers !

[keestux]

Wow. Good catch! The change loos good to me, and I guess (hope) you've tested it.

But I'm a bit puzzled. The old code was starting/stopping the APBA(!) clock for the SYSCTRL, leaving the SYSCTRL stopped. So, how could it have worked???

[dylad]

Unfortunately I don't own SAMR21-Xpro board so I cannot test it. I was looking in this file for my SAMD20-Xpro port to RIOT.

Maybe no one ever try to get a CORECLOCK > 24 Mhz ? But I agree with you, this is weird.

[keestux]

My Autonomo board runs at 48MHz. I will try to test it on it.

[dylad]

Many thanks @keestux !

[keestux]

It's good that I tested it. The patch is not OK :-)

But the original code is wrong too. It just happened to be somewhat harmless. Let's look at the code once more.

PM->APBAMASK.reg |= PM_AHBMASK_NVMCTRL;
NVMCTRL->CTRLB.reg |= NVMCTRL_CTRLB_RWS(1);
PM->APBAMASK.reg &= ~PM_AHBMASK_NVMCTRL;

Given the code above there are three things mixed up. APBA mask, APBB mask and AHB mask. (My head starts to spin with all these PM registers.)

The define PM_AHBMASK_NVMCTRL is meant for AHB mask, not the APBB mask. (( The define is for bit 4, and so we did a start/stop of the WDT APB clock. ))

Now, there is also an enable for the NVMCTRL APB Clock (notice the APB). The question is, which one did we try to start/stop? And, is it really needed? ArduinoCore does do it. They simply do this at startup.

/* Set 1 Flash Wait State for 48MHz, cf tables 20.9 and 35.27 in SAMD21 Datasheet */
NVMCTRL->CTRLB.bit.RWS = NVMCTRL_CTRLB_RWS_HALF_Val ;

Digging a little deeper in ASF, there is a driver/nvm.c. In that code they do start/stop of the APB clock when they modify the RWS field of the NVMCTRL. From that I conclude that our code must be:

PM->APBBMASK.reg |= PM_APBBMASK_NVMCTRL;
NVMCTRL->CTRLB.reg |= NVMCTRL_CTRLB_RWS(1);
PM->APBBMASK.reg &= ~PM_APBBMASK_NVMCTRL;

I've tried it with hello world and can confirm that it still works.

[dylad]

Thank you for the time you spent on it @keestux.
I absolutely didn't notice there were two clocks for NVMCTRL.
I'll change the code.

[keestux]

Can you squash the two commits?

[dylad]

Squashed.

[keestux]

ACK

[aabadie]

proxying @keestux's ACK

[aabadie]

All green. Merging. Should this one be backported ?