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

Add synchronous clear #42

Open
wants to merge 25 commits into
base: pulp-v1
Choose a base branch
from
Open

Add synchronous clear #42

wants to merge 25 commits into from

Conversation

yvantor
Copy link

@yvantor yvantor commented Feb 28, 2024

Synchronous clear is useful for applications where the core internal status must be flushed from the outside (for example, a redundant unit detects an error and needs to flush the core to bring its internals to a clear state), without compromising the regular asynchronous reset path.

@yvantor yvantor requested a review from niwis as a code owner February 28, 2024 13:55
core/controller.sv Outdated Show resolved Hide resolved
core/csr_regfile.sv Outdated Show resolved Hide resolved
core/controller.sv Outdated Show resolved Hide resolved
@yvantor yvantor force-pushed the astral-pulp-v1 branch 3 times, most recently from 7935cf9 to 571c615 Compare February 28, 2024 21:54
@alex96295 alex96295 changed the title Add synchronous clear. Add synchronous clear Mar 4, 2024
Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

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

Hi Yvan, thanks for the PR. I have just a couple of comments.

We have been planning for a while to change to the common cells register macros (https://github.com/pulp-platform/common_cells/blob/master/include/common_cells/registers.svh). That would give more flexibility when choosing FFs but I see that this would be out of the scope for this PR.

parameter SIM_INIT = "none",
parameter SIM_INIT = "zeros",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

It is not strictly needed for this PR, no. I swapped this when working on lockstepped redundant execution. In that case if the caches or any memories propagate Xs to the checker that controls the buses, it might have unwanted behaviour (also during RTL simulation). Since memory macros never have undefined states (would be either initialized at random values or at 0), to reproduce a scenario that is more similar to what we would have on the silicon I changed the init parameter. We could do this also in other ways (preloading in testbenches) or anyway remove this from this PR.

core/cva6.sv Outdated
Comment on lines 523 to 525
rst_uarch_n <= rst_uarch_controller_n;
rst_uarch_n <= rst_uarch_controller_n & ~clear_i;
Copy link
Collaborator

Choose a reason for hiding this comment

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

rst_uarch_n is asynchronous. There are some hazards when driving this with a synchronous signal (e.g. glitches). Are you sure you need to reset non-architected state?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If so, we need to make sure that this is stable e.g. by latching the signal

Copy link
Author

Choose a reason for hiding this comment

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

Did not figure out it to be asynchronous, I will tackle this differently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break fence.t, which resets all non-architectural, microarchitectural state while preserving the architectural state (which you are clearing here). could you connect this directly to clear_i from the top-level?

Copy link
Author

Choose a reason for hiding this comment

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

Sure!

@yvantor yvantor marked this pull request as draft March 5, 2024 17:40
@yvantor
Copy link
Author

yvantor commented Mar 5, 2024

@niwis turning this to draft after related discussion.

@yvantor yvantor force-pushed the astral-pulp-v1 branch 4 times, most recently from a1bf0bc to 046b645 Compare May 10, 2024 06:11
@yvantor yvantor marked this pull request as ready for review May 11, 2024 13:53
@yvantor
Copy link
Author

yvantor commented May 11, 2024

Hey @niwis, I mark this as ready for review even though some work still needs to be done. I have two questions, if you could provide a few hints that would be greatly appreciated.

  • It is not super clear to me what is going wrong with the CI. The execute-riscv-tests terminates with two errors but I could not spot any error message in the log. There are only WIDTHTRUNC and WIDTHEXPAND warnings on parameters I am quite sure I did not touch, do you have suggestions on how to fix this?
  • For the Cheshire integration, the CI checks out from a tag that is not keeping track of several things (for example the fact that the common_cells version changed and there are some new flip flops macros that cannot be found and break the code build). For now I am making an integration branch into Cheshire, I was curious to know if this thing can be automatized further.

Thank you!

@yvantor yvantor force-pushed the astral-pulp-v1 branch 3 times, most recently from 8dac0fd to 03819d3 Compare May 11, 2024 14:34
@yvantor
Copy link
Author

yvantor commented May 12, 2024

Hey Nils, I mark this as ready for review even though some work still needs to be done. I have two questions, if you could provide a few hints that would be greatly appreciated.

* It is not super clear to me what is going wrong with the CI. The execute-riscv-tests terminates with [two errors](https://github.com/pulp-platform/cva6/actions/runs/9028095974/job/24808527749?pr=42#step:4:471) but I could not spot any error message in the log. There are only [WIDTHTRUNC](https://github.com/pulp-platform/cva6/actions/runs/9028095974/job/24808527749?pr=42#step:4:71) and [WIDTHEXPAND](https://github.com/pulp-platform/cva6/actions/runs/9028095974/job/24808527749?pr=42#step:4:93) warnings on parameters I am quite sure I did not touch, do you have suggestions on how to fix this?

* For the Cheshire integration, the CI checks out from a [tag](https://github.com/pulp-platform/cheshire/tree/cva6/pulp-v1.0.0-rel) that is not keeping track of several things (for example the fact that the common_cells version changed and there are some new flip flops macros that cannot be found and break the code build). For now I am making an integration branch into Cheshire, I was curious to know if this thing can be automatized further.

Thank you!

The CI still does not work, there is still the issue of automatically testing the integration into Cheshire. The branch where I tested it is this. The CI succeeds even though there is still the VCU issue (all the Cheshire branches break because of it, I think it is a problem of board availability).

@niwis
Copy link
Collaborator

niwis commented May 13, 2024

Hi @yvantor,

  • regarding build-riscv-tests, the hypervisor test ref was deleted upstream. I have fixed this in 84422f0. Rebasing onto pulp-v1 should fix this.

  • regarding execute-riscv-tests, here seems to be an error with type conversions (although it looks good to me on a first look). Perhaps you have to add a typecast.

  • regarding cheshire integration, this looks indeed like an issue with our FPGA server. I'll look into it.

@yvantor
Copy link
Author

yvantor commented May 13, 2024

Hi @yvantor,

* regarding build-riscv-tests, the hypervisor test ref was deleted upstream. I have fixed this in [84422f0](https://github.com/pulp-platform/cva6/commit/84422f0c156f596744965c362f6e417319cbe1af). Rebasing onto `pulp-v1` should fix this.

* regarding execute-riscv-tests, [here](https://github.com/pulp-platform/cva6/actions/runs/9053882291/job/24873359549?pr=42#step:4:137) seems to be an error with type conversions (although it looks good to me on a first look). Perhaps you have to add a typecast.

* regarding cheshire integration, this looks indeed like an issue with our FPGA server. I'll look into it.

Thanks for the feedback, should be fine now!

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.

2 participants