PeakRDL-regblock icon indicating copy to clipboard operation
PeakRDL-regblock copied to clipboard

make field_logic.get_next_q_identifier register independent of reset

Open aszakacs opened this issue 1 year ago • 2 comments

If an asynchronous reset is employed, the value of field_logic.get_next_q_identifier gets updated when the reset signal changes, even though the clock hasn't changed. This causes an unwanted update in the register. This change makes the register independent of the reset signal (removes the reset signal from its sensitivity list).

aszakacs avatar Mar 21 '24 08:03 aszakacs

Could you share an example SystemRDL file so that I can reproduce the issue? I'm not sure I fully understand the problem you describe enough to review this yet.

amykyta3 avatar Mar 30 '24 05:03 amykyta3

An example produced by the original code:

always_ff @(posedge clk or negedge arst_n) begin // sensitive to clock & reset
    if(~arst_n) begin
        field_storage.GINT_IRQ.sts.value <= 8'h0;
    end else if(field_combo.GINT_IRQ.sts.load_next) begin
        field_storage.GINT_IRQ.sts.value <= field_combo.GINT_IRQ.sts.next;
    end
    field_storage.GINT_IRQ.sts.next_q <= hwif_in.GINT_RSTS.sts.next; // has no reset value, but changes on negedge of arst_n
 end

in this example, the line field_storage.GINT_IRQ.sts.next_q <= hwif_in.GINT_RSTS.sts.next; is in an always-ff block that is sensitive to the clock, as well as the reset signal. Therefore, GINT_IRQ.sts.next_q's value will be updated with hwif_in.GINT_RSTS.sts.next on the negedge of the reset signal. This does not seem intended, because when a reset is asserted, I expect the register to change to its reset value.

I see two solutions for this:

  1. Introduce a reset value, where HDL would look like the following:
always_ff @(posedge clk or negedge arst_n) begin
    if(~arst_n) begin
        field_storage.GINT_IRQ.sts.value <= 8'h0;
        field_storage.GINT_IRQ.sts.next_q <= 0; // some reset value for next_q
    end else if(field_combo.GINT_IRQ.sts.load_next) begin
        field_storage.GINT_IRQ.sts.value <= field_combo.GINT_IRQ.sts.next;
    end
    field_storage.GINT_IRQ.sts.next_q <= hwif_in.GINT_RSTS.sts.next;
end
  1. Remove the reset signal from the sensitivity list, which is the solution I propose in this PR:
always_ff @(posedge clk or negedge arst_n) begin
    if(~arst_n) begin
        field_storage.GINT_IRQ.sts.value <= 8'h0;
    end else if(field_combo.GINT_IRQ.sts.load_next) begin
        field_storage.GINT_IRQ.sts.value <= field_combo.GINT_IRQ.sts.next;
    end
end
always_ff @(posedge clk) begin // is not sensitive to reset and there is no reset value for next_q
    field_storage.GINT_IRQ.sts.next_q <= hwif_in.GINT_RSTS.sts.next;
end

aszakacs avatar Apr 18 '24 07:04 aszakacs