VHDL style and possible problems for first time user

[jacko]

hi

my first vhdl project is underway. here it is so far, does anyone have
any comments on style or problems i am not seeing?

-- indi16 vhdl first version --
-- indi16.4.0
-- (C)2007 K Ring Technologies Semiconductor

LIBRARY ieee;
USE ieee.std_logic_1164.all;

ENTITY indi16 IS
PORT
(
Clk, Reset, Halt, Cin, ACin : IN STD_LOGIC;
DataIn : IN STD_LOGIC_VECTOR(15 DOWNTO 0);
DataOut : OUT STD_LOGIC_VECTOR(15 DOWNTO 0);
Fetch, BusFree, OE, RW, CS0, CS1: OUT STD_LOGIC;
Cout, ACout, Hilo, RW16 : OUT STD_LOGIC;
Video : OUT STD_LOGIC_VECTOR(7 DOWNTO 0);
Address : OUT STD_LOGIC_VECTOR(15 DOWNTO 0)
);
END indi16;

ARCHITECTURE a OF indi16 IS
-- the sequence clock (256 pixels)
SIGNAL Seq : STD_LOGIC_VECTOR(10 DOWNTO 0);
-- the register set
SIGNAL P, Q, R, S, A: STD_LOGIC_VECTOR(15 DOWNTO 0);
-- other hidden registers
SIGNAL IR, ALUin : STD_LOGIC_VECTOR(15 DOWNTO 0);
-- flags
SIGNAL C : STD_LOGIC;
BEGIN
Main:
PROCESS (Clk, Reset)
-- ALU process
VARIABLE F0, F1, F2, F3 : STD_LOGIC_VECTOR(15 DOWNTO 0);
VARIABLE FA, FC, ALUout : STD_LOGIC_VECTOR(16 DOWNTO 0);
VARIABLE C0, ID, OD, PNul : STD_LOGIC;
VARIABLE LMB : STD_LOGIC;
VARIABLE Ins : STD_LOGIC_VECTOR(7 DOWNTO 0);
VARIABLE Op, Rin, Rout : STD_LOGIC_VECTOR(1 DOWNTO 0);
BEGIN
IF Reset = '1' THEN
-- reset happening
P = 0;
Q = 0;
R = 0;
S = 0;
A = 0;
ELSE IF Clk'EVENT AND Clk = '1' AND Halt = '0' THEN
-- instruction loop
IF LMB = '1' THEN
Ins = IR[7..0];
ELSE
Ins = IR[15..8];
END IF;
Op = Ins[7..6];
ID = Ins[5];
Rin = Ins[4..3];
OD = Ins[2];
Rout = Ins[1..0];
PNul = (Ins[2..0] == '000');
-- register decode

-- ALU operation
F1 = A AND ALUin;
F2 = A XOR ALUin;
F3 = ALUin;
-- full adder
F0 = F2 XOR (FC, C);
FA = F2 AND (FC, C)
C0, FC = F1 OR FA;
-- mux alu out
CASE Op IS
WHEN '00' =>
ALUout = F0;
WHEN '01' =>
ALUout = F1;
WHEN '10' =>
ALUout = F2;
WHEN '11' =>
ALUout = F3;
END CASE;
-- memory access
CASE Seq[3..1] IS
WHEN '000' =>
-- 0 Instruction Fetch
Address <= P;
Hilo <= '1';
RW <= '1';
OE <= '0';
Data <= 'ZZZZZZZZ';
WHEN '001' =>
-- 1
IR[15..8] <= Data;
Hilo <= '0';

WHEN '010' =>
-- 2
IR[7..0] <= Data;

WHEN '011' =>
-- 3
__statement;
__statement;
WHEN '100' =>
-- 4
__statement;
__statement;
WHEN '101' =>
-- 5
__statement;
__statement;
WHEN '110' =>
-- 6
__statement;
__statement;
WHEN '111' =>
-- 7
__statement;
__statement;
END CASE;
Hilo <= Seq[0];
-- 8 bit bus

-- clock next cycle
Seq <= Seq+1;
END IF;
END PROCESS Main;
END a;

cheers

 

[Colin Paul Gloster]

In timestamped Thu, 19 Jul 2007 13:21:17 -0000,
jacko <[email protected]> posted:
|--------------------------------------------------------------------------------------------------------------|
|"hi |
| |
|my first vhdl project is underway. here it is so far, does anyone have |
|any comments on style or problems i am not seeing? |
| |
|-- indi16 vhdl first version -- |
|-- indi16.4.0 |
|-- (C)2007 K Ring Technologies Semiconductor |
| |
|LIBRARY ieee; |
|USE ieee.std_logic_1164.all; |
| |
|ENTITY indi16 IS |
| PORT |
| ( |
| Clk, Reset, Halt, Cin, ACin : IN STD_LOGIC; |
| DataIn : IN STD_LOGIC_VECTOR(15 DOWNTO 0);|
| DataOut : OUT STD_LOGIC_VECTOR(15 DOWNTO 0);|
| Fetch, BusFree, OE, RW, CS0, CS1: OUT STD_LOGIC; |
| Cout, ACout, Hilo, RW16 : OUT STD_LOGIC; |
| Video : OUT STD_LOGIC_VECTOR(7 DOWNTO 0); |
| Address : OUT STD_LOGIC_VECTOR(15 DOWNTO 0) |
| ); |
|END indi16; |
| |
|ARCHITECTURE a OF indi16 IS |
| -- the sequence clock (256 pixels) |
| SIGNAL Seq : STD_LOGIC_VECTOR(10 DOWNTO 0); |
| -- the register set |
| SIGNAL P, Q, R, S, A: STD_LOGIC_VECTOR(15 DOWNTO 0); |
| -- other hidden registers |
| SIGNAL IR, ALUin : STD_LOGIC_VECTOR(15 DOWNTO 0); |
| -- flags |
| SIGNAL C : STD_LOGIC; |
|BEGIN |
| Main: |
| PROCESS (Clk, Reset) |
| -- ALU process |
| VARIABLE F0, F1, F2, F3 : STD_LOGIC_VECTOR(15 DOWNTO 0); |
| VARIABLE FA, FC, ALUout : STD_LOGIC_VECTOR(16 DOWNTO 0); |
| VARIABLE C0, ID, OD, PNul : STD_LOGIC; |
| VARIABLE LMB : STD_LOGIC; |
| VARIABLE Ins : STD_LOGIC_VECTOR(7 DOWNTO 0); |
| VARIABLE Op, Rin, Rout : STD_LOGIC_VECTOR(1 DOWNTO 0); |
| BEGIN |
| IF Reset = '1' THEN |
| -- reset happening |
| P = 0; |
|" |
|--------------------------------------------------------------------------------------------------------------|

It seems that you did not give this code to an analyzer ... P <= 0;
would be a correct assignment to a signal. P = 0; is
incorrect. (Similarly for Q; R; S; and A.)

|--------------------------------------------------------------------------------------------------------------|
|" |
| Q = 0; |
| R = 0; |
| S = 0; |
| A = 0; |
| ELSE IF Clk'EVENT AND Clk = '1' AND Halt = '0' THEN |
|" |
|--------------------------------------------------------------------------------------------------------------|

Halt should be in your sensitivity list. You should type
rising_edge(Clk) instead of Clk'EVENT AND Clk = '1'.

|--------------------------------------------------------------------------------------------------------------|
|" |
| -- instruction loop |
| IF LMB = '1' THEN |
| Ins = IR[7..0]; |
| ELSE |
| Ins = IR[15..8]; |
| END IF; |
| Op = Ins[7..6]; |
|" |
|--------------------------------------------------------------------------------------------------------------|

Another illegal attempt at an assignment. Op is a variable as opposed
to a signal, so it needs a different assignment operator
.... i.e. :=. Not that = is ever a legal assignment operator in VHDL
.... = is just for testing equality (so = in VHDL is unlike = in C and
is like == in C).

|--------------------------------------------------------------------------------------------------------------|
|" |
| ID = Ins[5]; |
| Rin = Ins[4..3]; |
| OD = Ins[2]; |
| Rout = Ins[1..0]; |
| PNul = (Ins[2..0] == '000'); |
|" |
|--------------------------------------------------------------------------------------------------------------|

More illegally placed equals signs.

|--------------------------------------------------------------------------------------------------------------|
|" |
| -- register decode |
| |
| -- ALU operation |
| F1 = A AND ALUin; |
| F2 = A XOR ALUin; |
| F3 = ALUin; |
| -- full adder |
| F0 = F2 XOR (FC, C); |
| FA = F2 AND (FC, C) |
| C0, FC = F1 OR FA; |
| -- mux alu out |
| CASE Op IS |
| WHEN '00' => |
| ALUout = F0; |
| WHEN '01' => |
| ALUout = F1; |
| WHEN '10' => |
| ALUout = F2; |
| WHEN '11' => |
| ALUout = F3; |
| END CASE; |
| -- memory access |
| CASE Seq[3..1] IS |
| WHEN '000' => |
| -- 0 Instruction Fetch |
| Address <= P; |
| Hilo <= '1'; |
| RW <= '1'; |
| OE <= '0'; |
| Data <= 'ZZZZZZZZ'; |
| WHEN '001' => |
| -- 1 |
| IR[15..8] <= Data; |
| Hilo <= '0'; |
| |
| WHEN '010' => |
| -- 2 |
| IR[7..0] <= Data; |
| |
| WHEN '011' => |
|" |
|--------------------------------------------------------------------------------------------------------------|

It is unclear to me whether each appearance of __statement; below is
intended to be literally in the VHDL file or whether this is supposed
to be some kind of pseudocode, with each of the appearances of
__statement; in the pseudocode to be replaced by something unique in
the genuine VHDL code. If they are not all supposed to be unique, then
you could simply have
when others =>
__statement;
__statement;
here instead of five unmaintainably identical branch bodies.

Regards and welcome,
Colin Paul Gloster

|--------------------------------------------------------------------------------------------------------------|
|" |
| -- 3 |
| __statement; |
| __statement; |
| WHEN '100' => |
| -- 4 |
| __statement; |
| __statement; |
| WHEN '101' => |
| -- 5 |
| __statement; |
| __statement; |
| WHEN '110' => |
| -- 6 |
| __statement; |
| __statement; |
| WHEN '111' => |
| -- 7 |
| __statement; |
| __statement; |
| END CASE; |
| Hilo <= Seq[0]; |
| -- 8 bit bus |
| |
| -- clock next cycle |
| Seq <= Seq+1; |
| END IF; |
| END PROCESS Main; |
|END a; |
| |
|cheers" |
|--------------------------------------------------------------------------------------------------------------|

 

[Szymon Janc]

Colin Paul Gloster napisa³:

You should type
rising_edge(Clk) instead of Clk'EVENT AND Clk = '1'.

According to XST user guide it is equivalent and in examples they use
clk'event and clk='1' so it shouldn't really matter.

 

[Mike Treseler]

my first vhdl project is underway. here it is so far, does anyone have
any comments on style or problems i am not seeing?

Consider compiling your code and fixing syntax
error before posting.

vcom -2002 -quiet -work work indi16.vhd
** Error: indi16.vhd(39): near "=": expecting: <= :=
** Error: indi16.vhd(40): near ";": expecting: GENERATE THEN
** Error: indi16.vhd(41): near ";": expecting: GENERATE THEN
** Error: indi16.vhd(42): near ";": expecting: GENERATE THEN
** Error: indi16.vhd(43): near ";": expecting: GENERATE THEN
** Error: indi16.vhd(47): near "=": expecting: <= :=
** Error: indi16.vhd(47): near "7..": (vcom-112) Number in abstract
literal must terminate with a digit.

etc. etc.

-- Mike Treseler

 

[Mike Treseler]

Colin Paul Gloster napisa³:
According to XST user guide it is equivalent and in examples they use
clk'event and clk='1' so it shouldn't really matter.

You asked for comments on style,
and that is a good one.

-- Mike Treseler

 

[Thomas Stanka]

hi

my first vhdl project is underway. here it is so far, does anyone have
any comments on style or problems i am not seeing?

IF Reset = '1' THEN
-- reset happening
P = 0;
Q = 0;
R = 0;
S = 0;
A = 0;

Is there a reason to set only a few registers?
And which version of VHDL accepts '=' instead of ':=' or '<='

ELSE IF Clk'EVENT AND Clk = '1' AND Halt = '0' THEN

This means you want Halt as an Enable for your FF. Traditional VHDL
expects this statement to be written as

ELSIF Rising_Edge(Clk) THEN
IF Halt='0' THEN
....

Some synthesis tools accepts your style, some not. Conservative
designer use old style for interoperability, but this is not
necessarily the best solution.

Ins = IR[7..0]; ....
PNul = (Ins[2..0] == '000');

This is maybe verilog, but not VHDL. Please use a compiler to ensure
your code is VHDL, you use some statements from verilog that won't
pass your compiler

bye Thomas

 

[Andy]

Halt should be in your sensitivity list. You should type
rising_edge(Clk) instead of Clk'EVENT AND Clk = '1'.

Actually, Halt need not be in the sensitivity list. This is an
alternate form of specifying a clock enable that is accepted by some
synthesizers. It is fine as long as there is no "halt'event". I agree,
it should be rising_edge(clk) and halt = '0'. And it should be "elsif"
not "else if".

You should really run this through a compiler to get past all the the
syntax issues. In addition to what Colin wrote, bit string literals
are enclosed in double quotes ("), not single quotes. Individual bit
literals are enclosed in singe quotes ('). In other words, "1" would
be a std_logic_vector of length one, whereas '1' is a std_logic value.
'000' is nothing, AFAIK.

Array indices are enclosed in parenthesis, not square brackets.

Index range specifications are not "..", but "to" or "downto".

Not exactly sure what you're trying to do here, but:

-- full adder
F0 = F2 XOR (FC, C);
FA = F2 AND (FC, C)
C0, FC = F1 OR FA;

will not work. Using the correct data types and operators (integer or
numeric_std.unsigned, etc.) will get you better results, since the
synthesis tool will recognize you want an adder, and give you an
optimized one. Anyway, concatenation uses the "&" operator, not
parentheses and commas.

In summary, you really need a vhdl text to reference. There are just
too many errors to deal with here.

Andy

 

[Colin Paul Gloster]

In timestamped Thu, 19 Jul 2007 14:20:20 -0700,
Andy <[email protected]> posted:
|----------------------------------------------------------------------|
|"On Jul 19, 10:40 am, Colin Paul Gloster <[email protected]> |
|wrote: |
| |
|> Halt should be in your sensitivity list. You should type |
|> rising_edge(Clk) instead of Clk'EVENT AND Clk = '1'. |
| |
| |
|Actually, Halt need not be in the sensitivity list. This is an |
|alternate form of specifying a clock enable that is accepted by some |
|synthesizers. It is fine as long as there is no "halt'event". [..]" |
|----------------------------------------------------------------------|

I concede that I made a mistake. Sorry.

|----------------------------------------------------------------------|
|" And it should be "elsif" |
|not "else if". |
| |
|You should really run this through a compiler to get past all the the |
|syntax issues. In addition to what Colin wrote, bit string literals |
|are enclosed in double quotes ("), not single quotes. [etc. .. ]" |
|----------------------------------------------------------------------|

I had not noticed those.

|----------------------------------------------------------------------|
|"[..] |
| |
|In summary, you really need a vhdl text to reference. There are just |
|too many errors to deal with here. |
| |
|Andy" |
|----------------------------------------------------------------------|

Though Andy's and others' similar remarks are valid, I hope Jacko
shall not feel disheartened as the code is not actually far off from
being legal VHDL (e.g. I often mix up .. with TO, or when trying Java
I can mix up where to place [ and ] for declaring an array with C's way,
but a quick error message from a compiler readily reminds me of the
syntax I should be using for a particular language).

Regards,
C. P. G.

 

[Colin Paul Gloster]

In timestamped Thu, 19 Jul 2007
20:12:05 +0200, Szymon Janc <szymon@nie_spamuj_mnie.janc.int.pl>
posted:
|---------------------------------------------------------------------|
|"Colin Paul Gloster napisa³: |
| |
| |
|> You should type |
|> rising_edge(Clk) instead of Clk'EVENT AND Clk = '1'. |
| |
|According to XST user guide it is equivalent" |
|---------------------------------------------------------------------|

I understand your reasoning but it is misguided (as are some of the
people who write documentation for Xilinx, e.g. check for complaints
from someone of Xilinx (Ben Jones) who campaigned for improvement in
"VHDL Library Madness" on timestamped Thu, 26 Apr
2007 17:50:41 +0100 with Message-ID: <[email protected]>
which is currently archived at
HTTP://groups.Google.com/group/comp...3f31445fd71?lnk=st&q=&rnum=2#797953f31445fd71
). XST is not a VHDL simulator. VHDL simulators do not treat
rising_edge(Clk) as being equivalent to Clk'EVENT AND Clk = '1' as
they are not equivalent in true VHDL. XST and other VHDL subset
synthesis tools are rather like Fortran compilers in that they do
not always reject unsynthesizable code as being unsynthesizable,
instead they synthesize it as if it meant something else. By changing
the meaning, end users suffer discrepancies between the actually
synthesized netlist and the simulations.

|---------------------------------------------------------------------|
|" and in examples they use |
|clk'event and clk='1' so it shouldn't really matter. |
| |
|-- |
|Szymon K. Janc |
|szymon#janc.int.pl // GG: 1383435" |
|---------------------------------------------------------------------|

It should not matter, but it does matter.

Regards,
C. P. G.

 

[Szymon Janc]

Colin Paul Gloster napisa³:

I understand your reasoning but it is misguided (as are some of the
people who write documentation for Xilinx, e.g. check for complaints
from someone of Xilinx (Ben Jones) who campaigned for improvement in
"VHDL Library Madness" on timestamped Thu, 26 Apr
2007 17:50:41 +0100 with Message-ID: <[email protected]>
which is currently archived at
HTTP://groups.Google.com/group/comp.lang.vhdl/browse_frm/thread/10f
a3a9f5ba9a41/797953f31445fd71?lnk=st&q=&rnum=2#797953f31445fd71
). XST is not a VHDL simulator. VHDL simulators do not treat
rising_edge(Clk) as being equivalent to Clk'EVENT AND Clk = '1' as
they are not equivalent in true VHDL. XST and other VHDL subset
synthesis tools are rather like Fortran compilers in that they do
not always reject unsynthesizable code as being unsynthesizable,
instead they synthesize it as if it meant something else. By changing
the meaning, end users suffer discrepancies between the actually
synthesized netlist and the simulations.

XST says they are equivalent ... if not, what is the difference? That
constructions should be avoided ?
It is getting a little confusing to me.

It should not matter, but it does matter.

How?

So what sould be a reference for writing code for Xilinx fpgas if not
their official user guide ?

 

[Evan Lavelle]

XST says they are equivalent ... if not, what is the difference? That
constructions should be avoided ?
It is getting a little confusing to me.

How?

So what sould be a reference for writing code for Xilinx fpgas if not
their official user guide ?

I wouldn't worry about it. "rising_edge(clk)" is more concise than
"clk'event and clk='1'", and it expresses your intent more clearly, so
you may consider it better style to use it.

Does it really matter? 'rising_edge' is defined in std_logic_1164 as

FUNCTION rising_edge (SIGNAL s : std_ulogic) RETURN BOOLEAN IS
BEGIN
RETURN (s'EVENT AND (To_X01(s) = '1') AND
(To_X01(s'LAST_VALUE) = '0'));
END;

so, if 'clk' actually is a std_logic, 'rising_edge' only catches
transitions from L->H, 0->H, L->1, and 0->1. Your alternative catches
various other irrelevant transitions, such as X->1, while missing
'good' transitions such as 0->H.

The reason that this doesn't matter too much is that synthesisers
aren't interested in meta-values; they're only interested in 0 and 1
(this is a bit of a simplification - consider tristate buses,
don't-care optimisation, and so on). XST knows exactly what the intent
of both alternatives is and will generate exactly the same logic in
both cases. you may potentially have a problem if your code actually
*does* use meta-values on your clock line, in which case you may get
different pre- and post-synthesis simulation results. However, if this
is the case, you've got a fundamental problem that you need to fix
anyway - you shouldn't be using meta-values such as L and H in code
that you intend to synthesise. You can fix this using strength
strippers at the ports of your device.

Evan

 

[Andy]

Rising_edge() takes less typing, is more functionally clear to the
reader, and is more accurate in simulation. Hmmm, what else is there?

Andy

 

[jacko]

Actually, Halt need not be in the sensitivity list. This is an
alternate form of specifying a clock enable that is accepted by some
synthesizers. It is fine as long as there is no "halt'event". I agree,
it should be rising_edge(clk) and halt = '0'. And it should be "elsif"
not "else if".

halt will be synchronous in the whole design. so do i need to have a
latch, and would it be removed if not needed automatically?

ok on the elsif i will do.

fixed all the syntax.

will go rising edge.

Not exactly sure what you're trying to do here, but:

-- full adder
F0 = F2 XOR (FC, C);
FA = F2 AND (FC, C)
C0, FC = F1 OR FA;

will not work. Using the correct data types and operators (integer or
numeric_std.unsigned, etc.) will get you better results, since the
synthesis tool will recognize you want an adder, and give you an
optimized one. Anyway, concatenation uses the "&" operator, not
parentheses and commas.

will it extract the common logic as i want .... three input carry
chain thing.

trying to generate XOR, AND, LOAD and + using minimum logic area.
the total vhdl seems (no rw and oe logic yet) to use less LEs than the
mega block schematic thing. to allow a longer operation decode time.

higher fmax too.

working on a 66MHz driven 14 (phase) colour (plus PAL flip and not
flip sync varieties) subcarrier generator at moment (sync not
generated, just rendered). 7% area of 1270 LEs

 

[jacko]

hi

what is the minimum 4 lut area for this thing if all 4 ops do not have
to be generated in one cycle?

 

[Andy]

halt will be synchronous in the whole design. so do i need to have a
latch, and would it be removed if not needed automatically?

ok on the elsif i will do.

fixed all the syntax.

will go rising edge.




will it extract the common logic as i want .... three input carry
chain thing.

trying to generate XOR, AND, LOAD and + using minimum logic area.
the total vhdl seems (no rw and oe logic yet) to use less LEs than the
mega block schematic thing. to allow a longer operation decode time.

higher fmax too.

working on a 66MHz driven 14 (phase) colour (plus PAL flip and not
flip sync varieties) subcarrier generator at moment (sync not
generated, just rendered). 7% area of 1270 LEs

Depending on the width of the operands, it will probably be faster
with the arithmetic operator since the carry propagation is through
dedicated, high speed pathways and circuitry (assuming this is an
FPGA). A good synthesis tool may see similar propagation in discrete
logic that can use the same dedicated resources, but then again it may
not.

In general, I try the obvious, straight-forward method first, since if
it works, it is more easily writable, readable and understandable.
Only if it will not meet requirements (space and speed) do I get into
trying less straight-forward methods.

Andy