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: improve can-initialization #21173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Jan 29, 2025

Contribution description

This makes the can initialization for the samd5x more stable

  • init: just write values not change them from an unknown state
  • setup clock: follow the Peripheral Channel Control setup method described in 14.6.3.3 of the SAM D5x/E5x Family datasheet.

Testing procedure

find a Board that uses this cpu but has sight problems of getting the can bus up reliably
find its more reliable with this Patch

Issues/PRs references

SAM D5x/E5x Family datasheet page 144 (DS60001507M) 14.6.3.3

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Jan 29, 2025
@kfessel kfessel requested a review from maribu January 29, 2025 23:39
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

I don't have a device to test this, but looks sensible from a quick glance. Do you have a pointer to the datasheet?

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved

/* setup */
pchctrl = GCLK_PCHCTRL_GEN(dev->conf->gclk_src);
GCLK->PCHCTRL[pchid].reg = pchctrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

why no busy wait loop here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at this point the register is changeable (since the channel is deactivated)

Copy link
Member

Choose a reason for hiding this comment

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

I think @mguetschow suggested to add a comment here to explain why not waiting here, in contrast to the other writes.

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved
cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved
cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved
cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved
@kfessel kfessel force-pushed the p-sam-caninit branch 2 times, most recently from 93a057b to 98ce32d Compare January 30, 2025 12:55
@maribu
Copy link
Member

maribu commented Jan 30, 2025

I tested this with my SAME54-XPRO.

In master: TX works, RX does not, unless make debug.
With this PR: TX works, RX does not, unless make debug.

But having the clock init better matching the process in the datasheet does have merit.

Please squash!

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 31, 2025
@riot-ci
Copy link

riot-ci commented Jan 31, 2025

Murdock results

✔️ PASSED

1fa6e09 cpu/samd5x: improve can-initialization

Success Failures Total Runtime
10271 0 10271 10m:42s

Artifacts

@kfessel kfessel enabled auto-merge January 31, 2025 10:59
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants