Skip to content

Commit

Permalink
core: add ElasticBuffer to replace AsyncFIFO
Browse files Browse the repository at this point in the history
  • Loading branch information
enjoy-digital committed Oct 13, 2016
1 parent 1c2cf8e commit 1a88bf9
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 10 deletions.
1 change: 0 additions & 1 deletion TODO
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
- use elastic buffers between transport layer and links.
- use GTX's QPLL for linerate > 6.6Gbps.
- (later) add deterministic latency.
- (optional) test PRBS generators and use them (we are currently using PRBS generators from the transceivers).
47 changes: 38 additions & 9 deletions jesd204b/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

from migen import *
from migen.genlib.cdc import MultiReg
from migen.genlib.fifo import AsyncFIFO
from migen.genlib.resetsync import AsyncResetSynchronizer

from misoc.interconnect.csr import *
Expand All @@ -13,6 +12,36 @@
from jesd204b.link import JESD204BLinkTX


class ElasticBuffer(Module):

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Oct 13, 2016

Member

This should move to misoc.

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Oct 13, 2016

Member

Or rather migen

def __init__(self, data_width, depth, initial_delay=0):
self.din = Signal(data_width)
self.dout = Signal(data_width)

# # #

wrpointer = Signal(max=depth, reset=initial_delay)
rdpointer = Signal(max=depth)

storage = Memory(data_width, depth)
self.specials += storage

wrport = storage.get_port(write_capable=True, clock_domain="write")
rdport = storage.get_port(clock_domain="read")
self.specials += wrport, rdport

self.sync.write += wrpointer.eq(wrpointer + 1)
self.sync.read += rdpointer.eq(rdpointer + 1)

This comment has been minimized.

Copy link
@jordens

jordens Oct 13, 2016

Member

Hmm. Is this really the best behavior?

  • If the clocks stumble/align, they just wrap around each other.
  • And there is a significant chance that they end up being very close (buffer either empty or full) both at the same location and sometimes (depending on slight phase changes/jitter of the two clocks) seeing "new", and sometimse seeing "old" data.
  • I think you want to prevent the two pointers from "passing"/getting too close to each other..

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Oct 13, 2016

Member

This relies on proper reset behavior (e.g. one reset synchronizer for each domain, both driven by the same async reset). The margin between the write and read pointer should be enough to absorb jitter between the clocks, plus uncertainties regarding the cycle at which reset is released. If the clocks are derived from the same oscillator, which is how an elastic buffer is supposed to be used, the pointers won't accumulate any change in their difference.

This comment has been minimized.

Copy link
@jordens

jordens Oct 13, 2016

Member

Sure. That's what I was saying. Accumulation with both clocks running is not an issue. But with this design, independent resets will lead to wrapping/passing problems. And synchronized resets basically guarantee that they are hitting the same location which converts slight (sub-cycle) phase jitter/wander into uncertainties about the read/write order at that location.
If you synchronize the reset and then reset the pointers initial_delay=depth//2 (i would not make that optional) mutual separation, then you are fine.

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Oct 13, 2016

Member

Yes. Pushing the reset synchronizers into that module (at the expense of potentially making them redundant, but they are cheap) would enforce the reset constraint.

This comment has been minimized.

Copy link
@enjoy-digital

enjoy-digital Oct 13, 2016

Author Contributor

OK I'll do that and put that in migen. Where do you want it to be? migen.genlib.buffer is fine?

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Oct 13, 2016

Member

genlib/cdc

This comment has been minimized.

Copy link
@enjoy-digital

enjoy-digital Oct 13, 2016

Author Contributor

OK


self.comb += [
wrport.we.eq(1),
wrport.adr.eq(wrpointer),
wrport.dat_w.eq(self.din),

rdport.adr.eq(rdpointer),
self.dout.eq(rdport.dat_r)
]


class JESD204BCoreTX(Module):
def __init__(self, phys, jesd_settings, converter_data_width):
self.enable = Signal()
Expand All @@ -32,12 +61,14 @@ def __init__(self, phys, jesd_settings, converter_data_width):
# clocking
# phys
for n, phy in enumerate(phys):
cd_phy = ClockDomain("phy"+str(n))
self.clock_domains.cd_phy = ClockDomain("phy"+str(n))
self.comb += [
cd_phy.clk.eq(phy.gtx.cd_tx.clk),
cd_phy.rst.eq(phy.gtx.cd_tx.rst)
self.cd_phy.clk.eq(phy.gtx.cd_tx.clk),
self.cd_phy.rst.eq(phy.gtx.cd_tx.rst)
]
self.clock_domains += cd_phy
self.clock_domains.cd_ebuf = ClockDomain("ebuf"+str(n))
self.comb += self.cd_ebuf.clk.eq(self.cd_phy.clk)
self.specials += AsyncResetSynchronizer(self.cd_ebuf, ~ready)
# user
self.clock_domains.cd_user = ClockDomain()
self.comb += self.cd_user.clk.eq(ClockSignal("phy0"))
Expand Down Expand Up @@ -67,9 +98,9 @@ def __init__(self, phys, jesd_settings, converter_data_width):
# buffers
self.bufs = bufs = []
for n, phy in enumerate(phys):
buf = AsyncFIFO(len(phy.data), 8) # FIXME use elastic buffers
buf = ElasticBuffer(len(phy.data), 8, 4)

This comment has been minimized.

Copy link
@jordens

jordens Oct 13, 2016

Member

Why 8? Can't this be 2 or even better, 1?

buf = ClockDomainsRenamer(
{"write": "user", "read": "phy"+str(n)})(buf)
{"write": "user", "read": "ebuf"+str(n)})(buf)
bufs.append(buf)
self.submodules += buf

Expand All @@ -86,9 +117,7 @@ def __init__(self, phys, jesd_settings, converter_data_width):
# connect modules together
for n, (link, buf) in enumerate(zip(links, bufs)):
self.comb += [
buf.we.eq(1),
buf.din.eq(getattr(transport.source, "lane"+str(n))),
buf.re.eq(1),
link.sink.data.eq(buf.dout),
phys[n].data.eq(link.source.data),
phys[n].ctrl.eq(link.source.ctrl)
Expand Down

0 comments on commit 1a88bf9

Please sign in to comment.