Comparison changes value?!

[MattHollands]

Hi!

I have a verilog system with two modules. The first one operates on some data then passes the processed data to the next module for processing. Both modules share memory space and so I need to pass memory control from one module to the next depending on who is processing. Therefore each module has an output called RAM_LOCK that indicates that the module wants to have control of RAM.

Each module has an input called NEW_DATA_IN that marks the start of operation and then operates for some clock cycles. The RAM_LOCK signal needs to be asserted as soon as NEW_DATA_IN is asserted so that it can set up the addresses for the RAM on the next clock cycle (ie it cannot wait until the next clock edge after NEW_DATA_IN is asserted because that is when it needs to do a memory operation). It will then de-assert RAM_LOCK after a certain number of RAM cycles.

Unfortunately, my system is far too complex to post all the code here but I'll post the most relevant parts.

Module A: (RAM_LOCK is an output, NEW_DATA is the input specifying new input data, cycle_count is the number of cycles since data was recieved

Code: [Select]

...
always @(*) begin
        // We need RAM control as soon as NEW_DATA is asserted so we can set the read address for the next cycle
        // We can deassert RAM control jsut before the last cycle as we do not need to set up any more read/writes
        RAM_LOCK = (NEW_DATA == 1 ? 1 : (cycle_count == READ_CYCLES+WRITE_CYCLES-1 ? 0 : RAM_LOCK));
    end
...

Module B:

Code: [Select]

...
    always @(*) begin
        // We need RAM control as soon as NEW_DATA_IN is asserted so we can set the read address for the next cycle
        // We can deassert RAM control just before the last cycle as we do not need to set up any more read/writes
        RAM_LOCK = (NEW_DATA_IN == 1 ? 1 : (CYCLE_COUNT == (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1 ? 0 : RAM_LOCK));
    end
...

Memory interface is main module:

Code: [Select]

...
    assign RAM_ADDR = (A_RAM_LOCK ? A_RAM_ADDR : (B_RAM_LOCK ? B_RAM_ADDR : 0));
    assign RAM_WE = (A_RAM_LOCK ? A_RAM_WE : (B_RAM_LOCK ? B_RAM_WE : 0));
    assign A_RAM_DO = (A_RAM_LOCK ? RAM_DO : 16'hBABE);
    assign B_RAM_DO = (B_RAM_LOCK ? RAM_DO : 16'hBEEF);
    assign RAM_DI = (A_RAM_LOCK ? A_RAM_DI : (B_RAM_LOCK ? B_RAM_DI : 16'hDEAD));
...RAM_ADDR, RAM_WE, RAM_DI (data in) and RAM_DO (data out) are the wires for the memory.

So what's the problem? Module A is working fine, but BEEF keeps showing up my output from module B indicating that control of the memory is not being passed correctly to module B.

If I replace the RAM_DO lines with these lines:

Code: [Select]

 
 assign A_RAM_DO =RAM_DO;
 assign B_RAM_DO = RAM_DO;
Then it fixes the issue, but clearly there is something going on in the background that I want to fix so it doesn't come back to bite me later.

What's even weirder? If I put in a comparison in my Main module it fixes the issue:

Code: [Select]

...
    assign LED[11] = ramalam;
    assign LED[10] = ramalam2;
    assign LED[9] = ramalam3;
... (in an always at posedge of clock)
        if(A_RAM_LOCK == 1) begin
            ramalam <= 1;
        end
   
        if(B_RAM_LOCK == 1) begin
           ramalam2 <= 1;
        end
       
        if(A_RAM_LOCK == 1 && B_RAM_LOCK == 1) begin
            ramalam3 <= 1;
        end
...
If I put these lines in (basically detecting whether each of the locks have been asserted) and show them on an LED, suddenly everything is fixed. What on earth?

I've seen a similar issue before where one of my inputs was labelled as an output, and fixing that fixed the issue, but I can't find anything similar in this case.

What could be causing this issue?!

[daveshah]

Not a verilog guy so thinking in VHDL terms but don't like the way RAM_LOCK depends upon itself in a non-clocked always block. I suspect the result of synthesis is marginal as to whether or not it works; so adding the comparison changes how the design is placed/routed enough to affect its behaviour.

[MattHollands]

I was also slightly concerned about that but did some research which said that it's fine as long as you use blocking assignment.

Anyway, I've changed it to:

Code: [Select]

    always @(*) begin
        // We need RAM control as soon as NEW_DATA_IN is asserted so we can set the read address for the next cycle
        // We can deassert RAM control just before the last cycle as we do not need to set up any more read/writes
        if(NEW_DATA_IN == 1) begin
            RAM_LOCK <= 1;
        end
        else begin
            if(CYCLE_COUNT == (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1) begin
                RAM_LOCK <= 0;
            end
        end
    end
And the issue is still there and can still be fixed by adding the comparison!

[daveshah]

What clock frequency are you running at and does timing analysis show any issues?

[MattHollands]

Oooh good shout. I'm running at 100MHz. Also, FYI I'm using a Digilent Nexys 4 DDR which has an Artix-7 FPGA from Xilinx.

I've never worried about timing before but you're right. Setup time has an issue (see attached image). Wonder where that's coming from.

How can I track this down?

[daveshah]

Have a look at the timing report to see which path is failing, as this could conceivably be a problem. Also if possible see if your design works at a reduced clock, say 50MHz. To solve the problem adding pipeline registers may help.

[MattHollands]

Ok the timing issues were coming from a divider in Module B. I've temporarily removed it which fixes the timing issues, but does not fix the overall problem.

[Edit] Removing the divider does kind of ruin what Module B is supposed to do, but I can remove it temporarily without affected other results [/Edit]

[daveshah]

Again, not a verilog guy but I believe it would be better practice to change the always block setting RAM_LOCK to either operate only on a clock edge or be truly combinatorial - assign a fixed value for all input combinations.  Otherwise I suspect a latch will be inferred which isn't great.

[MattHollands]

Oh man looks like you were right! Can't guarantee that it isn't broken but I think you got it.

I set RAM_LOCK to be a wire:

Code: [Select]

assign RAM_LOCK = ((NEW_DATA_IN == 1) || CYCLE_COUNT < (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1);
And now it appears to work.

My only concern is that (NEW_DATA_IN == 1) becomes false exactly at the same time as (CYCLE_COUNT < (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1) becomes true (see attached image). Could this potentially cause a glitch in RAM_LOCK at the falling edge of NEW_DATA_IN?

Technically this shouldn't matter as the glitch happens just after a clock rising edge and the memory is synchronous so the values only matter at rising edges.

[Edit] Uploaded wrong image [/Edit]

[daveshah]

I doubt it will be a problem, I imagine the propogation delays are such that the signal will not glitch until after the required hold time of the RAM inputs when the signal has to be stable.

[MattHollands]

Awesome. Thanks so much for your help. I've been doing FPGAs for about a year now but there's so much to consider!

[Someone]

Oh man looks like you were right! Can't guarantee that it isn't broken but I think you got it.

I set RAM_LOCK to be a wire:

Code: [Select]

assign RAM_LOCK = ((NEW_DATA_IN == 1) || CYCLE_COUNT < (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1);
And now it appears to work.

My only concern is that (NEW_DATA_IN == 1) becomes false exactly at the same time as (CYCLE_COUNT < (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1) becomes true (see attached image). Could this potentially cause a glitch in RAM_LOCK at the falling edge of NEW_DATA_IN?

Technically this shouldn't matter as the glitch happens just after a clock rising edge and the memory is synchronous so the values only matter at rising edges.

You need to use the tools constraints to do all the work for you, there is never a question about glitches or timing as the tools will cover all of that for you and report any paths that it cant fit to the requirements for you to look at.

[MattHollands]

You need to use the tools constraints to do all the work for you, there is never a question about glitches or timing as the tools will cover all of that for you and report any paths that it cant fit to the requirements for you to look at.

But nowhere in the code do I tell the system that RAM_LOCK should be a continuous pulse. All I've asked it to do is make sure that when NEW_DATA_IN ==1 or CYCLE_COUNT < x, set RAM_LOCK to 1. If there's a brief moment where NEW_DATA_IN is 0 and CYCLE_COUNT > x, then why wouldn't there be a moment where RAM_LOCK goes to 0 during that transition?

[Someone]

You need to use the tools constraints to do all the work for you, there is never a question about glitches or timing as the tools will cover all of that for you and report any paths that it cant fit to the requirements for you to look at.

But nowhere in the code do I tell the system that RAM_LOCK should be a continuous pulse. All I've asked it to do is make sure that when NEW_DATA_IN ==1 or CYCLE_COUNT < x, set RAM_LOCK to 1. If there's a brief moment where NEW_DATA_IN is 0 and CYCLE_COUNT > x, then why wouldn't there be a moment where RAM_LOCK goes to 0 during that transition?

The transitions are not the problem, there are probably many glitches occurring on complex logic paths. But the tools will ensure that the setup and hold timing of the flip flops are maintained and the implementation will function as intended, you don't question the implementation once it has passed timing as the tools have checked all the paths and determined for you that they are correct. Your problem is here:

My only concern is that (NEW_DATA_IN == 1) becomes false exactly at the same time as (CYCLE_COUNT < (KERNEL_WIDTH*KERNEL_HEIGHT*2)-1) becomes true (see attached image). Could this potentially cause a glitch in RAM_LOCK at the falling edge of NEW_DATA_IN?

You don't have timing information in the code, the code is specifying the behaviour and the behavioural simulation will match the physical implementation only if you tell it the physical constraints. Then you just write code, and it will work correctly as written.