deserializer

[Justas P]

Hi all,

first of all - sorry, if my questions look like homework/univ
questions. Never had Verilog at university, trying to learn it myself.

I am making deserializer. I have 3 inputs - clock, serial data and
sync so I know where block of data start. What I want to do is to say
to my uC that 8 bits were deserialised and are ready to be taken:

module deser(
clock,
ser_d_in,
par_d_out,
int_req,
int_ack,
sync
);

input clock, ser_d_in, int_ack, sync;
output [7:0] par_d_out;
output int_req;

reg [7:0] par_d_out;
reg [7:0] buffer;
reg [3:0] counter;
reg int_condition;

assign int_req = (int_condition == 'b1) ? 'b0 : 'b1;

always @(posedge clock) begin
counter = (sync) ? 'b0000 : counter + 'd1;
buffer = { ser_d_in, buffer[7:1]};
par_d_out = (counter == 'b1111) ? buffer : par_d_out;
int_condition = (counter == 'b1111) ? 'b1 : int_condition;
end

always @(negedge int_ack) begin
int_condition = 'b0;
end
endmodule

I am using Xilinx free ISE edition and get this when trying to
implement:
Xst:528 - Multi-source in Unit <deser> on signal <int_req>; this
signal is connected to multiple drivers.
Can anyone help me out with this?

Also - as I understand I should not use non-blocking assignments in
"always @(posedge clock)" because I increment "counter" and later do
actions that depend on value of "counter".

Finally - if I'm doing something terribly stupid, and there is a
better way to implement same functionality I am open to ideas.

Thanks in advance,
Justin

 

[Jonathan Bromley]

On Wed, 17 Mar 2010 12:25:16 -0700 (PDT), Justas P wrote:

I am making deserializer. I have 3 inputs - clock, serial data and
sync so I know where block of data start. What I want to do is to say
to my uC that 8 bits were deserialised and are ready to be taken:

module deser(
clock,
ser_d_in,
par_d_out,
int_req,
int_ack,
sync
);

input clock, ser_d_in, int_ack, sync;
output [7:0] par_d_out;
output int_req;

reg [7:0] par_d_out;
reg [7:0] buffer;
reg [3:0] counter;
reg int_condition;

assign int_req = (int_condition == 'b1) ? 'b0 : 'b1;

That line makes int_req effectively identical to int_condition,
which explains why the error message talks about int_req.

always @(posedge clock) begin
counter = (sync) ? 'b0000 : counter + 'd1;
buffer = { ser_d_in, buffer[7:1]};
par_d_out = (counter == 'b1111) ? buffer : par_d_out;
int_condition = (counter == 'b1111) ? 'b1 : int_condition;
end

As you said later, it is VERY important to get into the habit of
using nonblocking assignment to any variables that are declared
(and, therefore, that may be used)outside a clocked always block.
More on this later - you can't simply change all the = to <=
because it will break the logic.

always @(negedge int_ack) begin
int_condition = 'b0;
end

OK, this is the culprit.

I am using Xilinx free ISE edition and get this when trying to
implement:
Xst:528 - Multi-source in Unit <deser> on signal <int_req>; this
signal is connected to multiple drivers.

Right. For synthesis, it is important that a variable should
be written only from just one process (always block). The logic
you have created for int_condition is effectively a flip-flop
controlled by two distinct clocks - (posedge clock) and
(negedge int_ack).

Finally - if I'm doing something terribly stupid, and there is a
better way to implement same functionality I am open to ideas.

Not terribly stupid at all, but quite badly flawed.

There's a whole pile of small things. First, I think you're
counting off 15 or 16 bits, not 8. Second, the unnecessary
use of the conditional operator ?: is just confusing and messy;
an if-else would be far clearer. Third, you have no reset on
the clocked logic. This may be OK for this design; it'll
generate garbage until the first "sync" and then settle down.
Nevertheless, please think carefully about initialization.
And finally, in a clocked block it's just plain silly to
write a variable's value back to itself; that "keep current"
behaviour is precisely what Verilog variables, and flip-flops,
do anyway if you leave them in peace.

But then we come to the big stuff.

I assume the primary inputs sync and ser_d_in are
synchronous with posedge clock, otherwise you are
in deep trouble. So let's put that to one side.

Using blocking assignment to variables in a clocked
always block is a useful advanced technique, but a
good way for beginners to get themselves into trouble.
If you really want to do it, then please ensure that
- any variable written by blocking assignment = is
declared locally inside the block;
- any variable declared outside the block is written
by nonblocking <= assignment;
- don't mix the two types of assignment to any given
variable.

Now, obviously int_condition and par_d_out are used
outside the always block so must be declared outside;
therefore, they must be written by <=. The other
variables, which are written by blocking assignment
in your logic, must be declared inside the block.
With these changes we get...

reg [7:0] par_d_out;
reg int_condition;

always @(posedge clock) begin : clocked_logic
// Labelling a begin...end makes declarations legal:
reg [7:0] buffer;
reg [3:0] counter;
counter = (sync) ? 'b0000 : counter + 'd1;
buffer = { ser_d_in, buffer[7:1]};
if (counter == 'b1111) begin
par_d_out <= buffer;
int_condition <= 'b1;
end
end

This has the same functionality as your code, but is much
safer. Note, however, that it's testing the values of
counter and buffer AFTER you've updated them. This means
that the code must make use of the next-state value of
those variables - the *inputs* to the flops - to drive
other logic. Useful, but also risky because you can
easily create long combinational paths (although, in
reality, I don't think that's likely to be an issue for
such a simple design). Also, note that if you don't
get a new sync flag, your counter will wrap around
and eventually get back to 1111 for a second time,
which will cause it to spit out a new value on
par_d_out - is that correct?

Finally, the little matter of the int-ack handshake.
You simply can't use a different clock to set and
reset a flop. Use synchronous edge detection on
int-ack as a way to clear the request. Something
like (sketch only):

always @(posedge clock) begin : handshake
reg old_int_ack;
if (new data word detected)
int_req <= 1;
else if (int_ack & !old_int_ack)
int_req <= 0;
old_int_ack = int_ack;
...

Hope this has given you some pointers.
--
Jonathan Bromley

 

[Justas P]

Hi,

Firstly of all I would like to thank for such a complete answer.
Secondly - I redone my design to this:

module deser(clock,ser_d_in,par_d_out,int_req,int_ack,sync);
input clock, ser_d_in, int_ack, sync;
output [7:0] par_d_out;
output int_req;
reg [7:0] par_d_out;
reg int_condition;
reg int_req;

always @(posedge clock) begin : clocked_logic
reg [7:0] buffer;
reg [3:0] counter;
counter = (sync) ? 'b0000 : counter + 'd1;
buffer = { ser_d_in, buffer[7:1]};
if (counter == 'b1111) begin
par_d_out <= buffer;
int_condition <= 'b1;
end
end
always @(posedge clock) begin : interrupt_logic
if (int_condition == 'b1) begin
int_req <= 'b0; //int active low
end
else if (int_condition == 'b1 && int_ack == 'b1) begin
int_req <= 'b1; // I've ack'd int
int_condition <= 'b0; //I'm waiting for next data-ready
end
end
endmodule

I really did not get why we should use "old_int_ack". Right now as I
understand it should be enough just to monitor state of int_condition.
If it becomes one - logic deserialised data and uC should take it. I
raise an interrupt (int_req = 0) and keep it as long as I am not
ack'ing it.

Now when I am implementing it I get these errors (even though
implementation completes):

Xst:2679 - Register <int_condition> in unit <deser> has a constant
value of 1 during circuit operation. The register is replaced by
logic.
Xst:2679 - Register <int_req> in unit <deser> has a constant value of
0 during circuit operation. The register is replaced by logic.
Xst:647 - Input <int_ack> is never used. This port will be preserved
and left unconnected if it belongs to a top-level block or it belongs
to a sub-block and the hierarchy of this sub-block is preserved.

Am I still doing something wrong?

Thanks a LOT,
Justas

 

[John_H]

Two thing hit me right off.

One: how does int_condition ever become zero? Please describe how
that occurs, don't just point to the code; walk us through it.

Two: by using two always blocks which both are trying to assign
int_condition, what happens if the condition in the first block is
true setting the value to 1 and the conditions in the second block are
such that the value should be zero. Which wins?

Registers should always be assigned new values within only one always
block. By assigning the one register to two always blocks, the
synthesizer loses its ming and the person maintaining the code has no
clue what the designer was attempting. Even in the case of having
multiple items within one always block which *are* evaluated in
sequence, it's a supportability nightmare to *not* assign the value
within one and only one if/else if/else style construct.

 

[gabor]

On Mar 17, 6:39 pm, Jonathan Bromley <jonathan.brom...@MYCOMPANY.com>
wrote:

[snip]

   assign int_req = (int_condition == 'b1) ? 'b0 : 'b1;

That line makes int_req effectively identical to int_condition,
which explains why the error message talks about int_req.

[snip]

Second, the unnecessary
use of the conditional operator ?: is just confusing and messy;
an if-else would be far clearer.

[snip]

Hope this has given you some pointers.
--
Jonathan Bromley

I think that ? : is generally clear and it is usable in
assignment statements, unlike if / else. However I agree
that the usage here is unnecessary and enough to confuse
Jonathan into missing the inversion in the logic. Since
you're not using VHDL, you can simple write:

assign int_req = ~int_condition;
or
assign int_req = !int_condition;

Both quite readable and both having the same effect on
a single-bit value. Normally the ? : syntax is only
used when you need to generate a mux or tristate function
in a continuous assignment. if/else or case statements
are generally preferred in procedural code. I generally
detest the use of the equality operator where not needed
i.e. if (foo == 1) where if (foo) would work as well...

just my 2 cents
Gabor

 

[Justas P]

On Mar 26, 10:48 pm, John_H <newsgr...@johnhandwork.com> wrote:

Two thing hit me right off.

One: how does int_condition ever become zero?  Please describe how
that occurs, don't just point to the code; walk us through it.

OK. I think I should have started from hight level description what I
am trying to achieve. I want to deserialize a data stream and feed it
to uC. It should make an interrupt every n bits to uC that data is
ready to be taken. When uC receive an int from logic, it issue
int_ack, so that int line is once again high (/int = atcive low), read
parallel data and then continue to wait for next int.

And to answer your question - int_condition should be 0 when uC issue
int_ack.

Two: by using two always blocks which both are trying to assign
int_condition, what happens if the condition in the first block is
true setting the value to 1 and the conditions in the second block are
such that the value should be zero.  Which wins?

I understand that this is undefined, but with my knowledge of Verilog
I can't figure out a way to avoid this.

Registers should always be assigned new values within only one always
block.

OK. Got that!

By assigning the one register to two always blocks, the
synthesizer loses its ming and the person maintaining the code has no
clue what the designer was attempting.  Even in the case of having
multiple items within one always block which *are* evaluated in
sequence, it's a supportability nightmare to *not* assign the value
within one and only one if/else if/else style construct.

 

[John_H]

On Mar 26, 5:46 pm, Justas P <justas.pode...@gmail.com> wrote:

OK. I think I should have started from hight level description what I
am trying to achieve.

I'm sad to say that "I don't care what you're trying to achieve" when
you asked if you were doing something wrong; I pointed out some things
that are huge problems. By your response it sounds more like "an
answer somebody gave me isn't working because I don't know anything
about Verilog or hardware in general so make it work for me!"

And to answer your question - int_condition should be 0 when uC issue
int_ack.

That's not answering my question at all. I asked you to talk through
the conditions. And you haven't "said" when it's supposed to be 1.

"When the counter reaches a terminal count I want to set the
int_condition."
"When the int_condition is set, I want to issue an int_req but
otherwise (if the int_condition isn't set) if the int_condition is set
and... Umm...." There was an else there.

Okay. Imagine you have to make someone decide what color to make a
widget at *one moment* in time, either green or blue. They have all
the information available from the moment before the decision and they
have to figure out what to do based only on that abundance of
information and the rules you decide. Based on what you tell this
individual, they must paint the widget green or blue. Maybe the
widget was already the right color or maybe it wasn't. What do they
need to do? They only have one opportunity to get it right.

That's a beginning of an insight into parallelism. You have one
moment of time to decide based on all the available info. That one
moment has to get it right, no second chances.

You can't set an int_condition at a terminal count AND manipulate that
same "widget" elsewhere based on other criteria. There has to be ONE
decision. And you can't have an "if widget is blue wash a pan
otherwise if widget is blue and the sky is dark, paint the widget
green." There is no otherwise here. Ever. Because if the widget
isn't blue then it's green and the "if blue and dark" condition will
NEVER apply. If the else weren't there, PERHAPS the code would give
you the intend for resetting the int_condition but that could easily
conflict with the set condition that doesn't appear to care whether
the condition is already set or not.

You need to figure out what you want to get your system to flow. Do
you know how to flow chart?
You need to define signals to help guide that flow.
You need to clearly define how the signals behave for ALL conditions
at once; one set of complex rules that comes to one decision has to
get it right. No second chance.

I have difficulty seeing that you're actually trying to understand or
learn from this experience. I have other things I want to say but in
order to be a *little* civil, I'll stop.