# 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 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.
– user3624
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? 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.
– user3624
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. :-) Mar 10 '12 at 15:49
• @DavidKessner: have you an example of integers not using the carry chain effectively? To my understanding, it was a looong time ago when that was true, if ever! Mar 12 '12 at 11:19

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.

• -1 You don't need an async reset to make it synthesizeable-- in many cases you don't want an async reset (although it doesn't hurt much in a Coolrunner2 CPLD).
– user3624
Mar 10 '12 at 1:09
• @Per E, you seem to have been downvoted for your final advice, not your meat of the advice. No one stole your answer it seems, it seems they instead took the time to more explicitly explain the source of the issue. Sorry for any confusion this may have spawned. Mar 10 '12 at 8:39
• The test bench was generated automatically by ISE, so I supposed the code is without mistakes. I removed all clk_*k processes and now, simulation runs without problem. Mar 10 '12 at 14:54
• Sorry for the confusion. Let me elaborate. Recently my employer had a project slip by 6+ months due to bad VHDL coding. As a response, earlier this week I led VHDL training for a group of engineers. The first slide in my presentation was the title slide. The #2 slide talked about all the bad VHDL code/advice you find on the internet. Sadly, your comment about async-resets was like salt in a recent wound. I felt like others didn't give a detailed enough answer, that's why I answered it myself. I even gave Yann Vernier a +1 since he was the first to say "multiple drivers".
– user3624
Mar 10 '12 at 14:55
• @DavidKessner: Point taken. I guess it boils down to my digital design teacher's style preferences then. Though I cannot think of a reason not to initialize registers on reset I guess after some reading I will. Mar 10 '12 at 15:30

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.

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 Mar 9 '12 at 19:44