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

make field_logic.get_next_q_identifier register independent of reset #89

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aszakacs
Copy link

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).

…signal

If an asynchronous reset is employed, the value of field_logic.get_next_q_identifier gets updated when the reset signal changes. When the reset signal is asynchronous to the clock, 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).
@amykyta3
Copy link
Member

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.

@aszakacs
Copy link
Author

aszakacs commented Apr 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants