Is this "legal" in Verilog?

[SiliconWizard]

Maybe you could have also told us what was the target FPGA? (Unless I missed it.)

The 'LDCE' primitive is a "Transparent Data Latch with Asynchronous Clear and Gate Enable" (Xilinx). It can be found in various forms and names on various FPGAs. Possibly some FPGAs do not support that, but that would sound odd.

While latches should be avoided in general in synchronous designs (and yes, that can be fixed on a RTL level), they can usually be implemented in many FPGAs, so the message you get from Symbiflow is a bit odd. So, could you tell us what your target is? And maybe you have incorrectly defined the target when running the toolchain? We need more info. (But sure, getting rid of latches in your code should "fix" this, just saying that maybe this error message is revealing yet another issue in your setup.)

Sorry, yes, it's an Artix 7 board. The toolchain definitions were OK, I tested this board on other examples and it works fine. The definitions are from a common makefile, so that doesn't change.

The folks at the symbiflow chat also told me about removing the latches, so I changed the code into different sections for each register that holds data or a counter (by the way, this is a simple counter for a picosoc RV5 I'm trying things on). It generated the bitstream correctly, simulation worked, and the compiled firmware worked too. Thanks for all the input.

OK, so possibly this is a Symbiflow "shortcoming" here. (Even though, as said earlier, removing the latches is actually a good thing here, but just to say that the tools should probably have synthesized this for your FPGA target, so I guess this is a shortcoming or a bug...)

[Someone]

Even though, as said earlier, removing the latches is actually a good thing here, but just to say that the tools should probably have synthesized this for your FPGA target, so I guess this is a shortcoming or a bug...

With everything inside a @(posedge clk) is inferring latches anything other than a bug in the tool?

[Bassman59]

Things in that code making it harder for the tools:
if/else reset (avoid resets for FPGA targets)

Why avoid resets? In some fabrics, an async reset (released synchronously) is required as the configuration mechanism doesn't ensure that registers have a defined initial state.

So use a reset if you need one! If you dont need one then dont use one as its just a waste of time and resources. The particular example the OP posted is a register interface endpoint, exactly the sort of situation where a cargo-cult "reset everything" is unlikely required or desirable. Way too many code snippets just include (global) resets when they are rarely needed.

It's not a cargo cult. I well understand where resets are needed -- like for a state machine -- and where they are not, like in pipelined logic.

But whatever. Synplify Pro loves to complain about "Bad things happening" if you don't include a reset in Microsemichip ProASIC-3E, PolarFire, and on the Lattice side in MachXO2. These resets are async, but of course we release them synchronous to the relevant clocks.

But if you do use a reset, don't use an if/else block unless you want to maintain piles of redundant cases. People further into the thread start discussing these finer points.

I understand -- in the Xilinx world, reset are not necessary because of how the flip-flops' initial states are determined by initializers in the HDL, and when GSR is released, the states of all of the registers are known. Other architectures don't work that way, so we have to use resets.

case statement inside a wrapping if, nothing on the else

If the process is synchronous, an else is not necessary -- the flip-flops will retain their current value if you do not assign to them.

They sure will, but again why pick such a convoluted control process if its only going to make reading it harder, and the tools will have a harder time putting it together? Not everything ends up as muxes as the above posters are suggesting, the same functionality can be from register enables. As I said, its all permissible and functional but not the clearest way to do things. So whats your point?

What's interesting is that the advice Someone gives is at cross purposes with the advice others give.

I think we can all agree that there's no need for the "else" in this block:

Code: [Select]

always @(posedge clk) begin
    if (some_logic_is_true)
        q <= d;
    else
        q <= q;
end
and that was my point.

[betocool]

Ok, here's another question, something I wasn't aware of. Do I understand correctly that here:

Code: [Select]

  cnt <= cnt-1;    // we count down
  if UPDOWN cnt <=cnt+1; //unless UPDOWN is high , in which case we count up.

the "if" statement takes precedence (preference?) over the statement above?

Cheers,

Alberto

it is not because it is an 'if' statement. it's because it sits lower in the begin-end block.

"statements shall be executed in the order given". since it sits closer to the 'exit' it has priority. Like i explained : the synthesizer builds logic as it reads statements (in the order given)

counter <= counter;   : the synthesizer makes a register and wires the output to the input.
if (ENABLED) counter <= counter +1;   : the synthesizer detached what was already connected to the input of the register , injects a mux under control of 'ENABLE'. what was attached goes to the '0' state of the mux , the new clause is attached to the '1' state
if(RESET) counter <=0; : synthesizer detaches again and does the mux thing again

What software are you using for your graphs? I'm looking for something that looks like that...

Cheers,

Alberto

[Someone]

It's not a cargo cult. I well understand where resets are needed -- like for a state machine -- and where they are not, like in pipelined logic.

Its exactly that sort of blanket statement, that people mis-interpret and produce cargo cult resets from. Some state machines might need a reset, but its not something that every state machine needs. Resets that are essential for some specification/functionality are all well and good, but most are just added "because".

[betocool]

Alright, just for the sake of the discussion, I replaced this (#1):

Code: [Select]

  always @(posedge clk) begin
    if(!resetn) begin
      cfg_reg <= 32'h 0000_0000;
      cfg_wr_ready <= 0;
    end else begin
      if((reg_we != 4'b 0000) && (reg_addr == 4'b 0000)) begin
        cfg_reg <= reg_di;
        cfg_wr_ready <= 1;
      end else begin
        cfg_wr_ready <= 0;
      end
    end
  end

with this (#2):

Code: [Select]

  always @(posedge clk) begin

    cfg_wr_ready <= 0;
    cfg_reg <= cfg_reg; // This does look a bit funky...

    if((reg_we != 4'b 0000) && (reg_addr == 4'b 0000)) begin
        cfg_reg <= reg_di;
        cfg_wr_ready <= 1;
    end

    if(!resetn) begin
      cfg_reg <= 32'h 0000_0000;
      cfg_wr_ready <= 0;
    end
  end

Based on the "last statement wins"  this is the order of priority I'd think of, and both ways generate a bitstream and work just fine (simulation and real)... I assume they are technically the same? I still find the "something <= something" statement a bit unsettling... must be because I haven't done this in a while...

Not considering whether the "reset" MUST be there or not (let's say yes... for now)... is one way or the other "better" (that is very subjective, I know), or what is considered better practice?

Cheers,

Alberto

[Bassman59]

Alright, just for the sake of the discussion, I replaced this (#1):

Code: [Select]

  always @(posedge clk) begin
    if(!resetn) begin
      cfg_reg <= 32'h 0000_0000;
      cfg_wr_ready <= 0;
    end else begin
      if((reg_we != 4'b 0000) && (reg_addr == 4'b 0000)) begin
        cfg_reg <= reg_di;
        cfg_wr_ready <= 1;
      end else begin
        cfg_wr_ready <= 0;
      end
    end
  end

with this (#2):

Code: [Select]

  always @(posedge clk) begin

    cfg_wr_ready <= 0;
    cfg_reg <= cfg_reg; // This does look a bit funky...

    if((reg_we != 4'b 0000) && (reg_addr == 4'b 0000)) begin
        cfg_reg <= reg_di;
        cfg_wr_ready <= 1;
    end

    if(!resetn) begin
      cfg_reg <= 32'h 0000_0000;
      cfg_wr_ready <= 0;
    end
  end

Based on the "last statement wins"  this is the order of priority I'd think of, and both ways generate a bitstream and work just fine (simulation and real)... I assume they are technically the same? I still find the "something <= something" statement a bit unsettling... must be because I haven't done this in a while...

Not considering whether the "reset" MUST be there or not (let's say yes... for now)... is one way or the other "better" (that is very subjective, I know), or what is considered better practice?

I won't answer the "better practice" question, because I think there are too many opinions about this.

But in your second block, the assignment cfg_reg <= cfg_reg; is not needed because the flip-flops hold their state unless there's a new assignment. Also the code describes cfg_wr_ready as a one-shot strobe, asserted for only one clock tick, which is reasonable. cfg_reg, assigned a new value at the same time, will hold that new value until the next time that register is addressed. There is no reason to clear it after each register write finishes, right?

Regarding "flip-flops hold their state unless assigned," let's discuss that for a moment. On every tick of the clock, the output of the flip-flop updates. It might not change state, but it updates.

The basic flip-flop has two synchronous inputs (ignoring the synchronous preset or clear). These inputs are the D (data) input, which is obvious, and the CE (clock enable) input. The CE controls whether what is on the D input appears on the Q output, or whether the previous output continues to drive. It's called a clock enable because once upon a time (and still, in certain cases), the input literally gated the clock. If CE was off, then the flop never saw the clock pulse, so the output would not change. FPGA flops have a "recirculating mux" built in. This is exactly what it says on the tin. The D input feeds one input to that mux, the flop's Q output feeds the other input, and the CE chooses which to clock into the flop. CE = 1 usually means D input.

The logic that drives the CE input is connected to CLB resources, just like the D input, so the logic equations that drive the CE input can be pretty complex. This is why when you code something you might thinks is an obvious CE, the synthesis can do something which results in the same behavior but with a different implementation.

For your cfg_reg assignment then, you might expect the synthesizer to build a CE that is asserted true when ((reg_we != 4'b 0000) && (reg_addr == 4'b 0000)) is true, which allows the values on reg_di to update the flops. When that write enable/address compare result is false, the CE is off so cfg_reg doesn't change because the old value is fed back into the flop.

I didn't forget the synchronous preset and clear! The synthesis tool will use them, if they exist in your fabric (!), if any of your logic describes something that obviously sets or resets a flip-flop. You might see some complex logic driving the PRE or CLR signals in the same way you see it driving D or CE. Now, this is separate from the global reset. Your if(!resetn) statement should invoke the global reset assuming resetn is "global" in scope.

The crux here is that the synthesis tool will build logic out of the available resources to implement what you describe. The result should match what you get when you simulate your design. This is really all that matters, right? The logic generated may be "inefficient" or it might be clever. You really don't care until you don't meet timing constraints or you run out of resources. That's when you start looking into what the synthesis tool generated.

Good luck.

[free_electron]

What software are you using for your graphs? I'm looking for something that looks like that...

Place-  Drawing Tools - Line
Place-  Drawing Tools - text
Place-  Drawing Tools - circle

in altium schematic 
The nice thing is : it comes in as vectors when you copy-paste. so i can drop it in inkscape or adobe illustrator and 'massage' at will.

[free_electron]

The crux here is that the synthesis tool will build logic out of the available resources to implement what you describe.

be careful with that ...
Synthesizers build a fully minimized system first. Then they 'unroll' it to map it on the target architecture.

In the old altera tools ( Quartus 9.2 is the latest version i regularly used) you could see the post synthesis schematic. That always had simple d registers that had no reset or preset. only d,q and clk . Then you could also visualize the mapped logic that showed the actual implementation on your target. There you could see what features of the actual flipflops were used. depending on what family you used this schematic changed (and became unreadable for humans). the 'simple' post synthesis was actually what the logic 'does' as opposed ot how it is implemented.

That's why you find in the xilinx or altera tools directives that are not true verilog or vhdl. they are there to direct their synthesizer. a lot of the altera macro functions have AHDL ( altera hardware description language) pre-processors to fine control the synthesizer. they use the equivalent of ifdefs to alter implementation depending on family targeted) . Xilinx does that too.
If you use Synopsis that all goes out the window. You get raw gates. If you need a 7 input or gate , that is what you get. In the fpga tools that ends up being mapped on two LUTs since there is no such thing as a 7 input or gate. It needs to be built out of three or four 3-input ors. The problem ? Race conditions or speed. Cascading more gates may mean longer accumulated flight time but with constant delays. Doing the second implementations may mean you get glitches and managing setup and hold times is more tricky as signals arrive skewed. That's why FPGAs use LUTS. the time of a signal through a LUT is constant irrespective of how many inputs and outputs are used. In hard logic that problem does not exist. If you need a 23 input and gate the library will have that as it is geometrically smaller than a chain of ands and you can still run at fmax of the silicon. This is one of the reasons FPGAs are invariably slower than actual silicon. They need to 'expand' logic to fit the resources. it is not 'smallest possible' implementation

see diagram

[betocool]

Ha ha ha! I thought it was some automated schematic generator or something...

Cheers,

Alberto