What is a better write enable

[BrianHG]

Im curious, if you change:

  inout wire [7:0] io_mcu_d,

to:

  inout  [7:0] io_mcu_d,

it wont work?
Maybe someone else here knows why you are getting this problem.
If you really want, you can move the tristate gate outside the MCU module into your top module.  But that just adds a separate in and out ports plus an output enable port.

[pcprogrammer]

No it did not do the trick. Also tried using the same name on all levels. Did not do the trick.

I did find this settings menu, where it has the keep_hierarchy on auto. It allows flatten, auto, manual and all.

Setting it to flatten removes the warnings, but the number of used configuration bits also went down, so it changed the logic in some way.

Edit: Tried the bit stream on the hardware and it works as before, so the flatten option also does the trick.

[BrianHG]

No id did not do the trick. Also tried using the same name on all levels. Did not do the trick.

I did find this settings menu, where it has the keep_hierarchy on auto. It allows flatten, auto, manual and all.

Setting it to flatten removes the warnings, but the number of used configuration bits also went down, so it changed the logic in some way.

It probably merged some duplicate gates across multiple modules.

Also, in your MCU code, with your method, you are clocking logic using multiple clocks.
This is ok, but slows down FPGA fabric since the FPGA has a limited dedicated global clock array.

The way I wrote my interface, I'm clocking all the logic cells with the single 125MHz clock.
What is happening using my 'IF() ...', this causes the compiler to use the 'CE' or 'LE' on the D-flipflop logic cells.  So the FPGA fabric gates which runs the clock-enable/latch-enable are nets with the fabric designed for that purpose.  With your current code, the compiler needs to operate with multiple clock domains, 1 for each different @(posedge & negedge) net name and wire a special new net on the FPGA fabric for that purpose.  This is known as a bad practice as once that logic is clocked at a different time that the dedicated PLL clock line specifically timed to every logic cell on the FPGA, when feeding data from that custom home-made clock to the main system clock, you will be left with metastability issues.

To help fix that up, you will need a complex .sdc timing file describing each of your custom home-made generated clocks and how they relate to each other and how fast each one can go.

[pcprogrammer]

And that is why I started this thread

What I noticed in your solution is that there is no shift register for synchronizing the external signal. I read here https://www.doulos.com/knowhow/fpga/synchronization-and-edge-detection/ in the paragraph "So how do I do that edge detection?" that there is a possibility of instability on the first flip flop, so I used their setup with the three flip flops to do the synchronization code I started this thread with.

You just use the one register and then the "if" they are different to get it in sync with the 125MHz clock. Is there no risk with metastability there?

As per your advise I'm trying to move to an almost single clock design. Still need the extra 250MHz for the DAC clock and write signal.

[BrianHG]

Ok, let's look at it this way.
Twice, draw out on paper just 2 D-flipflop latches and how their clock are wired with simple gates.

#1, your example clock setup with your project;s net names using a separate gates to generate each clock to your final system 125MHz generator.

#2, same story, but tie all the clocks to the 125Mhz and use my 'logic enable' input to tell the D-flipflop whether it should go or not go.

Then I will show you how method #1 skews the output of each clocked flipflop data bus since the fpga's wired logic (which will now change every time you modify your code meaning a different delay) will add a random delay to those outputs.

Once you worked that one out, if you want to read this, you may help understand why 1 single clock design works simpler and how writing things differently may affect how the compiler wires the FPGA:
https://www.eevblog.com/forum/fpga/fpga-vga-controller-for-8-bit-computer/msg2767312/#msg2767312

[pcprogrammer]

A lot of this stuff is known to me, a bit rusty but known. Started "life" with hardware before I turned to software

I understand that having a single clock to all the flipflops is best practice. Also know a FPGA has dedicated clock lines with buffers throughout the chip and that they are limited.

As a "modern" FPGA and verilog learning step I made an asynchronous 14 bit counter to drive the DAC. I could see the difference on the logic analyzer where the different bits would be delayed from each other. Could also tell which bits were placed in the same slice, because they had minimal delay between them. (Tried reverse engineering the original programming of the FNIRSI 1013D FPGA, but failed on the routing between the blocks. I can tell how the IO is configured though and with a bit more effort also how the logic blocks are configured, but the connection matrix eludes me)

The VGA controller bit is very interesting and from a hardware perspective I would also have used version 3.

But this does not answer my last question about metastabillity. In your code the external signal is latched on the 125MHz clock and at the same time the compare is done to detect the edge. But what if at the moment of the rising edge of the clock the external signal is in transition? Could this cause problems like mentioned in the article. https://www.doulos.com/knowhow/fpga/synchronization-and-edge-detection/

[BrianHG]

Just draw the #1 and #2 schematic illustrations and post them here...

[pcprogrammer]

Ok here they are. Did not do the whole design since we are looking at the MCU interface.

Added a third one with the schematic of the code I started this thread with.

Number 1 is the MCU interface as it is now, and it is working, but I know morris6 has had weird problems with his design for the scope, which I copied this code from.
Number 2 is it modified to the 125MHz clock with the use of clock enables on the flip-flops.
Number 3 is just the external signal synchronization part.

The number 2 setup will take in the data on both the falling and the rising edge of the external clock pulse, for which you pointed out in your design that the MCU only needs to toggle instead of pulsing this line for each write.

The output of the data_x flip-flops is used directly by the signal_phase counter which is clocked on the 125MHz clock. The i_xtal signal in the first setup is 50MHz.

I left out the read part and the command based data select since it is about the principle and not the complete design.

[BrianHG]

Ok, first #1:

[BrianHG]

Ok, #2, lets start from scratch:

Code: [Select]

//---------------------------------------------------------------------------
//MCU interface for passing data in and out the FPGA based on a command set
//To avoid the loss of tri state control on the mcu databus, hierarchy is discarded

(*keep_hierarchy = "no"*)

module mcu_interface
(
  //Main clock input
  input i_main_clk,

  //Control input
  input i_clk,         //Clock signal:             Active low going pulse from the mcu to clock data
  input i_rw,          //Direction select signal:  Read 0 / Write 1
  input i_dc,          //Type select signal:       Data 0 / Command 1
 
  //Output to other module 
  output [31:0] o_channel1_negative_signal_step,
  output [31:0] o_channel1_positive_signal_step,
  output [31:0] o_channel2_negative_signal_step,
  output [31:0] o_channel2_positive_signal_step,

  //Data bus
  inout [7:0] io_data
);

  //---------------------------------------------------------------------------
  //Main registers

  reg [7:0] command;        //Stores the latest command
  reg [7:0] data_out;       //Stores data byte to be read by mcu
  reg [1:0] data_index;     //Index counter for multiple data byte handling

  //---------------------------------------------------------------------------
  //Command related data.

  reg [7:0] channel1_negative_signal_step[3:0];  //Command 0x00
  reg [7:0] channel1_positive_signal_step[3:0];  //Command 0x01
  reg [7:0] channel2_negative_signal_step[3:0];  //Command 0x02
  reg [7:0] channel2_positive_signal_step[3:0];  //Command 0x03


 
  //---------------------------------------------------------------------------
  //Sample the MCU control inputs to our system clock.

reg ireg_i_clk  ;
reg ireg_i_rw   ;
reg ireg_i_dc   ;
reg ireg_i_data ;
reg dly_i_clk  ;
reg dly_i_rw   ;
reg dly_i_dc   ;


always @(posedge i_main_clk) begin
                                ireg_i_clk  <= i_clk    ;
                                ireg_i_rw   <= i_rw     ;
                                ireg_i_dc   <= i_dc     ;
                                ireg_i_data <= io_data  ;

                                dly_i_clk  <= ireg_i_clk    ;
                                dly_i_rw   <= ireg_i_rw     ;
                                dly_i_dc   <= ireg_i_dc     ;
                                end


  //---------------------------------------------------------------------------
  //Ensure data_out is high Z during write.  i_rw == 1
  //And actual data for read.                i_rw == 0

  assign io_data = ireg_i_rw ? 8'bZ : data_out;

endmodule

//---------------------------------------------------------------------------
Why is this first step a good idea?
What did I just do?
Does it remind you of something else?
Why will the next step fix all your timing problems?

Now, everything you code next should minimize transitions on the MCI control port.
Everything looks to be 32bit controls.  You need to decide whether to latch 4bytes in a command,
or the entire 16 bytes in 1 shot.

[BrianHG]

Next step, add a 32bit clock in data register as well as triggers when and how that data should be stored.

Hint, this should only be 1 line.

[pcprogrammer]

With this first step you separate the external world from the internal FPGA making sure it is all in a known state for further processing. With the delayed signals you rule out the meta stability issue.

It is similar to having the DAC output clocked in the IO blocks to overcome the signal delays within the chip.

I'm leaning towards your idea with the transfer done signal from the MCU and have a 7 byte array for holding the command plus at max 6 bytes of data. On the other hand it might be better to have the step values for a single channel change at the same time, because otherwise the frequency can vary when the pulse width is changed.

But why is that article I pointed out stating it is better to have at least a second flip flop for making sure meta stability is kept out of the door, while you only use one?

[BrianHG]

But why is that article I pointed out stating it is better to have at least a second flip flop for making sure meta stability is kept out of the door, while you only use one?

They are assuming that that 125MHz clock is not the same speed as your system clock,
or,
That the enable out requires extra width since you may be sending data to a slower clock domain,
or,
That the data latch clock is in parallel with the data itself and you want the data to be all set and valid first.

We do not need the extra width and our source MCU toggles less than half the frequency of the 125MHz, so, once everything is already sampled on the 125Mhz bus, and we know the MCU sets the data clock latch on it's next cycle, we can assume the data on that next clock is truly valid and we can latch away treating it as synchronous data.

Now, for that 1 liner 32bit data register.

[pcprogrammer]

That is a tricky one.

From the MCU point of view four toggles are needed to get the data into the FPGA, which can be done with a shift register construction, but that is not a single line (well can be but it is multiple instructions)

And looking back on your example there is a need for an "if" to determine the when and possibly a case statement for deciding to which "variable" the data needs to be written.

I found out that it does not allow a byte array to be assigned with just the name.

Code: [Select]

reg [7:0] data_in[3:0];
reg [31:0] control_value;

assign control_value <= data_in;

Is not allowed, even though the number of bits is the same.

So I don't know how to do this in a single line.

[BrianHG]

Try this:

Code: [Select]

//---------------------------------------------------------------------------
  //Sample the MCU control inputs to our system clock.

reg ireg_i_clk           = 0 ;
reg ireg_i_rw            = 0 ;
reg ireg_i_dc            = 0 ;
reg ireg_i_data          = 0 ;
reg dly_i_clk            = 0 ;
reg dly_i_rw             = 0 ;
reg dly_i_dc             = 0 ;
reg [31:0] long_data_reg = 0 ;

// Choose one of the following
//wire shift_8_32_data = ( ireg_i_clk && !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load and shift in the new 8 bit data.
wire shift_8_32_data = ( ireg_i_clk !=  dly_i_clk ) ; // every time the dly_i_clk transitions high or low, load and shift in the new 8 bit data.

always @(posedge i_main_clk) begin
                                ireg_i_clk  <= i_clk    ;
                                ireg_i_rw   <= i_rw     ;
                                ireg_i_dc   <= i_dc     ;
                                ireg_i_data <= io_data  ;

                                dly_i_clk  <= ireg_i_clk    ;
                                dly_i_rw   <= ireg_i_rw     ;
                                dly_i_dc   <= ireg_i_dc     ;

                                if ( shift_8_32_data )  long_data_reg[31:0] <= { long_data_reg[23:0] , ireg_i_data[7:0]  } ;


                                end

Does this make sense to you?

[BrianHG]

Next, generate a load command trigger wire and point to a 16word 32bit array to store the command's sent address.

[pcprogrammer]

Well the { } construct is new to me, but I get the intention of it.

The full 32 bits register gets assigned the bits of the full register positions 23:0 in positions 31:8 and the 8 bits of the data register into the positions 7:0

Looks like a handy construct.

Guess I have to read the verilog manual again

Edit: I looked back into this tutorial http://www.asic-world.com/verilog/operators2.html and see it is the replication operator. That is the problem with aging, things don't stick in memory that easy anymore. Needs to be used multiple times before it registers.

[pcprogrammer]

Next, generate a load command trigger wire and point to a 16word 32bit array to store the command's sent address.

This is for tomorrow.

[BrianHG]

And don't forget, the load command, or you should have called the MCU input write_data should be written in 1 line, for all your 32bit registers.

[BrianHG]

Edit: I looked back into this tutorial http://www.asic-world.com/verilog/operators2.html and see it is the replication operator. That is the problem with aging, things don't stick in memory that easy anymore. Needs to be used multiple times before it registers.

Concatenation Operator...

Replicator Operator uses double { # {}} braces with a number in front...

[pcprogrammer]

Edit: I looked back into this tutorial http://www.asic-world.com/verilog/operators2.html and see it is the replication operator. That is the problem with aging, things don't stick in memory that easy anymore. Needs to be used multiple times before it registers.

Concatenation Operator...

Replicator Operator uses double { # {}} braces with a number in front...

Oh hell, I proved my own point. I scanned the article and found both the use cases of the braces, understood them and still provided the name of the wrong one

[pcprogrammer]

Code: [Select]

// Choose one of the following
//wire shift_8_32_data = ( ireg_i_clk && !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load and shift in the new 8 bit data.
wire shift_8_32_data = ( ireg_i_clk !=  dly_i_clk ) ; // every time the dly_i_clk transitions high or low, load and shift in the new 8 bit data.


I guess the following is also correct, but that you rather use the logical operator && instead of the bitwise operator & to avoid mistakes when the variable is multi bit.

Code: [Select]

wire shift_8_32_data = ( ireg_i_clk & !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load and shift in the new 8 bit data.


[BrianHG]

Code: [Select]

// Choose one of the following
//wire shift_8_32_data = ( ireg_i_clk && !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load and shift in the new 8 bit data.
wire shift_8_32_data = ( ireg_i_clk !=  dly_i_clk ) ; // every time the dly_i_clk transitions high or low, load and shift in the new 8 bit data.


I guess the following is also correct, but that you rather use the logical operator && instead of the bitwise operator & to avoid mistakes when the variable is multi bit.

Code: [Select]

wire shift_8_32_data = ( ireg_i_clk & !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load and shift in the new 8 bit data.


Whenever your result will be crunched down to a single bit, always use: and = &&, or = ||.

Since wire 'shift_8_32_data' is a single bit, it is safer to use && here.

If ireg_i_clk & dly_i_clk were 4 bits, the single & would make a 4 bit result, each individual bits anded together, but with a 1 bit destination wire, then only bits 0 and 0 of the sources would feed the output.

[pcprogrammer]

Code: [Select]

//---------------------------------------------------------------------------
//Sample the MCU control inputs to our system clock.

reg ireg_i_clk = 0 ;
reg ireg_i_rw = 0 ;
reg ireg_i_dc = 0 ;
reg dly_i_clk = 0 ;
reg dly_i_rw = 0 ;
reg dly_i_dc = 0 ;
reg [7:0] ireg_i_data = 0 ;
reg [31:0] long_data_reg = 0 ;
reg [7:0] command = 0;

wire clock_data = ( ireg_i_clk && !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load the data from the cpu

wire load_command = (dly_i_dc && dly_i_rw && clock_data);  //When dly_i_dc and dly_i_rw are high and there is an active clock transition load the command register

wire shift_8_32_data = (!dly_i_dc && dly_i_rw && clock_data ) ; //When dly_i_dc is low and dly_i_rw is high and there is an active clock transition load and shift in the new 8 bit data.

always @(posedge i_main_clk) begin
     ireg_i_clk  <= i_clk;
     ireg_i_rw   <= i_rw;
     ireg_i_dc   <= i_dc;
     ireg_i_data <= io_data;

     dly_i_clk  <= ireg_i_clk;
     dly_i_rw   <= ireg_i_rw;
     dly_i_dc   <= ireg_i_dc;

     if(load_command) command <= ireg_i_data;

     if ( shift_8_32_data )  long_data_reg[31:0] <= { long_data_reg[23:0] , ireg_i_data[7:0]  } ;

   end

Guess there is no need for "if else" constructs here since it is all parallel logic and not CPU wasting cycles on an "if" that won't be true if the other one is true.

With the other control lines mixed in to the "enable" wires it is more like the original setup. It needs more work of course to fully convert the original code to this proper code.

For sure a very useful exercise in making proper verilog, so thank you again BrianHG.

Regards,
Peter

[BrianHG]

Ok, problems:

Let's stick with 1 clock data.  Choose if you want to clock on the low or high, or both.

wire clock_data = ( ireg_i_clk && !dly_i_clk ) ; // every time the dly_i_clk transitions to high, load the data from the cpu

This means change this line:

    if ( clock_data )  long_data_reg[31:0] <= { long_data_reg[23:0] , ireg_i_data[7:0]  } ;

Now, next:

wire load_command = ( !dly_i_dc && ireg_i_dc ) && ( ireg_i_data < 8 ) ;  // When when dly_i_dc goes from low to high, load command so long as the command address is within range.

Now for the command reg:

reg [ 31:0 ] command [ 0:7 ] = '{default:0} ; // our command register memory, default to '0'.

now for the load command:

if (load_command) command[ ireg_i_data[ 2:0 ]] <= long_data_reg;


Correct your code and let's see if you can figure out how the MCU writes to each of your 8 control regs.