SAMD21: possible CMSIS bug ?

[dylad]

Hello,
I've noticed a possible bug in cpu/samd21/cpu.c file

    for (int i = 0x3; i <= 0x22; i++) {
        GCLK->CLKCTRL.reg = ( GCLK_CLKCTRL_ID(i) | GCLK_CLKCTRL_GEN_GCLK7 );
        while (GCLK->STATUS.bit.SYNCBUSY) {}
    }

Here, code disables all periphs clock. And it stops at 0x22. If we take a quick look in SAMD21 datasheet (Table 15.5 page 131-132) the last periph clock ID is 0x26.
Moreover, if we now take a look at Atmel CMSIS (cpu/sam0_common/include/vendor/samd21/include/component/gclk.h)
The last periph ID is 0x24 (and doesn't take into account the two blank lines from SAMD21 datasheet).
What do you think ?

[keestux]

Hmm. I vaguely remember that I brought this up in the SAMR21 versus SAMD21 discussion.
The reason for the 0x22 is that it samd21/cpu.c was first developed for a SAMR21, which has peripheral clocks up to 0x22 (GCLK_PTC). SAMD21 goes up to 0x26.

But not only that, there are some differences in the Generic Clock Selection ID. It's one of the very few areas where we have to apply #ifdef's and select alternative code for either SAMD21 or SAMR21.

[dylad]

Aaaw, ok.
Still, I don't get why Atmel define two I2S ID for SAMR21 in the CMSIS.
This issue may be change when someone will port SAMD21-xpro board.
But I'm still wondering why there is up to 0x24 in CMSIS and 0x26 in datasheet for SAMD21.

[keestux]

Indeed, the latest SAMD21 datasheet (Rev N) and the include files (up to ASF 3.34) don't match.

I also have an older datasheet (Rev E) which matches the ASF include files.

If the Rev N. datasheet is correct then Atmel forgot to update ASF.

[dylad]

In that case, what would be the best solution to integrate this change ?
There is an ongoing effort to port SAMD21-xpro. The current state is ok for SAMR21 but not for SAMD21.
Use #ifdef CPU_MODEL_SAMD21J18A for example ?

[keestux]

I don't have a straightforward answer. There is no define for the last ID. Would it hurt to set the clock for "reserved" ID's? If not we could initialize up to 0x3F.

But I do think that ifdef CPU_MODEL_SAMD21J18A is wrong. I don't like the J18A part of it. Isn't CPU_FAM_SAMD21 a better choice? See cpu/sam0_common/include/periph_cpu_common.h and cpu/sam0_common/periph/spi.c for examples.

[dylad]

CPU_FAM_SAMD21 is a better choice only if initialization of both samr21 and samd21 up to 0x3F is not harmful.
But if we have a problem with this, we should have a backup plan.

BTW, I'm sure there is a historical reason, but why samr21 is define as part of CPU_FAM_SAMD21 ?

[keestux]

O shit. I now see that CPU_FAM_SAMD21 is used for both samd21 and samr21. That's not what I meant. It was certainly not my choice. See story below.

BTW, I'm sure there is a historical reason, but why samr21 is define as part of CPU_FAM_SAMD21 ?

It's a long story. First there was support for SAMR21 (board samr21-xpro) and it was done "using" cpu/samd21 and the CMSIS files were from the samr21 tree, renamed to samd21. Then I came in with a request to move cpu/samr21 to cpu/samd21 to free up the name for "real" SAMD21 support. See #5553. But this was rejected, then there was #5556 and #5645 and #5743. Finally we now have a combined tree for both SAMD21 and SAMR21 because they are very much the same. Notice that there is even Atmel documentation where they explain that a SAMR21 has a SAMD21 inside.
In other words, yes, there is a historical reason why samr21 is part of the samd21 family.

[dylad]

Thanks @keestux for the story !
I can understand the choices which has been made. But I don't really like them.
The recent port of SAMD21 board ( #6839 ) will introduce 'conflicts'. The one on this issue is an example. SAMD21-xpro has I2S. So for (int i = 0x3; i <= 0x22; i++) will have to be update to 0x24 or 0x26
#6885 is another example.

@haukepetersen @keestux what can we do about that ?

[haukepetersen]

Let's see: the cleanest solution would be to move the samr21 to it's own CPU implementation. However, I would be kind of opposed to do this, as this would lead to a lot of code duplication, right? But I can't really tell the amount of differences we actually have between the samr21 and the samd21, are they really only this minor (e.g. easily resolvable with some simple ifdefs), or do we have more substantial differences (as between the saml21 and the samd21)?

So for a quick solution, I would propose the following:

• we introduce a new variable CPU_FAM_SUB to be defined by the sam0-based boards, being set to either samd21 or samr21
• in the implementation of the samd21 we introduce two custom header files, one for the samd21 and one for the samr21. From these, we include the one fitting the CPU_FAM_SUB variable.
• in those header files we can define the differences between those variant, e.g. the number of periph clock IDs...
• the existing implementation uses those defines.

[dylad]

I think we can keep samr21 and samd21 chip under the same tree and use some ifdef for the small differences.
@haukepetersen Your solution seems good to me !

[haukepetersen]

:-)

[keestux]

@haukepetersen Yes, I agree with your suggestion to introduce a new variable.
This also gives the possibility to simplify cpu/sam0_common/include/vendor/sam0.h. We could do

#if   defined(CPU_MODEL_SUB_samd)
  #include "samd21/include/samd21.h"
#elif defined(CPU_MODEL_SUB_saml)
  #include "saml21/include/saml21.h"

But that requires us to create a define like __SAMD21E15A__. Which can be done in makefiles/arch/cortexm.inc.mk

export CFLAGS += -DCPU_MODEL_$(MODEL) -D__$(MODEL)__

Or, in cpu/sam0_common/Makefile.include

# export the CPU model and architecture
MODEL = $(shell echo $(CPU_MODEL) | tr 'a-z' 'A-Z')
export CFLAGS += -D__$(MODEL)__

[keestux]

Before we do anything it may be good to look at all the variables we have today, like, CPU, CPU_MODEL, CPU_FAM. I'm losing track of what is used for what.

in boards/sodaq-autonomo/Makefile.include

# define the cpu used by SODAQ Autonomo board
export CPU = samd21
export CPU_MODEL = samd21j18a

in cpu/samd21/Makefile.include

export CPU_ARCH = cortex-m0plus
export CPU_FAM  = samd21

Why do we have CPU_FAM? Maybe CPU_FAM_SUB is a better name?

[keestux]

Have a look at my add-sam0-cpu-fam-sub branch to see what I mean.

https://github.com/keestux/RIOT

[dylad]

Maybe we should also think to get the CPU variant.
For example, SAML21J18 has two variants (A and B)
There are few difference between variant but maybe we should take it into account ?
Have a look at this link

[keestux]

Shall we close this issue? There is no activity for two years, and I think we have a working solution right now.