Add synchronous clear by yvantor · Pull Request #42 · pulp-platform/cva6

yvantor · 2024-02-28T13:55:17Z

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.

core/controller.sv

core/csr_regfile.sv

core/controller.sv

niwis

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.

niwis · 2024-03-04T14:40:49Z

common/local/util/sram_pulp.sv

    parameter USER_EN    = 0,
    parameter NUM_WORDS  = 1024,
-    parameter SIM_INIT   = "none",
+    parameter SIM_INIT   = "zeros",


Is this necessary?

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.

niwis · 2024-03-04T15:25:23Z

core/cva6.sv

-      rst_uarch_n <= rst_uarch_controller_n;
+      rst_uarch_n <= rst_uarch_controller_n & ~clear_i;


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?

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

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

niwis · 2024-03-04T15:27:09Z

core/issue_read_operands.sv

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?

yvantor · 2024-03-05T17:40:33Z

@niwis turning this to draft after related discussion.

yvantor · 2024-05-11T14:11:58Z

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 · 2024-05-12T20:09:41Z

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 · 2024-05-13T17:09:26Z

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 · 2024-05-13T22:03:57Z

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!

yvantor requested a review from niwis as a code owner February 28, 2024 13:55

github-actions bot reviewed Feb 28, 2024

View reviewed changes

core/controller.sv Outdated Show resolved Hide resolved

core/csr_regfile.sv Outdated Show resolved Hide resolved

yvantor force-pushed the astral-pulp-v1 branch from 53cb97f to c836128 Compare February 28, 2024 14:01

github-actions bot reviewed Feb 28, 2024

View reviewed changes

core/controller.sv Outdated Show resolved Hide resolved

yvantor force-pushed the astral-pulp-v1 branch 3 times, most recently from 7935cf9 to 571c615 Compare February 28, 2024 21:54

alex96295 changed the title ~~Add synchronous clear.~~ Add synchronous clear Mar 4, 2024

niwis requested changes Mar 4, 2024

View reviewed changes

yvantor marked this pull request as draft March 5, 2024 17:40

yvantor force-pushed the astral-pulp-v1 branch 4 times, most recently from a1bf0bc to 046b645 Compare May 10, 2024 06:11

yvantor marked this pull request as ready for review May 11, 2024 13:53

yvantor force-pushed the astral-pulp-v1 branch from 046b645 to 46ab066 Compare May 11, 2024 14:08

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

Yvan Tortorella added 7 commits May 13, 2024 22:50

Added synchronous clear for fault recovery setback.

1b5e9b5

[WIP] - Cleaning up synchronous clear integration.

2db198d

[WIP - 2] - Cleaning up synchronous clear integration.

2027c60

[WIP - 3] - Cleaning up synchronous clear integration.

5382b10

[WIP - 4] - Cleaning up synchronous clear integration.

597f271

Bump common_cells

c253363

Add FF with clear.

4fc35d7

Yvan Tortorella added 16 commits May 13, 2024 22:50

Fix error in hpdcache IF adapter.

5abe28c

Better propagate clear_i.

2254f5c

Fix missing comma.

e3d38d3

Propagate clear.

6bdb11c

Fix reset connection.

77c40cd

Fix FF in clic controller.

ff5f435

Fix lint.

a383476

[WIP - 5] - Cleaning up synchronous clear integration.

dd633f8

Clear all BTB and BHT entries.

2fdd7c0

Missing comma in miss handler FFARNC.

e485947

Assign default mask value in CSRs regfile.

098de6a

GLORIOUS FIX.

82655fc

Connect clear in serdiv.

5f61783

Try fix verible-verilog-format.

cb69af5

Try fix lint...

0a27bcf

Try fix lint...

a089bd7

yvantor force-pushed the astral-pulp-v1 branch from 6abb0a1 to a089bd7 Compare May 13, 2024 20:50

Try fix enum assign.

c423efc

Use clearable FF for microarchitectural reset.

2f0d702

		rst_uarch_n <= rst_uarch_controller_n;
		rst_uarch_n <= rst_uarch_controller_n & ~clear_i;

Conversation

yvantor commented Feb 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niwis left a comment

Choose a reason for hiding this comment

Uh oh!

niwis Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

yvantor Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

niwis Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

niwis Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

yvantor Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

niwis Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

yvantor Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

yvantor commented Mar 5, 2024

Uh oh!

yvantor commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yvantor commented May 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niwis commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yvantor commented May 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yvantor commented May 11, 2024 •

edited

Loading

yvantor commented May 12, 2024 •

edited

Loading

niwis commented May 13, 2024 •

edited

Loading