Add SAMD20J18A support #555

kaizensparc · 2021-12-24T17:05:18Z

Summary

This Pull Request integrates the SAMD20J18A microcontroller:

It adds the SVD file and its XSL patches in the svd directory
It generates the corresponding pac
It updates the HAL to have support for it
It adds the SAMD20 Xplained Pro board in the BSP with a bliny_basic example

This is only a proof of concept, not all features have been tested in real life even though everything compiles fine :)

Checklist

CHANGELOG.md for the BSP or HAL updated
All new or modified code is well documented, especially public items
No new warnings or clippy suggestions have been introduced (see CI or check locally)

If Adding a new Board

Board CI added to crates.json
Board is properly following "Tier 2" conventions, unless otherwise decided to be "Tier 1"

boards/samd20_xplained_pro/.cargo/config

boards/samd20_xplained_pro/Cargo.toml

TDHolmes · 2021-12-24T17:30:15Z

boards/samd20_xplained_pro/Cargo.toml

+optional = true
+
+[dependencies.atsamd-hal]
+path = "../../hal"


I think we'll want this to be a tier two board, so no path dependency

If I remove the path dependency, it will use the published crate which does not contains the current updates, how should I replace it?
Btw why does including this path makes it a Tier 1 dependency?

ah, good point... I think we'll need to leave it as a path dependency for now since it's new.

Tier one BSPs use path dependencies on the HAL, which means for any HAL change affecting BSPs, all Tier one BSPs must be upgraded in the same PR for CI to pass. Tier two BSPs only rely on released versions of the HAL/PACs, so they can be locked to an older version and updated if someone is interested in doing so. This lowers the burden on maintainers as well as folks looking to change the HAL. See the README for more details.

Ok, I understand now. Maybe we can remove the board implementation from this PR, and I open a new PR with it after this one is merged? (If yes, I would probably keep it until the end of the PR for practical reasons and remove it at the end :))

FWIW, to facilitate development, one option is to use a patch in Cargo.toml . The idea would be to PR the changes that you want to eventually be published, but don't include the patch section which just refers to your local copy of atsamd-rs instead of the one on crates.io .

That said, it probably still makes sense to publish the PAC and HAL changes, before a separate PR to add the new boards.

TDHolmes · 2021-12-24T17:32:22Z

boards/samd20_xplained_pro/src/lib.rs

+        name: pb31,
+        aliases: { AlternateB: Sercom5Pad1SpiSS }
+    },
+);


It would be nice to have some of the other BSP helper functions, like SPI/I2C/UART.

hal/Cargo.toml

TDHolmes · 2021-12-24T18:03:05Z

hal/src/thumbv6m/clock.rs

@@ -180,23 +180,35 @@ impl GenericClockController {
        }

        // Feed 32khz into the DFLL48
-        state.enable_clock_generator(DFLL48, GCLK1);
+        #[cfg(not(feature = "samd20"))]
+        state.enable_clock_generator(ClockId::DFLL48, GCLK1);


why DFLL48 here but DFLL48M in state.set_gclk_divider_and_source`?

The svd file defines it that way. Maybe I can try adding a replacement rule in the xslt, I'll check that

I just made a replacement in the xslt so the naming is now identical :)

hal/src/thumbv6m/clock.rs

hal/src/thumbv6m/sercom/v1/i2c.rs

TDHolmes · 2021-12-24T18:30:39Z

hal/src/thumbv6m/sercom/v1/spi.rs

+                #[cfg(feature = "samd20")]
+                while sercom.spi().status.read().syncbusy().bit_is_set()
+                    || sercom.spi().ctrla.read().swrst().bit_is_set()
+                {}


same syncbusy peripheral comment

I think you missed SPI in your reg sync cleanup

Yes, sorry I just pushed my work of tonight and I will continue tomorrow, there are still a few things I need to clean :)

pac/atsamd20j/Cargo.toml

hal/src/dmac/channel/mod.rs

Sympatron · 2021-12-24T21:27:55Z

I'm not that familiar with SAMD20. But I think it's mostly SAMD21 without USB. So it's probably useful to have a common feature for them (samd2x?) to simplify the #[cfg]s.

kaizensparc · 2021-12-27T10:12:06Z

I'm not that familiar with SAMD20. But I think it's mostly SAMD21 without USB. So it's probably useful to have a common feature for them (samd2x?) to simplify the #[cfg]s.

Yes, good idea! I'll try to make a samd2x feature which is activated from any of the samd20/samd21 features. I never wrote anything with features yet so that's going to be something new to learn :)

TDHolmes

Looking better! the samd2x feature helps a lot I think

TDHolmes · 2021-12-27T23:23:34Z

boards/samd20_xplained_pro/.cargo/config.toml

+
+[target.thumbv6m-none-eabi]
+runner = 'arm-none-eabi-gdb -q -x openocd.gdb'
+#runner = 'probe-run --chip ATSAMD21G18A'


nit: even this is commented out, it's the wrong chip name

TDHolmes · 2021-12-27T23:26:10Z

boards/samd20_xplained_pro/Cargo.toml

+optional = true
+
+[dependencies.atsamd-hal]
+path = "../../hal"


ah, good point... I think we'll need to leave it as a path dependency for now since it's new.

Tier one BSPs use path dependencies on the HAL, which means for any HAL change affecting BSPs, all Tier one BSPs must be upgraded in the same PR for CI to pass. Tier two BSPs only rely on released versions of the HAL/PACs, so they can be locked to an older version and updated if someone is interested in doing so. This lowers the burden on maintainers as well as folks looking to change the HAL. See the README for more details.

boards/samd20_xplained_pro/SAMD20.yaml

TDHolmes · 2021-12-27T23:32:18Z

hal/src/gpio/v1.rs

    (PA28, Pa28),
    (PA30, Pa30),
    (PA31, Pa31),
-    #[cfg(any(feature = "min-samd21j", feature = "min-samd51j"))]
+    #[cfg(any(feature = "min-samd2x", feature = "min-samd51g"))]


Are you sure about all of these samd51j -> samd51g changes?

I don't think these changes are correct. I was pretty confident about these attributes when I wrote the module.

TDHolmes · 2021-12-27T23:33:58Z

hal/src/sercom/v2.rs

@@ -40,6 +40,14 @@ pub mod uart;
 #[cfg(feature = "dma")]
 pub mod dma;

+/// Register synchronization flags
+pub enum reg_sync {


I think enums are usually camel case RegSync

I would like to look at this and verify it's the best approach to integrating with the rest of the sercom code.

TDHolmes · 2021-12-27T23:38:06Z

hal/src/spi_common.rs

+        #[cfg(feature = "samd20")]
+        return !self.spi().status.read().syncbusy().bit_is_set();
+        #[cfg(not(feature = "samd20"))]
+        !self.spi().syncbusy.read().enable().bit_is_set()


nit: I'd prefer either both using return or both not just for consistency

TDHolmes · 2021-12-27T23:40:12Z

hal/src/thumbv6m/sercom/v1/i2c.rs

@@ -19,6 +19,12 @@ const BUS_STATE_BUSY: u8 = 3;
 const MASTER_ACT_READ: u8 = 2;
 const MASTER_ACT_STOP: u8 = 3;

+enum i2c_reg_sync {


same camel case comment here

TDHolmes · 2021-12-27T23:41:12Z

hal/src/thumbv6m/sercom/v1/i2c.rs

+
+    /// Wait for the registers synchronization
+    #[inline]
+    fn is_register_synced(&mut self, synced_reg: i2c_reg_sync) -> bool {


not sure &mut self is needed for just a read

Same suggestion as for the SPI and UART modules: consider using the approach that doesn't require maintaining a bespoke enum.

TDHolmes · 2021-12-27T23:42:22Z

hal/src/thumbv6m/sercom/v1/spi.rs

+                #[cfg(feature = "samd20")]
+                while sercom.spi().status.read().syncbusy().bit_is_set()
+                    || sercom.spi().ctrla.read().swrst().bit_is_set()
+                {}


I think you missed SPI in your reg sync cleanup

TDHolmes · 2021-12-27T23:43:07Z

hal/src/thumbv6m/sercom/v1/uart.rs

+
+                            // 16x oversample fractional
+                            #[cfg(not(feature = "samd20"))]
+                            w.sampr().bits(0x00);


does SAMD20 not have oversampling setting? If it does and it's just different, make sure to still set it

bradleyharden · 2021-12-28T03:11:52Z

hal/src/sercom/v1/pads.rs

 pad_alias!(Sercom2, Sercom3);
-#[cfg(any(feature = "min-samd21g", feature = "min-samd51g"))]
+#[cfg(any(feature = "min-samd2x", feature = "min-samd51g"))]


What's the difference between min-samd2x and samd2x? That seems redundant. min-samd21g is intended to exclude samd21e.

bradleyharden · 2021-12-28T03:14:30Z

hal/src/sercom/v2/spi/reg.rs

+    /// Wait for the registers synchronization
+    #[inline]
+    fn is_register_synced(&mut self, synced_reg: reg_sync) -> bool {
+        #[cfg(feature = "samd20")]
+        return !self.spi().status.read().syncbusy().bit_is_set();
+        #[cfg(not(feature = "samd20"))]
+        match synced_reg {
+            reg_sync::CtrlB => !self.spi().syncbusy.read().ctrlb().bit_is_set(),
+            reg_sync::Length => !self.spi().syncbusy.read().length().bit_is_set(),
+            reg_sync::Enable => !self.spi().syncbusy.read().enable().bit_is_set(),
+            reg_sync::SwRst => !self.spi().syncbusy.read().swrst().bit_is_set(),
+        }
+    }
+


I would like to look through the SAMD20 datasheet myself before we move forward with this.

I would like to propose an alternate approach. Instead of declaring a function that takes an enum, we could do something like:

#[cfg(not(feature = "samd20"))] #[inline] fn sync_ctrlb(&self) { while self.spi().syncbusy.read().ctrlb().bit_is_set() {} } #[cfg(feature = "samd20")] #[inline] fn sync_ctrlb(&self) { while self.spi().status.read().syncbusy().bit_is_set() {} }

This has the advantage of not having any overhead of passing enums around, and reduces chances of making mistakes. At any rate, a shared reference to self is sufficient.

Also, SYNCBUSY.LENGTH seems to be exclusive to SAMx5x targets.

bradleyharden · 2021-12-28T03:20:11Z

hal/src/sercom/v2/uart/reg.rs

+            #[cfg(feature = "samd20")]
+            w.txpo().bit(txpo != 0);
+            #[cfg(not(feature = "samd20"))]
+            w.txpo().bits(txpo);
+            w.rxpo().bits(rxpo)


We should review the differences in SERCOM carefully. @jbeaurivage, have you looked at this yet?

I haven't yet, but I'd definitely like to comb through this finely with the datasheet next to me. Annoyingly enough, it seems the SAMD20 isn't exactly 'just a SAMD21 minus the USB'.

@didjcodt, can you please explain what's the reasoning here? This kind of thing should probably be handled at the impl_rxpotxpo level rather than inside the functions.

jbeaurivage · 2021-12-30T17:34:47Z

hal/src/dmac/dma_controller.rs

-#[cfg(any(feature = "samd11", feature = "samd21"))]
+#[cfg(any(feature = "samd11", feature = "samd2x"))]


Please revert this change

jbeaurivage · 2021-12-30T17:35:25Z

hal/src/dmac/channel/reg.rs

-//! SAMD11/SAMD21, the CHID register must be written with the correct channel ID
-//! before accessing the channel specific registers. There is a provided
-//! `with_chid` method that takes a closure with the register read/write proxies
-//! to ensure any read/write to these registers are done in an interrupt-safe
-//! way. For SAMD51+, `with_chid` returns the register block which contains the
-//! registers owned by a specific channel.
+//! SAMD11/SAMD20/SAMD21, the CHID register must be written with the correct
+//! channel ID before accessing the channel specific registers. There is a
+//! provided `with_chid` method that takes a closure with the register
+//! read/write proxies to ensure any read/write to these registers are done in
+//! an interrupt-safe way. For SAMD51+, `with_chid` returns the register block
+//! which contains the registers owned by a specific channel.


Please remove SAMD20 from this comment

jbeaurivage · 2021-12-30T17:35:51Z

hal/src/dmac/channel/mod.rs

-            #[cfg(any(feature = "samd11", feature = "samd21"))]
+            #[cfg(not(feature = "min-samd51g"))]


Please revert this change

jbeaurivage · 2021-12-30T17:36:05Z

hal/src/dmac/channel/mod.rs

-        #[cfg(any(feature = "samd11", feature = "samd21"))]
+        #[cfg(not(feature = "min-samd51g"))]


Please revert this change

jbeaurivage · 2021-12-30T17:36:38Z

hal/src/dmac/channel/mod.rs

-    not(any(feature = "samd11", feature = "samd21")),
+    feature = "min-samd51g",


I would remove all changes to the dmac module, because the SAMD20 chips do not have a DMAC peripheral on board. Could you please feature-gate the dmac module to exclude SAMD20?

jbeaurivage · 2021-12-30T18:42:38Z

hal/src/thumbv6m/pwm.rs

-#[cfg(feature = "samd21")]
-use crate::pac::{TC3, TC4, TC5, TCC1, TCC2};
-#[cfg(feature = "samd21j")]
+#[cfg(feature = "samd2x")]


Does this mean that SAMD20s don't have TC1 and TC2?

jbeaurivage · 2021-12-30T18:44:07Z

hal/src/thumbv6m/sercom/v1/i2c.rs

+
+    /// Wait for the registers synchronization
+    #[inline]
+    fn is_register_synced(&mut self, synced_reg: i2c_reg_sync) -> bool {


Same suggestion as for the SPI and UART modules: consider using the approach that doesn't require maintaining a bespoke enum.

jbeaurivage · 2021-12-30T18:47:49Z

hal/src/thumbv6m/sercom/v1/i2c.rs

+            #[cfg(feature = "samd20")]
+            self.i2cm()
+                .addr
+                .write(|w| w.addr().bits(addr));
+            #[cfg(not(feature = "samd20"))]


Why is this different?

jbeaurivage · 2021-12-30T18:49:47Z

hal/src/thumbv6m/sercom/v1/i2c.rs

+
+        #[cfg(feature = "samd20")]
+        if status.lowtout().bit_is_set() {
+            return Err(I2CError::Timeout);
+        }
+        #[cfg(not(feature = "samd20"))]
        if status.lowtout().bit_is_set() || status.sexttout().bit_is_set()
            || status.mexttout().bit_is_set()
        {


You might want to break this up into separate SAMD20/non-SAMD20 functions to avoid so many #[cfg]s, or at least into config blocks.

jbeaurivage · 2021-12-30T18:50:21Z

hal/src/thumbv6m/sercom/v1/i2c.rs

-#[cfg(feature = "min-samd21g")]
+#[cfg(feature = "min-samd2x")]


min-samd21g -> min-samd2x

TDHolmes reviewed Dec 24, 2021

View reviewed changes

boards/samd20_xplained_pro/.cargo/config Outdated Show resolved Hide resolved

TDHolmes reviewed Dec 24, 2021

View reviewed changes

Sympatron reviewed Dec 24, 2021

View reviewed changes

hal/src/dmac/channel/mod.rs Outdated Show resolved Hide resolved

Sympatron reviewed Dec 24, 2021

View reviewed changes

hal/src/dmac/channel/mod.rs Outdated Show resolved Hide resolved

Sympatron reviewed Dec 24, 2021

View reviewed changes

hal/src/dmac/channel/mod.rs Outdated Show resolved Hide resolved

Add ATSAMD20J18A chip

5316edf

kaizensparc force-pushed the add-samd20 branch from 71800b2 to 74bf234 Compare December 27, 2021 10:06

Kaizen Wolf added 5 commits December 27, 2021 11:15

Add support of SAMD20 in HAL

d37e970

Add SAMD20 Xplained Pro eval board

9b45cfa

Add SAMD20J in crates.json

a6f7165

Add SAMD20 Xplained Pro board in crates.json

07f0252

Remove doc for SAMD20 in hal

da90c84

kaizensparc force-pushed the add-samd20 branch from 74bf234 to da90c84 Compare December 27, 2021 10:15

Kaizen Wolf added 3 commits December 27, 2021 14:52

Merge samd20/samd21 common features in samd2x feature

bd55881

Update ATSAM20J18A xslt to rename DFLL48M to DFLL48

ca44022

Merge min-samd20x features

c86f731

kaizensparc force-pushed the add-samd20 branch from 85b143b to 7c3958b Compare December 27, 2021 23:18

Implement is_reg_synced on sercom registers

b76a5a5

kaizensparc force-pushed the add-samd20 branch from 7c3958b to b76a5a5 Compare December 27, 2021 23:25

TDHolmes reviewed Dec 27, 2021

View reviewed changes

bradleyharden reviewed Dec 28, 2021

View reviewed changes

jbeaurivage reviewed Dec 30, 2021

View reviewed changes

		#[cfg(any(feature = "samd11", feature = "samd21"))]
		#[cfg(any(feature = "samd11", feature = "samd2x"))]

		#[cfg(any(feature = "samd11", feature = "samd21"))]
		#[cfg(not(feature = "min-samd51g"))]

		not(any(feature = "samd11", feature = "samd21")),
		feature = "min-samd51g",

		#[cfg(feature = "min-samd21g")]
		#[cfg(feature = "min-samd2x")]

Add SAMD20J18A support #555

Are you sure you want to change the base?

Add SAMD20J18A support #555

Conversation

kaizensparc commented Dec 24, 2021

Summary

Checklist

If Adding a new Board

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sympatron commented Dec 24, 2021 • edited

kaizensparc commented Dec 27, 2021

TDHolmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sympatron commented Dec 24, 2021 •

edited