Refactor i2c core to be more user friendly

Author: lmbollen

clash-cores/src/Clash/Cores/I2C.hs

@@ -8,7 +8,19 @@ import Clash.Cores.I2C.BitMaster
 import Clash.Cores.I2C.ByteMaster
 import Clash.Annotations.TH
 
--- | Core for I2C communication
+-- | Core for I2C communication. Returns the output enable signals for SCL en SDA
+-- These signals assume that when they are `True`, they pull down SCL and SDA respectively.
+-- For 2-wire I2C, you can use BiSignals (`Clash.Signal.Bidirectional.BiSignalIn` and `Clash.Signal.Bidirectional.BiSignalOut`)
+-- An example i2c design could look like this:
+-- i2cComp clk rst ena sclIn sdaIn = (sclOut, sdaOut)
+--  where
+--   sclOut = writeToBiSignal sclIn (mux sclOe (pure $ Just 0) (pure Nothing))
+--   sdaOut = writeToBiSignal sdaIn (mux sdaOe (pure $ Just 0) (pure Nothing))
+--   (sclOe, sdaOe) = unbundle i2cO
+--   i2cIn = bundle (readFromBiSignal sclIn, readFromBiSignal sdaIn)
+--   (dout,i2cOpAck,busy,al,ackWrite,i2cOut) = i2c clk arst rst ena clkCnt claimBus i2cOp ackRead i2cI
+--   ...
+
 i2c ::
   forall dom .
   KnownDomain dom =>
@@ -22,45 +34,61 @@ i2c ::
   "ena" ::: Signal dom Bool ->
   -- | Clock divider
   "clkCnt" ::: Signal dom (Unsigned 16) ->
-  -- | Start signal
-  "start" ::: Signal dom Bool ->
-  -- | Stop signal
-  "stop" ::: Signal dom Bool ->
-  -- | Read signal
-  "read" ::: Signal dom Bool ->
-  -- | Write signal
-  "write" ::: Signal dom Bool ->
-  -- | Ack signal
-  "ackIn" ::: Signal dom Bool ->
-  -- | Input data
-  "din" ::: Signal dom (BitVector 8) ->
+  -- | Claim bus signal
+  "claimBus" ::: Signal dom Bool ->
+  -- | I2C operation
+  "i2cOp" ::: Signal dom (Maybe I2COperation) ->
+  -- | Acknowledge signal to be transmitted from master to slave on read operations
+  --   True means SDA is low.
+  "ackRead" ::: Signal dom Bool ->
   -- | I2C input signals (SCL, SDA)
   "i2c" ::: Signal dom ("scl" ::: Bit, "sda" ::: Bit) ->
   -- |
   -- 1. Received data
   -- 2. Command acknowledgement
   -- 3. I2C bus busy
   -- 4. Arbitration lost
-  -- 5. I2C slave acknowledgement
+  -- 5. Received acknowledge signal from slave to master on write operations.
+  --    True means SDA is low.
   -- 6. Outgoing I2C signals
-  --    6.1 SCL
-  --    6.2 SCL Output enable`
-  --    6.3 SDA
-  --    6.4 SDA Output enable
+  --    6.1 SCL Tri-state signals, Nothing means pulled high.
+  --    6.2 SDA Tri-state signals, Nothing means pulled high.
   "" :::
     ( "i2cO" ::: Signal dom (BitVector 8)
-    , "scl" ::: Signal dom Bool
-    , "sclOEn" ::: Signal dom Bool
-    , "sda" ::: Signal dom Bool
-    , "sdaOEn" ::: Signal dom Bool
-    , "i2cO" ::: Signal dom ("scl" ::: Bit, "sclOEn" ::: Bool, "sda" ::: Bit, "sdaOEn" ::: Bool))
-i2c clk arst rst ena clkCnt start stop read write ackIn din i2cI = (dout,hostAck,busy,al,ackOut,i2cO)
+    , "i2cOpAck" ::: Signal dom Bool
+    , "busy" ::: Signal dom Bool
+    , "al" ::: Signal dom Bool
+    , "ackWrite" ::: Signal dom Bool
+    , "i2cO" ::: Signal dom ("sclOut" ::: Maybe Bit, "sclOut" ::: Maybe Bit))
+i2c clk arst rst ena clkCnt claimBus i2cOp ackRead i2cI =
+  (dout,i2cOpAck,busy,al,ackWrite,i2cO)
+
   where
-    (hostAck,ackOut,dout,bitCtrl) = byteMaster clk arst enableGen (rst,start,stop,read,write,ackIn,din,bitResp)
-    (bitResp,busy,i2cO)           = bitMaster  clk arst enableGen (rst,ena,clkCnt,bitCtrl,i2cI)
-    (_cmdAck,al,_dbout)           = unbundle bitResp
+    (i2cOpAck,ackWrite,dout,bitCtrl)
+      = byteMaster clk arst enableGen (rst,claimBus,i2cOp,ackRead,bitResp)
+    (bitResp,busy,i2cO)
+      = bitMaster  clk arst enableGen (rst,ena,clkCnt,bitCtrl,i2cI)
+    (_cmdAck,al,_dout) = unbundle bitResp
 -- See: https://github.com/clash-lang/clash-compiler/pull/2511
 {-# CLASH_OPAQUE i2c #-}
 
+i2cTop ::
+  "clk" ::: Clock System ->
+  "arst" ::: Reset System ->
+  "rst" ::: Signal System Bool ->
+  "ena" ::: Signal System Bool ->
+  "clkCnt" ::: Signal System (Unsigned 16) ->
+  "claimBus" ::: Signal System Bool ->
+  "i2cOp" ::: Signal System (Maybe I2COperation) ->
+  "ackRead" ::: Signal System Bool ->
+  "i2cI" ::: Signal System ("scl" ::: Bit, "sda" ::: Bit) ->
+  ""    :::
+    ( "i2cO" ::: Signal System (BitVector 8)
+    , "i2cOpAck" ::: Signal System Bool
+    , "busy" ::: Signal System Bool
+    , "al" ::: Signal System Bool
+    , "ackWrite" ::: Signal System Bool
+    , "i2cO" ::: Signal System ("sclOut" ::: Maybe Bit, "sdaOut" ::: Maybe Bit)
+    )
 i2cTop = i2c @System
 makeTopEntity 'i2cTop

clash-cores/src/Clash/Cores/I2C/BitMaster.hs

@@ -52,7 +52,11 @@ type BitMasterI = (Bool,Bool,Unsigned 16,BitCtrlSig,I2CIn)
 -- 3. Contains the SCL and SDA output signals
 type BitMasterO = (BitRespSig,Bool,I2COut)
 
-
+-- | Bit level I2C controller that contains a statemachine to properly:
+-- * Monitor the bus for activity and arbitration.
+-- * Read singular bits from the bus.
+-- * Write singular bits to the bus.
+-- * Return bits read from the bus.
 bitMaster
   :: KnownDomain dom
   => Clock dom
@@ -118,9 +122,8 @@ bitMasterT s@(BitS { _stateMachine = StateMachine  {..}
   zoom stateMachine (bitStateMachine rst _al _clkEn cmd din)
 
   -- assign outputs
-  let sclO = low
-      sdaO = low
-      i2cO = (sclO,_sclOen,sdaO,_sdaOen)
-      outp = ((_cmdAck,_al,_dout),_busy,i2cO)
+  let
+    i2cO =  (if _sclOen then Nothing else Just 0, if _sdaOen then Nothing else Just 0)
+    outp = ((_cmdAck,_al,_dout),_busy,i2cO)
 
   return outp

clash-cores/src/Clash/Cores/I2C/BitMaster/StateMachine.hs

@@ -21,8 +21,8 @@ data BitStateMachine
 -- | Defines the state machine with control and status registers.
 data StateMachine
   = StateMachine
-  { _sclOen    :: Bool            -- ^ Enables SCL output
-  , _sdaOen    :: Bool            -- ^ Enables SDA output
+  { _sclOen    :: Bool            -- ^ Inverted SCL output enable, False pulls the scl low.
+  , _sdaOen    :: Bool            -- ^ Inverted SDA output enable, False pulls the sda low.
   , _sdaChk    :: Bool            -- ^ Checks SDA status
   , _cmdAck    :: Bool            -- ^ Acknowledges command completion
   , _bitStateM :: BitStateMachine -- ^ Current state of the bit-level state machine

clash-cores/src/Clash/Cores/I2C/ByteMaster.hs

@@ -1,6 +1,6 @@
 {-# LANGUAGE CPP #-}
 {-# LANGUAGE RecordWildCards #-}
-module Clash.Cores.I2C.ByteMaster (byteMaster) where
+module Clash.Cores.I2C.ByteMaster (byteMaster, I2COperation(..)) where
 
 import Clash.Prelude hiding (read)
 
@@ -11,10 +11,16 @@ import Data.Tuple
 
 import Clash.Cores.I2C.ByteMaster.ShiftRegister
 import Clash.Cores.I2C.Types
+import Data.Maybe (fromJust)
 
-data ByteStateMachine = Idle | Start | Read | Write | Ack | Stop
+data ByteStateMachine = Idle | Active | Start | Read | Write | Ack | Stop
   deriving (Show, Generic, NFDataX)
 
+data I2COperation = ReadData | WriteData (BitVector 8)
+  deriving (Generic, NFDataX)
+getWriteData :: I2COperation -> BitVector 8
+getWriteData ReadData = 0
+getWriteData (WriteData d) = d
 data ByteMasterS
   = ByteS
   { _srState    :: ShiftRegister
@@ -23,27 +29,26 @@ data ByteMasterS
   , _coreTxd    :: Bit              -- coreTxd register
   , _shiftsr    :: Bool             -- shift sr
   , _ld         :: Bool             -- load values in to sr
-  , _hostAck    :: Bool             -- host cmd acknowlegde register
-  , _ackOut     :: Bool             -- slave ack register
+  , _i2cOpAck    :: Bool             -- host cmd acknowlegde register
+  , _slaveAck     :: Bool             -- slave ack register
   }
   deriving (Generic, NFDataX)
 
 makeLenses ''ByteMasterS
 
 -- |
 -- 1. Statemachine reset
--- 2. Start
--- 3. Stop
--- 4. Read
--- 5. Write
--- 6. Acknowledge
--- 7. Data in
--- 8. Bitmaster response
-type ByteMasterI = (Bool,Bool,Bool,Bool,Bool,Bool,BitVector 8,BitRespSig)
+-- 2. Claim bus
+-- 3. Bus operation
+-- 4. Acknowledge signal to be transmitted from master to slave on read operations.
+--    True means SDA is low.
+-- 5. Bitmaster response
+type ByteMasterI = (Bool,Bool,Maybe I2COperation,Bool,BitRespSig)
 
 -- |
--- 1. Acknowledge for I2C controller
--- 2. I2C acknowledgement
+-- 1. Acknowledge for `I2COperation`
+-- 2. Received acknowledge signal from slave to master on write operations. True
+--    means SDA is low.
 -- 3. Data output
 -- 4  Bitmaster control signals
 type ByteMasterO = (Bool,Bool,BitVector 8,BitCtrlSig)
@@ -73,90 +78,94 @@ byteMasterInit
   , _coreTxd    = low
   , _shiftsr    = False
   , _ld         = False
-  , _hostAck    = False
-  , _ackOut     = True
+  , _i2cOpAck   = False
+  , _slaveAck   = True
   }
 
 byteMasterT :: ByteMasterS -> ByteMasterI -> (ByteMasterS, ByteMasterO)
 byteMasterT s@(ByteS {_srState = ShiftRegister {..}, ..})
-            (rst,start,stop,read,write,ackIn,din,~(coreAck,al,coreRxd)) = swap $ flip runState s $ do
-      -- generate go-signal
-  let go = (read || write || stop) && (not _hostAck)
+            (rst,claimBus,maybeI2COp,ackIn,~(coreAck,al,coreRxd)) = swap $ flip runState s $ do
 
       -- assign dOut the output of the shift-register
-      dout = _sr
+  let dout = _sr
 
-  cntDone <- zoom srState (shiftRegister rst _ld _shiftsr (bv2v din) coreRxd)
+  cntDone <- zoom srState (shiftRegister rst _ld _shiftsr (bv2v (getWriteData $ fromJust maybeI2COp )) coreRxd)
 
   -- state machine
   coreTxd .= head dout
   shiftsr .= False
   ld      .= False
-  hostAck .= False
+  i2cOpAck .= False
 
   if rst || al then do
     coreCmd    .= I2Cnop
     coreTxd    .= low
     byteStateM .= Idle
-    ackOut     .= True
-  else case _byteStateM of
-    Idle -> when go $ do
+    slaveAck     .= True
+  else case (_byteStateM, maybeI2COp) of
+    (Idle, _) -> when claimBus $ do
       ld .= True
-      if start then do
-        byteStateM .= Start
-        coreCmd    .= I2Cstart
-      else if read then do
-        byteStateM .= Read
-        coreCmd    .= I2Cread
-      else if write then do
-        byteStateM .= Write
-        coreCmd    .= I2Cwrite
-      else do-- stop
-        byteStateM .= Stop
-        coreCmd    .= I2Cstop
-    Start -> when coreAck $ do
+      byteStateM .= Start
+      coreCmd    .= I2Cstart
+    (Active, Just ReadData) -> do
+      byteStateM .= Read
+      coreCmd    .= I2Cread
+    (Active, Just (WriteData _)) -> do
       ld .= True
-      if read then do
-        byteStateM .= Read
-        coreCmd    .= I2Cread
-      else do
-        byteStateM .= Write
-        coreCmd    .= I2Cwrite
-    Write -> when coreAck $ do
+      byteStateM .= Write
+      coreCmd    .= I2Cwrite
+    (Active ,Nothing) -> do
+      byteStateM .= Active
+      coreCmd    .= I2Cnop
+    (Start, Nothing) -> when coreAck $ do
+      byteStateM .= Active
+      coreCmd    .= I2Cnop
+    (Start, Just ReadData) -> when coreAck $ do
+      byteStateM .= Read
+      coreCmd    .= I2Cread
+    (Start, Just (WriteData _)) -> when coreAck $ do
+      ld .= True
+      byteStateM .= Write
+      coreCmd    .= I2Cwrite
+    (Write, _) -> when coreAck $ do
       if cntDone then do
         byteStateM .= Ack
         coreCmd    .= I2Cread
       else do
         coreCmd    .= I2Cwrite
         shiftsr    .= True
-    Read -> when coreAck $ do
+
+    (Read, _) -> when coreAck $ do
       shiftsr .= True
       coreTxd .= bitCoerce ackIn
       if cntDone then do
         byteStateM .= Ack
         coreCmd    .= I2Cwrite
       else do
         coreCmd    .= I2Cread
-    Ack -> if coreAck then do
-        ackOut  .= bitCoerce coreRxd
+
+    (Ack, _) ->
+      if coreAck then do
+        slaveAck  .= bitCoerce coreRxd
         coreTxd .= high
         -- check for stop; Should a STOP command be generated?
-        if stop then do
-          byteStateM .= Stop
-          coreCmd    .= I2Cstop
-        else do
-          byteStateM .= Idle
+        if claimBus then do
+          byteStateM .= Active
           coreCmd    .= I2Cnop
           -- generate command acknowledge signal
-          hostAck    .= True
+          i2cOpAck    .= True
+        else do
+          byteStateM .= Stop
+          coreCmd    .= I2Cstop
       else
         coreTxd .= bitCoerce ackIn
-    Stop -> when coreAck $ do
+
+    (Stop, _) -> when coreAck $ do
       byteStateM .= Idle
       coreCmd    .= I2Cnop
-      hostAck    .= True
+      i2cOpAck   .= True
 
   let bitCtrl = (_coreCmd,_coreTxd)
-      outp    = (_hostAck,_ackOut,v2bv dout,bitCtrl)
+      outp    = (_i2cOpAck,_slaveAck,v2bv dout,bitCtrl)
 
   return outp

clash-cores/src/Clash/Cores/I2C/Types.hs

@@ -15,5 +15,6 @@ type BitRespSig = (Bool, Bool, Bit)
 -- | I2C input signals (SCL, SDA).
 type I2CIn = (Bit, Bit)
 
--- | I2C output signals (SCL, SCL enable, SDA, SDA enable).
-type I2COut = (Bit, Bool, Bit, Bool)
+-- | I2C output Tri-state signals (SCL, SDA)
+-- Since I2C is a protocol with pull ups, Nothing means pulled high.
+type I2COut = (Maybe Bit, Maybe Bit)

clash-cores/test/Test/Cores/I2C.hs

@@ -7,24 +7,29 @@ import qualified Data.List as L
 import Clash.Explicit.Prelude
 import Clash.Cores.I2C
 
+import Data.Maybe
 import Test.Cores.I2C.Slave
 import Test.Cores.I2C.Config
+import Clash.Cores.I2C.ByteMaster (I2COperation(..))
 
 system0 :: Clock System -> Reset System -> Signal System (Vec 16 (Unsigned 8), Bool, Bool)
 system0 clk arst = bundle (registerFile,done,fault)
  where
   (_dout,hostAck,_busy,al,ackOut,i2cO) =
-    i2c clk arst rst (pure True) (pure 19) start stop (pure False) write (pure True) din i2cI
+    i2c clk arst rst (pure True) (pure 19) claim i2cOp (pure True) i2cI
 
-  (start,stop,write,din,done,fault) = unbundle $
+  i2cOp = mux claim (Just <$> mux write (WriteData <$> din) (pure ReadData)) (pure Nothing)
+
+  (claim,write,din,done,fault) = unbundle $
     config clk (bundle (rst, fmap not rst,hostAck,ackOut,al))
 
-  (_,sclOen,_,sdaOen) = unbundle i2cO
-  scl  = fmap bitCoerce sclOen
+  (sclOut,sdaOut) = unbundle i2cO
+  scl  = fmap (bitCoerce . isNothing) sclOut
+  sda  = fmap (bitCoerce . isNothing) sdaOut
   i2cI = bundle (scl,sdaS)
 
   (sdaS,registerFile) = unbundle
-    (i2cSlave clk (bundle (scl, bitCoerce <$> sdaOen)))
+    (i2cSlave clk (bundle (scl, sda)))
 
   rst = liftA2 (<) rstCounter 500
   rstCounter = register clk arst enableGen (0 :: Unsigned 18) (rstCounter + 1)