Skip to content

cpu/samd5x/periph_can: fix RX#21181

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
maribu:cpu/samd5x/can-fix-rx
Feb 1, 2025
Merged

cpu/samd5x/periph_can: fix RX#21181
maribu merged 1 commit intoRIOT-OS:masterfrom
maribu:cpu/samd5x/can-fix-rx

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Jan 31, 2025

Contribution description

CAN required CLK_CANx_APB and CLK_CANx_APB to be running and will not request any clock by itself. We can ensure both clocks to be running by preventing the MCU from entering IDLE state.

The SAMD5x/SAME5x Family Data Sheet says in Section "39.6.9 Sleep Mode Operation" says:

The CAN can be configured to operate in any idle sleep mode. The CAN
cannot operate in Standby sleep mode.

[...]

To leave low power mode, CLK_CANx_APB and GCLK_CANx must be active
before writing CCCR.CSR to '0'. The CAN will acknowledge this by
resetting CCCR.CSA = 0. Afterwards, the application can restart CAN
communication by resetting bit CCCR.INIT.

tl;dr: At most SAM0_PM_IDLE is allowed while not shutting down the CAN controller, but even that will pause communication (including RX).

Apparently, the CAN controller was never tested without also using the USB peripheral, which kept the clocks running as side effect.

Testing procedure

Set up USB CAN stick

$ sudo slcand /dev/ttyACM0 # <-- only needed for serial CAN adapters such as https://canable.io/ (or it's cheap clones from AliExpress)
$ sudo ip link set can0 up type can bitrate 500000

Sending to the SAME54-XPRO from Linux

$ cansend can0 '001#cafe'

Output of candump any

  can0  001   [3]  AB CD EF
  can0  001   [2]  CA FE

RIOT output with master

$ make BOARD=same54-xpro -C tests/drivers/candev flash term
[...]
   text	  data	   bss	   dec	   hex	filename
  20276	   128	 13432	 33836	  842c	/home/[email protected]/Repos/software/RIOT/master/tests/drivers/candev/bin/same54-xpro/tests_candev.elf
[...]
Debugger: ATMEL EDBG CMSIS-DAP ATML2748051800006032 03.25.01B6 (S)
Clock frequency: 16.0 MHz
Target: SAM E54P20A (Rev A)
Programming...... done.
Verification........................................... done.
Done flashing
[...]
2025-01-31 22:13:40,156 # main(): This is RIOT! (Version: 2025.04-devel-56-g775f59)
2025-01-31 22:13:40,158 # candev test application
2025-01-31 22:13:40,158 # 
2025-01-31 22:13:40,161 # Initializing CAN periph device
> send
2025-01-31 22:13:43,438 # send
> receive
2025-01-31 22:13:45,208 # receive
2025-01-31 22:13:45,210 # Reading from Rxbuf...

RIOT output with this PR

$ make BOARD=same54-xpro -C tests/drivers/candev flash term
[...]
2025-01-31 22:17:17,065 # main(): This is RIOT! (Version: 2025.04-devel-57-gd93f0-cpu/samd5x/can-fix-rx)
2025-01-31 22:17:17,067 # candev test application
2025-01-31 22:17:17,067 # 
2025-01-31 22:17:17,070 # Initializing CAN periph device
> send
2025-01-31 22:17:19,297 # send
> receive
2025-01-31 22:17:20,831 # receive
2025-01-31 22:17:20,833 # Reading from Rxbuf...
2025-01-31 22:17:22,548 # id: 1 dlc: hx data: 0xCA 0xFE

Conclusion

In both master and this PR sending from RIOT was picked up on the Linux side. But sending from Linux to RIOT was not picked up by RIOT in master, but with this PR.

Issues/PRs references

None

@maribu maribu requested review from benpicco and dylad as code owners January 31, 2025 21:19
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Jan 31, 2025
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Platform: ARM Platform: This PR/issue effects ARM-based platforms and removed Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jan 31, 2025
@riot-ci
Copy link
Copy Markdown

riot-ci commented Jan 31, 2025 •
edited
Loading

Murdock results

✔️ PASSED

c48525f cpu/samd5x/periph_can: fix RX

Success Failures Total Runtime
10271 0 10271 09m:44s

Artifacts

@maribu maribu enabled auto-merge January 31, 2025 21:39
dylad
dylad reviewed Feb 1, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Spotted a typo. Feels feel to squash directly.

@maribu maribu force-pushed the cpu/samd5x/can-fix-rx branch from 58dc586 to 5a28f45 Compare February 1, 2025 12:34
@dylad
Copy link
Copy Markdown
Member

dylad commented Feb 1, 2025

Should we provide a way to unblock it btw ? Or leave it to the user ?

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Feb 1, 2025

You probably want to unblock that again in _power_off()

@maribu maribu force-pushed the cpu/samd5x/can-fix-rx branch from 5a28f45 to c48525f Compare February 1, 2025 13:13
@maribu maribu added this pull request to the merge queue Feb 1, 2025
@maribu
cpu/samd5x/periph_can: fix RX
b6ebdfe 
CAN required CLK_CANx_APB and CLK_CANx_APB to be running and will not
request any clock by itself. We can ensure both clocks to be running
by preventing the MCU from entering IDLE state.

The SAMD5x/SAME5x Family Data Sheet says in Section
"39.6.9 Sleep Mode Operation" says:

> The CAN can be configured to operate in any idle sleep mode. The CAN
> cannot operate in Standby sleep mode.
>
> [...]
>
> To leave low power mode, CLK_CANx_APB and GCLK_CANx must be active
> before writing CCCR.CSR to '0'. The CAN will acknowledge this by
> resetting CCCR.CSA = 0. Afterwards, the application can restart CAN
> communication by resetting bit CCCR.INIT.

tl;dr: At most SAM0_PM_IDLE is allowed while not shutting down the CAN
controller, but even that will pause communication (including RX).

Apparently, the CAN controller was never tested without also using the
USB peripheral, which kept the clocks running as side effect.
@maribu maribu force-pushed the cpu/samd5x/can-fix-rx branch from c48525f to b6ebdfe Compare February 1, 2025 13:36
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 1, 2025

I moved the pm_block() and pm_unblock() to _power_on() and _power_off(). I also added a bitfield to track the state, so that code relying on powering on and off the CAN to be idempotent to still work. (E.g. powering it own twice and powering it of once every now and then would eventually overflow the blockers value, if we would increment the ref count twice and decrement the ref count only once.)

auto-merge was automatically disabled February 1, 2025 13:40

Pull Request is not mergeable

Merged via the queue into RIOT-OS:master with commit 8665ba9 Feb 1, 2025
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 1, 2025

Thx :-)

@maribu maribu deleted the cpu/samd5x/can-fix-rx branch February 1, 2025 15:50
@maribu maribu mentioned this pull request Feb 3, 2025
Merged
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 3, 2025

Backport provided in #21184

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 3, 2025

This PR was not merged correctly. The commit that got merged was no longer part of the PR at the time of merge.

@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dylad dylad dylad approved these changes

@benpicco benpicco Awaiting requested review from benpicco benpicco is a code owner

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2025.04

Development

Successfully merging this pull request may close these issues.

5 participants

@maribu @riot-ci @dylad @benpicco @mguetschow