Skip to content

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 18, 2019

The samr21 is an ARM Cortex-M0 part very simmilar to the samd21, but comes with a on-chip radio (AT86RF233) connected internally via SPI.

This adds support for the SoC family and the SAM R21 Xplained Pro eveluation board.
Most of it is copy & paste from atmel_sam0/samd21.

The AT86RF233 radio is not supported yet at this point.

@zephyrbot
Copy link

zephyrbot commented Feb 18, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #13494 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13494      +/-   ##
==========================================
+ Coverage   51.96%   51.99%   +0.03%     
==========================================
  Files         309      309              
  Lines       45576    45576              
  Branches    10554    10554              
==========================================
+ Hits        23683    23699      +16     
+ Misses      17083    17071      -12     
+ Partials     4810     4806       -4
Impacted Files Coverage Δ
subsys/testsuite/ztest/src/ztest.c 72.72% <0%> (+7.43%) ⬆️
subsys/testsuite/ztest/include/ztest_assert.h 77.77% <0%> (+38.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a546d1...18fa35d. Read the comment docs.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few doc changes ...

@mnkp
Copy link
Member

mnkp commented Feb 19, 2019

This PR looks good overall, thanks @benpicco, but it needs to be split in three parts:

  • ASF files provided by Atmel located in ext/
  • samr21 soc files located in soc/
  • atsamr21_xpro board support located in board/

The commit messages have to follow documentation guidelines Identifying Contribution Origin.

The header files provided by Atmel often have mixed line endings. We should change them to the Unix '\n'. Otherwise the first commit shouldn't contain any modifications to the original files.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for doc changes, thanks!

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several minor issues. Overall it looks good.

One thing which needs to be globally fixed are copyright headers. You'll need to include your name there. I'm only not sure what should happen with the name of the original author. @nashif or @galak should know.

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Power management system has been recently updated. As you probably have noticed by now this PR needs to be rebased on top of the latest master.

@benpicco benpicco force-pushed the samr21 branch 5 times, most recently from 0eac4ef to a5fe7b5 Compare February 22, 2019 17:29
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments.

Unfortunately, there is an ongoing work regarding Power Management API (#13632) which will break current implemenation. When #13632 is merged one will need to run a few string replacement operations in soc/arm/atmel_sam0/common/power.c.

@@ -0,0 +1,35 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is no longer necessary and can be removed. Functions declarations are available via include/power.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like soc/arm/atmel_sam0/common/power.h is still present in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, sorry - I hope I didn't miss anything else, but I think here I just used rm instead of git rm and forgot to commit it.

@benpicco benpicco force-pushed the samr21 branch 2 times, most recently from 944b98d to 0a9091e Compare February 28, 2019 12:40
@benpicco benpicco force-pushed the samr21 branch 2 times, most recently from a47eae9 to bb93263 Compare March 6, 2019 10:42
@benpicco
Copy link
Contributor Author

benpicco commented Mar 6, 2019

@mnkp I replaced the strings with their new versions from #13632

The rest of the changes should be addressed now too.

@benpicco
Copy link
Contributor Author

Once #14005 #13717 and #14128 are merged they can be enabled here as well.

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benpicco Sorry for a delay, I was busy. LGTM but some things have not yet been fixed. Maybe you didn't push all the local changes you did?

Also a minor thing, the "soc: atmel: auto-select peripherals on SAM0" could be squashed with "soc: atmel: add SAMR21" as it logically belongs there.

@@ -0,0 +1,35 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like soc/arm/atmel_sam0/common/power.h is still present in this PR.

@benpicco benpicco force-pushed the samr21 branch 2 times, most recently from 43bdc86 to b28b2db Compare March 12, 2019 22:43
@benpicco benpicco force-pushed the samr21 branch 6 times, most recently from e1d887f to bc66a08 Compare April 9, 2019 13:44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(at least gpio and usb, i2c?)

supported:                                                                                                                                                                   
  - gpio                                                                                      
  - usb_device                                                                                

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add gpio & usb - i2c depends on #14128

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah turns out this makes CI build some more projects with the board!
The build failure should be fixed by #14832 then. I'd rather have this merged first than manually enabling the driver implementations to the board default config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still fails!
#15584 should fix this 🤞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should write it as

zephyr_sources_ifdef(
  (CONFIG_SOC_SERIES_SAMD20 OR CONFIG_SOC_SERIES_SAMD21 OR 
   CONFIG_SOC_SERIES_SAMR21)
  soc_samd2x.c)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I write it like that, soc_samd2x.c does not get build.

Atmel Software Framework (ASF) provides a set of low-level header
files that give access to different hardware peripherals of Atmel's
ICs.

Origin: Atmel SAMR21 Series Device Support (1.1.72)
License: Apache-2.0
URL: http://packs.download.atmel.com/Atmel.SAMR21_DFP.1.1.72.atpack
Purpose: Introduction of ASF for the SAM0 series.
Maintained-by: External

Signed-off-by: Benjamin Valentin <[email protected]>
@galak galak force-pushed the samr21 branch 2 times, most recently from b8eb0d1 to 53cc42f Compare April 26, 2019 11:10
benpicco and others added 5 commits April 26, 2019 14:59
Adds Atmel SAMR21 soc which is based on SAMD21, but with a AT86RF233
radio connected internally via SPI.

The AT86RF233 is not yet supprted by Zephyr at this point.

This code is very much copy & paste from atmel_sam0/samd21

Signed-off-by: Benjamin Valentin <[email protected]>
Add the Atmel SAM R21 Xplained Pro Evaluation Kit to zephyr.

So far, UART, SPI, I2C (depends on zephyrproject-rtos#14128), debug LED and user button
have been tested.

Signed-off-by: Benjamin Valentin <[email protected]>
Take ownership of external SDK files.

Signed-off-by: Benjamin Valentin <[email protected]>
The Atmel SAMD21 (and therefore also the SAMR21) comes with the same
RTC peripheral as the Atmel SAMD20.

Enable it in dts_fixup.h and enable it in the dts for samr21_xpro.

Signed-off-by: Benjamin Valentin <[email protected]>
The init routines are the same for SAMD20, SAMD21 and SAMR21, so
move them into common/ to not have three copies of the same code.

Signed-off-by: Benjamin Valentin <[email protected]>
@nashif nashif merged commit 86bd319 into zephyrproject-rtos:master Apr 28, 2019
@benpicco benpicco deleted the samr21 branch April 28, 2019 17:27
@KwonTae-young KwonTae-young mentioned this pull request May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants