-
Notifications
You must be signed in to change notification settings - Fork 53
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
Sequencer integration #607
Sequencer integration #607
Conversation
- Reworked sequencer valid/ready/kill/halt semantics. Mul/Div will be updated later. - Not allowing interrupts, NMI or debug min-sequence. - Not allowing to halt ID stage while any of the above are pending, if ID stage is mid-sequence. - Only stopping instructions to exit IF stage due to single step when the last operation of an instruction is done in IF. SEC clean when ZC_EXT=0 Signed-off-by: Oystein Knauserud <[email protected]>
Will turn it back on when RVFI is updated to handle sequences. Signed-off-by: Oystein Knauserud <[email protected]>
Signed-off-by: Oystein Knauserud <[email protected]>
end else if (if_id_pipe_i.instr_valid) begin | ||
sequence_interruptible = first_op_id_i; | ||
end else begin | ||
sequence_interruptible = first_op_if_i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can first_op_if_i be trusted without qualification with some instr_valid signal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a sequenced instruction IF, then the first_op_if will be driven to match the current state of the sequencer. If there is no instruction in IF, the sequencer will by default drive first_op to 1. As soon as a sequenced instruction enter IF, the stage will contain that instruction until it is done. As such, if the IF stage does not contain any instruction, first_op will be 1 and a sequence is not in IF at all.
// Trigger matches will get all write enables deasserted, and debug entered before executing | ||
// any operations. A trigger match is a last_op by definition. | ||
// todo: Factor CLIC pointers? | ||
assign last_op_o = trigger_match_i ? 1'b1 : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is supposed to happen for a trigger match on a sequenced instruction? Will the sequencer keep on sequencing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sequencer will keep on sequencing, but all write enables will be deasserted in the ID stage. As soon as the first operation reaches WB, a debug entry will be performed with the PC of the sequenced instruction in DPC.
rtl/cv32e40x_if_stage.sv
Outdated
// todo: factor in seq_first. For now only tablejump pointer may be the only non-first operation. | ||
assign first_op = prefetch_is_tbljmp_ptr ? 1'b0 : 1'b1; | ||
// todo: factor in CLIC pointers? | ||
assign first_op_o = prefetch_is_tbljmp_ptr ? 1'b0 : seq_first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first and last do not seem to get handled consistently. For example seq_valid is used to decide whether to use seq_last or not, whereas seq_first seems always used.
In general the IF stage can have various reasons/submodules to drive first_op/last_op and I would expect these modules to be clear and to be handled similarly for first_op and last_op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, they are not consistent. Making them consistent (including using seq_valid for calculating first_op) seems very difficult without introducing combinatorial loops.
); | ||
end else begin : gen_no_seq | ||
assign seq_valid = 1'b0; | ||
assign seq_last = 1'b0; | ||
assign seq_instr = '0; | ||
assign seq_ready = 1'b1; | ||
assign seq_first = 1'b0; | ||
assign seq_first = 1'b1; // Tie high to enable default first_op when ZC_EXT=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a bit like a hack because of a non-consistent strategy with respect to merging first/last signals of various modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, relates to the comment above for inconsistency around first/last. The first_op code has to rely on seq_first=1 for seq_valid=0, and thus the tieoff also had to be 1.
- Mainly comments - Added default to sequencer case block. Signed-off-by: Oystein Knauserud <[email protected]>
Signed-off-by: Oystein Knauserud <[email protected]>
Integrating sequencer with pipeline.
SEC clean when localparam ZC_EXT = 0 and correct OBI behavior is assumed (no instr_rvalid_i before instr_req_o)