Verilog Shift Register interface to AVR

Question

I'm very new to Verilog and HDL in general. I recently purchased a Mojo FPGA (Spartan 6) development board and have been playing around with it.

One concept of interest to me is the shift register, specifically, interfacing an ATmega to the FPGA to load a shift register on the FPGA. I have this working between with this Verilog:

module sr8(
    input clk,        // FPGA clock
    input sshift,     // Shift signal from ATmega
    input sdata,      // Data from ATmega
    output [7:0] PO   // Parallel output
);

// Shift register
reg [7:0] srdata = 8'd0;

reg sshift_FF;
wire sshift_en;
always @(posedge clk) begin
    sshift_FF <= sshift;
end
assign sshift_en = sshift & !sshift_FF;

always @(posedge clk) begin
    if ( sshift_en ) begin
        srdata <= { srdata[6:0], sdata };
    end
end

assign PO = srdata;

endmodule

It's really simple, it shifts data in on the positive edge of the clock (50 MHz) when sshift_en is 1. sshift_en is the result of AND'ing the current value of sshift and the compliment of the output of the flip-flip sshift_FF (i.e. it checks for a rising edge).

My real question here: is this a good/proper way to do this or not? I've done a good amount of research on implementing shift registers in Verilog and my concept is essentially the same only I want the "shift" signal of my shift register to be dictated by the AVR (operating much slower than the 50 MHz clock on the FPGA) not the clock signal (like all of the examples I've seen).

If I need to provide more information about anything here, let me know.

Thank you in advance!

Edit: I've modified the code to include a synchronization section:

module sr8(
    input clk,
    input sshift,
    input sdata,
    output [7:0] PO
);

// Shift register
reg [7:0] srdata = 8'd0;

// Start of modified synchronization section
reg sshift_d, sshift_dd, sshift_ddd, sshift_en;
always @(posedge clk) begin
    sshift_d <= sshift;
    sshift_dd <= sshift_d;
    sshift_ddd <= sshift_dd;
    sshift_en <= sshift_dd & ~sshift_ddd;
end
// End of modified section

always @(posedge clk) begin
    if ( sshift_en ) begin
        srdata <= { srdata[6:0], sdata };
    end
end

assign PO = srdata;

endmodule

Answer 1

Your shift register description looks ok to me.

The sshift signalis from AVR is an asynchronous signal wrt your FPGA clock, which is a source of possible metastability. I would use a synchronizer to reduce the chance.

See Section "Synchronization Registers" in this document:

http://www.altera.com/literature/wp/wp-01082-quartus-ii-metastability.pdf

Another minor point: Although interchangable in the case of your example, ! (together with && and ||) is used for logical operations whereas ~ is used for bitwise negation, which IMHO is more proper for this context. A common use of ! is:

if (!condition)