VHDL Gated clock error #NEWBIE

[austnais]

Hi guys,

Pretty new to VHDL. I'm basically still exploring the possibilities, and have had a lot of problems getting the concept. However, I have a concrete problem I need help to solve, and understand.

Code: [Select]

--debounce switch 1
process(clk_50)
begin
IF rising_edge(clk_50) THEN
      flipflops(0) <= but1_in;
      flipflops(1) <= flipflops(0);
      If(counter_set = '1') THEN                  --reset counter because input is changing
        counter_out <= (OTHERS => '0');
      ELSIF(counter_out(counter_size) = '0') THEN --stable input time is not yet met
        counter_out <= counter_out + 1;
      ELSE                                        --stable input time is met
        but1 <= flipflops(1);
      END IF;   
    END IF;
  END PROCESS;

--debounce switch 2

process(clk_50)
begin
IF rising_edge(clk_50) THEN
      flipflops1(0) <= but2_in;
      flipflops1(1) <= flipflops1(0);
      If(counter_set1 = '1') THEN                  --reset counter because input is changing
        counter_out1 <= (OTHERS => '0');
      ELSIF(counter_out1(counter_size1) = '0') THEN --stable input time is not yet met
        counter_out1 <= counter_out1 + 1;
      ELSE                                        --stable input time is met
        but2 <= flipflops1(1);
      END IF;   
    END IF;
  END PROCESS;

bothbut <= but1 AND but2;

process(bothbut)
begin
IF rising_edge(bothbut) THEN
led <= not led;
ELSE
led <= "0000";
END IF;
end process;
END Behavioral;

Point of the code is to debounce two buttons (works perfectly) then if both buttons are pressed, light turns on. (Simple, I know. But one has to start somewhere... 
Code synthesizes nicely, but I get a warning when doing the "generate programming file"

WARNING:PhysDesignRules:372 - Gated clock. Clock net bothbut 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

Can anyone tell me why, and maybe suggest a solution?

[bktemp]

Avoid using generated (combinatorial) logic signals as clocks in an FPGA.
Instead, use a single system clock whenever possible.

So instead of using the rising edge of bothbut directly as a clock, use clk_50 and check for bothbut changing to 1. This will result in a gated clock, that's what the software is telling you.

[lem_ix]

Been a while since I've done any vhdl but I'll give it a shot but do take my advice with a grain of salt.

Your  bothbut process is not clocked and you're doing a memory write. Basically you want to synchronize your whole system with the same clock which would be clk_50 in your case.
So make the process sensitive to bothbut and clk_50 and check for clk_50 edge. Also without looking too much into it, you have a lot of if statements, remember in vhdl you can easily make memory with incomplete state changes. Be very defensive when writing states.

[austnais]

Thanks for replies both!

So I tried your suggestion in several manners.

Code: [Select]

process(clk_50)
begin
IF rising_edge(clk_50) THEN
IF rising_edge(bothbut) THEN
led <= not led;
END IF;
END IF;
end process;
END Behavioral;
This gives the same error as before. I also tried adding bothbut to the sensitivity list, same thing.

Code: [Select]

process(clk_50)
begin
IF rising_edge(clk_50) THEN
IF bothbut = '1' THEN
led <= not led;
END IF;
END IF;
end process;
END Behavioral;
This synthesizes nicely, but simply doesn't work on my CMOD S6 development board.

Code: [Select]

process(clk_50)
begin
IF rising_edge(clk_50) THEN
IF rising_edge(bothbut) THEN
led <= not led;
END IF;
END IF;
end process;
END Behavioral;
This also gives gated clock error.

Please note that if I use a single button to trigger the leds it works just fine. So the problem only arises when I want to use both.

[bktemp]

Instead of rising_edge(bothbut) try this:

Code: [Select]

signal lastbothbut : std_logic:='0';

...

process(clk_50)
begin
IF rising_edge(clk_50) THEN
lastbothbut<=bothbut;
IF (lastbothbut='0') and (bothbut='1') THEN
led <= not led;
END IF;
END IF;
end process;
END Behavioral;

It performs an edge detection, but operates in the clk_50 global clock domain.

[austnais]

Instead of rising_edge(bothbut) try this:

Code: [Select]

signal lastbothbut : std_logic:='0';

...

process(clk_50)
begin
IF rising_edge(clk_50) THEN
lastbothbut<=bothbut;
IF (lastbothbut='0') and (bothbut='1') THEN
led <= not led;
END IF;
END IF;
end process;
END Behavioral;

It performs an edge detection, but operates in the clk_50 global clock domain.

Thanks!
Quickly tested it before I left. It synthesized, but it didn't work on the fpga. I'll double check tomorrow if I did something wrong, but I don't think so.

[james_s]

Something to consider also, a lot of the warning messages it spits out are not necessarily something you have to worry about. Often they will note something that is not good practice but may still work just fine. It's a good idea to try to minimize that sort of thing but it's not something I would worry about too much early on provided the design is working.

[austnais]

james_s: yep. This I've understood. But it seems like the warnings I get actually matters since they specify a flaw in the bothbut logic.

Anyway it didn't work this morning neither. If there are VHDL-afficienados out there, heeelp. 

[AndyC_772]

You're absolutely right to take the warnings seriously. They may not matter for a relatively trivial design, but understanding and eliminating them now will definitely get you into good habits which will be more important as your projects grow in complexity.

Don't use multiple clock domains unless you need to. In this case, using clk_50 as your one and only clock is absolutely the right thing to do.

If you have multiple clocks, then you need to be very sure you understand the timing relationship between them. It's not unusual for the synthesis tool to have to include a lot of routing delay (== wasted logic space) just to ensure that the actual behaviour of the circuit is in accordance with your VHDL source. For example, suppose you have logic along the lines of:

- on clock A, read some inputs, and assign a new value to signal B. Also, under some conditions, generate a rising edge on clock C.

- on clock C, read the value of signal B and generate output D

This is a problem, because things have to happen in a strict order:

- B and C depend on A
- D depends on the new, updated value of B

There's a problem if calculating the new value of signal B takes longer than calculating the new value of C. Signal (clock) C must be delayed for at least as long as it takes for the new value B to become valid, so that D is generated correctly. This means sending it up, down and across the chip just to waste time.

It's much better, therefore, to have everything depend on one clock, even if the logic is more complex.

The right thing to do in this case, is to retain the previous state of 'bothbut', and (in the clk_50 domain) to check whether the current value is '1' and the previous, stored value was '0'.

[hamster_nz]

I'm not sure you are managing "counter_out1" and "counter_out" correctly - I would do it like this:

Code: [Select]

      If but2 = flipflops1(1) THEN                 
         --reset counter because it matches
        counter_out1 <= (OTHERS => '0');
      ELSIF counter_out1(counter_out1'high) = '0' THEN
         --stable input time is not yet met, increment counter
        counter_out1 <= counter_out1 + 1;
      ELSE
        --stable input time is met, set output and reset
        but2 <= flipflops1(1);
        counter_out1 <= (OTHERS => '0');
      END IF;


[hamster_nz]

Just had a thought last night.... maybe things would be easier if your debounce logic generate a "state_changed" and "new_state" signal for each button, rather than just a debouced signal.

[Someone]

Anyway it didn't work this morning neither. If there are VHDL-afficienados out there, heeelp. 

What you've posted so far is a mess, the mysterious "counter_set" doesn't show where or why its set.

WARNING:PhysDesignRules:372 - Gated clock. Clock net bothbut 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

Can anyone tell me why, and maybe suggest a solution?

Its only a warning, the full explanation is just a google away:
https://www.xilinx.com/support/answers/46375.html

While all sorts of designs are legal VHDL code and will simulate fine, only a very small subset are able to be mapped into FPGAs well. There are "rules" to follow while you don't understand the underlying details:

Clocking

"In a synchronous design, only one clock and one edge of the clock should be used..." - Xilinx

An important technique in design is the synchronization of designs with a clock. A regularly alternating signal, the clock provides a time during which signals can be checked and memory units (registers, flip-flops, etc.) can be updated. There are a number of considerations that must be made when using the clock, however.
When clocking a design, one clock signal and one clock edge should be used.
Generating internal clocks or gating clock signals is generally problematic because of glitching and clock skew problems

I disagree with their next "rule" so haven't copied it across but interested parties can google it to find out. Once you know what you are doing then you can start "breaking the rules" running combinatorial signals into clocks of flip flops can be a good design on some architectures and tools, and completely unworkable on others so the rules stick to the safe side and say to never do it.

[hamster_nz]

I disagree with their next "rule" so haven't copied it across but interested parties can google it to find out.

Smart move - Xilinx agree with you too - https://www.xilinx.com/support/documentation/white_papers/wp272.pdf

Once you know what you are doing then you can start "breaking the rules" running combinatorial signals into clocks of flip flops can be a good design on some architectures and tools, and completely unworkable on others so the rules stick to the safe side and say to never do it.

Often the rules only really apply when you want to get the best out of a design (e.g. run at very fast clock frequency, make efficient use of logic).

For small and slow designs you can generally get away with the most awful of design patterns (gated clocks, inferred latches, resets galore, high levels of logic, lots of nets with huge fan-outs, long counters, super complex FSMs, deeply nested 'IF's and 'CASE' statements, not having registered outputs on sub-modules....).

The problem then comes later on, when you have developed a style that incorporates these patterns, and you then start building more challenging projects. You then get implementation issues that need a lot of refactoring to resolve. Eventually you have to forget the old ways, and learn the better ones. It can be really frustrating!

There are lots of little things too - like not using a 3-bit counter if a 5-bit one-hot shift register could be far more efficient (as it trades off two extra flipflops for maybe 6 LUTs, and possibly reduces the 'levels of logic' by one).

And of course the golden rule for high performance designs: "The best way to make timing closure seem easy is to design so you do not have any timing closure problems"

[austnais]

Anyway it didn't work this morning neither. If there are VHDL-afficienados out there, heeelp. 

What you've posted so far is a mess, the mysterious "counter_set" doesn't show where or why its set.

WARNING:PhysDesignRules:372 - Gated clock. Clock net bothbut 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

Can anyone tell me why, and maybe suggest a solution?

Its only a warning, the full explanation is just a google away:
https://www.xilinx.com/support/answers/46375.html

While all sorts of designs are legal VHDL code and will simulate fine, only a very small subset are able to be mapped into FPGAs well. There are "rules" to follow while you don't understand the underlying details:

Clocking

"In a synchronous design, only one clock and one edge of the clock should be used..." - Xilinx

An important technique in design is the synchronization of designs with a clock. A regularly alternating signal, the clock provides a time during which signals can be checked and memory units (registers, flip-flops, etc.) can be updated. There are a number of considerations that must be made when using the clock, however.
When clocking a design, one clock signal and one clock edge should be used.
Generating internal clocks or gating clock signals is generally problematic because of glitching and clock skew problems

I disagree with their next "rule" so haven't copied it across but interested parties can google it to find out. Once you know what you are doing then you can start "breaking the rules" running combinatorial signals into clocks of flip flops can be a good design on some architectures and tools, and completely unworkable on others so the rules stick to the safe side and say to never do it.

I sens a tad cheeky tone from you. As stated, I'm a newbie in VHDL and thus, concepts are not clear to me. Furthermore I (of course) "google'd" the gated clock fault, but it was not clear to me what it meant and why it was applicable in my case.

As for the debounce circuit you may find may whole code below. I already mentioned that it works impeccable when I trigger the leds with one button only. I thought therefore that there is no issues in the debounce logic.

Code: [Select]

library ieee;
use ieee.std_logic_1164.all;
USE ieee.std_logic_unsigned.all;

entity top is
GENERIC(
    counter_size  :  INTEGER := 19; --counter size (19 bits gives 10.5ms with 50MHz clock)
counter_size1  :  INTEGER := 19); --counter size (19 bits gives 10.5ms with 50MHz clock
Port (
-- FPGA_GCLK, 8MHz
CLK : in STD_LOGIC;
-- Cmod LED outputs
LED_0 : out STD_LOGIC;
LED_1 : out STD_LOGIC;
LED_2 : out STD_LOGIC;
LED_3 : out STD_LOGIC;

-- Switch control Reset inputs
   but1_in     : IN   STD_LOGIC; --pushbutton on fpga 1
  but2_in     : IN   STD_LOGIC    --pushbutton on fpga 2

);

end top;

-------------------------------------------------------------------
-- Architecture Start
-------------------------------------------------------------------
architecture Behavioral of top is


  signal led : STD_LOGIC_VECTOR(3 DOWNTO 0);
  signal clk1 : std_logic :='0';
  signal but1 : std_logic :='0';
  signal but2 : std_logic :='0';
  signal bothbut : STD_LOGIC;
  signal lastbothbut : std_logic:='0';
 
  -----------------------------------------------------------------
  --debounce of switch 1
  -----------------------------------------------------------------
SIGNAL flipflops   : STD_LOGIC_VECTOR(1 DOWNTO 0); --input flip flops
SIGNAL counter_set : STD_LOGIC;                    --sync reset to zero
SIGNAL counter_out : STD_LOGIC_VECTOR(counter_size DOWNTO 0) := (OTHERS => '0'); --counter output

  -----------------------------------------------------------------
  --debounce of switch 2
  -----------------------------------------------------------------
SIGNAL flipflops1   : STD_LOGIC_VECTOR(1 DOWNTO 0); --input flip flops
SIGNAL counter_set1 : STD_LOGIC;                    --sync reset to zero
SIGNAL counter_out1 : STD_LOGIC_VECTOR(counter_size1 DOWNTO 0) := (OTHERS => '0'); --counter output

-------------------------------------------------------------------
-- 200 MHz clock
-------------------------------------------------------------------
signal clk_50 : STD_LOGIC;

component clk_gen_50MHz is
port ( CLK_IN1           : in     std_logic;
  CLK_OUT1          : out    std_logic
);
end component;
-------------------------------------------------------------------
-- Behavioral Begin
-------------------------------------------------------------------
BEGIN

CLK_GEN: clk_gen_50MHz port map ( CLK_IN1 => CLK,
  CLK_OUT1 => clk_50);
  LED_0 <= led(0);
  LED_1 <= led(1);
  LED_2 <= led(2);
  LED_3 <= led(3);



-- debounce of switches
counter_set <= flipflops(0) xor flipflops(1);    --determine when to start/reset counter
counter_set1 <= flipflops1(0) xor flipflops1(1);   --determine when to start/reset counter

--debounce switch 1
process(clk_50)
begin
IF rising_edge(clk_50) THEN
      flipflops(0) <= but1_in;
      flipflops(1) <= flipflops(0);
      If(counter_set = '1') THEN                  --reset counter because input is changing
        counter_out <= (OTHERS => '0');
      ELSIF(counter_out(counter_size) = '0') THEN --stable input time is not yet met
        counter_out <= counter_out + 1;
      ELSE                                        --stable input time is met
        but1 <= flipflops(1);
      END IF;   
    END IF;
  END PROCESS;

--debounce switch 2

process(clk_50)
begin
IF rising_edge(clk_50) THEN
      flipflops1(0) <= but2_in;
      flipflops1(1) <= flipflops1(0);
      If(counter_set1 = '1') THEN                  --reset counter because input is changing
        counter_out1 <= (OTHERS => '0');
      ELSIF(counter_out1(counter_size1) = '0') THEN --stable input time is not yet met
        counter_out1 <= counter_out1 + 1;
      ELSE                                        --stable input time is met
        but2 <= flipflops1(1);
      END IF;   
    END IF;
  END PROCESS;

bothbut <= but1 AND but2;

process(clk_50)
begin
 IF rising_edge(clk_50) THEN
  lastbothbut<=bothbut;
  IF (lastbothbut='0') AND (bothbut='1') THEN
   led <= not led;
  END IF;
 END IF;
end process;
END Behavioral;

The debounce is stolen from the Internet and adapted.

[hamster_nz]

As for the debounce circuit you may find may whole code below. I already mentioned that it works impeccable when I trigger the leds with one button only. I thought therefore that there is no issues in the debounce logic.

Code: [Select]

library ieee;
use ieee.std_logic_1164.all;
USE ieee.std_logic_unsigned.all;

entity top is
GENERIC(
    counter_size  :  INTEGER := 19; --counter size (19 bits gives 10.5ms with 50MHz clock)
counter_size1  :  INTEGER := 19); --counter size (19 bits gives 10.5ms with 50MHz clock
Port (
-- FPGA_GCLK, 8MHz
CLK : in STD_LOGIC;
-- Cmod LED outputs
LED_0 : out STD_LOGIC;
LED_1 : out STD_LOGIC;
LED_2 : out STD_LOGIC;
LED_3 : out STD_LOGIC;

-- Switch control Reset inputs
   but1_in     : IN   STD_LOGIC; --pushbutton on fpga 1
  but2_in     : IN   STD_LOGIC    --pushbutton on fpga 2

);

end top;

-------------------------------------------------------------------
-- Architecture Start
-------------------------------------------------------------------
architecture Behavioral of top is


  signal led : STD_LOGIC_VECTOR(3 DOWNTO 0);
  signal clk1 : std_logic :='0';
  signal but1 : std_logic :='0';
  signal but2 : std_logic :='0';
  signal bothbut : STD_LOGIC;
  signal lastbothbut : std_logic:='0';
 
  -----------------------------------------------------------------
  --debounce of switch 1
  -----------------------------------------------------------------
SIGNAL flipflops   : STD_LOGIC_VECTOR(1 DOWNTO 0); --input flip flops
SIGNAL counter_set : STD_LOGIC;                    --sync reset to zero
SIGNAL counter_out : STD_LOGIC_VECTOR(counter_size DOWNTO 0) := (OTHERS => '0'); --counter output

  -----------------------------------------------------------------
  --debounce of switch 2
  -----------------------------------------------------------------
SIGNAL flipflops1   : STD_LOGIC_VECTOR(1 DOWNTO 0); --input flip flops
SIGNAL counter_set1 : STD_LOGIC;                    --sync reset to zero
SIGNAL counter_out1 : STD_LOGIC_VECTOR(counter_size1 DOWNTO 0) := (OTHERS => '0'); --counter output

-------------------------------------------------------------------
-- 200 MHz clock
-------------------------------------------------------------------
signal clk_50 : STD_LOGIC;

component clk_gen_50MHz is
port ( CLK_IN1           : in     std_logic;
  CLK_OUT1          : out    std_logic
);
end component;
-------------------------------------------------------------------
-- Behavioral Begin
-------------------------------------------------------------------
BEGIN

CLK_GEN: clk_gen_50MHz port map ( CLK_IN1 => CLK,
  CLK_OUT1 => clk_50);
  LED_0 <= led(0);
  LED_1 <= led(1);
  LED_2 <= led(2);
  LED_3 <= led(3);



-- debounce of switches
counter_set <= flipflops(0) xor flipflops(1);    --determine when to start/reset counter
counter_set1 <= flipflops1(0) xor flipflops1(1);   --determine when to start/reset counter

--debounce switch 1
process(clk_50)
begin
IF rising_edge(clk_50) THEN
      flipflops(0) <= but1_in;
      flipflops(1) <= flipflops(0);
      If(counter_set = '1') THEN                  --reset counter because input is changing
        counter_out <= (OTHERS => '0');
      ELSIF(counter_out(counter_size) = '0') THEN --stable input time is not yet met
        counter_out <= counter_out + 1;
      ELSE                                        --stable input time is met
        but1 <= flipflops(1);
      END IF;   
    END IF;
  END PROCESS;

--debounce switch 2

process(clk_50)
begin
IF rising_edge(clk_50) THEN
      flipflops1(0) <= but2_in;
      flipflops1(1) <= flipflops1(0);
      If(counter_set1 = '1') THEN                  --reset counter because input is changing
        counter_out1 <= (OTHERS => '0');
      ELSIF(counter_out1(counter_size1) = '0') THEN --stable input time is not yet met
        counter_out1 <= counter_out1 + 1;
      ELSE                                        --stable input time is met
        but2 <= flipflops1(1);
      END IF;   
    END IF;
  END PROCESS;

bothbut <= but1 AND but2;

process(clk_50)
begin
 IF rising_edge(clk_50) THEN
  lastbothbut<=bothbut;
  IF (lastbothbut='0') AND (bothbut='1') THEN
   led <= not led;
  END IF;
 END IF;
end process;
END Behavioral;

The debounce is stolen from the Internet and adapted.

I tested your code above and it works as expected - if I hold down both buttons at the same time the LEDs toggle off or on.

Hummm.... are you sure that your button inputs are "logic low, and logic high when pressed", not "logic high, and logic low when pressed"?

[lem_ix]

A gated clock is a clock signal you derived from the main clock, for example with an enable signal. You'd do this to maybe slow down the clk for one part of the design. This isn't good design practice because timing errors between clock domains can occur. Instead you would want to clock all your processes with the same clock but slow down the process by using some counter/enable.

Hope that helps.

[hamster_nz]

A gated clock is a clock signal you derived from the main clock, for example with an enable signal. You'd do this to maybe slow down the clk for one part of the design. This isn't good design practice because timing errors between clock domains can occur. Instead you would want to clock all your processes with the same clock but slow down the process by using some counter/enable.

Hope that helps.

To expand on this a little...

Your design is running at 50MHz, so each cycle takes 20ns

Code: [Select]

  gated_clock = clock AND some_signal;

  .... in a process ....
  if rising_edge(gated_clock) then
    ... do stuff ....
  end if;
If you 'gate the clock' like the code above, the clock signal has to come off the clocking network, be routed to a Look Up Table, the Look Up Table then implements the logic, and then routed from the LUT's output to the clock input of the flipflops. If each of the steps along the way (route, LUT, route, target flipflops) take 0.5ns, then the output of the target flipflops will take 2ns to react to the clock edge. Downstream logic will then only have 18ns to get ready for the next (ungated) clock edge,

Code: [Select]

  if rising_edge(gated_clock) then
    if some_signal = '1' then
        ... do stuff ....
    end if;
  end if;

If you use a the "clock enable" signal on the flipflop (like in the code just above) , the extra "route, LUT, route" steps are avoided, so the same output is available after just 0.5ns - just the time it takes the flip-flop to respond to the edge.

If this was path of the critical timing path in the design (the one that limits the maximum clock speed), and it only just makes timing with the gated clock running at 20ns per cycle, then the cleaner design could run with an 18.5 ns clock period - about 54 MHz - or nearly 10% faster.

The faster the design runs, the more painful the extra 1.5ns of delay will be  - for a slow design a gated clock might be fine. But why bother to use a bad design pattern?

[austnais]

Ahh, Murphy got me...

When debugging I changed the UCF file and forgot to change it back... So the second button was assigned to wrong input. Bloody idiot! Now it works, of course. 

Thanks for all help! Albeit, I learned a lot!

[dmills]

rising_edge or falling_edge implies a flipflop clocked by whatever the parameter is, and thus implies that parameter is a clock, in much the same way as an if statement implies a multiplexer.

Combinatoric logic feeding a clock is not in general a good thing, instead:

Code: [Select]

signal old_btn : std_logic := '0';

... In a clocked process.....
if rising_edge (clk_50) then
if btn and not old_btn then ........

end if;
old_btn <= btn;
end if;
Note that there is a single instance of rising_edge which is sensitive to the ungated 50Mhz clock, and an implied register old_btn that tracks the state of the button one clock cycle ago, you could even make the logic purely combinatorial with the single register providing a delay line.

Code: [Select]

if rising_edge_clk then
old_btn <= btn;
end if;

if btn and not old_btn then .....
but this has the disadvantage of being not registering the output.