Code Review: SPI Transmitter

[Rob Gaddi]

So things have been to my mind unfortunately quiet over here for a while
now, so I figured I'd try do get something new going, just in case any
of us have gotten bored with the 118th iteration of telling some
wet-nosed pup to use numeric_std instead of std_logic_arith and
explaining what a process is. Let's actually talk about VHDL code and
the improving thereof.

So the rules of the game are simple. Someone posts their code, and we
talk about it. Code should be at the bare minimum syntactically valid,
and preferably working. But how could it have been cleaner, or more
elegant? Does it scan when you read it? What obvious low-hanging fruit
of functionality was missed? Basically, if this was code that you were
going to bring in to your project, what would you have liked to be
different about it?

Today I've got code for an SPI transmitter. The goal was to create
thing that can be kicked off by a register write and update either a
serial DAC or relay-driving shift register. It requires no
configuration from the software side, and if the software stops writing
to the register it will always ultimately set the connected thing to the
last written value.

-------------------------------------------------------------------------------
-- Title : SPI Transmitter
-- Project : T904 TEM Testset
-------------------------------------------------------------------------------
-- File : spi_transmitter.vhd
-- Author : Rob Gaddi <rgaddi@highlandtechnology.com>
-- Company : Highland Technology, Inc.
-- Created : 04-Jun-2019
-- Last update: 04-Jun-2019
-- Platform : Xilinx Zynq 7020 (uZed)
-- Standard : VHDL-2002
-------------------------------------------------------------------------------
-- Description: Sends framed words out over SPI on request.
--
-- A pulse on go sets a request flag asking the engine to run. The engine
-- runs at the next available time, which is the time at which it has been
-- idle for at least the MIN_IDLE time. At "run" time it will latch whatever
-- the current data on din is and begin the SPI transaction.
--
-- This means that, if the engine is and has been idle, the go pulse will
-- cause the data on din to start immediately. If the engine is
-- currently running, an update to din and pulse on go will cause the engine
-- to finish the transaction it is currently running, then take the new din.
-- But a second update while the engine is running will cause the first to
-- simply be ignored; the idea being that devices being communicated with by
-- this engine have only a current state rather than a history or multiple
-- current states, and that the point of the engine is to make the current
-- state correct.
--
-- idle_out represents the state of the transmitter. done goes high for a
-- single clock on the completion of the transmit.
--
-- SPI Information: Clock idle state is low. Data is transitioned on the
-- falling edge of SCLK, under the assumption that it is sampled on the rising
-- edge. Data transmits MSB first.
-------------------------------------------------------------------------------
-- Revision History:
-------------------------------------------------------------------------------

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

use work.standard_functions.all;

entity spi_transmitter is
generic (
BUS_FREQ_MHZ : integer;
SCLK_PERIOD : time;
MIN_IDLE : time := 0 ns;
CS_EXTRA : time := 0 ns;
BITS : positive
);
port (
-- Logic side
din : in std_logic_vector(BITS-1 downto 0);
go : in boolean;
idle_out : out boolean;
done : out boolean;

-- Serial side
MOSI : out std_logic;
SCLK : out std_logic;
CS_N : out std_logic;

clk : in std_logic;
rst : in std_logic
);
end entity spi_transmitter;

architecture Behavioral of spi_transmitter is

type t_state is (
IDLE, DROP_CS,
SCLK_LOW, SCLK_HIGH,
HOLD_CS
);

constant tClk : time := 1 us / BUS_FREQ_MHZ;

-- Number of clocks to spend in each of the states. For IDLE this is
-- a minimum, for everyone else it's a fixed time.
function time_to_clocks(x: time) return natural is
begin
return time_to_clocks(x, tClk);
end function time_to_clocks;

constant IDLE_LEN : natural := time_to_clocks(MIN_IDLE);
constant CSD_LEN : natural := time_to_clocks(CS_EXTRA);

constant SCLK_LEN : natural := time_to_clocks(SCLK_PERIOD);
constant SCLKH_LEN : natural := SCLK_LEN / 2;
constant SCLKL_LEN : natural := SCLK_LEN - SCLKH_LEN;

constant CSH_LEN : natural := time_to_clocks(CS_EXTRA + SCLK_PERIOD/2);

-- Sharing a single timer for all these values requires that it be large
-- enough to hold the maximum value.
function MAXTIMER return natural is
variable x : natural;
begin
x := IDLE_LEN;
if x < SCLKH_LEN then x := SCLKH_LEN; end if;
if x < CSH_LEN then x := CSH_LEN; end if;
return x;
end function MAXTIMER;
subtype t_timer is integer range 0 to MAXTIMER-1;

begin

MACHINE: process(clk)
variable go_pending: boolean;
variable sr: std_logic_vector(din'range);
variable timer: t_timer;
variable bits_left : integer range din'range;
variable state: t_state;

-- Procedure for handling simple wait states.
procedure waitandthen(clocks : in positive; nextstate : in t_state) is
constant LIMIT : natural := clocks-1;
begin
if timer = LIMIT then
timer := 0;
state := nextstate;
else
timer := timer + 1;
end if;
end procedure waitandthen;

begin
if rising_edge(clk) then
-- Manage the state machine.
done <= false;
case state is
when IDLE =>
if timer = (IDLE_LEN-1) then
if go_pending then
go_pending := false;

sr := din;
bits_left := BITS-1;
timer := 0;

-- We don't even have a DROP_CS state if we don't
-- request any extra CS time.
if CSD_LEN = 0 then
state := SCLK_LOW;
else
state := DROP_CS;
end if;
end if;
else
timer := timer + 1;
end if;

when DROP_CS => waitandthen(CSD_LEN, SCLK_LOW);
when SCLK_LOW => waitandthen(SCLKL_LEN, SCLK_HIGH);
when SCLK_HIGH =>
if timer = (SCLKH_LEN-1) then
timer := 0;
sr := sr(sr'high-1 downto 0) & 'X';
if bits_left = 0 then
state := HOLD_CS;
else
bits_left := bits_left - 1;
state := SCLK_LOW;
end if;
else
timer := timer + 1;
end if;

when HOLD_CS =>
waitandthen(CSH_LEN, IDLE);
done <= (state = IDLE);
end case;

-- A new data request always sets the go_pending flag, but a reset
-- overrides everything.
if go then
go_pending := true;
end if;
if (rst = '1') then
state := IDLE;
timer := 0;
sr := (others => 'X');
go_pending := false;
end if;

-- Drive the outputs based on the state.
SCLK <= POS_LOGIC(state = SCLK_HIGH);
CS_N <= NEG_LOGIC(state /= IDLE);
MOSI <= sr(sr'high);
idle_out <= (state = IDLE);
end if;
end process MACHINE;

end architecture Behavioral;



--
Rob Gaddi, Highland Technology -- www.highlandtechnology.com
Email address domain is currently out of order. See above to fix.

 

[HT-Lab]

On 17/06/2019 17:58, Rob Gaddi wrote:

So things have been to my mind unfortunately quiet over here for a while
now, so I figured I'd try do get something new going, just in case any
of us have gotten bored with the 118th iteration of telling some
wet-nosed pup to use numeric_std instead of std_logic_arith and
explaining what a process is.  Let's actually talk about VHDL code and
the improving thereof.

So the rules of the game are simple.  Someone posts their code, and we
talk about it.  Code should be at the bare minimum syntactically valid,
and preferably working.  But how could it have been cleaner, or more
elegant?  Does it scan when you read it?  What obvious low-hanging fruit
of functionality was missed?  Basically, if this was code that you were
going to bring in to your project, what would you have liked to be
different about it?

I applaud you for trying to bring back some live in this newsgroup as
everybody (with exceptions of some old guys) seemed to have moved to
stack exchange, reddit or any of the censorship free vendor mailing lists.

I don't think your code review idea will kick off as the traffic is
simply to low and engineers generally don't want to expose their bad
coding style (including me).

However, to give you some comments your code looks great and is easy to
read. My only minor suggestion (apart from some personal coding style
preferences) would be to add some functional verification directives.

It is relative easy to sprinkle some PSL cover directives (or asserts)
over your code which then makes the verification job later on a lot easier.

Regards,
Hans.
www.ht-lab.com

Today I've got code for an SPI transmitter.  The goal was to create
thing that can be kicked off by a register write and update either a
serial DAC or relay-driving shift register.  It requires no
configuration from the software side, and if the software stops writing
to the register it will always ultimately set the connected thing to the
last written value.

-------------------------------------------------------------------------------

-- Title      : SPI Transmitter
-- Project    : T904 TEM Testset
-------------------------------------------------------------------------------

-- File       : spi_transmitter.vhd
-- Author     : Rob Gaddi  <rgaddi@highlandtechnology.com
-- Company    : Highland Technology, Inc.
-- Created    : 04-Jun-2019
-- Last update: 04-Jun-2019
-- Platform   : Xilinx Zynq 7020 (uZed)
-- Standard   : VHDL-2002
-------------------------------------------------------------------------------

-- Description: Sends framed words out over SPI on request.
--
--  A pulse on go sets a request flag asking the engine to run.  The engine
--  runs at the next available time, which is the time at which it has been
--  idle for at least the MIN_IDLE time.  At "run" time it will latch
whatever
--  the current data on din is and begin the SPI transaction.
--
--  This means that, if the engine is and has been idle, the go pulse will
--  cause the data on din to start immediately.  If the engine is
--  currently running, an update to din and pulse on go will cause the
engine
--  to finish the transaction it is currently running, then take the new
din.
--  But a second update while the engine is running will cause the first to
--  simply be ignored; the idea being that devices being communicated
with by
--  this engine have only a current state rather than a history or multiple
--  current states, and that the point of the engine is to make the current
--  state correct.
--
--  idle_out represents the state of the transmitter.  done goes high for a
--  single clock on the completion of the transmit.
--
--  SPI Information: Clock idle state is low. Data is transitioned on the
--  falling edge of SCLK, under the assumption that it is sampled on the
rising
--  edge.  Data transmits MSB first.
-------------------------------------------------------------------------------

-- Revision History:
-------------------------------------------------------------------------------


library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

use work.standard_functions.all;

entity spi_transmitter is
    generic (
        BUS_FREQ_MHZ : integer;
        SCLK_PERIOD  : time;
        MIN_IDLE     : time := 0 ns;
        CS_EXTRA     : time := 0 ns;
        BITS : positive
    );
    port (
        -- Logic side
        din  : in  std_logic_vector(BITS-1 downto 0);
        go   : in  boolean;
        idle_out : out boolean;
        done : out boolean;

        -- Serial side
        MOSI : out std_logic;
        SCLK : out std_logic;
        CS_N : out std_logic;

        clk  : in  std_logic;
        rst  : in  std_logic
    );
end entity spi_transmitter;

architecture Behavioral of spi_transmitter is

    type t_state is (
        IDLE, DROP_CS,
        SCLK_LOW, SCLK_HIGH,
        HOLD_CS
    );

    constant tClk : time := 1 us / BUS_FREQ_MHZ;

    -- Number of clocks to spend in each of the states.  For IDLE this is
    -- a minimum, for everyone else it's a fixed time.
    function time_to_clocks(x: time) return natural is
    begin
        return time_to_clocks(x, tClk);
    end function time_to_clocks;

    constant IDLE_LEN   : natural := time_to_clocks(MIN_IDLE);
    constant CSD_LEN    : natural := time_to_clocks(CS_EXTRA);

    constant SCLK_LEN   : natural := time_to_clocks(SCLK_PERIOD);
    constant SCLKH_LEN  : natural := SCLK_LEN / 2;
    constant SCLKL_LEN  : natural := SCLK_LEN - SCLKH_LEN;

    constant CSH_LEN    : natural := time_to_clocks(CS_EXTRA +
SCLK_PERIOD/2);

    -- Sharing a single timer for all these values requires that it be
large
    -- enough to hold the maximum value.
    function MAXTIMER return natural is
        variable x : natural;
    begin
        x := IDLE_LEN;
        if x < SCLKH_LEN then   x := SCLKH_LEN; end if;
        if x < CSH_LEN then     x := CSH_LEN; end if;
        return x;
    end function MAXTIMER;
    subtype t_timer is integer range 0 to MAXTIMER-1;

begin

    MACHINE: process(clk)
        variable go_pending: boolean;
        variable sr: std_logic_vector(din'range);
        variable timer: t_timer;
        variable bits_left : integer range din'range;
        variable state: t_state;

        -- Procedure for handling simple wait states.
        procedure waitandthen(clocks : in positive; nextstate : in
t_state) is
            constant LIMIT : natural := clocks-1;
        begin
            if timer = LIMIT then
                timer := 0;
                state := nextstate;
            else
                timer := timer + 1;
            end if;
        end procedure waitandthen;

    begin
        if rising_edge(clk) then
            -- Manage the state machine.
            done <= false;
            case state is
                when IDLE =
                    if timer = (IDLE_LEN-1) then
                        if go_pending then
                            go_pending := false;

                            sr := din;
                            bits_left := BITS-1;
                            timer := 0;

                            -- We don't even have a DROP_CS state if we
don't
                            -- request any extra CS time.
                            if CSD_LEN = 0 then
                                state := SCLK_LOW;
                            else
                                state := DROP_CS;
                            end if;
                        end if;
                    else
                        timer := timer + 1;
                    end if;

                when DROP_CS    =>  waitandthen(CSD_LEN, SCLK_LOW);
                when SCLK_LOW =>    waitandthen(SCLKL_LEN, SCLK_HIGH);
                when SCLK_HIGH =
                    if timer = (SCLKH_LEN-1) then
                        timer := 0;
                        sr := sr(sr'high-1 downto 0) & 'X';
                        if bits_left = 0 then
                            state := HOLD_CS;
                        else
                            bits_left := bits_left - 1;
                            state := SCLK_LOW;
                        end if;
                    else
                        timer := timer + 1;
                    end if;

                when HOLD_CS =
                    waitandthen(CSH_LEN, IDLE);
                    done <= (state = IDLE);
            end case;

            -- A new data request always sets the go_pending flag, but
a reset
            -- overrides everything.
            if go then
                go_pending := true;
            end if;
            if (rst = '1') then
                state := IDLE;
                timer := 0;
                sr := (others => 'X');
                go_pending := false;
            end if;

            -- Drive the outputs based on the state.
            SCLK <= POS_LOGIC(state = SCLK_HIGH);
            CS_N <= NEG_LOGIC(state /= IDLE);
            MOSI <= sr(sr'high);
            idle_out <= (state = IDLE);
        end if;
    end process MACHINE;

end architecture Behavioral;

 

[KJ]

Why is the 'Bus' clock specified as an integer frequency in MHz? All of the other time related things are specified as time units... the same should be done for the clock period. Not to mention I can't remember the last time I dealt with a clock that had such a nice integer frequency. Lastly, if you did want to specify frequency it would not be integers but positives.

Possibly outside of the scope of your design, but typically I would have to communicate with more than one SPI device on the board but never independently since they are controlled by some single processor. In that scenario, having multiple SPI controllers is wasteful, better to have a single controller that produces multiple CS outputs and share use of all other SPI signals. With that design, you would would want generics for the number of SPI devices and arrays of genetics to specify SCLK periods and bits. But again that is probably outside the scope of what are intending here.

Kevin Jennings

 

[KJ]

No reset for your state machine... that's an 'oops'.

Kevin Jennings

 

[KJ]

My 'oops', I now see the reset signal 'rst' being used in the state machine. Also unclear is why 'clk' and 'rst' aren't considered part of the 'logic' set of signals. By the way, the design process isn't 'Wheel of Fortune' where vowels cost money.

Kevin Jennings

 

[KJ]

For the 'logic' side, the signal names leave much to the imagination. Let's say you have no clue at all what the design is supposed to do, would this set of signals give you any idea if what this part of the interface is supposed to do?

  din  : in  std_logic_vector(BITS-1 downto 0); 
         go   : in  boolean; 
         idle_out : out boolean; 
         done : out boolean; 

My guess is 'No'. 'Go', 'idle_out', 'done', really? What this interface really is, is a write only processor type of interface (that may or may not actually be controlled by a processor). If you accept that, then you should be looking to use an already existing convention for that type of interface.. Wishbone or Avalon come to mind. Then the interface signals would have names like 'Write', 'WriteData', and 'WaitRequest' and you could describe it as an Avalon or Wishbone compatible interface and be done. Your audience would know what the interface is or could look it up in the appropriate documentation for Avalon or Wishbone. No need to go into the guts of the design to figure out how to use it.

On the one hand, this may seem like just that the signal names are being renamed so it is cosmetic and not important. But look again. Using an accepted interface there are two control and status signals where you used three and there is no discernable advantage to the user of your code having that extra status. Being superfluous, that extra signal makes your design harder to use, not easier. As a user of your code I wouldn't want to have to look beyond the interface to have to figure out how to use it.

Don't reinvent the wheel, this one came out not round. Use existing standards and conventions for every interface.

Kevin Jennings

 

[KJ]

"so I figured I'd try do get something new going"

Well, that was short lived

Kevin

 

[Rob Gaddi]

On 6/19/19 12:32 PM, KJ wrote:

My 'oops', I now see the reset signal 'rst' being used in the state machine. Also unclear is why 'clk' and 'rst' aren't considered part of the 'logic' set of signals. By the way, the design process isn't 'Wheel of Fortune' where vowels cost money.

Kevin Jennings

Yeah, but all those keystrokes on my poor tired fingers!

I'd argue that clk at least is pretty much standard in VHDL code; it's the
reference name for the one and only clock you've got in tutorials going back
decades. Reset, on the other hand, well that's just a mess everywhere.

--
Rob Gaddi, Highland Technology -- www.highlandtechnology.com
Email address domain is currently out of order. See above to fix.

 

[Rob Gaddi]

On 6/19/19 12:04 PM, KJ wrote:> Why is the 'Bus' clock specified as an integer
frequency in MHz? All of the other time related things are specified as time
units... the same should be done for the clock period. Not to mention I can't
remember the last time I dealt with a clock that had such a nice integer
frequency. Lastly, if you did want to specify frequency it would not be integers
but positives.
>

A function of how I've done things for a while, back when I couldn't trust the
synthesis tools to handle math on time objects correctly even when it all cooked
down to integers at compile time. But you're absolutely right, having already
decided that the tools will handle time right the rest of the time, I should
really make that interface consistent. I haven't yet because whenever I do it
it's going to be a nuisance, but if it'll be a nuisance whenever then the time
to do it is now.

> Possibly outside of the scope of your design, but typically I would have to
communicate with more than one SPI device on the board but never independently
since they are controlled by some single processor. In that scenario, having
multiple SPI controllers is wasteful, better to have a single controller that
produces multiple CS outputs and share use of all other SPI signals. With that
design, you would would want generics for the number of SPI devices and arrays
of genetics to specify SCLK periods and bits. But again that is probably outside
the scope of what are intending here.

Kevin Jennings

As you said, outside the scope. At the hardware level on this design we've got
plenty of pins, and the SPI target devices are situated somewhat radially out
from the FPGA, so trying to share copper makes signal integrity a mess.

Also, I'm amazed by how many SPI devices deal with being multidropped badly.
I've seen a couple of ADCs that get deeply confused if the SCLK coming in is too
fast, even if they're not even chip-selected during the process.

One of these days I've got a vision in the back of my head of doing a really
GOOD queued SPI controller to use as a standard peripheral. Generic as you
suggested for number of devices, but with an array of registers describing the
devices that get configured by the processor at runtime for clock rate, polarity
stuff, and a general purpose bitmask of "And how should the CS lines look when
you address this device" given the number of wacky not-quite-SPI devices with
positive CS, or maybe you also need to control a tri-state buffer along the way,
etc. Then arrays as well of write-data and read-data registers, one for each
device, but they all funnel through the same queue and the core takes care of
all the details.


--
Rob Gaddi, Highland Technology -- www.highlandtechnology.com
Email address domain is currently out of order. See above to fix.

 

[Rob Gaddi]

On 7/1/19 9:49 AM, Rob Gaddi wrote:

On 6/19/19 12:04 PM, KJ wrote:> Why is the 'Bus' clock specified as an integer
frequency in MHz? All of the other time related things are specified as time
units... the same should be done for the clock period. Not to mention I can't
remember the last time I dealt with a clock that had such a nice integer
frequency. Lastly, if you did want to specify frequency it would not be integers
but positives.


A function of how I've done things for a while, back when I couldn't trust the
synthesis tools to handle math on time objects correctly even when it all cooked
down to integers at compile time.  But you're absolutely right, having already
decided that the tools will handle time right the rest of the time, I should
really make that interface consistent.  I haven't yet because whenever I do it
it's going to be a nuisance, but if it'll be a nuisance whenever then the time
to do it is now.

Ah-HA! I'm back into the design now, and I'm remembering why, at least in
upper-level code, I use BUS_FREQ_MHZ. It's because system builders like the
Vivado IP Integrator, in the interest of playing nicely with Verilog, are so
utterly brain-damaged that they're not able to use /time/ as a generic, only
/integer/.

Because otherwise they might have to actually support VHDL, and god knows we
can't have that.

--
Rob Gaddi, Highland Technology -- www.highlandtechnology.com
Email address domain is currently out of order. See above to fix.