Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warnings by GoWin IDE #14

Closed
harbaum opened this issue Sep 20, 2023 · 2 comments
Closed

Warnings by GoWin IDE #14

harbaum opened this issue Sep 20, 2023 · 2 comments

Comments

@harbaum
Copy link

harbaum commented Sep 20, 2023

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
Copy link
Owner

ijor commented Sep 20, 2023

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
Copy link
Author

harbaum commented Sep 20, 2023

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 ...

@harbaum harbaum closed this as completed Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants