Wrong outputs in VHDL entity

I have lessons about VHDL in one of my university class and I have to write simple entity which will generate clock from 1MHz source. I'm using CoolRunner-II CPLD Starter Kit with ISE Webpack 13.1.

When I run simulation of my code, I've got odd results. I have no idea, where the problem is. My VHDL entity looks like this:

entity clock is
Port ( clk_in : in  STD_LOGIC;
clk_1M : out  STD_LOGIC;
clk_500k : out  STD_LOGIC;
clk_100k : out  STD_LOGIC;
clk_1k : out  STD_LOGIC;
clk_1hz: out STD_LOGIC);
end clock;


Input is 1MHz signal from oscilator and I want to create 1MHz, 500kHz, 100kHz, 1kHz and 1Hz output signal. I defined several signals:

signal c100k: std_logic_vector(3 downto 0) := (others => '0' );
signal c1k:  std_logic_vector(9 downto 0) := (others => '0' );
signal c1hz: std_logic_vector(9 downto 0) := (others => '0' );
--
signal c500k_out: std_logic := '0';
signal c100k_out: std_logic := '0';
signal c1k_out: std_logic := '0';
signal c1hz_out: std_logic := '0';


And finally, my code is:

process (clk_in) begin
if clk_in'event and clk_in = '1' then
-- 500kHz
c500k_out <= not c500k_out;
end if;
end process;

process (clk_in) begin
if clk_in'event and clk_in = '1' then
-- 100kHz
c100k <= c100k + '1';
if c100k = X"A" then
c100k <= (others => '0' );
c100k_out <= '1';
else
c100k_out <= '0';
end if;
end if;
end process;

--
-- Code for 1kHz and 1Hz is same as 100kHz
--

clk_1M <= clk_in;           -- Clock source 1Mhz
clk_500k <= c500k_out;  -- Clock source 500kHz
clk_100k <= c100k_out;  -- Clock source 100kHz
clk_1k <= c1k_out;      -- Clock source 1kHz
clk_1hz <= c1hz_out;        -- Clock source 1Hz


When I run simulation, I got this odd results:

What is wrong with my code?

-
 I added working code here -- pub.uart.cz/vhdl – vasco Mar 10 '12 at 16:08

The first thing I see, which isn't likely causing your problem, is a lack of any kind of reset. ModelSim does not automatically declare all signals to be '0', and trying to say s <= not s when s is not 1 or 0 is not going to give you what you want. I see you declaring them all '0' at the top but a proper circuit would have a reset input that you briefly drive at the start of your simulation.

Your basic counter process looks wrong; basically when the count reaches ten you're outputting a '1' and for the other 9/10ths of the time you're outputting a '0' (for the 100k example, the 1Hz example would be a 1us pulse high and a 999us low time. I think what you want is something like this:

gen_clk100: process(clk, rst)
begin
if rising_edge(clk) then
if count = 10 then
clk100 <= not clk100;
count <= 0;
else
count <= count + 1;
end if;
end if;

if rst = '1' then
clk100 <= '0';
count <= 0;
end if;
end process;


Note several things:

• I'm not incrementing all the time. I increment if the count is not at its max, and increment otherwise.

• I'm toggling the output clock when the maximum count is reached

• I've included an asynchronous reset (with synchronous clear) -- this ensures that your signals initialize correctly and have no metastability problems when reset is released.

You also mention that the other sections are the same. I think you're having a problem with multiple drivers like Yann mentioned. Check and double-check your code to make sure you are not assigning the output in two different processes, because that is exactly what the simulation is saying you're doing. Better to either make a generic counter module or...

Why so many counters? All your frequency dividers can be handled with a single count and then "peeling off" the conditions to do the divisions you're interested in. You could also have a single counter and use the mod (modulus) operator to handle each divider case.

Finally, I'd also use real or integer types for the count instead of std_logic_vectors but that's just me.

-
 Lack of a reset isn't going to cause issues in ModelSim or any simulator (he's using ISIM, I think). All signals are given an initial value when the signal is declared so that is enough for the simulator. Your example counter code would result in bad logic due to multiple drivers (the "if rising" and "if reset" blocks are considered to be separate processes). The correct way would be to do "if reset...else if rising_edge...". I advise not using real or integer types for signals that will be synthesized into actual logic (not just simulated). But that's just me. – David Kessner Mar 10 '12 at 1:31 I agree, and that's why I specifically said the resets weren't causing the issue. The process example I gave is a textbook example that is used in dozens of designs; there aren't any issues with it, at least not with Quartus or ISE. Real/Integer types also synthesize correctly in my experience. Have you run into issues? – Andrew Kohlsmith Mar 10 '12 at 4:37 Real doesn't synthesize. You can use a real, but it must resolve to an integer or bit vector type at synthesis time. You cannot, for example, have a counter of type "real" that counts by 0.184739 every clock cycle. While integer does synthesize, using SLV instead gives you much more control. Often times I can optimize logic to be much smaller with much higher performance using SLV than integer (in Xilinx FPGA's). With integers you don't have control or access to the internal logic representation in the FPGA, while SLV lets you do tricks like utilizing the carry chain efficiently. – David Kessner Mar 10 '12 at 4:51 You're right; the only time I've used real is as an integer, so the egg is on my face for suggesting it without a proper disclaimer. :-/ I have however used ranged integer types for my counters almost exclusively (signal/variable foo: integer range 0 to 255;) -- I don't think it is any less controlled than slv in that regard, and it makes your code a lot cleaner to boot (none of this unsigned(some_slv) or x"0400" nonsense.). I know that ISE's methodology vs Quartus' is quite different. It's one of the reasons I prefer Altera over Xilinx, but that's another holy war altogether. :-) – Andrew Kohlsmith Mar 10 '12 at 15:49 Thank you. Your answer helped me a lot. Now, all clock sources are working well and simulation gives right results. Right now, I'm trying to rewrite my code into one process with one counter. – vasco Mar 10 '12 at 15:59

I don't see it in the code snippets, but the simulation looks a whole lot like you have multiple drivers for some signals. When they agree, you get a logic level, but whenever they disagree you get X - a conflict in simulation, which is probably not synthesizable (if it were, it would mean a chip-frying short). One guess is that you may have made a mistake somewhere when copying that clock divider process, and would be well served by a component you could instantiate instead.

-
 Thanks, but I do not see any part, where I have "multiple drivers". ISE is reporting this type of error, so I suppose, there are no one. I've put whole code and test here: pub.uart.cz/vhdl – vasco Mar 9 '12 at 19:44

You are assigning signals clk_1M, clk_500k, clk_100k, clk_1k and clk_1hz in different processes in the test bench. At the same time you have instantiated your DUT which is (as Yann Vernier suggests) driving the same signals. Uncomment the test bench processes (except for clk_in!) and you will be fine.

Furthermore I would advice you to add an asynchronous reset signal to the clock entity to make it synthesizable.

-

You have multiple drivers. They are not shown in your posted code snippets, but are obvious in the non-snippetized files.

Looking at the file test_clock.vhd you instantiate your unit under test. In the port map you assign an output of clock() to clk_1M. Later, you have this chunk of code:

clk_1M_process :process
begin
clk_1M <= '0';
wait for clk_1M_period/2;
clk_1M <= '1';
wait for clk_1M_period/2;
end process;


This code also assigns a value to clk_1M-- therefore you have multiple drivers on your signals.

Other signals have similar issues so I won't go over them here.

-

Right..there's one i think might be causing these issues.

The simulator produces those red lines because when the 'clk_in' is low it has no idea what to do.Remember that you're describing hardware, not programming. Your descriptions must take all possible scenarios into account and have a plan for them. Now back to your problem..for all the 'if' statements that you've got in your program try adding a corresponding 'else' statement because what you're doing is executing statements when 'clk_in' is high, forgetting that it is not always high.

You might want the internal signals to retain their current values when the clock is low(using an 'else') but you need to clearly specify this. This is what's known as latching, or unintentionally creating latches. Hope it helps?

-
 I don't think your comment about the simulator "not knowing what to do" on the low side of clk_in is correct; he's describing flip flops and the code should be synthesizing a normal positive-edge-triggered FF. The outputs should hold state when clock is static or falling edge. – Andrew Kohlsmith Mar 9 '12 at 21:41 I don't think you understand the meaning of the "clk_in'event" in the if statements. This causes your answer to not make much sense-- especially the part about "for all the 'if' statements you need an 'else'". – David Kessner Mar 10 '12 at 1:12