rkrajnc / minimig-mist

Minimig for the MiST board
GNU General Public License v3.0
63 stars 40 forks source link

Race condition in Copper code? #86

Open dirkwhoffmann opened 5 years ago

dirkwhoffmann commented 5 years ago

While reviewing the Copper code, I came across the following two code blocks:

always @(posedge clk)
    ...
        copjmp1 = 1;
...
always @(posedge clk)
   ...
    if (copjmp1 && clk_ena)

Isn’t there a race condition here, because copjmp1 is written via a blocking assignment and read in the if statement at the same clock event?

sorgelig commented 5 years ago

I didn't analyzed the copper code, but this style is valid. With blocking assignment copjmp1 will be valid in the same clock cycle (not in the next in case of non-blocking). Synthesizer is aware of such assignment and will synthesize it correct, although synthesized logic will be more complex. Blocking assignment for non-local signal is generally bad idea, but if you will change it to non-blocking assignment it may brake something.

dirkwhoffmann commented 5 years ago

Thanks a lot for your replay, Sorgelig. I agree that timing will break if we changed it to non-blocking statements. But can we really assume that copjmp1 is assigned 1 before the if statement is evaluated. As far as I understand the Verilog spec, there is no guaranteed evaluation order between the two always blocks. If the if statement is evaluated first, it would be evaluated with an old value of copjmp1.

rkrajnc commented 5 years ago

Hi,

thanks for pointing this out.

In implemented design, there should be no issue.

The synthesis tool should do 'the right thing', that is, ignore blocking assignment and treat it as a non-blocking.

Simulation tools, on the other hand, won't, so this is probably a simulation - synthesis mismatch, and needs to be fixed.

(Pull requests are welcome ;) )

dirkwhoffmann commented 5 years ago

I'm still a little bit confused about the intended behaviour:

always @(posedge clk)
    ...
        copjmp1 = 1;
...
always @(posedge clk)
   ...
    if (copjmp1 && clk_ena)
        strobe = 1; 

If the synthesis tool ignores the blocking assignments and treats the assignments as non-blocking, this means that:

Is this correct?

rkrajnc commented 5 years ago

Yes, that looks correct. I would test if first though, just to be sure.

While I wrote that the synthesis tool will do the right thing, you shouldn't assume that will always be the case, just that in this specific case it should be* - that is why it is very unwise to use blocking assignments in an edge-sensitive block.

In this specific case, the copjmp1 reg is written in a separate block than the block where it is read, so this should* synthesize as non-blocking.

rkrajnc commented 5 years ago

BTW, if you look at the code, you can see that if copjmp1 was updated immediately, then the if (copjmp1 && clk_ena) would never be true, so this can't be how the synthesized design works.

dirkwhoffmann commented 5 years ago

Yes, you're right, I didn't look at the else part testing clk_ena. So everything must be treated as non-blocking as it is typical for sequential logic. Thanks a lot for your help!

I did ask, because I am currently trying to translate the Copper code into a graphical representation that exhibits the state model together with the exact timing, simply to learn the details about Copper. I started out with looking at UAE, but I gave up quickly, because the UAE code has gotten so complex over the years. While looking for alternatives, I stumbled across the Minimig project which I wasn't aware of before. What I've seen so far is very pleasing, and the code is well documented. Great project!

rkrajnc commented 5 years ago

Looking forward to your work on the vAmiga!