Fixed todo's related to mul_en/div_en i EX stage #902
Conversation
… EX stage. Routing halt_ex and kill_ex into the multiplier and divider, making sure no state updates occur while halted and that the modules properly drive valid/ready according to halt/kill. SEC clean (multiplier operation was converted to addition to be able to complete SEC run) Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
rtl/cv32e40x_mult.sv
Outdated
| @@ -115,7 +118,7 @@ module cv32e40x_mult import cv32e40x_pkg::*; | |||
| mulh_acc_next = mulh_acc; | |||
|
|
|||
| // Case statement assumes valid_i = 1; the valid_i = 0 scenario | |||
| // is handled after the case statement. | |||
| // is handled after the case statement. | |||
There was a problem hiding this comment.
Remove comment (no longer accurate)
There was a problem hiding this comment.
Removed comment.
There was a problem hiding this comment.
Comment seems still there
rtl/cv32e40x_mult.sv
Outdated
|
|
||
| if (ready_i) begin | ||
| ready_o = 1'b1; | ||
| ready_o = !halt_i || kill_i; |
There was a problem hiding this comment.
Normally we would set ready_o = 1 upon kill even if ready_i=0. Would that be SEC clean as well?
There was a problem hiding this comment.
I will check :)
There was a problem hiding this comment.
Updated, SEC clean.
rtl/cv32e40x_mult.sv
Outdated
| mulh_a = mulh_ah; | ||
| mulh_b = mulh_bh; | ||
|
|
||
| if (ready_i) begin | ||
| ready_o = 1'b1; | ||
| ready_o = !halt_i || kill_i; |
There was a problem hiding this comment.
Updated code, SEC clean.
rtl/cv32e40x_mult.sv
Outdated
| mulh_state_next = MUL_ALBL; | ||
| mulh_acc_next = '0; | ||
| end | ||
| end | ||
| default: ; | ||
| endcase | ||
|
|
||
| // Allow kill at any time | ||
| if (!valid_i) begin | ||
| // Allow kill at any time unless EX stage is halted |
There was a problem hiding this comment.
I don't understand the changed comment here
There was a problem hiding this comment.
Me neither, I will revert it (likely remnants of another approach at using halt/kill in the multiplier)
| @@ -177,8 +180,10 @@ module cv32e40x_mult import cv32e40x_pkg::*; | |||
| mulh_acc <= '0; | |||
| mulh_state <= MUL_ALBL; | |||
| end else begin | |||
| mulh_acc <= mulh_acc_next; | |||
| mulh_state <= mulh_state_next; | |||
| if ((valid_i && !halt_i) || kill_i) begin | |||
There was a problem hiding this comment.
Above mulh_state_next is changed also for !valid, but here that scenario is not handled
There was a problem hiding this comment.
Fixed, SEC clean.
rtl/cv32e40x_div.sv
Outdated
| if (ready_i) begin | ||
| ready_o = 1'b1; | ||
| ready_o = !halt_i || kill_i; |
There was a problem hiding this comment.
Same remark as for multiplier
There was a problem hiding this comment.
Fixed, SEC clean.
| @@ -304,6 +305,8 @@ module cv32e40x_div import cv32e40x_pkg::*; | |||
| comp_inv_q <= 1'b0; | |||
| res_inv_q <= 1'b0; | |||
| end else begin | |||
| // If stage is halted, the divider should have no state updates | |||
| if ((valid_i && !halt_i) || kill_i) begin | |||
There was a problem hiding this comment.
Same remark as for multiplier
There was a problem hiding this comment.
Fixed, SEC clean.
SEC clean. Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
rtl/cv32e40x_div.sv
Outdated
| @@ -258,9 +259,9 @@ module cv32e40x_div import cv32e40x_pkg::*; | |||
| end | |||
|
|
|||
| DIV_FINISH: begin | |||
| valid_o = 1'b1; | |||
| valid_o = valid_i && !(halt_i || kill_i); // No valid outputs while halted or killed | |||
| ready_o = (ready_i && !halt_i) || kill_i; | |||
There was a problem hiding this comment.
Now halt impacts ready_o but not next_state. I see that next-state is not used later on in that case, but it is just difficult to understand like this.
Maybe keep case statement as in original design and assume valid_i=1, halt_i=0, kill_i=0 within the case and deal with exceptions to that assumption after the case statement.
There was a problem hiding this comment.
Reverted to original case statement.
rtl/cv32e40x_mult.sv
Outdated
| @@ -115,7 +118,7 @@ module cv32e40x_mult import cv32e40x_pkg::*; | |||
| mulh_acc_next = mulh_acc; | |||
|
|
|||
| // Case statement assumes valid_i = 1; the valid_i = 0 scenario | |||
| // is handled after the case statement. | |||
| // is handled after the case statement. | |||
There was a problem hiding this comment.
Comment seems still there
…fter/in parallell with the case. Clock gating flipflops when halted or when there are no valid inputs. SEC clean (with multiplier operation modified to addition) Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
…eink/cv32e40x into silabs-oysteink-div-mul-todo
rtl/cv32e40x_mult.sv
Outdated
| mulh_b = mulh_bl; | ||
| end else begin | ||
| if (halt_i) begin | ||
| valid_o = 1'b0; |
There was a problem hiding this comment.
mulh_state_next assignment missing
Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
Factored halt_ex/kill_ex out of mul_en and div_en - multiplier and divider now handles them theirselves.
In the divider, div_en_i and valid_i was then duplicates, changed to using only valid_i.
SEC clean (with multiplier operation converted to add to enable converging SEC)