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.

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

Move i2c to clash cores #2584

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
12 changes: 12 additions & 0 deletions clash-cores/clash-cores.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,21 @@ common basic-config
QuickCheck,
string-interpolate ^>= 0.3,
template-haskell,
transformers,

library
import: basic-config
hs-source-dirs: src

exposed-modules:
Clash.Cores.Experimental.I2C
Clash.Cores.Experimental.I2C.BitMaster
Clash.Cores.Experimental.I2C.BitMaster.BusCtrl
Clash.Cores.Experimental.I2C.BitMaster.StateMachine
Clash.Cores.Experimental.I2C.ByteMaster
Clash.Cores.Experimental.I2C.ByteMaster.ShiftRegister
Clash.Cores.Experimental.I2C.Types

Clash.Cores.LatticeSemi.ECP5.Blackboxes.IO
Clash.Cores.LatticeSemi.ECP5.IO
Clash.Cores.LatticeSemi.ICE40.Blackboxes.IO
Expand Down Expand Up @@ -188,6 +197,9 @@ test-suite unittests
buildable: False

other-Modules:
Test.Cores.Experimental.I2C
Test.Cores.Experimental.I2C.Config
Test.Cores.Experimental.I2C.Slave
Test.Cores.Internal.SampleSPI
Test.Cores.Internal.Signals
Test.Cores.SPI
Expand Down
87 changes: 87 additions & 0 deletions clash-cores/src/Clash/Cores/Experimental/I2C.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
{-|
Copyright : (C) 2014, University of Twente
2024, Google LLC
License : BSD2 (see the file LICENSE)
Maintainer : QBayLogic B.V. <devops@qbaylogic.com>

I2C core
-}

{-# LANGUAGE CPP #-}

module Clash.Cores.Experimental.I2C
( i2c
, Clash.Cores.Experimental.I2C.ByteMaster.I2COperation(..)
) where

import Clash.Prelude hiding (read)

import Clash.Cores.Experimental.I2C.BitMaster
import Clash.Cores.Experimental.I2C.ByteMaster

-- | Core for I2C communication. Returns the output enable signals for SCL en SDA
-- These signals assume that when they are `high`, they pull down SCL and SDA respectively.
-- For 2-wire I2C, you can use BiSignals (`Clash.Signal.Bidirectional.BiSignalIn` and `Clash.Signal.Bidirectional.BiSignalOut`)
--
-- === __Example__
--
-- @
-- 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
-- @
Comment on lines +22 to +36
Copy link
Member

Choose a reason for hiding this comment

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

The outputs for SCL and SDA are Maybe Bit, neither the haddock comment, nor the code example seem to get this rights.

Copy link
Member

Choose a reason for hiding this comment

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

(I did not have a chance to actually look at this code and the changes; and I don't have time anymore to do so now. So I'm making a bunch of assumptions and next week I'll see if I was off base)

SCL and SDA are open-drain I/O pins: they are both input and output, and they are either driven low or high-impedance. I assume the Maybe Bit therefore has two states: Nothing (Hi-Z) and Just 0 (drive low). And I suppose this encoding was chosen because in an FPGA, that's how you would drive the tri-state used to implement an open-drain pin on the FPGA. Specifically, the trivial way to implement open-drain with a tri-state is to have the input from the FPGA tied to ground and put your data on the gate: gated when high, outputting when low.

So this Maybe Bit is purely pragmatic: it encodes how you drive a tristate, but it only has two states. It's also not entirely practical; we have two tri-state primitives, one for the ICE40 and one for the ECP5, and one of those uses an enable signal to gate, and the other a Signal Bool to gate; neither actually uses Maybe to gate.

So I'd like to propose we change the SCL and SDA outputs to just be Bit. And this would then encode the two possible states: 1 = Hi-Z, 0 = drive low, just like I²C defines it afaik.

And related but not for this PR, I'd like to harmonise our two tri-state primitives, and define wrappers that can be used to implement open-drain and open-source, where the data inputs to those wrappers are just Bit and they wrap the underlying tri-state primitives in the logical way (data input tied to rail, gate input tied to data, either inverted or direct). But that's for later; it just clarifies how I see the future for using this I²C core with I/O buffer primitives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this Maybe Bit is purely pragmatic: it encodes how you drive a tristate, but it only has two states

Indeed! It's to show that it drives a tristate.

It's also not entirely practical; we have two tri-state primitives, one for the ICE40 and one for the ECP5, and one of those uses an enable signal to gate, and the other a Signal Bool to gate; neither actually uses Maybe to gate.

I have never seen these primitives to be honest, the choice was made to be compatible with BiSignals. at least Vivado simply infers the tristate buffer when you use BiSignal. We don't use any explicit primitives in the Bittide implementations.

Personally I prefer the Maybe Bit over `Bit.

And related but not for this PR, I'd like to harmonise our two tri-state primitives, and define wrappers that can be used to implement open-drain and open-source, where the data inputs to those wrappers are just Bit and they wrap the underlying tri-state primitives in the logical way (data input tied to rail, gate input tied to data, either inverted or direct). But that's for later; it just clarifies how I see the future for using this I²C core with I/O buffer primitives.

Personally I don't find it intuitive to make the behavior depend on the outside world, whats the downside of using Maybe Bit to you?

Copy link
Member

@rowanG077 rowanG077 Apr 22, 2024

Choose a reason for hiding this comment

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

Maybe Bit can do the wrong thing. You could output Just 1 which is wrong for i2c. I would like to propose a third option. Have a unit data type newtype Low = Low or something like that and output Maybe Low. If you want you can even make a custom BitPack so that it still implemented as 2 bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed the core should never output Just 1, and it never does. I don't see a problem in using Maybe Bit as tristrate drive type for i2c. It intuitively hints to the user that its mean to be a tristate driver

Copy link
Member

Choose a reason for hiding this comment

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

at least Vivado simply infers the tristate buffer when you use BiSignal.

Oh that's good to know! I just immediately thought of those two primitives.

Thinking about it a bit more, a wrapper for a BiSignal is less pretty in API than I initially imagined it could be. But still I'm leaning towards it being the right way. It would also hide a bit of the complexity of BiSignal for this use.

We could add this to the Prelude:

openDrainIO :: BiSignalIn ds dom (BitSize a) -> Signal dom a -> (BiSignalOut ds dom (BitSize a), Signal dom a)
openSourceIO :: BiSignalIn ds dom (BitSize a) -> Signal dom a -> (BiSignalOut ds dom (BitSize a), Signal dom a)

and people wouldn't have to do all the readFromBiSignal and writeToBiSignal themselves.

Personally I don't find it intuitive to make the behavior depend on the outside world, whats the downside of using Maybe Bit to you?

It's semantically wrong. It is a two-valued signal, not a three-valued one. If you want to force people towards a specific implementation, you should perhaps create a new type OpenDrain or something like that. So what's above becomes:

openDrainIO :: BiSignalIn ds dom (BitSize a) -> Signal dom (OpenDrain a) -> (BiSignalOut ds dom (BitSize a), Signal dom a)

(that's my counter-suggestion to Rowan's Low. I think it is more obvious in its intention.)

It intuitively hints to the user that its mean to be a tristate driver

In an FPGA, that is the usual implementation, yes, since it's there anyway. But it's not supposed to be a tri-state driver in my view. It's supposed to be an open-drain driver. But in an FPGA, every I/O pin is connected to a tri-state driver, and that's what you use. A standard totem-pole output is a tri-state that is never Hi-Z, an open-drain output is a tri-state that is never driven high, and an open-source output is a tri-state that is never driven low. Three types of outputs, all of them having two states. Only the true tri-state output has three states.

In an ASIC, you would never use a tri-state to implement an open drain, it would be wastage of die space on a staggering scale, relatively speaking.

Copy link
Member

@rowanG077 rowanG077 Apr 22, 2024

Choose a reason for hiding this comment

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

Note that I would hint to shy away from BiSignal use in APIs currently because of #2168 and #1138

Copy link
Member

Choose a reason for hiding this comment

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

I don't intend to codify the BiSignal in the I²C stuff; people are free to choose their pin driver. But I think you either use platform-specific primitives or the BiSignal stuff. As I said, I don't mean to include these Prelude extensions as part of this PR, it's just how I envision it being used, which supports my argument in favour of just Bit.


i2c ::
forall dom .
KnownDomain dom =>
-- | Input Clock
"clk" ::: Clock dom ->
-- | Low level reset
"arst" ::: Reset dom ->
-- | Statemachine reset
"rst" ::: Signal dom Bool ->
-- | BitMaster enable
"ena" ::: Signal dom Bool ->
-- | Clock divider
"clkCnt" ::: Signal dom (Unsigned 16) ->
-- | 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. Received acknowledge signal from slave to master on write operations.
-- True means SDA is low.
-- 6. Outgoing I2C signals
-- 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)
, "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
(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 #-}
134 changes: 134 additions & 0 deletions clash-cores/src/Clash/Cores/Experimental/I2C/BitMaster.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
{-|
Copyright : (C) 2014, University of Twente
2024, Google LLC
License : BSD2 (see the file LICENSE)
Maintainer : QBayLogic B.V. <devops@qbaylogic.com>
-}

{-# LANGUAGE CPP #-}
{-# LANGUAGE RecordWildCards #-}
module Clash.Cores.Experimental.I2C.BitMaster
( bitMaster
, BitMasterI
, BitMasterO
) where

import Clash.Prelude

import Control.Lens
import Control.Monad
import Control.Monad.Trans.State
import Data.Tuple

import Clash.Cores.Experimental.I2C.BitMaster.BusCtrl
import Clash.Cores.Experimental.I2C.BitMaster.StateMachine
import Clash.Cores.Experimental.I2C.Types

-- | Internal state of the I2C BitMaster.
data BitMasterS
= BitS
{ _busState :: BusStatusCtrl -- ^ Manage overall status of the I2C bus.
, _stateMachine :: StateMachine -- ^ Handles the bit-level I2C operations
, _dout :: Bit -- ^ Data to be sent out on the I2C bus
, _dsclOen :: Bool -- ^ Delayed version of the SCL output enable signal
, _clkEn :: Bool -- ^ Enable the clock for the state machine
, _slaveWait :: Bool -- ^ Whether the slave is pulling the SCL line low, causing the master to wait
, _cnt :: Unsigned 16 -- ^ Counter used for clock division
}
deriving (Generic, NFDataX)

makeLenses ''BitMasterS


-- | 5-tuple containing the input interface for the BitMaster.
--
-- 1. Resets the internal state when asserted
-- 2. Enables or disables the BitMaster
-- 3. Used for clock division
-- 4. Carries command and data in signals
-- 5. Contains the SCL and SDA input signals
type BitMasterI = (Bool,Bool,Unsigned 16,BitCtrlSig,I2CIn)

-- | 3-tuple containing the output interface for the BitMaster.
--
-- 1. Carries command acknowledgment and other flags
-- 2. Indicates if the BitMaster is currently busy
-- 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
-> Reset dom
-> Enable dom
-> Unbundled dom BitMasterI
-> Unbundled dom BitMasterO
bitMaster = exposeClockResetEnable (mealyB bitMasterT bitMasterInit)
-- See: https://github.com/clash-lang/clash-compiler/pull/2511
{-# CLASH_OPAQUE bitMaster #-}

bitMasterInit :: BitMasterS
bitMasterInit = BitS { _stateMachine = stateMachineStart
, _busState = busStartState
, _dout = high
, _dsclOen = False
, _clkEn = True
, _slaveWait = False
, _cnt = 0
}


bitMasterT :: BitMasterS -> BitMasterI -> (BitMasterS, BitMasterO)
bitMasterT s@(BitS { _stateMachine = StateMachine {..}
, _busState = BusStatusCtrl {..}
, ..
})
(rst,ena,clkCnt,(cmd,din),i2cI@(_sclI,_sdaI)) =
swap $ flip runState s $ do
-- Whenever the slave is not ready it can delay the cycle by pulling SCL low
-- delay scloEn
dsclOen .= _sclOen

-- slaveWait is asserted when the master wants to drive SCL high, but the slave pulls it low
-- slaveWait remains asserted until the slave releases SCL
let masterSclHigh = _sclOen && not _dsclOen
(sSCL,sSDA) = _sI2C
slaveWait .= ((masterSclHigh || _slaveWait) && sSCL == 0)

-- master drives SCL high, but another master pulls it low
-- master start counting down it low cycle now (clock synchronization)
let dSCL = fst _dI2C
sclSync = dSCL == high && sSCL == low && _sclOen

-- generate clk enable signal
if rst || _cnt == 0 || not ena || sclSync then do
cnt .= clkCnt
clkEn .= True
else if _slaveWait then do
clkEn .= False
else do
cnt -= 1
clkEn .= False

-- bus status controller
zoom busState (busStatusCtrl rst ena clkCnt cmd _clkEn i2cI _bitStateM _sdaChk _sdaOen)

-- generate dout signal, store dout on rising edge of SCL
when (sSCL == high && dSCL == low) $
dout .= sSDA

-- state machine
zoom stateMachine (bitStateMachine rst _al _clkEn cmd din)

-- assign outputs
let
i2cO = (if _sclOen then Nothing else Just 0, if _sdaOen then Nothing else Just 0)
outp = ((_cmdAck,_al,_dout),_busy,i2cO)

return outp
Loading