Skip to content
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

cpu/samd5x/periph_can: fix RX #21181

Merged
merged 1 commit into from
Feb 1, 2025
Merged

Conversation

maribu
Copy link
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/marian.buschsieweke@ml-pa.loc/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

riot-ci commented Jan 31, 2025

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
Copy link
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.

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved
@maribu maribu force-pushed the cpu/samd5x/can-fix-rx branch from 58dc586 to 5a28f45 Compare February 1, 2025 12:34
@dylad
Copy link
Member

dylad commented Feb 1, 2025

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

@benpicco
Copy link
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
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
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
2 of 3 checks passed
@maribu
Copy link
Member Author

maribu commented Feb 1, 2025

Thx :-)

@maribu maribu deleted the cpu/samd5x/can-fix-rx branch February 1, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants