-
Notifications
You must be signed in to change notification settings - Fork 159
Move i2c to clash cores #2584
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
Open
lmbollen
wants to merge
14
commits into
master
Choose a base branch
from
move-i2c-to-clash-cores
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Move i2c to clash cores #2584
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
c35889b
Move I2C example files to `clash-cores`
lmbollen 9616fb2
Fix warnings in `i2c` core.
lmbollen 14af81e
Add comments to i2c core
lmbollen c4bb357
Make I2C core polymorphic in its domain
lmbollen 7bf0b2f
Remove HDL build test for I2C in examples
lmbollen 5e0368e
Refactor i2c core to be more user friendly
lmbollen a1ce13e
Add I2C unit test to `clash-cores:unittests`
lmbollen c13627b
Respect boolean values of acknowledgements. Make acknowledgements com…
hiddemoll 98e806c
Fix incorrect I2C test
hiddemoll 28fdb7a
Refactor I2C core and unittest
hiddemoll 0d2cbb7
Add debug flag to I2C unittest
hiddemoll ca4a097
Add module headers to all moved I2C modules
hiddemoll 7676ced
Extend haddock documentation for I2C core
hiddemoll 59cb3c7
Move I2C core to Experimental submodule
hiddemoll File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. <[email protected]> | ||
|
||
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 | ||
-- @ | ||
|
||
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
134
clash-cores/src/Clash/Cores/Experimental/I2C/BitMaster.hs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. <[email protected]> | ||
-} | ||
|
||
{-# 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 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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) andJust 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 aSignal Bool
to gate; neither actually usesMaybe
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! It's to show that it drives a tristate.
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.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?There was a problem hiding this comment.
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 outputJust 1
which is wrong for i2c. I would like to propose a third option. Have a unit data typenewtype Low = Low
or something like that and outputMaybe Low
. If you want you can even make a customBitPack
so that it still implemented as 2 bits.There was a problem hiding this comment.
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 usingMaybe Bit
as tristrate drive type for i2c. It intuitively hints to the user that its mean to be a tristate driverThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofBiSignal
for this use.We could add this to the Prelude:
and people wouldn't have to do all the
readFromBiSignal
andwriteToBiSignal
themselves.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:(that's my counter-suggestion to Rowan's
Low
. I think it is more obvious in its intention.)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.
There was a problem hiding this comment.
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 #1138There was a problem hiding this comment.
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 theBiSignal
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 justBit
.