# Unexpected Verilog warning re FPGA clock assignment

I've got a question about something I don't understand that is going on in my FPGA project. I need to control two devices (AGC and ADC) through an SPI bus. As as the FPGA will be the master device, I'm generating a clock signal, SCK, in code by dividing the system clock. I then rout that signal to an output wire through a tristate buffer. Below is my relevant bit of code. It's not shown, but the signal that controls the tristate buffer, en_SCK controlled by a FSM, when it is set low in the idle state and then high for the rest of the states.

output wire SDI

//for SCK_clock
reg SCK_gen, SCK_hold;
integer i;
reg en_SCK;
wire neg_edge_SCK;

//SCK_generator
always @(posedge clk)
begin
i <= i+1;
SCK_hold <= SCK_gen;
if(i == 10)
begin
SCK_gen <= ~SCK_gen;
i <= 0;
end
end

assign SCK = (en_SCK) ? SCK_gen : 1'bz;


When I get implement the design i get the following warning:

WARNING:PhysDesignRules:372 - Gated clock. Clock net en_SCK_not0001 is sourced by a combinatorial pin. This is not good design practice. Use the CE pin to control the loading of data into the flip-flop.

Also I notice my clock seems very distorted. But if I don't use the tristate device in my code and direclty assign the clock signal to the output wire (as in the code below) I get a nice clean clock signal.

assign SCK = SCK_gen;


Below is a side by side of the signal SCK without the tristate buffer (left) and with the tristate buffer (right). I'm fairly new to FPGA and Verilog, but my understanding is that using that style of assign code implies a tristate buffer, so I'm confused why it seems to be interpreted as a gated clock source (The XST generated schematic shows it implimented with an and gate. I'm also confused about how it's distorting the clock signal. The FSM should be forcing the en_SCK enable signal high for many times the period of the clock so I'm not sure what's happening. Also according to the demo board manual, other devices share this signal so I have to set it to high impedance when it's not in use. If someone could point me in the right direction, or explain it to me I'd be very great full. Thanks

Here's a link to my origional question at Stackoverflow.com

Here is the entire Module if that makes things more clear. I've only done a couple of simple Verilog projects before, so I'm sure my code is not great. But I did try to design it using a FSM. Maybe my problem is not what I thought. Thanks for you patience.

module AGC_controller
(
input wire clk, reset, set_AGC, AMP_DO,
output wire SDI,  SCK,
output reg inhibit_ADC, AMP_CS,
//spi disable signals  ** all disabled for =1 except AD_CONV.  When AD_CONV=0, disabled.
output wire DAC_CS, AD_CONV, SF_CEO, FPGA_INIT_B,

output reg [7:0] led

);

localparam [2:0]
idle            =   3'b000,
set_up      =   3'b001,
start_data  =   3'b010,
wait_for_neg=   3'b011,
wait_4      =   3'b100,
next_bit    =   3'b101;
//  wait_last   =   3'b110;

//signals
//for SCK_clock
reg SCK_gen, SCK_hold;
integer i;
reg en_SCK;
wire neg_edge_SCK;

initial
begin
SCK_gen = 0;
i=0;
end

//SCK_generator
always @(posedge clk)

begin
i <= i+1;
SCK_hold <= SCK_gen;
if(i == 10)
begin
SCK_gen <= ~SCK_gen;
i <= 0;
end
end

//detect neg edge of SCK
assign neg_edge_SCK = SCK_hold & ~SCK_gen;

//general signals
reg [2:0] state_reg, state_next;
integer bit_position;
reg [7:0] AGC_buf;
reg en_SDI;

//FSM control
always @(posedge clk, posedge reset)
begin
if (reset)
state_reg <= idle;
else
state_reg <= state_next;
end

//FSM next state logic
always @*
begin
state_next = state_reg;
case(state_reg)
idle:
begin
bit_position=7;
AGC_buf = 8'b10011001;
en_SCK = 1'b0;
en_SDI = 1'b0;
AMP_CS = 1'b1;
if(set_AGC)
begin
state_next = set_up;
end
end
set_up:
begin
AMP_CS = 1'b0;
if ((SCK_gen) && (i < 9))
state_next = start_data;
end
start_data:
begin
en_SDI = 1'b1;
if ((SCK_gen == 0) && (i==2))
begin
state_next = wait_for_neg;
en_SCK = 1'b1;
end
end
wait_for_neg:
begin
if (neg_edge_SCK)
begin
state_next = wait_4;

end
end
wait_4:
begin
if (i==4)
begin
//test code to light leds
led[bit_position] = SDI;
if (bit_position == 0)
begin
state_next = idle;

end
else
begin
state_next = next_bit;

end
end
end
next_bit:
begin
bit_position = bit_position - 1;
state_next = wait_for_neg;
end

endcase
end

assign SDI = (en_SDI) ? AGC_buf[bit_position] : 1'bz;
assign SCK = (en_SCK) ? SCK_gen : 1'bz;
//spi disable signals  ** all disabled for =1 except AD_CONV.  When AD_CONV=0, disabled.
assign DAC_CS = (state_reg != idle);
assign AD_CONV = (state_reg == idle);
assign SF_CEO = (state_reg != idle);
assign FPGA_INIT_B = (state_reg != idle);

endmodule

• Provide a reference to the photos and somebody will add them or email them to me if you are unable to put them on a website. – Russell McMahon Mar 22 '12 at 4:33
• @RussellMcMahon I added a link to the photos. Thanks. – Frank Dejay Mar 22 '12 at 4:58
• A few questions... why are you using a tristate buffer for the clock? Why not just hold it high or low (depending on your SPI CPOL/CPHA settings) if it's not enabled? What is the purpose of SCK_hold? (removed tristate question, too early in the morning and I was not reading clearly.) – akohlsmith Mar 22 '12 at 11:32
• It's possible the warning is not complaining about what you think its complaining about. The code you posted doesn't show how clk or en_SCK are generated, or even show a declaration for clk. If you can post more complete code, you are likely to get a better answer. – The Photon Mar 22 '12 at 16:05
• The most likely reason for the warning is if you have some external clock coming in to your design, and you are AND'ing it with something to enable and disable it, and then using that to trigger the code you've posted. – The Photon Mar 22 '12 at 16:06

The warning is basically suggesting that you use an explicit clock primitive, which for whatever reason isn't inferred automatically by the tools.

There should be a reference for your particular FPGA that tells what primitives are available. For a Spartan3E, for example, Xilinx UG617 describes the BUFGCE primitive ("Global Clock Buffer with Clock Enable") and shows how to instantiate it. Other vendors will have similar documentation.

Your clock distortion is probably occurring because tristating an output line isn't something that can be done instantly. Whatever is being driven has both inductance and capacitance, so even if the driver could go high-Z in zero real time (which it can't), the driven network will continue to ring for some time. If that's a problem, you would need to find a way to ensure that the clock line is low, and has been for some time, before tristating it. All in all, there is probably a better way to accomplish your goal besides tristating a clock.

I agree with user572 on the answer to your first question, what is causing the warning. Try explicitly intantiating a clock buffer on your clk input, and let us know if that clears the warning.

I think the strange output behavior is totally unrelated to the warning, though. I can't spot the cause of the problem, partly because your FSM coding style is not very clear. I'm not sure if your synthesis tool will do the right thing with your FSM code or not, which could be one cause of problems.

For your FSM code, I recommend the following changes:

• Use <= instead of =. The difference might not change the logical meaning of the code, but it could lead to differences between simulated behavior and synthesized behavior that will be hard to debug.

• Every output of the state machine should be assigned in every branch of the case, and for every combination of inputs. You are relying on values to be held constant if you don't assign them, and logically that should happen. But in synthesis, this could result in instantiating latches instead of flip-flops and its not a a best-practice coding style.

Yes, the style I'm recommending is very verbose and tedious to write. Verbose tedious code is a fact of life in Verilog. Just be glad you're not writing VHDL.

In particular, where you have state_next = state_reg; before the case statement, I'm not sure what will be the resulting behavior. I think you mean this to create a default output for state_reg, if you don't assign state_reg within the case statement. It's better to include state_next <= state_next in every branch of the case where you don't want it to change.

I think, but you'll have to experiment to find out for sure, that the combination of the blocking assignment style and the @* sensitivity on the always block surrounding the case statement could explain why you see the glitchy output behavior. You basically have circular logic. In one block you assign state_reg <= state_next and in another (combinatorial) block you assign state_next = state_reg.

Logically your code means that the state_next = state_reg assignment happens whenever there's any change to any of the inputs of the block with the case. Which could result in a race condition when state_reg changes, or if another input changes at the same time as a clk edge triggers an update of state_reg. And its not at all clear how this will synthesize.

• OK. I'll try it with those changes and see what the result is. Also I'm trying to use the FSM coding style used in Pong P. Chu's Verilog book. Generally he's uses an always block with combinational logic i.e. = to assign next state logic, and updates the state register with state_reg <= state_next. I tried to follow that coding style, but like I said I'm just learning. By the way. In simulation it worked exactly as I expected. It looks like I'm learning the subtleties between simulation and implementation. Thanks – Frank Dejay Mar 23 '12 at 1:14
• RE "uses an always block with combinational logic i.e. = to assign next state logic, and updates the state register with state_reg <= state_next."; this is perfectly fine. But the state_next = state_reg in the always @* block seems wrong to me...Does your textbook really use that kind of style? – The Photon Mar 23 '12 at 22:45
• Yeah. In Pong P. Chu's "FPGA Prototyping by Verilog Examples." link It's not a text book though. If you follow the link you can download the code. Look at listing 5.2 for a generic FSM. He uses this style through out his book. You notice he starts his next state logic always blocks with **state_next = state_reg // default next state **. Anyways if you're interested you can have a look. – Frank Dejay Mar 24 '12 at 20:02
• I recommend to use whatever coding style is recommended by your synthesis program. If you're using Xilinx tools, have a look at the XST user guide (xilinx.com/support/documentation/sw_manuals/xilinx11/xst.pdf). Starting on pg 194 are examples of three different coding styles that they support, using 1, 2, or 3 always blocks. If you're using a different FPGA vendor or a 3rd-party synthesis engine, they will provide similar documentation. The FSM concept remains the same --- but its a good idea to use a coding style that the synthesis tool expects. – The Photon Mar 24 '12 at 21:43