Better verilog standard compliance

[hansfbaier]

These items show up as compile errors when compiling in Vivado:

• blocks with locally declared signals need to be named blocks
• signals which are written to from always-blocks need to be reg type
• array size with single digit is only allowed in system verilog

[hansfbaier]

Compiles successfully in Quartus 17.0
log.gz

[birdybro]

There does seem to be some kind of change here. There are new SDC warnings that weren't present before. Just a heads up.

Left is the current template core, right is compiled with your changes.

Extra warnings change:

blockram utilization changes (take it with a grain of salt, the "seeding" changed here too obviously):

[hansfbaier]

Hi, thanks for looking into that. I think my changes to conf_str were causing the changes in block ram/logic utilization.
I just pushed a new version (without that change).
Do you want to rebuild and compare?

[hansfbaier]

Block ram utilization is the same now:

[hansfbaier]

Hmm the ignored filter warnings are still there. Wait a minute, I will commit a fix.
The hierarchy changed because of the named blocks. Those elements now have the block name prefixed.

[hansfbaier]

OK I adjusted the sdc accordingly. The sdc warnings are now gone.

[birdybro]

Yup, the SDC issues seem to be fixed.

[jotego]

Now that you're looking at that... are these changes working with simulators? The current code is rather incompatible with simulation tools from several vendors. I think your changes will make it more compatible. Did you give it a go?

[hansfbaier]

@jotego The changes here are mainly to fix vivado syntax errors during elaboration.
If I run verilator with --lint-only, I am getting a ton of warnings, and some errors.
The most common type of error is this:

%Error-BLKANDNBLK: osd.v:104:13: Unsupported: Blocked and non-blocking assignments to same variable: 'osd.pix_block.cnt'
  104 |  reg [21:0] cnt = 0;
      |             ^~~
                   osd.v:104:19: ... Location of blocking assignment
  104 |  reg [21:0] cnt = 0;
      |                   ^
                   osd.v:108:6: ... Location of nonblocking assignment
  108 |  cnt <= cnt + 1'd1;
      |      ^~

But there is also:

%Error: scanlines.v:5:20: Unsupported in C: Variable has same name as instance: 'scanlines'
    5 |  input       [1:0] scanlines,
      |                    ^~~~~~~~~

Which means it won't work when it is compiled down to C.

But all of those are not related with the changes in this PR.

[birdybro]

In practice, the MiSTer cores that use verilator often don't include the framework present in the template. Jimmystones' verilator template completely leaves out the sys folder.

https://github.com/JimmyStones/Verilator_Template

[jotego]

I also leave the framework out in simulations, which is a shame because we should be able to simulate the whole system. Anyway, this PR is a step in the right direction.

[sorgelig]

MiSTer is currently uses Intel/Altera FPGA and corresponding compiler. Different manufacturers have different requirements, but i see no point to make it complaint with other compilers and make code more complicated.

[sorgelig]

ok. i'll leave it open for some reasons.
Some changes break original concept. For example adding default values for parameters gives a potential hidden bug where parameters may be incompatible with specific core. Leaving parameters undefined requires it to be set in upper level module.

[hansfbaier]

Означает ли это, что если я откатываю это изменение, его можно будет объединить?

[jotego]

ok. i'll leave it open for some reasons. Some changes break original concept. For example adding default values for parameters gives a potential hidden bug where parameters may be incompatible with specific core. Leaving parameters undefined requires it to be set in upper level module.

I think default values for parameters are required by the standard and should be in place. Many tools produce errors if default values are missing.

Just set a default value that would be compatible with the current situation, i.e a default value that would cause the same effect as not having a definition in the current code.

[hansfbaier]

Just set a default value that would be compatible with the current situation, i.e a default value that would cause the same effect as not having a definition in the current code.

I set the default values the same as in the instance site .

[hansfbaier]

@jotego I am seeing with this version quartus seems to infer more register. This is a bit weird, because the output reg signals were all part of synchronous always blocks, so they should have inferred registers anyway.

[hansfbaier]

@jotego

This here would explain 24 of those, but I can't see in any case how that could be done without inferring registers.

[birdybro]

Minor adjustments in the amount of registers happens because of seeding changing sometimes. The fitter is like an aluminum annealing algorithm, there is no absolute solution, it's a best fit given a pseudorandom starting point and a time limit for how much effort to apply. If you add whitespaces to the code you will see registers fluctuate by these small amounts because the physical starting points lead to buffer logic being added sometimes, or not being needed.