OpenCores
Issue List
CALL instruction doesn´t seem to properly store PC #4
Closed mcleod_ideafix opened this issue almost 6 years ago
mcleod_ideafix commented almost 6 years ago

Good day!

I´m experiencing issues with CALL instruction. SP register (latch register as defined in line 71 of NextZ80Reg.v) contains 0xFF56. The CALL instruction is located at 0x128B. Return address is hence 0x128E. After reading the destination call address, I can see two write cycles, but the address to write to is 0xFFFF (for both writes). After that, SP register has the value 0xFFFF (instead of 0xFF54 as it should be).

I've tracked the offending behaviour down to a specific case statement in NextZ80Reg.v which assigns a value to mux_rdor. This case statement needs proper values, but you supply values with "x" bits, which is not acceptable. I´ve temporarily fixed it by changing the case into a casex statement, but I'm well aware that casex should be avoided by all means.

I guess all your "x" bits should be "?" bits instead, and use casez instead of casex when applicable.

I'm using iSIM with ISE 14.7 . The design uses the same WAIT pattern that you use for your example app.

ndumitrache commented almost 6 years ago

Can you give me the exact line in NextZ80Reg.v where you found the issue?

mcleod_ideafix commented almost 6 years ago

Of course:

NextZ80CPU.v : line 789, there is this: REG_RSEL = 4'b101x;

Using "x" as "don't care" value is not the recommended way. It's better to use "?".

Because of this, when REG_RSEL is going to be evaluated here:

NextZ80Reg.v : line 139 and following. case({ALU16OP == 4, REG_RSEL3:0}) 5'b01001,5'b11001 : mux_rdor = {rdor15:8, r}; 5'b01010,5'b01011 : mux_rdor = sp; 5'b01100, 5'b01101, 5'b11100, 5'b11101: mux_rdor = {8'b0, CONST}; default : mux_rdor = rdor; endcase

It would be expected that the case with 5'b01010,5'b01011 : mux_rdor = sp; should be chosen, but actually, it's the "default" one which is chosen because "case" doesn't work with "x", "z" or "?". It works in synthesis because there is no "x" in synthesis, but there is on simulation, and because of that, iSIM cannot show me a correct behaviour of the design.

A temporary fix is to replace "case" with "casex", which does take "x" into account, but it's advisable not to use this type of "casex" for synthesizable designs.

IMHO, the right thing to do would be to use "?" instead of "x" to denote "don't cares" and use "casez" instead of "case" or "casex" to evaluate expressions containing these "don't care" bits.

ndumitrache commented almost 6 years ago

I assign 'x' as don't care in order to help for a tighter synthesis. I'm not sure what '?' is doing, so I don' use it (maybe it is more like high impedance, but I would not use high impedance for internal signals in FPGA). If you don't like 'x', the simplest solution is to replace all assigned 'x' values with '0' or '1'. But then, you may expect an increased number of used LUTs. As I don't use 'x' in the case constants, I don't think <casex> is required anywhere in the design.

Indeed, as you noticed, ISIM simulator may have problems with these 'x' values, but I consider it is a simulator issue and not a design one. You can avoid it by using <Post translate> simulation instead of <Behavioral> one.

ndumitrache commented almost 6 years ago

I assign 'x' as don't care in order to help for a tighter synthesis. I'm not sure what '?' is doing, so I don' use it (maybe it is more like high impedance, but I would not use high impedance for internal signals in FPGA). If you don't like 'x', the simplest solution is to replace all assigned 'x' values with '0' or '1'. But then, you may expect an increased number of used LUTs. As I don't use 'x' in the case constants, I don't think casex is required anywhere in the design. Indeed, as you noticed, ISIM simulator may have problems with these 'x' values, but I consider it is a simulator issue and not a design one. You can avoid it by using simulation instead of one.

ndumitrache commented almost 6 years ago

I assign 'x' as don't care in order to help for a tighter synthesis. I'm not sure what '?' is doing, so I don' use it (maybe it is more like high impedance, but I would not use high impedance for internal signals in FPGA). If you don't like 'x', the simplest solution is to replace all assigned 'x' values with '0' or '1'. But then, you may expect an increased number of used LUTs. As I don't use 'x' in the case constants, I don't think is required anywhere in the design. Indeed, as you noticed, ISIM simulator may have problems with these 'x' values, but I consider it is a simulator issue and not a design one. You can avoid it by using "Post-Translate" simulation instead of "Behavioral" one. (I repeated the answer as I noticed some of the words were cut because they were inside angle brackets).

ndumitrache commented almost 6 years ago

I assign 'x' as don't care in order to help for a tighter synthesis. I'm not sure what '?' is doing, so I don' use it (maybe it is more like high impedance, but I would not use high impedance for internal signals in FPGA). If you don't like 'x', the simplest solution is to replace all assigned 'x' values with '0' or '1'. But then, you may expect an increased number of used LUTs. As I don't use 'x' in the case constants, I don't think "casex" is required anywhere in the design. Indeed, as you noticed, ISIM simulator may have problems with these 'x' values, but I consider it is a simulator issue and not a design one. You can avoid it by using "Post-Translate" simulation instead of "Behavioral" one. (I repeated the answer as I noticed some of the words were cut because they were inside angle brackets).

mcleod_ideafix commented almost 6 years ago

According to this post: https://electronics.stackexchange.com/questions/225104/assigning-x-in-verilog

"x" should be used only to propagate "unkown" conditions along the design, and only for simulation purposes, but not to denote "don't cares", although some tools may use it that way.

In this other post: https://embdev.net/topic/276558

there seems to be some confusion about using "x", "z" or "?". At the end, the Verilog standard is what dictates the right form:

"You are right, see IEEE Std 1364-2001 Chapter 9.5.1 To make the intent more vissible I suggest using casez with '?'."

Verilog standard (2002): http://www.iuma.ulpgc.es/~nunez/clases-FdC/verilog/Verilog-IEEE-1364.pdf

On section 5.5 this can be read:

"5.5 Support for values x and z

The value x may be used as a primary on the RHS of an assignment to indicate a don’t care value for synthesis. The value x may be used in case item expressions (may be mixed with other expressions, such as 4’b01x0) in a casex statement to imply a don’t care value for synthesis. The value x shall not be used with any operators or mixed with other expressions. The value z may be used as a primary on the RHS of an assignment to infer a three-state driver as described in 5.4. The value z (or ?) may be used in case item expressions (may be mixed with other expressions, such as 4’bz1z0) for casex and casez statements to imply a don’t care value for synthesis. The value z shall not be used with any operators or mixed with other expressions."

So, yes, "x" can be used no denote "don't care" values for synthesis, but then, you have to use "casex" for correct behavioural simulation, not "case" as NextZ80Reg.v shows.

"?" is a synonim of "z", but only for casez/casex statements. If you want to state that a signal is a "don't care", and not "unkown" or "high-impedance", "?" is the way to go.

mcleod_ideafix commented almost 6 years ago

At the end, I've decided to get rid of all "x" values and put "0" instead. A quick check synthesizing the CPU yields very similar numbers for LUT occupancy

ndumitrache commented almost 6 years ago

Yes, you may replace the 'x' with '0' or '1',or use the "Post-Translate" simulation.

ndumitrache closed this almost 6 years ago
ndumitrache commented almost 6 years ago

Regarding the number of LUTs, the situation may be different on other synthesis tools (Vivado, Quartus, Lattice Diamond).


Assignee
No one
Labels
Bug