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] Use top-level straps for PMP reset values #2147

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Mar 1, 2024

By using top-level straps for the PMP reset configuration its easier to implement different reset configurations if there are multiple Ibex cores in the system.

@Razer6 Razer6 requested a review from rswarbrick March 1, 2024 16:42
@Razer6 Razer6 added the Component:RTL RTL issue label Mar 1, 2024
@Razer6 Razer6 requested a review from GregAC March 1, 2024 16:43
@Razer6 Razer6 force-pushed the pmp-reset-straps branch 4 times, most recently from 392f556 to 3048b86 Compare March 1, 2024 16:57
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

This looks like a sensible change to me.

@GregAC can you see any problem with this? If not, I'm happy to take it.

@GregAC
Copy link
Collaborator

GregAC commented Mar 4, 2024

I had intentionally avoid doing this with parameters when I first wrote it to avoid adding yet more parameters to Ibex, though given we've already got loads perhaps it doesn't matter.

Ultimately this is probably what we have to do but I'll have a think about alternative methods.

@GregAC
Copy link
Collaborator

GregAC commented Mar 5, 2024

@Razer6 we intend to take this, I just need to do a couple of checks to ensure there's no issues for non PMP configs or breakages in other things we don't check in CI I hope to get this done over the next couple of days.

@Razer6
Copy link
Member Author

Razer6 commented Mar 5, 2024

@GregAC No hurry on that one. We have changed that in our environment and are not dependent on vendoring in a new version of ibex at the moment.

@Razer6
Copy link
Member Author

Razer6 commented Sep 3, 2024

@GregAC @rswarbrick Could you take another look?

@GregAC
Copy link
Collaborator

GregAC commented Sep 16, 2024

@Razer6 yes will do

Copy link
Collaborator

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

This looks good to me thanks @Razer6 just a couple of small nits to address.

doc/03_reference/pmp.rst Outdated Show resolved Hide resolved
rtl/ibex_lockstep.sv Outdated Show resolved Hide resolved
By using top-level straps for the PMP reset configuration its
easier to implement different reset configurations if there are
multiple Ibex cores in the system.

Signed-off-by: Robert Schilling <[email protected]>
@Razer6
Copy link
Member Author

Razer6 commented Sep 19, 2024

Thanks @GregAC for the review. I just updated the changes for the nits.

@GregAC GregAC added this pull request to the merge queue Sep 23, 2024
@GregAC
Copy link
Collaborator

GregAC commented Sep 23, 2024

Thanks @Razer6

Merged via the queue into lowRISC:master with commit f92d599 Sep 23, 2024
11 checks passed
@Razer6 Razer6 deleted the pmp-reset-straps branch September 23, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL RTL issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants