ijor / fx68k

FX68K 68000 cycle accurate SystemVerilog core
GNU General Public License v3.0
135 stars 31 forks source link

Warnings by GoWin IDE #14

Closed harbaum closed 9 months ago

harbaum commented 9 months ago

I am running fx68k on a Tang Nano 20k. It worked out of the box and at least boots a MiSTery core to the desktop. Still there are a few warnings which might be worth looking at:

WARN  (EX2420) : Latch inferred for net 'ccrMask[4]'; We do not recommend the use of latches in FPGA designs, as they may lead to timing problems("fx68k/fx68kAlu.sv":830)
WARN  (EX3101) : 'ccrMask' inside always_comb block does not represent combinational logic("fx68k/fx68kAlu.sv":830)

This warning only refers to bit 4. But from similar cases i've learned that this may also apply to lower bits which aren't reported. This is related to the default case in line 805 in fx68kAlu being commented out:

            row[14]:    ccrMask = KNZ00;
            row[15]:    ccrMask = 5'b0;         // TAS/Scc, not used in col 3
            // default: ccrMask = CUNUSED;
            endcase     

Why is this commented? Enabling line 805 will at least still boot to the TOS desktop. So it doesn't break the CPU completely.

A similar warning is

WARN  (EX2420) : Latch inferred for net 'stype[1]'; We do not recommend the use of latches in FPGA designs, as they may lead to timing problems("fx68k/fx68kAlu.sv":742)
WARN  (EX3101) : 'stype' inside always_comb block does not represent combinational logic("fx68k/fx68kAlu.sv":742)

Why is stype a reg? Isn't it all supposed to be just combinatorics? And instead of

                reg [1:0] stype;

                if( size11)                 // memory shift/rotate
                    stype = ird[ 10:9];
                else                        // register shift/rotate
                    stype = ird[ 4:3];

                case( {stype, ird[8]})

use something like

case( { (size11?ird[ 10:9]:ird[ 4:3]), ird[8]})

This change also doesn't seem to affect the very basic functionality and the resulting CPU also boots to TOS desktop.

ijor commented 9 months ago

Hi Till,

Seems like a defect in the Verilog compiler you are using. Or at least a very particular interpretation of the language. I'm not an expert on the subtle details of the language. But in the second case it definitely seems like your compiler is broken. May be the compiler ignores the "unique" clause used in the case statement, or it doesn't support local variables?

reg type doesn't necessarily specific a latch or a register. It can be used in combinational logic. Actually in cases like this, reg (or logic) must be used and a wire declaration is not allowed.

Anyway, your modifications seems fine.

Regards,

Anyway,

harbaum commented 9 months ago

Hi Ijor. Thanks for the fast reply.

I am definitely no expert. But I think I understand why the tool complains (I am unsure, but IMHO under the hood it's Synopsis/sinplify). In both cases there are combinatorics which aren't set under all circumstances. So there are situations where the expected behavior seem to be to keep the current state. Which in turn is a latch.

Anyway. It works all and also the blitter works and I am actually pretty impressed that the code works out of the box on a synthesis tool it has never been tested for and which is not very mature.

Thanks again. I am having a lots of fun with your code ...