blocking assignment must be used for a clock divider

[hirooih]

Let me propose this again. It gets longer, but I hope this let you understand.

---

We have the following rule;

Do not use #delay in synthesizable design modules.

This rule does not work with a divided clock.

The following code demonstrates the issue.

module tb;
    logic clk = 0;
    always
        #5ns clk = ~clk;

    logic rst_n;
    logic [3:0] d;
    always_ff @(posedge clk, negedge rst_n)
        if (~rst_n)
            d <= '0;
        else
            d <= 4'(d + 4'h1);

    // clock with nonblocking assignment
    logic clk_div_nb;
    always_ff @(posedge clk, negedge rst_n)
        if (~rst_n)
            clk_div_nb <= '0;
        else
            clk_div_nb <= ~clk_div_nb;

    logic [3:0] q_nb_ng;
    always_ff @(posedge clk_div_nb)
        q_nb_ng <= d;   // not work by race condition

    // clock with blocking assignment
    logic clk_div_b;
    always_ff @(posedge clk, negedge rst_n)
        if (~rst_n)
            clk_div_b = '0;
        else
            clk_div_b = ~clk_div_b;

    logic [3:0] q_b_ok;
    always_ff @(posedge clk_div_b)
        q_b_ok  <= d;   // works

    initial begin
        rst_n = 1'b0;
        #10ns
        rst_n = 1'b1;
        // $dumpfile("div_clock.vcd");
        // $dumpvars;
        #100ns
        $stop;
    end
endmodule

q_nb_ng and q_b_ok are D-Flip-flops clocked by divided clocks. They capture the output value of d.

• d is clocked by clk.
• q_nb_ng is clocked by clk_div_nb which is divided by using non-blocking assignment (conforming our current rule).
• q_b_ok is clocked by clk_div_b which is divided by using blocking assignment (as I am proposing this pull-request).

The following is the simulation result of the sample code above.

q_b_ok works as we expect, but q_nb_ok does not. For example at the timing on the yellow line they have to capture value 0b0010 as q_b_ok does.

The following shows how they works on the yellow line.

	previous time slot	1st active region	1st NBA region	2nd active region	2nd NBA region
clk	L	L->H	H	H	H
d	0010	0010	0010->0011	0011	0011
clk_div_nb	L	L	L->H	H	H
q_nb_ng	0001	0001	0001	0001	0001->0011
clk_div_b	L	L->H	H	H	H
q_b_ok	0000	0000	0000->0010	0010	0010

1. clk_div_nb (non-blocking) is asserted at the first NBA region.
2. The state goes back to the second active region and at the 2nd NBA region q_nb_ng captures value (0b0011) of d updated at the first NBA region of this time slot.

This is not the behavior expected. (cf. LRM 2017 Figure 4-1—Event scheduling regions)

---

Plan B: Use #delay for all FFs (when we have a divided clock)

Many engineers are choosing this solution. But we know this degrades simulation performance.

Plan C: Use #delay for FFs clocked by divided clock and capture signals derived from FFs clocked by the original clock.

This may be better performance than plan B, but very easy to cause bugs.

---

I should create a branch after fetching updated upstream. Sorry.

[hirooih]

I tried to send the pull request of the fix on 957948e.
But something wrong... Sorry.

[sjgitty]

Suggesting:

Exception: for a clock divider, blocking assignment must be used to not cause race condition.

That said, I'm not agreeing with it yet, and we'll discuss some more internally.

[rswarbrick]

Also, I really think this needs different terminology: a race condition causes non-deterministic behaviour, depending on the whims of the simulator's scheduling. Your example has deterministic behaviour (I think), but not the behaviour you want.

[hirooih]

@sjgitty and @rswarbrick ,
Thank you for your comments.

How about this?

Exception: for a clock divider, blocking assignment must be used to have the behavior you want. See #44 for details.

I'm not sure if a link to this pull request is allowed. But it is too much to include explanation above in the Style Guide.

That said, I'm not agreeing with it yet, and we'll discuss some more internally.

Thank you.

[rich-ho]

I was pointed to this conversation by a colleague. My thinking about this is along these lines:

The general rules (for flop-based designs) are:
D1. State element updates are edge triggered on a clock and assigns must be NB
D2. Combinational logic are always blocking.
D3. Do not use delays (recommendation for cleanliness and avoidance of simulation-synthesis mismatch)

With these rules in place, I believe you should create all your clocks using blocking assigns.
This forces all clock events into the active region and all state updates into the NB region, guaranteed to be after all clocks update.

I believe this is what you're proposing, i.e. to add a style guide statement that all clocks should be generated using blocking assigns. I think that perhaps the wording needs to be clearer and the reason made explicit.

[hirooih]

@rich-ho, thank you for you comment.

I believe this is what you're proposing,

Yes.

• In my proposal always_ff for a clock divider is an exception to non-blocking assign.
• In your proposal always_ff for a clock divider is an exception to "State element".

In either case we need an example code of clock divider for readers.

[hirooih]

I'm sorry I left this PR for a long time.

I've push update, but something wrong.

This branch cannot be rebased due to conflicts
Only those with write access to this repository can merge pull requests.

I did "git rebase". Is it the cause?

Sorry for my mess.

[marnovandermaas]

Ok, I think I've fixed the PR by removing the merge commits. @hirooih do you want to go through and fix the merge conflicts?

[hirooih]

@marnovandermaas

Thanks.
It's been long time since I sent this PR. Give me some time to recall.

[hirooih]

Over the past four years, I have gained experience with git. Now I could make it a clean PR.

• I moved this topic to the end of the section since it is not the main topic of the section.
• I added an example of generating a normal clock in addition to the example of the divided clock.

Don't ask me why non-blocking assignment is used in clk <= 1'b0;. I've been using this style from the example in RTL Modeling with SystemVerilog For Simulation and Synthesis: Using SystemVerilog for ASIC and FPGA Design.

[a-will]

We have the following rule;

Do not use #delay in synthesizable design modules.

This rule does not work with a divided clock.

To be clear (since this seems to be accidental misdirection), the rule is about synthesizable design modules, and it will not be changed. Delay statements only apply to simulation models, which is what you investigated and provided guidance for.

[hirooih]

@a-will

Synthesizable design module often includes clock dividers.

But the following code does not work.

    // clock with nonblocking assignment
    logic clk_div_nb;
    always_ff @(posedge clk, negedge rst_n)
        if (~rst_n)
            clk_div_nb <= '0;
        else
            clk_div_nb <= ~clk_div_nb;

We can solve this issue by using #delay only for this case. (The Plan C in my first comment)

This PR proposes to use blocking assignment only for clock generation.

    // clock with blocking assignment
    logic clk_div_b;
    always_ff @(posedge clk, negedge rst_n)
        if (~rst_n)
            clk_div_b = '0;
        else
            clk_div_b = ~clk_div_b;

The following timing chart using Wavedrom would be easier to understand the problem.

---

I've added the following comments by reading your feedback. Thank you.

// only for test bench code
...
// for both synthesizable and test bench code

The discussion confused what's happening in the actual code.

[a-will]

Some nits.

[a-will]

This should probably be a new section. It's currently written under Sequential Logic (Registers), but it is not about registers. Perhaps Sequential Logic (Clock Dividers) or some such?

[hirooih]

I agree with you.

On my first PR it was only for clock dividers. During the discussion on this PR @rich-ho suggested me applying this rule to clock generation in general.

... to add a style guide statement that all clocks should be generated using blocking assigns. I think that perhaps the wording needs to be clearer and the reason made explicit.

I agreed with him, too. But if we do that, we also need an example for clock generation other than clock divider, so I added it.

As you wrote this rule is not for the section "Sequential Logic (Registers)" now.

I have separated it into a section called “Generating Clock”.
This time I did not squash the commits so that the changes are visible.

How about this?

[a-will]

This portion of the code isn't really relevant for this section. Since this guidance is about how to construct the divider, we don't need to see how clk is generated. I'd recommend removing it.

Edit: Or am I misunderstanding and you also want to show how to generate clock source stimulus in a testbench?

[hirooih]

Edit: Or am I misunderstanding and you also want to show how to generate clock source stimulus in a testbench?

No, I don't.