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

pmp: add PMP struct #2064

Merged
merged 2 commits into from Aug 13, 2020
Merged

pmp: add PMP struct #2064

merged 2 commits into from Aug 13, 2020

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jul 31, 2020

Pull Request Overview

The PMP wasn't working on the arty because the configure function wasn't actually iterating the PMP regions. This separates out the PMP from the config, and makes a lot more sense to me. And it works on the arty-e21 board.

Testing Strategy

This pull request was tested by running on arty e21.

TODO or Help Wanted

n/a

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

The PMP wasn't working on the arty because the configure function wasn't
actually iterating the PMP regions. This separates out the PMP from the
config, and makes a lot more sense to me. And it works on the arty-e21
board.
@bradjc bradjc added the risc-v RISC-V architecture label Jul 31, 2020
hudson-ayers
hudson-ayers previously approved these changes Aug 5, 2020
@bradjc bradjc requested a review from alistair23 August 10, 2020 14:36
Comment on lines -142 to +156
pub struct PMPConfig<N: PMPConfigType> {
pub struct PMPConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is fine, but just noting that it is moving us more towards the macro approach as we get further away from the array generators.

Copy link
Contributor

Choose a reason for hiding this comment

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

For general awareness, the minimum subset of const generics that will be stabilized now has its own feature gate (rust-lang/rust#74877). Its not stable yet, but clearly on the fast track now.

fn default() -> Self {
PMPConfig {
regions: [None; $x / 2],
is_dirty: Cell::new(true),
last_configured_for: MapCell::empty(),

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an extra new line here

alistair23
alistair23 previously approved these changes Aug 11, 2020
arch/rv32i/src/pmp.rs Outdated Show resolved Hide resolved
@bradjc bradjc dismissed stale reviews from alistair23 and hudson-ayers via 0175fb9 August 13, 2020 20:25
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.

Clearly an improvement today, though I share Alistair's concern about this making the move back away from using a macro harder.

bors r+

@bors bors bot merged commit 5a1e105 into master Aug 13, 2020
@bors bors bot deleted the pmp-updates-macro branch August 13, 2020 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
risc-v RISC-V architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants