Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPI: Generalize to multi-lane MOSI/MISO #2635

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SPI: Generalize to multi-lane MOSI/MISO #2635

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7428dff
SPI: Represent inputs/outputs as records
bgamari Apr 29, 2024
c8e8826
SPI: Generalize to multi-lane MISO/MISO
bgamari Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 71 additions & 47 deletions clash-cores/src/Clash/Cores/SPI.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@
module Clash.Cores.SPI
( SPIMode(..)
-- * SPI master
, SpiMasterIn(..)
, SpiMasterOut(..)
, spiMaster
-- * SPI slave
, SpiSlaveIn(..)
, SpiSlaveOut(..)
, SPISlaveConfig(..)
, spiSlave
-- ** Vendor configured SPI slaves
Expand All @@ -26,6 +30,53 @@ import Clash.Sized.Internal.BitVector

import Clash.Cores.LatticeSemi.ICE40.IO
import Clash.Cores.LatticeSemi.ECP5.IO
import Clash.Class.HasDomain

-- | Input signals of an SPI subordinate.
data SpiSlaveIn (ds :: BiSignalDefault) (dom :: Domain) (misoLanes :: Nat) (mosiLanes :: Nat) =
SpiSlaveIn {
spiSlaveMosi :: "MOSI" ::: Signal dom (BitVector mosiLanes),
-- ^ Master-out, slave-in
spiSlaveMisoIn :: "MISO" ::: BiSignalIn ds dom misoLanes,
-- ^ Master-in, slave-out
spiSlaveSck :: "SCK" ::: Signal dom Bit,
-- ^ Serial Clock
spiSlaveSs :: "SS" ::: Signal dom Bit
-- ^ Slave select
}

type instance TryDomain t (SpiSlaveIn ds dom misoLanes mosiLanes) = 'Found dom

-- | Output signals of an SPI subordinate.
data SpiSlaveOut (ds :: BiSignalDefault) (dom :: Domain) (misoLanes :: Nat) (mosiLanes :: Nat) =
SpiSlaveOut {
spiSlaveMisoOut :: "MISO" ::: BiSignalOut ds dom misoLanes
-- ^ Master-in, slave-out
}

type instance TryDomain t (SpiSlaveOut ds dom misoLanes mosiLanes) = 'Found dom

-- | Output signals of an SPI master.
data SpiMasterOut (dom :: Domain) (misoLanes :: Nat) (mosiLanes :: Nat) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you want to keep the type-level arguments all the same across these types, but I think it's better to just declare arguments that are actually used. So SpiMasterOut doesn't need misoLanes and SpiMasterIn doesn't need mosiLanes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the functions, you use the names mosiW and misoW, but here you use mosiLanes and misoLanes to refer to the same quantity. Could you pick one and apply that name everywhere?

SpiMasterOut {
spiMasterMosi :: "MOSI" ::: Signal dom (BitVector mosiLanes),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the data inputs and outputs of spiMaster are of type Vec mosiW (BitVector n) and Vec misoW (BitVector n) respectively, but MOSI is defined as BitVector mosiLanes and MISO as BitVector misoLanes.

The first thing that I think when I see this is: wait, a Vec is numbered with ascending inices (!!) and a BitVector is numbered with descending indices (!)! So... how do those indices map?

And even if they were the same, I'd still wonder about the mapping to HDL indices. Does din !! 0 map to MOSI[0] or MOSI[n] in my SDC file?

However, I think BitVector _ for MOSI/MISO does have an advantage: it doesn't give the annoying

[...]: warning:
    Make sure HDL port names are correct:
Backtracked when constructing Clash.Sized.Vector.Vec
(Type appears recursive)

I'm not really sure about the type. We should clearly document the mapping of Vec indices to pins on the top entity, and ideally they'd match, so din !! 0 does map to MOSI[0]. But whether MOSI should be BitVector or Vec _ Bit I'm not so sure about.

-- ^ Master-out, slave-in
spiMasterSck :: "SCK" ::: Signal dom Bit,
-- ^ Serial Clock
spiMasterSs :: "SS" ::: Signal dom Bit
-- ^ Slave select
}

type instance TryDomain t (SpiMasterOut dom misoLanes mosiLanes) = 'Found dom

-- | Input signals of an SPI master.
data SpiMasterIn (dom :: Domain) (misoLanes :: Nat) (mosiLanes :: Nat) =
SpiMasterIn {
spiMasterMiso :: "MISO" ::: Signal dom (BitVector misoLanes)
-- ^ Master-in, slave-out
}

type instance TryDomain t (SpiMasterIn dom misoLanes mosiLanes) = 'Found dom

-- | SPI mode
--
Expand Down Expand Up @@ -184,35 +235,29 @@ spiSlave
. (HiddenClockResetEnable dom, KnownNat n, 1 <= n)
=> SPISlaveConfig ds dom
-- ^ Configure SPI mode and tri-state buffer
-> Signal dom Bool
-- ^ Serial Clock (SCLK)
-> Signal dom Bit
-- ^ Master Output Slave Input (MOSI)
-> BiSignalIn ds dom 1
-- ^ Master Input Slave Output (MISO)
--
-- Inout port connected to the tri-state buffer for the MISO
-> Signal dom Bool
-- ^ Slave select (SS)
-> SpiSlaveIn ds dom 1 1
-- ^ SPI interface
-> Signal dom (BitVector n)
-- ^ Data to send from master to slave
-- ^ Data to send from slave to master.
--
-- Input is latched the moment slave select goes low
-> ( BiSignalOut ds dom 1
-> ( SpiSlaveOut ds dom 1 1
, Signal dom Bool
, Signal dom (Maybe (BitVector n)))
-- ^ Parts of the tuple:
--
-- 1. The "out" part of the inout port of the MISO; used only for simulation.
--
-- 2. (Maybe) the word send by the master
spiSlave (SPISlaveConfig mode latch buf) sclk mosi bin ss din =
-- 2. the acknowledgement for the data sent from the master to the slave.
--
-- 2. (Maybe) the word sent by the master
spiSlave (SPISlaveConfig mode latch buf) (SpiSlaveIn mosi bin sclk ss) din =
let ssL = if latch then delay undefined ss else ss
mosiL = if latch then delay undefined mosi else mosi
sclkL = if latch then delay undefined sclk else sclk
(miso, ack, dout) = spiCommon mode ssL mosiL sclkL din
bout = buf bin (not <$> ssL) miso
in (bout, ack, dout)
(miso, ack, dout) = spiCommon mode (bitToBool <$> ssL) (head . bv2v <$> mosiL) (bitToBool <$> sclkL) din
bout = buf bin (not . bitToBool <$> ssL) miso
in (SpiSlaveOut bout, ack, dout)

-- | SPI master configurable in the SPI mode and clock divider
--
Expand All @@ -237,11 +282,8 @@ spiMaster
-> Signal dom (Maybe (BitVector n))
-- ^ Data to send from master to slave, transmission starts when receiving
-- /Just/ a value
-> Signal dom Bit
-- ^ Master Input Slave Output (MISO)
-> ( Signal dom Bool -- SCK
, Signal dom Bit -- MOSI
, Signal dom Bool -- SS
-> SpiMasterIn dom 1 1
-> ( SpiMasterOut dom 1 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the Haddock for the output of this function?

, Signal dom Bool -- Busy
, Signal dom Bool -- Acknowledge
, Signal dom (Maybe (BitVector n)) -- Data: Slave -> Master
Expand All @@ -254,15 +296,15 @@ spiMaster
-- 4. Busy signal indicating that a transmission is in progress, new words on
-- the data line will be ignored when /True/
-- 5. (Maybe) the word send from the slave to the master
spiMaster mode fN fW din miso =
let (mosi, ack, dout) = spiCommon mode ssL misoL sclkL
spiMaster mode fN fW din (SpiMasterIn miso) =
let (mosi, ack, dout) = spiCommon mode ssL (head . bv2v <$> misoL) sclkL
(fromMaybe undefined# <$> din)
latch = snatToInteger fN /= 1
ssL = if latch then delay undefined ss else ss
misoL = if latch then delay undefined miso else miso
sclkL = if latch then delay undefined sclk else sclk
(ss, sclk, busy) = spiGen mode fN fW din
in (sclk, mosi, ss, busy, ack, dout)
in (SpiMasterOut (v2bv . singleton <$> mosi) (boolToBit <$> sclk) (boolToBit <$> ss), busy, ack, dout)

-- | Generate slave select and SCK
spiGen
Expand Down Expand Up @@ -339,21 +381,12 @@ spiSlaveLatticeSBIO
-- Clock: 2*SCK < Core
--
-- * Set to /False/ when core clock is twice as fast, or as fast, as the SCK
-> Signal dom Bool
-- ^ Serial Clock (SCLK)
-> Signal dom Bit
-- ^ Master Output Slave Input (MOSI)
-> BiSignalIn 'Floating dom 1
-- ^ Master Input Slave Output (MISO)
--
-- Inout port connected to the tri-state buffer for the MISO
-> Signal dom Bool
-- ^ Slave select (SS)
-> SpiSlaveIn 'Floating dom 1 1
-> Signal dom (BitVector n)
-- ^ Data to send from slave to master
--
-- Input is latched the moment slave select goes low
-> ( BiSignalOut 'Floating dom 1
-> ( SpiSlaveOut 'Floating dom 1 1
, Signal dom Bool
, Signal dom (Maybe (BitVector n)))
-- ^ Parts of the tuple:
Expand Down Expand Up @@ -385,21 +418,12 @@ spiSlaveLatticeBB
-- Clock: 2*SCK < Core
--
-- * Set to /False/ when core clock is twice as fast, or as fast, as the SCK
-> Signal dom Bool
-- ^ Serial Clock (SCLK)
-> Signal dom Bit
-- ^ Master Output Slave Input (MOSI)
-> BiSignalIn 'Floating dom 1
-- ^ Master Input Slave Output (MISO)
--
-- Inout port connected to the tri-state buffer for the MISO
-> Signal dom Bool
-- ^ Slave select (SS)
-> SpiSlaveIn 'Floating dom 1 1
-> Signal dom (BitVector n)
-- ^ Data to send from slave to master
--
-- Input is latched the moment slave select goes low
-> ( BiSignalOut 'Floating dom 1
-> ( SpiSlaveOut 'Floating dom 1 1
, Signal dom Bool
, Signal dom (Maybe (BitVector n)))
-- ^ Parts of the tuple:
Expand Down
8 changes: 4 additions & 4 deletions clash-cores/test/Test/Cores/Internal/SampleSPI.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,16 @@ sampleCycling genM genS divHalf wait mVals sVals mode latch duration =
samples = sampleN (getDuration duration) $ bundle (mOut, mAck, sOut, sAck)
slaveIn = genS clk rst sVals sAck

(misoZ, sAck, sOut) =
(SpiSlaveOut misoZ, sAck, sOut) =
withClockResetEnable clk rst enableGen
(spiSlaveLatticeSBIO mode latch sclk mosi miso ss slaveIn)
(spiSlaveLatticeSBIO mode latch (SpiSlaveIn mosi miso sclk ss) slaveIn)

miso = veryUnsafeToBiSignalIn misoZ
masterIn = genM clk rst mVals mAck bp

(sclk, mosi, ss, bp, mAck, mOut) =
(SpiMasterOut mosi sclk ss, bp, mAck, mOut) =
withClockResetEnable clk rst enableGen
(spiMaster mode divHalf wait masterIn (readFromBiSignal miso))
(spiMaster mode divHalf wait masterIn (SpiMasterIn $ readFromBiSignal miso))

clk = systemClockGen
rst = systemResetGen
Expand Down
30 changes: 16 additions & 14 deletions clash-cores/test/Test/Cores/SPI/MultiSlave.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,20 @@ slaveAddressRotate
. (KnownDomain dom, KnownNat n, 1 <= n)
=> Clock dom
-> Reset dom
-> (Signal dom Bool, Signal dom Bool)
-> Vec n (Signal dom Bool)
slaveAddressRotate clk rst =
E.mealyB clk rst enableGen
(\(cntQ, bQ) (ss, b) ->
-> (Signal dom Bit, Signal dom Bool)
-> Vec n (Signal dom Bit)
slaveAddressRotate clk rst (ss,bp) =
map (fmap boolToBit)
$ E.mealyB clk rst enableGen
(\(cntQ, bQ) (ss', b) ->
let bF = bQ && not b
cntD | bF = if cntQ == maxBound then 0 else cntQ + 1
| otherwise = cntQ

oH = (ss ||) <$> (unpack . complement $ bin2onehot cntQ)
oH = (ss' ||) <$> (unpack . complement $ bin2onehot cntQ)
in ((cntD, b), oH))
(0 :: Index n, False)
(bitToBool <$> ss, bp)
where
bin2onehot = setBit 0 . fromEnum

Expand All @@ -60,17 +62,17 @@ testMasterMultiSlave divHalf wait mVal sVal mode latch duration =
,(L.nub masterOutS,P.length masterOutS))
where
slaveIn = pure sVal
(misoZ0, _, slaveOut0) =
(SpiSlaveOut misoZ0, _, slaveOut0) =
withClockResetEnable clk rst enableGen
(spiSlaveLatticeSBIO mode latch sclk mosi miso ss0 slaveIn)
(spiSlaveLatticeSBIO mode latch (SpiSlaveIn mosi miso sclk ss0) slaveIn)

(misoZ1, _, slaveOut1) =
(SpiSlaveOut misoZ1, _, slaveOut1) =
withClockResetEnable clk rst enableGen
(spiSlaveLatticeSBIO mode latch sclk mosi miso ss1 slaveIn)
(spiSlaveLatticeSBIO mode latch (SpiSlaveIn mosi miso sclk ss1) slaveIn)

(misoZ2, _, slaveOut2) =
(SpiSlaveOut misoZ2, _, slaveOut2) =
withClockResetEnable clk rst enableGen
(spiSlaveLatticeSBIO mode latch sclk mosi miso ss2 slaveIn)
(spiSlaveLatticeSBIO mode latch (SpiSlaveIn mosi miso sclk ss2) slaveIn)

miso = veryUnsafeToBiSignalIn
(mergeBiSignalOuts (misoZ2 :> misoZ1 :> misoZ0 :> Nil))
Expand All @@ -79,9 +81,9 @@ testMasterMultiSlave divHalf wait mVal sVal mode latch duration =

(ss2 `Cons` ss1 `Cons` ss0 `Cons` _) = slaveAddressRotate @3 clk rst (ss,bp)

(sclk, mosi, ss, bp, masterAck, masterOut) =
(SpiMasterOut mosi sclk ss, bp, masterAck, masterOut) =
withClockResetEnable clk rst enableGen
(spiMaster mode divHalf wait masterIn (readFromBiSignal miso))
(spiMaster mode divHalf wait masterIn (SpiMasterIn $ readFromBiSignal miso))

clk = systemClockGen
rst = systemResetGen
Expand Down