cpu/saml1x: add support for SAML10 and SAML11 MCUs (Cortex-M23) by dylad · Pull Request #10653 · RIOT-OS/RIOT

dylad · 2018-12-24T16:01:53Z

Contribution description

This PR adds a very basic support for our first Cortex-M23 MCUs, SAML10 and SAML11 so people can start to play with it. These chips are the same except that SAML11 add security stuff (ARM TrustZone, non-secure+secure registers,...)

~~In the current state, only GPIOs, timer and UART work.~~

SAML10/SAML11 IP peripherals are really close to SAML21. But I don't know if these MCUs should re-use stuff within sam0_common since they are cortex-m23 based...So right now, I copied GPIOs, timer , uart SAML21 drivers and make the required changes to use them but this can be optimized. (now or later). In fact, we can re-use all sam0 drivers with these MCUs but it will take much more time to fully test it. We can stay as it now and optimize later or we can do it now but I was hoping to see this PR merged for the incoming release.

Regarding MPU support, I intentionally prevent from using this feature on Cortex-M23 because armv8 introduces several changes and I need more time to rework and test this feature. I (or someone with more MPU knowledge) will do it later.

Testing procedure

run make BOARD=saml10-xpro -C [test_you_want]
or make BOARD=saml11-xpro -C [test_you_want]

EDBG will be used to flash the board.

Issues/PRs references

This PR is based on top of #10558
see also #10570 for EDBG update which supports these MCUs.

aabadie · 2018-12-24T16:48:19Z

Very nice! Thanks.

I'll review and test it in 2019 :)

aabadie

I could test this PR on saml11-xpro and confirm that it's working.

Unfortunately, my impression is that there is too much code duplication, at least between the saml10-xpro and saml11-xpro boards. I would recommend to directly introduce a boards/common/saml1x common implementation: the timer and uart configuration are the same, the LEDs and buttons are the same, the features provided are the same, etc. The only difference I could see is the used LED0_ON in board_init in one board but not in the other (which is maybe be a leftover).

Regarding the duplication of code at cpu level, I would suggest to directly rename sam0_common to samx_common and depends on it: it will contain the peripheral implementation for sam0 and sam23 (we can keep sam3 appart). And then as you said, sam23 will have support for a lot of peripherals.
To me, this won't be a big effort and it is worth doing right now. I can help if you want.

aabadie · 2019-01-04T14:03:15Z

@@ -0,0 +1,9 @@
+# Put defined MCU peripherals here (in alphabetical order)
+FEATURES_PROVIDED += periph_gpio periph_gpio_irq


This is not needed and can be provided at CPU levels (several PRs were recently merged in that direction)

I'll remove it. This is a leftover because this PR was started some times before this change was merged.

dylad · 2019-01-04T15:18:48Z

@aabadie Thanks for the review.

I would suggest to directly rename sam0_common to samx_common

In this case, I'll rework this PR but it'll take some time.

I can help if you want.

Since you're pretty busy with the release management I don't think you should afford such effort. I'll rework all SAM0 to support SAML1x MCUs.

dylad · 2019-01-09T18:57:34Z

@aabadie I've forced-push the branch.
SAML10/SAML11 now use SAM0 driver. I was able to test ADC and I2C on SAML10.
This PR is ready for another round !

aabadie

Thanks for updating this PR @dylad !

I just reviewed/tried (saml11-xpro) it again and found some problems:

gpio_params.h is missing but saul_gpio is included with the saul_default module
the gpio driver is broken with interrupts
I found the saml21 and saml1x peripheral implementations very close. I won't ask you to factorize in this PR though, but I see room for improvement here.
tests/periph_rtc and tests/periph_rtt doesn't work: the first one is stuck and doesn't print anything, the second one doesn't print "many hellos". Maybe just remove for now the support for these periphs ?
tests/periph_flashpage doesn't build, it needs to be adapted.

I haven't test I2C and SPI but others I tested (cpuid, timer, adc) work.

There are other minor stuff see below.

aabadie · 2019-01-13T14:54:37Z

@@ -0,0 +1,3 @@
+ifneq (,$(filter saul_default,$(USEMODULE)))
+  USEMODULE += saul_gpio
+endif


missing newline

aabadie · 2019-01-13T14:57:44Z

+MODULE = board
+DIRS = $(RIOTBOARD)/common/saml1x
+
+include $(RIOTBASE)/Makefile.base


missing newline

aabadie · 2019-01-13T14:58:39Z

@@ -0,0 +1,9 @@
+FEATURES_PROVIDED += periph_adc


It would make more sense to move these provided features to the common boards. They are the same for saml11-xpro.

aabadie · 2019-01-13T15:04:11Z

@@ -99,7 +99,11 @@ int uart_init(uart_t uart, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)
    if ((rx_cb) && (uart_config[uart].rx_pin != GPIO_UNDEF)) {
        uart_ctx[uart].rx_cb = rx_cb;
        uart_ctx[uart].arg = arg;
+#if defined (CPU_FAM_SAML10) || defined (CPU_FAM_SAML11)


Suggested change

#if defined (CPU_FAM_SAML10) || defined (CPU_FAM_SAML11)

#if defined (CPU_SAML1X)

Good catch.

aabadie · 2019-01-13T15:04:29Z

+# (file triggers compiler bug. see #5775)
+SRC_NOLTO += vectors.c
+
+include $(RIOTBASE)/Makefile.base


missing newline

aabadie · 2019-01-13T15:04:40Z

@@ -0,0 +1 @@
+-include $(RIOTCPU)/sam0_common/Makefile.features


aabadie · 2019-01-13T15:06:22Z

@@ -0,0 +1 @@
+include $(RIOTMAKE)/periph.mk


aabadie · 2019-01-13T15:06:46Z

+#endif
+
+#endif /* PERIPH_CPU_H */
+/** @} */


newline, and maybe in other places (I won't check all of them)

aabadie · 2019-01-13T15:14:26Z

@@ -0,0 +1,3 @@
+ifneq (,$(filter saul_default,$(USEMODULE)))
+  USEMODULE += saul_gpio


The gpio_params.h is missing in the include directory. Maybe a mistake when factorizing the code ? It doesn't build examples/default

I'll find the way.

dylad · 2019-01-15T12:57:04Z

the gpio driver is broken with interrupts

On which board ? L10 or L11 ?

I found the saml21 and saml1x peripheral implementations very close. I won't ask you to factorize in this PR though, but I see room for improvement here.

Yes, this should be factorize (later). I had not idea how to reuse SAML21 drivers without adding another dependency. IMHO, the best way to do is merge timer, rtc and rtt driver to sam0_common. I'll work on this later I swear.

tests/periph_rtc and tests/periph_rtt doesn't work:

I'll fix.

tests/periph_flashpage doesn't build, it needs to be adapted.

I probably miss this driver.

gpio_params.h is missing but saul_gpio is included with the saul_default module

I'll fix.

dylad · 2019-01-15T16:03:03Z

@aabadie I'm sorry I had to rebased the branch to the current master to include some fix to flashpage for SAM0 but It does work yet on SAML10/SAML11. I'll investigate further but I'm not an expert.
Good news are :

GPIO IRQ are working (I missed some commits from my private repo)
RTC/RTT are working
SAUL is working
minor comments have been addressed

I didn't squash, so you can have a look at the changes introduce by the new commits.

aabadie

I retested with saml11-xpro and still found small things with the saul gpio, see below.

Also the tests/buttons doesn't work. Maybe the exti array needs an update (haven't checked the datasheet). Or there is something else with the peripheral driver ?

I'm running some tests on samr30-xpro (saml21 based) to check everything is ok.

aabadie · 2019-01-16T13:47:42Z

+    {
+        .name = "LED(orange)",
+        .pin = LED0_PIN,
+        .mode = GPIO_OUT


For me saml11-xpro, the LED and buttons are inverted.

Suggested change

.mode = GPIO_OUT

.mode = GPIO_OUT,

.flags = (SAUL_GPIO_INVERTED | SAUL_GPIO_INIT_CLEAR)

aabadie · 2019-01-16T13:48:11Z

+    {
+        .name = "Button(SW0)",
+        .pin  = BTN0_PIN,
+        .mode = BTN0_MODE


Suggested change

.mode = BTN0_MODE

.mode = BTN0_MODE,

.flags = SAUL_GPIO_INVERTED

aabadie · 2019-01-16T15:39:48Z

@dylad, I tested your last commit and it fixes the tests/buttons application and saul LED/button, good job!

Just in case, for rebase/squash ease, you may want to split the commit in 2, one for saul, one for periph gpio.

dylad · 2019-01-16T15:39:59Z

@aabadie Thanks for the SAUL tips, it is now fixed.

Also the tests/buttons doesn't work. Maybe the exti array needs an update (haven't checked the datasheet). Or there is something else with the peripheral driver ?

I forget something important for this ~~damned~~ SAML11 MCU in the GPIO driver, the EXTI array should be fine, I looked at the datasheet.
tests/buttons should now works for SAML11 (it was working but for SAML10 only).

dylad · 2019-01-16T15:52:05Z

Just in case, for rebase/squash ease, you may want to split the commit in 2, one for saul, one for periph gpio.

Sure.
I'll take another look tonight to see if I didn't miss something else. If everything is good I'll rebase/squash tomorrow.

aabadie · 2019-01-16T15:54:37Z

Now that #10558, you need to rebase this PR.

I'll do a full pass with all test applications to verify saml11-xpro before merging this PR.

dylad · 2019-01-16T15:57:05Z

I'll do a full pass with all test applications to verify saml11-xpro before merging this PR.

Is there a way to easily disable flashpage driver for SAML1X (which depend on sam0_common) ?
I don't think I'll be able to fix this driver in time for the release.

aabadie · 2019-01-16T15:58:41Z

Is there a way to easily disable flashpage driver for SAML1X

I tested it and it was working, so there's no need to disable it.

dylad · 2019-01-16T16:05:11Z

I tested it and it was working, so there's no need to disable it.

Really ? I'll triple-check that tonight.

dylad · 2019-01-16T18:28:04Z

@aabadie I can confirm that driver_flashpage is broken right now for SAML11 only. If I cannot fix it in time I suggest to disable it.

dylad · 2019-01-16T19:50:44Z

Removed #10558 commits and rebased.

aabadie · 2019-01-16T19:53:00Z

I can confirm that driver_flashpage is broken right now for SAML11 only

This is strange, I tried flash test on a saml11-xpro and it succeeded. Are you manually testing ?

dylad · 2019-01-16T20:09:20Z

This is strange, I tried flash test on a saml11-xpro and it succeeded. Are you manually testing ?

Yes, the test_last command, ran from shell, fails.

aabadie · 2019-01-16T20:17:04Z

Ok, I'm not with the board right now so I can't re-test, will do tomorrow. I checked the automatic test code and it calls also test_last, there should be no difference with what you do 😕

dylad · 2019-01-21T09:56:00Z

Not proud of it but I found a possible workaround for my weird unittests bug. see lastest commit.
This PR is ready for one last round.

aabadie · 2019-01-21T12:18:44Z

+}
+
+#ifdef CPU_SAML1X
+void isr_eic0(void)


I would do this differently do avoid all this code duplication:

#ifdef CPU_SAML1X void isr_eic0(void) { isr_eic(); } [...] void isr_eic3(void) { isr_eic(); } void isr_eic_other(void) { isr_eic(); } #endif /* SAML1X */ void isr_eic(void) { for (unsigned i = 0; i < NUMOF_IRQS; i++) { if (_EIC->INTFLAG.reg & (1 << i)) { _EIC->INTFLAG.reg = (1 << i); gpio_config[i].cb(gpio_config[i].arg); } } cortexm_isr_end(); }

This is untested but I think it should work.

I'll give it a try.

aabadie · 2019-01-21T12:20:08Z

@@ -92,19 +92,6 @@ void i2c_init(i2c_t dev)

    /* I2C using CLK GEN 0 */
    sercom_set_gen(bus(dev),i2c_config[dev].gclk_src);
-#if defined(CPU_FAM_SAML21) || defined(CPU_FAM_SAMR30)


This seems unrelated. Is there a good reason to remove this part ?

What sorcery is this ?

Oh right I remember now.
In fact I removed this part on purpose because this is pointless. Sercom slow clock is used only by some SMBus features like timeout. But we don't use them at all, so no need for this clock (and it saves some power).

It is unclear to me what this code is about. Are you sure it's strictly not required ? What if someone wants to use the slow clock ?

This code setup the SERCOM_GCLK_SLOW clock for the SERCOM your are using. (which is completely unrelated to the SERCOM_GCLK)
According to, SAML21 datasheet, the SERCOM_GLCK_SLOW is used for :

Two generic clocks ared used by SERCOM, GCLK_SERCOMx_CORE and GCLK_SERCOM_SLOW. The core clock (GCLK_SERCOMx_CORE) can clock the I 2 C when working as a master. The slow clock (GCLK_SERCOM_SLOW) is required only for certain functions, e.g. SMBus timing. These clocks must be configured and enabled in the Generic Clock Controller (GCLK) before using the I 2 C.

Those SMBus features can be active in the CTRLA register for each SERCOM. We're not using them in RIOT codebase.

See chapter 34.5.3 of SAML21 datasheet.

The SERCOM_SLOW clock is NOT used by UART nor SPI.

I know it's annoying but I think this change should be in it's own PR because it's not only related to saml1x. In any case, there is no need for this PR to depend on it IIUC.

I reapplied this piece of code. (And I will remove it as soon as this PR is merged ;) )

aabadie · 2019-01-21T12:33:40Z

I have 2 remaining code comments, and I think they will be my last ones.

I also re-ran the magic Python script and got no failure this time (except the 2 ones that we already know to be unrelated to this PR).

dylad · 2019-01-21T13:04:29Z

comments answered/addressed.

aabadie

No more comment for me. Code changes are ok. I ran the the python script on samr21-xpro (SAM0 based), samr30-xpro (SAML21) and didn't notice any regression.

For me, this PR is ready, so ACK. You can now squash, @dylad.

Add SAML10/SAML11 support through SAM0 because hardware IP are the same so reuse to avoid duplication

Disable optimization for this function only with CPU_SAML1X MCUs due to an internal crosscompiler bug

aabadie · 2019-01-22T07:52:59Z

The branch was squashed and the CI is now all green. Let's go.

Thanks again for the good work and your perseverance @dylad !

dylad · 2019-01-22T07:57:54Z

Thanks again for the good work and your perseverance @dylad !

Thanks for your help and your tests @aabadie ! I'm happy this one is finally merged :)

emmanuelsearch · 2019-01-22T21:22:22Z

great!

keestux · 2019-01-29T20:07:44Z

What I'm missing in the merge is a precise description where the vendor files came from. Is a copy from ASF? Then I want to see which version ASF and whether it was a verbatim copy, and if not, then describe what was changed.

Now that the merge is still fresh we should add the information before it is too late.

The place to document is cpu/sam0_common/include/vendor/README.md. There it is written that ASF 3.35 was used.

Notice the same can be said of #9437. In that PR the file cpu/sam0_common/include/vendor/samr30/README.md was added with a note that ASF 3.34.2 was used. Huh? Why?

dylad · 2019-01-30T08:24:10Z

@keestux When I started to write this PR, Microchip didn't include SAML10/SAML11 into ASF. It seems this is still the same today. (ASF 3.45).
Which is weird since Atmel Studio and Atmel Start support these devices. So I ended up downloading an .atpack archive for Atmel Studio [1] and I copy the vendor files from there. I know this is not the best solution but I didn't find a better source back in December. Maybe I miss something ?

[1] http://packs.download.atmel.com/

keestux · 2019-01-31T19:43:35Z

@dylad OK, I see. In that case we need to write down the version of these atpacks. I'll take a look and will come up with a (small) PR. Thanks for the URL, that helped. That said, URLs of Atmel are moving targets (to put it mildly).

keestux · 2019-01-31T20:07:45Z

See #10914

dylad added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Dec 24, 2018

dylad added this to the Release 2019.01 milestone Dec 24, 2018

aabadie self-requested a review December 24, 2018 16:47

aabadie added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Dec 24, 2018

aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 4, 2019

aabadie requested changes Jan 4, 2019

View reviewed changes

dylad force-pushed the pr/saml1x_support branch from 739a0d5 to 0ea1763 Compare January 9, 2019 18:54

aabadie requested changes Jan 13, 2019

View reviewed changes

dylad force-pushed the pr/saml1x_support branch from 0ea1763 to 23dcca3 Compare January 15, 2019 15:58

aabadie requested changes Jan 16, 2019

View reviewed changes

dylad removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 16, 2019

dylad force-pushed the pr/saml1x_support branch from 1e10266 to 1b4ce7c Compare January 16, 2019 19:49

aabadie reviewed Jan 21, 2019

View reviewed changes

aabadie approved these changes Jan 21, 2019

View reviewed changes

dylad added 10 commits January 21, 2019 17:04

cpu/cortexm: add cortex-m23 support

6d3fda7

cpu/saml1x: add support for saml10/saml11 MCUs

adeb193

cpu/sam0_common: add support for saml10/saml11

9450fa7

Add SAML10/SAML11 support through SAM0 because hardware IP are the same so reuse to avoid duplication

board/common/saml1x: add common configuration

99966b3

board/saml10-xpro: add support for SAML10-XPRO board

2132f5b

board/saml11-xpro: add support for SAML11-XPRO board

e838f0c

pkg/qDSA: add support for cortex-M23

b3e1802

tests/unittests: disable optimization for test_atomic_inc_rollover

762e525

Disable optimization for this function only with CPU_SAML1X MCUs due to an internal crosscompiler bug

examples/*: add saml1*-xpro to BOARD_INSUFFICIENT_MEMORY

eaa8d83

tests/*: add saml1*-xpro to BOARD_INSUFFICIENT_MEMORY

7afacf9

dylad force-pushed the pr/saml1x_support branch from 19406f1 to 7afacf9 Compare January 21, 2019 16:12

aabadie merged commit 7e3c382 into RIOT-OS:master Jan 22, 2019

dylad mentioned this pull request Jan 22, 2019

cpu/sam0_common: remove unneeded GCLK_SLOW setup in i2c driver #10839

Merged

dylad deleted the pr/saml1x_support branch January 23, 2019 08:42

aabadie mentioned this pull request Jan 30, 2019

cpu/saml21: extend and fix exti configuration for saml21 variants [backport 2019.01] #10907

Merged

PeterKietzmann mentioned this pull request Feb 21, 2019

extend support for SAM D and L family #6792

Closed

dylad mentioned this pull request Feb 26, 2020

tests/unittests: fix GCC compiler bug in core-atomic for SAML1X #13462

Merged

		@@ -0,0 +1,9 @@
		# Put defined MCU peripherals here (in alphabetical order)
		FEATURES_PROVIDED += periph_gpio periph_gpio_irq

	#if defined (CPU_FAM_SAML10) \|\| defined (CPU_FAM_SAML11)
	#if defined (CPU_SAML1X)

		@@ -0,0 +1 @@
		-include $(RIOTCPU)/sam0_common/Makefile.features

		@@ -0,0 +1,3 @@
		ifneq (,$(filter saul_default,$(USEMODULE)))
		USEMODULE += saul_gpio

	.mode = GPIO_OUT
	.mode = GPIO_OUT,
	.flags = (SAUL_GPIO_INVERTED \| SAUL_GPIO_INIT_CLEAR)

	.mode = BTN0_MODE
	.mode = BTN0_MODE,
	.flags = SAUL_GPIO_INVERTED

Uh oh!

Conversation

dylad commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

aabadie commented Dec 24, 2018

Uh oh!

aabadie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Jan 4, 2019

Uh oh!

dylad commented Jan 9, 2019

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Jan 15, 2019

Uh oh!

dylad commented Jan 15, 2019

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 16, 2019

Uh oh!

dylad commented Jan 16, 2019

Uh oh!

dylad commented Jan 16, 2019

Uh oh!

aabadie commented Jan 16, 2019

Uh oh!

dylad commented Jan 16, 2019

Uh oh!

aabadie commented Jan 16, 2019

Uh oh!

dylad commented Jan 16, 2019

Uh oh!

dylad commented Jan 16, 2019

Uh oh!

dylad commented Jan 16, 2019

Uh oh!

dylad commented Dec 24, 2018 •

edited

Loading

aabadie left a comment •

edited

Loading

aabadie Jan 13, 2019 •

edited

Loading

aabadie Jan 21, 2019 •

edited

Loading

dylad commented Jan 22, 2019 •

edited

Loading