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

Added support for external clock source and systick user-defined clock value for ARM #1987

Merged
merged 6 commits into from Jul 1, 2020

Conversation

valexandru
Copy link
Contributor

@valexandru valexandru commented Jun 28, 2020

Pull Request Overview

This pull request adds support for setting external clock source for systick
and extends the support for setting a user-defined clock speed for the
systick.

This pull request is needed for #1918 to work because for the i.MX RT 1052 EVKB
board, even though syst_calib.SKEW = 0 and syst.calib.NOREF = 0, the
calibration value is not correct and causes thesystick to overflow too fast.

Also, if I select external clock source in syst_csr, which causes an external
24 MHz clock source to be prescaled to 100KHz and used as systick, the
system will work.

Testing Strategy

This pull request was tested using the i.MX RT 1052 EVKB Board.

Documentation Updated

  • No updates are required.

Formatting

  • Ran make prepush.

arch/cortex-m/src/systick.rs Outdated Show resolved Hide resolved
arch/cortex-m/src/systick.rs Outdated Show resolved Hide resolved
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

I believe @alevy wrote the original code here, so I'd appreciate his sign-off on the revised hertz() just in case there was something magic about reading the TENMS that I don't know about. Otherwise I think this is good to go.

@ppannuto ppannuto requested a review from alevy June 29, 2020 12:40
Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

Though it would lead to some duplicated code, I would lean towards just creating a new ExternalSystick type and reimplementing the SysTick trait for it.

Many of these functions are called on every single iteration of the scheduler, so it seems a shame to have to make these decisions dynamically on every call to enable() and hertz()

@valexandru
Copy link
Contributor Author

Though it would lead to some duplicated code, I would lean towards just creating a new ExternalSystick type and reimplementing the SysTick trait for it.

Many of these functions are called on every single iteration of the scheduler, so it seems a shame to have to make these decisions dynamically on every call to enable() and hertz()

The hertz() function was not significantly changed. If a value was configured in the chip, self.hertz would be different than 0, otherwise everything remains the same. Would there be a problem for checking the value for external_clock in enable()?

Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

I thought about it some more and given that hertz was already checked at runtime instead of compile time I suppose this approach is consistent with what we were already doing, so I will retract my requested changes.

Copy link
Member

@alevy alevy left a comment

Choose a reason for hiding this comment

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

Looks good to me

@hudson-ayers
Copy link
Contributor

bors r+

@bors bors bot merged commit 709a33c into tock:master Jul 1, 2020
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.

None yet

4 participants