-
Notifications
You must be signed in to change notification settings - Fork 3k
Update SAML21J18A target to Mbed OS 5.12 #8658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update SAML21J18A target to Mbed OS 5.12 #8658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tools changes. Targets.json seems fine to me.
@maclobdell please have a look. I understand you're involved in conversations about SAMx devices. |
Not that I'm aware of. @ARMmbed/mbed-os-test-team , thoughts? |
@cmonr Not at the moment no |
@janjongboom This'll need a rebase to pull the targets.json updates. @ARMmbed/mbed-os-hal @ashok-rao @ARMmbed/mbed-os-maintainers @maclobdell Please review when able. |
@janjongboom Mind rebasing any time soon? @mbed-os-hal @ashok-rao @ARMmbed/mbed-os-maintainers @maclobdell Would anyone else like to review? |
@cmonr Yes, will do soon. |
f21c170
to
0a1eed4
Compare
@0xc0170 There is a startup script in the IAR folder (although it doesn't seem to do much), but no linker script. I have no idea how to write this either, and I don't have IAR. But if I remove '5' from the target version list then I cannot compile any programs anymore ( |
We can create a feature branch, but it still should pass tests (in this case without IAR support, it would fail?). Here's some good example for the icf (I checked one Sam device and adjusted sections):
Just set RAM, NVIC regions and heap size, this should help you. I assumed there's only one RAM region at address 0x20000000. |
@0xc0170 I assume this still requires a startup script right? It seems empty for this target right now. |
Yes, what about feature branch? Or is this targeting master (needs IAR support still) ? |
@0xc0170 If we need IAR anyway for master I'll go for master straight away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments, but otherwise looks good to me.
@@ -50,7 +50,7 @@ | |||
* Initial system clock frequency. The System RC Oscillator (RCSYS) provides | |||
* the source for the main clock at chip startup. | |||
*/ | |||
#define __SYSTEM_CLOCK (4000000) | |||
#define __SYSTEM_CLOCK (4000000 * 12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment indicates that this is the "System RC Oscillator" which is typically internal to the chip. Why is the being multiplied by 12? If a clock needs to be updated shouldn't it be for an external or PLL clock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, the previous version ran on 4 MHz. In this PR we're upping the clock frequency to 48 MHz (normal speed of the device). However, if this define is not in sync with the actual clock speed all timers are off in Mbed OS. But I cannot update this value after boot (I've tried but it doesn't do anything), so I think it should be the value after boot here, not the one during boot. Have to update the comment though.
@@ -948,9 +948,9 @@ static inline void system_main_clock_set_failure_detect( | |||
const bool enable) | |||
{ | |||
if (enable) { | |||
MCLK->CTRLA.reg |= MCLK_CTRLA_CFDEN; | |||
MCLK->CTRLA.reg |= 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a define that can be used here rather than 4?
"base-address": "0x30000", | ||
"size": "0x10000", | ||
"num-blocks": 3, | ||
"max-test-size": "64 * 1024" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the size need to be reduced for this target? Not enough storage space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Only 256K flash and half is already taken by the binary, so too little to use bigger FlashIAP file system.
Besides the review ongoing, how are tests for this target? Are all green or still some to be fixed? My assumption is that this target will be added in 5.12.x rather than 5.12.0 |
@0xc0170 Some issues with sleep. We'll have a hackathon next week to resolve those. Other than that still blocked on the IAR scripts. |
Note, #9571 - might have implications for new targets. Please review (there's design document, and targets updated, if any questions, let us know). In any case, please reply to this comment if this change is already done or when completed. We switched to IAR8 for 5.12 , so will need IAR8 update . |
0xc0170 Yep, rebasing now and will make sure the patch is in there. |
Looking at the board layout (SAML21 Xplained Pro) we can see that out of three EXT pinouts only two (EXT1 and EXT3) have a set of four pins available on a single SERCOM. For example (PB16, PB17, PB22, PB23) on EXT3 are mapped to SERCOM5. So if we wish to add a peripheral that uses 4 pin SPI like a SX1276 Wing radio, we can only choose EXt3 or EXT1. However, we found out that we can't use radio shield on EXT3 as the pin PB10 used as dio2 interrupt line is also mapped to the LED. For both SPI and I2C peripherlas, HAL chooses Default PAD. This would result in picking up SERCOM0 for I2C pins (PA08 and PA09) which will not work for us as we have attached the secure element to EXT3 and we need to map these pins to SERCOM2. One way of solving the issue is to mark PA08 and PA09 as NC on SERCOM0 by default. Another is to swap the SERCOMx mapping for the PA08 and PA09 between the Default PAD and Extended PAD. A proper fix would probably be to physically cut PB10 and LED connection. This would mean that we don't need to hack the pin map.
f639faa
to
a2c9a43
Compare
Rebased. Some issues with RTC:
|
Add the function analogout_pinmap to all targets.
@janjongboom I take it work on this PR is still ongoing, correct? |
@cmonr Yes, still need to do IAR linker script. |
Test run: FAILEDSummary: 2 of 7 test jobs failed Failed test jobs:
|
@janjongboom any update on this ? |
We can close this and reopen when there is an update. |
Description
Co-authored by @martinichka, follow up from #7276.
This PR adds Mbed OS 5.10 (incl. RTOS) support for the SAML21J18A target. It also updates to the latest available device pack from MDK (to Keil.SAM-L_DFP.1.2.1). There are a few issues left to figure out, mostly timer related (see #8656). Tested on: SAML21 XPLAINED PRO development board (already landed in ARMmbed/mbed-ls#390) with GCC ARM 6.
Note that you need the latest interface firmware from the XPLAINED PRO board (automatically pops up when connecting in Atmel Studio).
There are issues with flashing binaries in succession via the USB drag-n-drop interface, which makes the automated testing process very painful (running into SYNC_FAILED often, restarting board solves this). Are there any ways to cut power to a board between GT tests?
Todo
Test results
Full test log
Pull request type