Xilinx synthesis warning regarding clock nets

[M. Norton]

Hello,

I'm somewhat new to Xilinx as I haven't used them for 3 or 4 years.
The company I was working for prior to this one was using Actel
anti-fuse type of FPGAs. Anyhow, something I'm doing is synthesizing
as a gated clock, when I would really rather prefer it to be a mux
followed by a flop. In the Actel devices using a Synopsys synthesizer,
the code below always instantiated as a mux followed a gate, rather
than as a latch or gated clock sort of thing. Explicitly stating the
cases for the D input (and one of which being the current output)
created a mux and the if/then/elsif/end created a D-flop with a reset.

Anyhow, I'm not getting that now with the Xilinx and I expect a
different code structure is preferred for this sort of thing. If
someone who is familiar with Xilinx code could take a look and see if
there's something really outrageous going on, I'd sure appreciate it.
In a nutshell, I've got a byte buffer that is a series of ASCII
characters and I'm decoding a command. I'd just like to turn something
on, and turn it off, and otherwise, leave it alone.

signal clk : std_logic;
signal data_ascii_q : std_logic_vector(47 downto 0);
signal new_cmd : std_logic;
signal signal_d : std_logic;
signal signal_q : std_logic;

....

process (clk, signal_q, data_ascii_q, new_cmd)
begin
if (new_cmd = '1' and
data_ascii(7 downto 0) = X"0D" and -- CR delimiter
data_ascii(16 downto 8) = X"57") then -- Char "W" is write

case data_ascii(47 downto 32) is -- Decode the address
part
when X"3031" => -- Address is "01"
case data_ascii(31 downto 16) is
when X"3031" => -- Data "01"
signal_d <= '1'; -- Turn it on
when X"3030" => -- Data "00"
signal_d <= '0'; -- Turn it off
when others =>
signal_d <= signal_q -- Retain state on bad
data
end case;
when others =>
null;
end case;
else
signal_d <= signal_q; -- if there's nothing going on, hold last
state
end if;

if (por_l = '0') then
signal_q <= '0';
elsif (clk'event and clk='1') then
signal_q <= signal_d;
end if;
end process;

The exact warning I'm getting is:
WARNINGesignRules:372 - Netcheck: Gated clock. Clock net UMAIN__n0093
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.

Thanks for any help or insight that's out there in the community. I
sure aprpeciate it.

Best regards,
Mark Norton

 

[Andy Peters]

signal clk : std_logic;
signal data_ascii_q : std_logic_vector(47 downto 0);
signal new_cmd : std_logic;
signal signal_d : std_logic;
signal signal_q : std_logic;

process (clk, signal_q, data_ascii_q, new_cmd)
begin
if (new_cmd = '1' and
data_ascii(7 downto 0) = X"0D" and -- CR delimiter
data_ascii(16 downto 8) = X"57") then -- Char "W" is write

case data_ascii(47 downto 32) is -- Decode the address
part
when X"3031" => -- Address is "01"
case data_ascii(31 downto 16) is
when X"3031" => -- Data "01"
signal_d <= '1'; -- Turn it on
when X"3030" => -- Data "00"
signal_d <= '0'; -- Turn it off
when others =>
signal_d <= signal_q -- Retain state on bad
data
end case;
when others =>
null;
end case;
else
signal_d <= signal_q; -- if there's nothing going on, hold last
state
end if;

if (por_l = '0') then
signal_q <= '0';
elsif (clk'event and clk='1') then
signal_q <= signal_d;
end if;
end process;

The exact warning I'm getting is:
WARNINGesignRules:372 - Netcheck: Gated clock. Clock net UMAIN__n0093
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.

Thanks for any help or insight that's out there in the community. I
sure aprpeciate it.

Your coding style has confused the synthesis tool. You need to
separate the synchronous logic (the stuff that assigns signal_q) from
the combinatorial logic.

I notice that your process is also sensitive to a signal called
data_ascii_q but that signal is used nowhere in the process. However,
a signal called data_ascii IS used in your process, which is cause for
more confusion.

I think what's happening is that the combination of data_ascii and
new_cmd and signal_q is generating the "clock" for the storage unit
that holds the signal signal_d.

Please refer to a book's or tool documentation's section on coding
style.

--a

 

[M. Norton]

Your coding style has confused the synthesis tool. You need to
separate the synchronous logic (the stuff that assigns signal_q) from
the combinatorial logic.

Well that's essentially what I'm attempting to do. You'll notice that
there are two main structures to the process. The first is entirely
combinatorial, and the second is clocked. The exact way of wording the
combinatorial bits such that I get a mux is the real issue.

I notice that your process is also sensitive to a signal called
data_ascii_q but that signal is used nowhere in the process. However,
a signal called data_ascii IS used in your process, which is cause for
more confusion.

My apologies for the confusion on that one. "data_ascii" should be
"data_ascii_q" in that case. This is not a cut and pasted code
snippet, but rather an interpreted snippet with signal names changed to
protect the innocent. So ultimately I had to retype it into the
newsgroup editor and don't have the luxury of a syntax checker built-in
to spot unnamed signals.

I think what's happening is that the combination of data_ascii and
new_cmd and signal_q is generating the "clock" for the storage unit
that holds the signal signal_d.

Please refer to a book's or tool documentation's section on coding
style.

Well I think you're right and I've tried a few other styles. So far,
all still create a latch when I try to retain state. I've tried
breaking it down into individual if statements, putting the check for
the command as a clock enable in the sequential if statement, and other
varieties. The only thing I haven't yet done is create an explicit
clock enable signal and that's next on the list. To wit (in admittedly
hurried pseudo-code):

....
if there's a new command strobe and if the address is right and if
the command says to write register then
if the data for the register is A then
signal_d <= '0';
clock_en <= '1';
else if the data for the register is B then
signal_d <= '1';
clock_en <= '1';
else
clock_en <= '0';
end if
end if

if clock event and clock rising edge then
if clock_en = '1' then
signal_q <= signal_d;
end if;
end if;
....

The only thing I don't like about that is the fact that the enable
signal won't really properly be updated until the process is finished.
I know this will screw up the simulation, and it very likely will screw
up the synthesis as well. I'm loathe to create holding registers for
every single signal_d (this isn't a nice memory map, this is about 2-3
dozen bi-level signals, each of which would need to be decoded
separately for various behaviors). I'll do it if I have to, but I'd
like to find a nice lexical way of expressing this while retaining the
desired synthesized behavior (and not take forever to simulate as
well).

Thanks for the thoughts. I think I'm headed down the right track, just
looking to see if there are Xilinx old hands who might have hit this
before and their experience in structuring it.

Best regards,
Mark Norton

 

[M. Norton]

I think I've found the answer and I thought I would post it back to the
thread, if for no other reason than maybe it'll help someone else who
searches google for that particular warning. (I did originally before
posting and did not find this answer.)

The explicit D and Q style was one I adopted from my previous employer.
As I said it worked well for the Actel devices, and as they did a lot
of ASIC work and used the same style, it worked well for the ASICs.
However, for Xilinx, it seems like it is overly complex. In an effort
to *demand* explicit rendering of a combinatorially sourced D input and
a sequential Q output, the Xilinx synthesizer misses out on some of the
shortcuts within it's own architecture.

The following code structure seemed to work well and did not generate
any latches, and does the same thing. Apparantly from other bits of
papers I've read today, this coding style lets the Xilinx synthesizer
use the internal Q to D loopback and mux to create clock enables. I'm
going to have to restructure a lot of my other code as well, as I
expect I'm using a lot more resources than I should to do the same
thing. I expect it's creating a lot of individual combinatorial slices
feeding flops and may be missing out on further optimization possible.

This is pseudocode, but the style is the important part, not the
syntax. Note the absence of explicit D signals and not having to
explicitly define the "do not change state" cases. That seems to be
inherent in the Xilinx cell structure and the synthesizer knows how to
handle "everything else".

if (por_l = '0') then
-- Assign signal resets here
elsif (clk'event and clk='1') then
if (new_data_strobe and the command is a write command) then
case address is
when address_1 =>
if (data = X"3031") then -- "01" in ascii
signal_q <= '1';
elsif (data = X"3030") then -- "00" in ascii
signal_q <= '0';
end if;
when others =>
null;
end case;
end if;
end if;

I hope this helps someone out.

Best regards,
Mark Norton

 

[Duane Clark]

I think I've found the answer and I thought I would post it back to the
thread, if for no other reason than maybe it'll help someone else who
searches google for that particular warning. (I did originally before
posting and did not find this answer.)

The explicit D and Q style was one I adopted from my previous employer.
As I said it worked well for the Actel devices, and as they did a lot
of ASIC work and used the same style, it worked well for the ASICs.
However, for Xilinx, it seems like it is overly complex. In an effort
to *demand* explicit rendering of a combinatorially sourced D input and
a sequential Q output, the Xilinx synthesizer misses out on some of the
shortcuts within it's own architecture.

I think you missed the source of the problem. Let's take a look at your
previous (pseudo)code:

if there's a new command strobe and if the address is right and if
the command says to write register then
if the data for the register is A then
signal_d <= '0';
clock_en <= '1';
else if the data for the register is B then
signal_d <= '1';
clock_en <= '1';
else
clock_en <= '0';
end if
end if

Notice that in the else statement, you don't assign anything to
signal_d. So therefore, the previous value cannot change, regardless of
what any other signals are doing. Therefore the data must be latched.
You even explicitely did this in the original code:
else
signal_d <= signal_q; -- if there's nothing going on, hold last

This code is not illegal, but it is bad design practice. I am pretty
sure that Actel in fact created a latch around the mux, which is easily
done. Actel (at least the Act1 and Act2 families that I am familiar
with) always uses the mux to create a latch, since the S module is edge
triggered. So your code simulated and worked correctly, but I am
guessing you never realized you were creating a bunch of latches. If the
Actel tools did not create a latch, then they are broken

And as for the old coding style, definitely get rid of it. Your new
style is vastly better. About the only thing I would suggest is instead of
elsif (clk'event and clk='1') then
use
elsif rising_edge(clk) then
But then again, that is just my preference; I think it is less cluttered.

 

[Andy Peters]

The explicit D and Q style was one I adopted from my previous employer.

One wonders if they're still in business ...

As I said it worked well for the Actel devices, and as they did a lot
of ASIC work and used the same style, it worked well for the ASICs.
However, for Xilinx, it seems like it is overly complex. In an effort
to *demand* explicit rendering of a combinatorially sourced D input and
a sequential Q output, the Xilinx synthesizer misses out on some of the
shortcuts within it's own architecture.

I wonder if that's true. My experience has been that trying to be
clever simply confuses the tools and your results are worse than what
you'd get had you simply wrote some reasonable code.

The following code structure seemed to work well and did not generate
any latches, and does the same thing. Apparantly from other bits of
papers I've read today, this coding style lets the Xilinx synthesizer
use the internal Q to D loopback and mux to create clock enables. I'm
going to have to restructure a lot of my other code as well, as I
expect I'm using a lot more resources than I should to do the same
thing. I expect it's creating a lot of individual combinatorial slices
feeding flops and may be missing out on further optimization possible.

This is pseudocode, but the style is the important part, not the
syntax. Note the absence of explicit D signals and not having to
explicitly define the "do not change state" cases. That seems to be
inherent in the Xilinx cell structure and the synthesizer knows how to
handle "everything else".

if (por_l = '0') then
-- Assign signal resets here
elsif (clk'event and clk='1') then
if (new_data_strobe and the command is a write command) then
case address is
when address_1 =>
if (data = X"3031") then -- "01" in ascii
signal_q <= '1';
elsif (data = X"3030") then -- "00" in ascii
signal_q <= '0';
end if;
when others =>
null;
end case;
end if;
end if;

One further comment. The ONLY signals that should be in this process's
sensitivity list are the clock (clk) and async reset (por_l).

-a