OpenCores
Issue List
Instruction JP (HL) stalls the CPU #6
Closed mcleod_ideafix opened this issue almost 6 years ago
mcleod_ideafix commented almost 6 years ago

When executin the JP (HL) instruction during a simulation, the CPU reads the opcode and the PC is not updated, so it stucks reading again the JP (HL) opcode from the same address, stalling the CPU if no interrupts are triggered.

While tracking down this bug, I noticed that although register mux_rdor had the right value (the current value from HL) for some reason, that value wasn't propaged to register ALU160 even when signal ALU160_sel had also the right value (0). Line #127 of NextZ80Reg.v should assign the current value of mux_rdor to ALU160 according to the current value of ALU160_sel, and therefore, input D0 to ALU16 should be the right value, making ADDR to be also the right value. None of this happened.

Then I noticed the combinatorial always block located from line #123 in NextZ80Reg.v that assigns values to several of such said signals. Comb always use blocking assignments so the order of such do matter.

The first sentences of this always block are: always @* begin DIN = DINW_SEL ? {DI, DI} : ALU8OUT; ALU80 = REG_WSEL0 ? rdow7:0 : rdow15:8; ALU81 = REG_RSEL0 ? mux_rdor7:0 : mux_rdor15:8; ALU160 = ALU160_sel ? pc : mux_rdor;

The last one is that causes the issue. ALU160 is assigned with the current value of pc or mux_rdor depending on the value of ALU160_sel. The problem is that mux_rdor is not yet assigned a value. This is done later inside this always block. As order matter, the value of mux_rdor assigned to ALU160 is the one before the new assignment. In synthesis this bug get unnoticed because synthesizers can infer the right order based upon real dependencies between RHS and LHS parts of assignments.

In order for simulation to work correctly, the assignments inside the always block must be reordered so the ones that rely on variables which also get a value in a different assignment in the same always block, should be written after them. Another option is to break the always block into various always blocks so each one of them models a simpler combinational circuit with no interdependent assignments inside it. This is the approach I've taken. So, the always block starting from line #123 at NextZ80Reg.v ...

always @* begin DIN = DINW_SEL ? {DI, DI} : ALU8OUT; ALU80 = REG_WSEL0 ? rdow7:0 : rdow15:8; ALU81 = REG_RSEL0 ? mux_rdor7:0 : mux_rdor15:8; ALU160 = ALU160_sel ? pc : mux_rdor;

case({REG_WSEL[3], DO_SEL})
    0: DO = ALU80;
    1: DO = th;
    2: DO = FLAGS;
    3: DO = ALU8OUT[7:0];
    4: DO = pc[15:8];
    5: DO = pc[7:0];
    6: DO = sp[15:8];
    7: DO = sp[7:0];
endcase
case({ALU16OP == 4, REG_RSEL[3:0]})
    5'b01001, 5'b11001: mux_rdor = {rdor[15: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

end

Becomes a series of always, like this:

always @* begin DIN = DINW_SEL ? {DI, DI} : ALU8OUT; ALU80 = REG_WSEL0 ? rdow7:0 : rdow15:8; ALU81 = REG_RSEL0 ? mux_rdor7:0 : mux_rdor15:8; ALU160 = ALU160_sel ? pc : mux_rdor; end

always @* begin case({REG_WSEL3, DO_SEL}) 0: DO = ALU80; 1: DO = th; 2: DO = FLAGS; 3: DO = ALU8OUT7:0; 4: DO = pc15:8; 5: DO = pc7:0; 6: DO = sp15:8; 7: DO = sp7:0; endcase end

always @* begin 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 end

which fixed my issue.

ndumitrache commented almost 6 years ago

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

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

The bug manifested after I changed all "x" by "0" in the code.


Assignee
No one
Labels
Bug