From 8af1873dcdaf55ed78fa2ca6f0580a7097b740f4 Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Mon, 4 Mar 2019 20:31:44 +0000 Subject: [PATCH 1/9] lib.cdc: Add docstrings and basic parameter checking to MultiReg and ResetSynchroniser. --- nmigen/lib/cdc.py | 22 ++++++++++++++++++++++ nmigen/test/test_lib_cdc.py | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index e68b9cb5..39597dc0 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -50,6 +50,8 @@ class MultiReg(Elaboratable): MultiReg is reset by the ``odomain`` reset only. """ def __init__(self, i, o, odomain="sync", n=2, reset=0, reset_less=True): + if not isinstance(n, int) or n < 1: + raise TypeError("n must be a positive integer, not '{!r}'".format(n)) self.i = i self.o = o self.odomain = odomain @@ -70,7 +72,27 @@ def elaborate(self, platform): class ResetSynchronizer(Elaboratable): + """Synchronize the deassertion of a reset to a local clock. + + Output `assertion` is asynchronous, so the local clock need not be free-running. + + Parameters + ---------- + arst : Signal(1), out + Asynchronous reset signal, to be synchronized. + domain : str + Name of domain to synchronize reset to. + n : int, >=1 + Number of clock edges from input deassertion to output deassertion + + Override + -------- + Define the ``get_reset_sync`` platform attribute to override the implementation of + ResetSynchronizer, e.g. to instantiate library cells directly. + """ def __init__(self, arst, domain="sync", n=2): + if not isinstance(n, int) or n < 1: + raise TypeError("n must be a positive integer, not '{!r}'".format(n)) self.arst = arst self.domain = domain diff --git a/nmigen/test/test_lib_cdc.py b/nmigen/test/test_lib_cdc.py index 9aea5d42..324c6bf9 100644 --- a/nmigen/test/test_lib_cdc.py +++ b/nmigen/test/test_lib_cdc.py @@ -5,6 +5,20 @@ class MultiRegTestCase(FHDLTestCase): + def test_paramcheck(self): + i = Signal() + o = Signal() + with self.assertRaises(TypeError): + m = MultiReg(i, o, n=0) + with self.assertRaises(TypeError): + m = MultiReg(i, o, n="x") + with self.assertRaises(ValueError): + m = MultiReg(i, o, n=2, reset="a") + with self.assertRaises(TypeError): + m = MultiReg(i, o, n=2, reset=i) + m = MultiReg(i, o, n=1) + m = MultiReg(i, o, reset=-1) + def test_basic(self): i = Signal() o = Signal() @@ -43,6 +57,14 @@ def process(): class ResetSynchronizerTestCase(FHDLTestCase): + def test_paramcheck(self): + arst = Signal() + with self.assertRaises(TypeError): + r = ResetSynchronizer(arst, n=0) + with self.assertRaises(TypeError): + r = ResetSynchronizer(arst, n="a") + r = ResetSynchronizer(arst) + def test_basic(self): arst = Signal() m = Module() From 4608d9fef4cafeb856cfeeb30d3742e87d308378 Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Mon, 4 Mar 2019 20:38:18 +0000 Subject: [PATCH 2/9] lib.cdc: Add PulseSynchronizer and Gearbox modules, and smoke tests for these --- nmigen/lib/cdc.py | 135 +++++++++++++++++++++++++++++++++++- nmigen/test/test_lib_cdc.py | 79 +++++++++++++++++++++ 2 files changed, 213 insertions(+), 1 deletion(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 39597dc0..6c43d5ae 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -1,7 +1,15 @@ from .. import * +from math import gcd -__all__ = ["MultiReg", "ResetSynchronizer"] +__all__ = ["MultiReg", "ResetSynchronizer", "PulseSynchronizer", "Gearbox"] + + +def _incr(signal, modulo): + if modulo == 2 ** len(signal): + return signal + 1 + else: + return Mux(signal == modulo - 1, 0, signal + 1) class MultiReg(Elaboratable): @@ -114,3 +122,128 @@ def elaborate(self, platform): ResetSignal(self.domain).eq(self._regs[-1]) ] return m + + +class PulseSynchronizer(Elaboratable): + """A one-clock pulse on the input produces a one-clock pulse on the output. + + If the output clock is faster than the input clock, then the input may be safely asserted at + 100% duty cycle. Otherwise, if the clock ratio is n : 1, the input may be asserted at most once + in every n input clocks, else pulses may be dropped. + + Other than this there is no constraint on the ratio of input and output clock frequency. + + Parameters + ---------- + + idomain : str + Name of input clock domain. + odomain : str + Name of output clock domain. + sync_stages : int + Number of synchronisation flops between the two clock domains. 2 is the default, and + minimum safe value. High-frequency designs may choose to increase this. + """ + def __init__(self, idomain, odomain, sync_stages=2): + if not isinstance(sync_stages, int) or sync_stages < 1: + raise TypeError("sync_stages must be a positive integer, not '{!r}'".format(sync_stages)) + + self.i = Signal() + self.o = Signal() + self.idomain = idomain + self.odomain = odomain + self.sync_stages = sync_stages + + def elaborate(self, platform): + m = Module() + + itoggle = Signal() + otoggle = Signal() + mreg = m.submodules.mreg = \ + MultiReg(itoggle, otoggle, odomain=self.odomain, n=self.sync_stages) + otoggle_prev = Signal() + + m.d[self.idomain] += itoggle.eq(itoggle ^ self.i) + m.d[self.odomain] += otoggle_prev.eq(otoggle) + m.d.comb += self.o.eq(otoggle ^ otoggle_prev) + + return m + + +class Gearbox(Elaboratable): + """Adapt the width of a continous datastream. + + Input: m bits wide, clock frequency f MHz. + Output: n bits wide, clock frequency m / n * f MHz. + + Used to adjust width of a datastream when interfacing system logic to a SerDes. The input and + output clocks must be derived from the same reference clock, to maintain distance between + read and write pointers. + + Parameters + ---------- + iwidth : int + Bit width of the input + idomain : str + Name of input clock domain + owidth : int + Bit width of the output + odomain : str + Name of output clock domain + + Attributes + ---------- + i : Signal(iwidth), in + Input datastream. Sampled on every input clock. + o : Signal(owidth), out + Output datastream. Transitions on every output clock. + """ + def __init__(self, iwidth, idomain, owidth, odomain): + if not isinstance(iwidth, int) or iwidth < 1: + raise TypeError("iwidth must be a positive integer, not '{!r}'".format(iwidth)) + if not isinstance(owidth, int) or owidth < 1: + raise TypeError("owidth must be a positive integer, not '{!r}'".format(owidth)) + + self.i = Signal(iwidth) + self.o = Signal(owidth) + self.iwidth = iwidth + self.idomain = idomain + self.owidth = owidth + self.odomain = odomain + + storagesize = iwidth * owidth // gcd(iwidth, owidth) + while storagesize // iwidth < 4: + storagesize *= 2 + while storagesize // owidth < 4: + storagesize *= 2 + + self._storagesize = storagesize + self._ichunks = storagesize // self.iwidth + self._ochunks = storagesize // self.owidth + assert(self._ichunks * self.iwidth == storagesize) + assert(self._ochunks * self.owidth == storagesize) + + def elaborate(self, platform): + m = Module() + + storage = Signal(self._storagesize, attrs={"no_retiming": True}) + i_faster = self._ichunks > self._ochunks + iptr = Signal(max=self._ichunks - 1, reset=(self._ichunks // 2 if i_faster else 0)) + optr = Signal(max=self._ochunks - 1, reset=(0 if i_faster else self._ochunks // 2)) + + m.d[self.idomain] += iptr.eq(_incr(iptr, self._storagesize)) + m.d[self.odomain] += optr.eq(_incr(optr, self._storagesize)) + + with m.Switch(iptr): + for n in range(self._ichunks): + s = slice(n * self.iwidth, (n + 1) * self.iwidth) + with m.Case(n): + m.d[self.idomain] += storage[s].eq(self.i) + + with m.Switch(optr): + for n in range(self._ochunks): + s = slice(n * self.owidth, (n + 1) * self.owidth) + with m.Case(n): + m.d[self.odomain] += self.o.eq(storage[s]) + + return m diff --git a/nmigen/test/test_lib_cdc.py b/nmigen/test/test_lib_cdc.py index 324c6bf9..45bfee4e 100644 --- a/nmigen/test/test_lib_cdc.py +++ b/nmigen/test/test_lib_cdc.py @@ -101,3 +101,82 @@ def process(): yield Tick(); yield Delay(1e-8) sim.add_process(process) sim.run() + + +# TODO: test with distinct clocks +class PulseSynchronizerTestCase(FHDLTestCase): + def test_paramcheck(self): + with self.assertRaises(TypeError): + ps = PulseSynchronizer("w", "r", sync_stages=0) + with self.assertRaises(TypeError): + ps = PulseSynchronizer("w", "r", sync_stages="abc") + ps = PulseSynchronizer("w", "r", sync_stages = 1) + + def test_smoke(self): + m = Module() + m.domains += ClockDomain("sync") + ps = m.submodules.dut = PulseSynchronizer("sync", "sync") + + with Simulator(m, vcd_file = open("test.vcd", "w")) as sim: + sim.add_clock(1e-6) + def process(): + yield ps.i.eq(0) + # TODO: think about reset + for n in range(5): + yield Tick() + # Make sure no pulses are generated in quiescent state + for n in range(3): + yield Tick() + self.assertEqual((yield ps.o), 0) + # Check conservation of pulses + accum = 0 + for n in range(10): + yield ps.i.eq(1 if n < 4 else 0) + yield Tick() + accum += yield ps.o + self.assertEqual(accum, 4) + sim.add_process(process) + sim.run() + + +# TODO: test with distinct clocks +# (since we can currently only test symmetric aspect ratio) +class GearboxTestCase(FHDLTestCase): + def test_paramcheck(self): + with self.assertRaises(TypeError): + g = Gearbox(0, "i", 1, "o") + with self.assertRaises(TypeError): + g = Gearbox(1, "i", 0, "o") + with self.assertRaises(TypeError): + g = Gearbox("x", "i", 1, "o") + with self.assertRaises(TypeError): + g = Gearbox(1, "i", "x", "o") + g = Gearbox(1, "i", 1, "o") + g = Gearbox(7, "i", 1, "o") + g = Gearbox(7, "i", 3, "o") + g = Gearbox(7, "i", 7, "o") + g = Gearbox(3, "i", 7, "o") + + def test_smoke_symmetric(self): + m = Module() + m.domains += ClockDomain("sync") + g = m.submodules.dut = Gearbox(8, "sync", 8, "sync") + + with Simulator(m, vcd_file = open("test.vcd", "w")) as sim: + sim.add_clock(1e-6) + def process(): + pipeline_filled = False + expected_out = 1 + yield Tick() + for i in range(g._ichunks * 4): + yield g.i.eq(i) + if (yield g.o): + pipeline_filled = True + if pipeline_filled: + self.assertEqual((yield g.o), expected_out) + expected_out += 1 + yield Tick() + self.assertEqual(pipeline_filled, True) + self.assertEqual(expected_out > g._ichunks * 2, True) + sim.add_process(process) + sim.run() \ No newline at end of file From 41ca5ac563f9da691d1fc28ff2916e89f093d25c Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Mon, 4 Mar 2019 21:33:24 +0000 Subject: [PATCH 3/9] test_lib_cdc: test platform dependency injection --- nmigen/test/test_lib_cdc.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/nmigen/test/test_lib_cdc.py b/nmigen/test/test_lib_cdc.py index 45bfee4e..815e8d0d 100644 --- a/nmigen/test/test_lib_cdc.py +++ b/nmigen/test/test_lib_cdc.py @@ -19,6 +19,14 @@ def test_paramcheck(self): m = MultiReg(i, o, n=1) m = MultiReg(i, o, reset=-1) + def test_platform(self): + platform = lambda: None + platform.get_multi_reg = lambda m: "foobar{}".format(len(m._regs)) + i = Signal() + o = Signal() + m = MultiReg(i, o, n=5) + self.assertEqual(m.elaborate(platform), "foobar5") + def test_basic(self): i = Signal() o = Signal() @@ -65,6 +73,13 @@ def test_paramcheck(self): r = ResetSynchronizer(arst, n="a") r = ResetSynchronizer(arst) + def test_platform(self): + platform = lambda: None + platform.get_reset_sync = lambda m: "foobar{}".format(len(m._regs)) + arst = Signal() + rs = ResetSynchronizer(arst, n=6) + self.assertEqual(rs.elaborate(platform), "foobar6") + def test_basic(self): arst = Signal() m = Module() From e76c187b28d3e8853a4546b4ab7acd90aa308f41 Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Tue, 5 Mar 2019 08:52:20 +0000 Subject: [PATCH 4/9] lib.cdc: Implement BusSynchronizer and add smoke tests --- nmigen/lib/cdc.py | 95 ++++++++++++++++++++++++++++++++++++- nmigen/test/test_lib_cdc.py | 46 ++++++++++++++++++ 2 files changed, 139 insertions(+), 2 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 6c43d5ae..75d309a7 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -2,7 +2,13 @@ from math import gcd -__all__ = ["MultiReg", "ResetSynchronizer", "PulseSynchronizer", "Gearbox"] +__all__ = [ + "MultiReg", + "ResetSynchronizer", + "PulseSynchronizer", + "BusSynchronizer", + "Gearbox" +] def _incr(signal, modulo): @@ -135,7 +141,6 @@ class PulseSynchronizer(Elaboratable): Parameters ---------- - idomain : str Name of input clock domain. odomain : str @@ -169,6 +174,92 @@ def elaborate(self, platform): return m +class BusSynchronizer(Elaboratable): + """Pass a multi-bit signal safely between clock domains. + + Ensures that all bits presented at ``o`` form a single word that was present synchronously at + ``i`` in the input clock domain (unlike direct use of MultiReg). + + Parameters + ---------- + width : int > 0 + Width of the bus to be synchronized + idomain : str + Name of input clock domain + odomain : str + Name of output clock domain + sync_stages : int >= 2 + Number of synchronisation stages used in the req/ack pulse synchronizers. Lower than 2 is + unsafe. Higher values increase safety for high-frequency designs, but increase latency too. + timeout : int >= 0 + The request from idomain is re-sent if ``timeout`` cycles elapse without a response. + ``timeout`` = 0 disables this feature. + + Attributes + ---------- + i : Signal(width), in + Input signal, sourced from ``idomain`` + o : Signal(width), out + Resynchronized version of ``i``, driven to ``odomain`` + """ + def __init__(self, width, idomain, odomain, sync_stages=2, timeout = 127): + if not isinstance(width, int) or width < 1: + raise TypeError("width must be a positive integer, not '{!r}'".format(width)) + if not isinstance(sync_stages, int) or sync_stages < 2: + raise TypeError("sync_stages must be an integer > 1, not '{!r}'".format(sync_stages)) + if not isinstance(timeout, int) or timeout < 0: + raise TypeError("timeout must be a non-negative integer, not '{!r}'".format(timeout)) + + self.i = Signal(width) + self.o = Signal(width, attrs={"no_retiming": True}) + self.width = width + self.idomain = idomain + self.odomain = odomain + self.sync_stages = sync_stages + self.timeout = timeout + + def elaborate(self, platform): + m = Module() + if self.width == 1: + m.submodules += MultiReg(self.i, self.o, odomain=self.odomain, n=self.sync_stages) + return m + + req = Signal() + ack_o = Signal() + ack_i = Signal() + + sync_io = m.submodules.sync_io = \ + PulseSynchronizer(self.idomain, self.odomain, self.sync_stages) + sync_oi = m.submodules.sync_oi = \ + PulseSynchronizer(self.odomain, self.idomain, self.sync_stages) + + if self.timeout != 0: + countdown = Signal(max=self.timeout, reset=self.timeout) + with m.If(ack_i | req): + m.d[self.idomain] += countdown.eq(self.timeout) + with m.Else(): + m.d[self.idomain] += countdown.eq(countdown - countdown.bool()) + + start = Signal(reset=1) + m.d[self.idomain] += start.eq(0) + m.d.comb += [ + req.eq(start | ack_i | (self.timeout != 0 and countdown == 0)), + sync_io.i.eq(req), + ack_o.eq(sync_io.o), + sync_oi.i.eq(ack_o), + ack_i.eq(sync_oi.o) + ] + + buf_i = Signal(self.width, attrs={"no_retiming": True}) + buf_o = Signal(self.width) + with m.If(ack_i): + m.d[self.idomain] += buf_i.eq(self.i) + sync_data = m.submodules.sync_data = \ + MultiReg(buf_i, buf_o, odomain=self.odomain, n=self.sync_stages - 1) + with m.If(ack_o): + m.d[self.odomain] += self.o.eq(buf_o) + + return m class Gearbox(Elaboratable): """Adapt the width of a continous datastream. diff --git a/nmigen/test/test_lib_cdc.py b/nmigen/test/test_lib_cdc.py index 815e8d0d..25f79670 100644 --- a/nmigen/test/test_lib_cdc.py +++ b/nmigen/test/test_lib_cdc.py @@ -153,6 +153,52 @@ def process(): sim.add_process(process) sim.run() +class BusSynchronizerTestCase(FHDLTestCase): + def test_paramcheck(self): + with self.assertRaises(TypeError): + bs = BusSynchronizer(0, "i", "o") + with self.assertRaises(TypeError): + bs = BusSynchronizer("x", "i", "o") + + bs = BusSynchronizer(1, "i", "o") + + with self.assertRaises(TypeError): + bs = BusSynchronizer(1, "i", "o", sync_stages = 1) + with self.assertRaises(TypeError): + bs = BusSynchronizer(1, "i", "o", sync_stages = "a") + with self.assertRaises(TypeError): + bs = BusSynchronizer(1, "i", "o", timeout=-1) + with self.assertRaises(TypeError): + bs = BusSynchronizer(1, "i", "o", timeout="a") + + bs = BusSynchronizer(1, "i", "o", timeout=0) + + def test_smoke_w1(self): + self.check_smoke(width=1, timeout=127) + + def test_smoke_normalcase(self): + self.check_smoke(width=8, timeout=127) + + def test_smoke_notimeout(self): + self.check_smoke(width=8, timeout=0) + + def check_smoke(self, width, timeout): + m = Module() + m.domains += ClockDomain("sync") + bs = m.submodules.dut = BusSynchronizer(width, "sync", "sync", timeout=timeout) + + with Simulator(m, vcd_file = open("test.vcd", "w")) as sim: + sim.add_clock(1e-6) + def process(): + for i in range(10): + testval = i % (2 ** width) + yield bs.i.eq(testval) + # 6-cycle round trip, and if one in progress, must complete first: + for j in range(11): + yield Tick() + self.assertEqual((yield bs.o), testval) + sim.add_process(process) + sim.run() # TODO: test with distinct clocks # (since we can currently only test symmetric aspect ratio) From d4a6f28be5be0a7ed3da637fa2815f522d6044e4 Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Tue, 5 Mar 2019 11:16:51 +0000 Subject: [PATCH 5/9] lib.cdc: Add ElasticBuffer and smoke test --- nmigen/lib/cdc.py | 61 +++++++++++++++++++++++++++++++++++++ nmigen/test/test_lib_cdc.py | 41 +++++++++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 75d309a7..2c743bc2 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -7,6 +7,7 @@ "ResetSynchronizer", "PulseSynchronizer", "BusSynchronizer", + "ElasticBuffer", "Gearbox" ] @@ -261,6 +262,66 @@ def elaborate(self, platform): return m +class ElasticBuffer(Elaboratable): + """Pass data between two clock domains with the same frequency, and bounded phase difference. + + Increasing the storage depth increases tolerance for clock wander and jitter, but still within + some bound. For less-well-behaved clocks, consider AsyncFIFO. + + Parameters + ---------- + width : int > 0 + Width of databus to be resynchronized + depth : int > 1 + Number of storage elements in buffer + idomain : str + Name of input clock domain + odomain : str + Name of output clock domain + + Attributes + ---------- + i : Signal(width) + Input data bus + o : Signal(width) + Output data bus + """ + def __init__(self, width, depth, idomain, odomain): + if not isinstance(width, int) or width < 1: + raise TypeError("width must be a positive integer, not '{!r}'".format(width)) + if not isinstance(depth, int) or depth <= 1: + raise TypeError("depth must be an integer > 1, not '{!r}'".format(depth)) + + self.i = Signal(width) + self.o = Signal(width) + self.width = width + self.depth = depth + self.idomain = idomain + self.odomain = odomain + + def elaborate(self, platform): + m = Module() + + wptr = Signal(max=self.depth, reset=self.depth // 2) + rptr = Signal(max=self.depth) + m.d[self.idomain] += wptr.eq(_incr(wptr, self.depth)) + m.d[self.odomain] += rptr.eq(_incr(rptr, self.depth)) + + storage = Memory(self.width, self.depth) + wport = m.submodules.wport = storage.write_port(domain=self.idomain) + rport = m.submodules.rport = storage.read_port(domain=self.odomain) + + m.d.comb += [ + wport.en.eq(1), + wport.addr.eq(wptr), + wport.data.eq(self.i), + rport.addr.eq(rptr), + self.o.eq(rport.data) + ] + + return m + + class Gearbox(Elaboratable): """Adapt the width of a continous datastream. diff --git a/nmigen/test/test_lib_cdc.py b/nmigen/test/test_lib_cdc.py index 25f79670..9d6b1ae7 100644 --- a/nmigen/test/test_lib_cdc.py +++ b/nmigen/test/test_lib_cdc.py @@ -200,6 +200,47 @@ def process(): sim.add_process(process) sim.run() +class ElasticBufferTestCase(FHDLTestCase): + def test_paramcheck(self): + with self.assertRaises(TypeError): + e = ElasticBuffer(0, 2, "i", "o") + with self.assertRaises(TypeError): + e = ElasticBuffer("i", 2, "i", "o") + with self.assertRaises(TypeError): + e = ElasticBuffer(1, 1, "i", "o") + with self.assertRaises(TypeError): + e = ElasticBuffer(1, "i", "i", "o") + e = ElasticBuffer(1, 2, "i", "o") + + def test_smoke_po2(self): + self.check_smoke(8, 8) + + def test_smoke_nonpo2(self): + self.check_smoke(8, 7) + + def check_smoke(self, width, depth): + m = Module() + m.domains += ClockDomain("sync") + e = m.submodules.dut = ElasticBuffer(width, depth, "sync", "sync") + + with Simulator(m, vcd_file = open("test.vcd", "w")) as sim: + sim.add_clock(1e-6) + def process(): + pipeline_filled = False + expected_out = 1 + yield Tick() + for i in range(depth * 4): + yield e.i.eq(i) + if (yield e.o): + pipeline_filled = True + if pipeline_filled: + self.assertEqual((yield e.o), expected_out) + expected_out = (expected_out + 1) % (2 ** width) + yield Tick() + self.assertEqual(pipeline_filled, True) + sim.add_process(process) + sim.run() + # TODO: test with distinct clocks # (since we can currently only test symmetric aspect ratio) class GearboxTestCase(FHDLTestCase): From 1ea1a9d39f01d53632a1e1ca9007510d1779ee0a Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Tue, 11 Jun 2019 16:47:29 +0100 Subject: [PATCH 6/9] BusSynchronizer: lengthen request path, rather than shortening data path, so that data path benefits from MCP/falsepath constraints from MultiReg. --- nmigen/lib/cdc.py | 5 +++-- nmigen/test/test_lib_cdc.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 2c743bc2..4006f9b0 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -229,8 +229,9 @@ def elaborate(self, platform): ack_o = Signal() ack_i = Signal() + # Extra flop on i->o to avoid race between data and request sync_io = m.submodules.sync_io = \ - PulseSynchronizer(self.idomain, self.odomain, self.sync_stages) + PulseSynchronizer(self.idomain, self.odomain, self.sync_stages + 1) sync_oi = m.submodules.sync_oi = \ PulseSynchronizer(self.odomain, self.idomain, self.sync_stages) @@ -256,7 +257,7 @@ def elaborate(self, platform): with m.If(ack_i): m.d[self.idomain] += buf_i.eq(self.i) sync_data = m.submodules.sync_data = \ - MultiReg(buf_i, buf_o, odomain=self.odomain, n=self.sync_stages - 1) + MultiReg(buf_i, buf_o, odomain=self.odomain, n=self.sync_stages) with m.If(ack_o): m.d[self.odomain] += self.o.eq(buf_o) diff --git a/nmigen/test/test_lib_cdc.py b/nmigen/test/test_lib_cdc.py index 9d6b1ae7..cfe589a5 100644 --- a/nmigen/test/test_lib_cdc.py +++ b/nmigen/test/test_lib_cdc.py @@ -193,8 +193,8 @@ def process(): for i in range(10): testval = i % (2 ** width) yield bs.i.eq(testval) - # 6-cycle round trip, and if one in progress, must complete first: - for j in range(11): + # 7-cycle round trip, and if one in progress, must complete first: + for j in range(13): yield Tick() self.assertEqual((yield bs.o), testval) sim.add_process(process) From 8357fbcac46866a488d4e61f876281197244d1d3 Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Tue, 11 Jun 2019 17:14:33 +0100 Subject: [PATCH 7/9] lib.cdc: Fix description of ResetSynchronizer --- nmigen/lib/cdc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 4006f9b0..f4cd18e8 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -98,7 +98,7 @@ class ResetSynchronizer(Elaboratable): domain : str Name of domain to synchronize reset to. n : int, >=1 - Number of clock edges from input deassertion to output deassertion + Number of metastability flops between input and output Override -------- From c84cd8d3d21c004a6e0d426567e5cfe68cd4cd5b Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Fri, 28 Jun 2019 06:46:33 +0100 Subject: [PATCH 8/9] lib.cdc: update clock domain naming conventions according to #97 (bikeshed) --- nmigen/lib/cdc.py | 116 ++++++++++++++++++++++----------------------- nmigen/lib/fifo.py | 4 +- 2 files changed, 60 insertions(+), 60 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index f4cd18e8..b9bed264 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -31,15 +31,15 @@ class MultiReg(Elaboratable): Signal to be resynchronised o : Signal(), out Signal connected to synchroniser output - odomain : str - Name of output clock domain + cd_o : str + Name of output (capturing) clock domain n : int Number of flops between input and output. reset : int Reset value of the flip-flops. On FPGAs, even if ``reset_less`` is True, the MultiReg is still set to this value during initialization. reset_less : bool - If True (the default), this MultiReg is unaffected by ``odomain`` reset. + If True (the default), this MultiReg is unaffected by ``cd_o`` reset. See "Note on Reset" below. Platform override @@ -57,19 +57,19 @@ class MultiReg(Elaboratable): consider setting ``reset_less`` to False if any of the following is true: - You are targeting an ASIC, or an FPGA that does not allow arbitrary initial flip-flop states; - - Your design features warm (non-power-on) resets of ``odomain``, so the one-time + - Your design features warm (non-power-on) resets of ``cd_o``, so the one-time initialization at power on is insufficient; - Your design features a sequenced reset, and the MultiReg must maintain its reset value until - ``odomain`` reset specifically is deasserted. + ``cd_o`` reset specifically is deasserted. - MultiReg is reset by the ``odomain`` reset only. + MultiReg is reset by the ``cd_o`` reset only. """ - def __init__(self, i, o, odomain="sync", n=2, reset=0, reset_less=True): + def __init__(self, i, o, cd_o="sync", n=2, reset=0, reset_less=True): if not isinstance(n, int) or n < 1: raise TypeError("n must be a positive integer, not '{!r}'".format(n)) self.i = i self.o = o - self.odomain = odomain + self.cd_o = cd_o self._regs = [Signal(self.i.shape(), name="cdc{}".format(i), reset=reset, reset_less=reset_less, attrs={"no_retiming": True}) @@ -81,7 +81,7 @@ def elaborate(self, platform): m = Module() for i, o in zip((self.i, *self._regs), self._regs): - m.d[self.odomain] += o.eq(i) + m.d[self.cd_o] += o.eq(i) m.d.comb += self.o.eq(self._regs[-1]) return m @@ -95,8 +95,8 @@ class ResetSynchronizer(Elaboratable): ---------- arst : Signal(1), out Asynchronous reset signal, to be synchronized. - domain : str - Name of domain to synchronize reset to. + cd : str + Name of clock domain to synchronize reset to. n : int, >=1 Number of metastability flops between input and output @@ -105,11 +105,11 @@ class ResetSynchronizer(Elaboratable): Define the ``get_reset_sync`` platform attribute to override the implementation of ResetSynchronizer, e.g. to instantiate library cells directly. """ - def __init__(self, arst, domain="sync", n=2): + def __init__(self, arst, cd="sync", n=2): if not isinstance(n, int) or n < 1: raise TypeError("n must be a positive integer, not '{!r}'".format(n)) self.arst = arst - self.domain = domain + self.cd = cd self._regs = [Signal(name="arst{}".format(i), reset=1, attrs={"no_retiming": True}) @@ -124,9 +124,9 @@ def elaborate(self, platform): for i, o in zip((0, *self._regs), self._regs): m.d._reset_sync += o.eq(i) m.d.comb += [ - ClockSignal("_reset_sync").eq(ClockSignal(self.domain)), + ClockSignal("_reset_sync").eq(ClockSignal(self.cd)), ResetSignal("_reset_sync").eq(self.arst), - ResetSignal(self.domain).eq(self._regs[-1]) + ResetSignal(self.cd).eq(self._regs[-1]) ] return m @@ -142,22 +142,22 @@ class PulseSynchronizer(Elaboratable): Parameters ---------- - idomain : str + cd_i : str Name of input clock domain. - odomain : str + cd_o : str Name of output clock domain. sync_stages : int Number of synchronisation flops between the two clock domains. 2 is the default, and minimum safe value. High-frequency designs may choose to increase this. """ - def __init__(self, idomain, odomain, sync_stages=2): + def __init__(self, cd_i, cd_o, sync_stages=2): if not isinstance(sync_stages, int) or sync_stages < 1: raise TypeError("sync_stages must be a positive integer, not '{!r}'".format(sync_stages)) self.i = Signal() self.o = Signal() - self.idomain = idomain - self.odomain = odomain + self.cd_i = cd_i + self.cd_o = cd_o self.sync_stages = sync_stages def elaborate(self, platform): @@ -166,11 +166,11 @@ def elaborate(self, platform): itoggle = Signal() otoggle = Signal() mreg = m.submodules.mreg = \ - MultiReg(itoggle, otoggle, odomain=self.odomain, n=self.sync_stages) + MultiReg(itoggle, otoggle, cd_o=self.cd_o, n=self.sync_stages) otoggle_prev = Signal() - m.d[self.idomain] += itoggle.eq(itoggle ^ self.i) - m.d[self.odomain] += otoggle_prev.eq(otoggle) + m.d[self.cd_i] += itoggle.eq(itoggle ^ self.i) + m.d[self.cd_o] += otoggle_prev.eq(otoggle) m.d.comb += self.o.eq(otoggle ^ otoggle_prev) return m @@ -185,25 +185,25 @@ class BusSynchronizer(Elaboratable): ---------- width : int > 0 Width of the bus to be synchronized - idomain : str + cd_i : str Name of input clock domain - odomain : str + cd_o : str Name of output clock domain sync_stages : int >= 2 Number of synchronisation stages used in the req/ack pulse synchronizers. Lower than 2 is unsafe. Higher values increase safety for high-frequency designs, but increase latency too. timeout : int >= 0 - The request from idomain is re-sent if ``timeout`` cycles elapse without a response. + The request from cd_i is re-sent if ``timeout`` cycles elapse without a response. ``timeout`` = 0 disables this feature. Attributes ---------- i : Signal(width), in - Input signal, sourced from ``idomain`` + Input signal, sourced from ``cd_i`` o : Signal(width), out - Resynchronized version of ``i``, driven to ``odomain`` + Resynchronized version of ``i``, driven to ``cd_o`` """ - def __init__(self, width, idomain, odomain, sync_stages=2, timeout = 127): + def __init__(self, width, cd_i, cd_o, sync_stages=2, timeout = 127): if not isinstance(width, int) or width < 1: raise TypeError("width must be a positive integer, not '{!r}'".format(width)) if not isinstance(sync_stages, int) or sync_stages < 2: @@ -214,15 +214,15 @@ def __init__(self, width, idomain, odomain, sync_stages=2, timeout = 127): self.i = Signal(width) self.o = Signal(width, attrs={"no_retiming": True}) self.width = width - self.idomain = idomain - self.odomain = odomain + self.cd_i = cd_i + self.cd_o = cd_o self.sync_stages = sync_stages self.timeout = timeout def elaborate(self, platform): m = Module() if self.width == 1: - m.submodules += MultiReg(self.i, self.o, odomain=self.odomain, n=self.sync_stages) + m.submodules += MultiReg(self.i, self.o, cd_o=self.cd_o, n=self.sync_stages) return m req = Signal() @@ -231,19 +231,19 @@ def elaborate(self, platform): # Extra flop on i->o to avoid race between data and request sync_io = m.submodules.sync_io = \ - PulseSynchronizer(self.idomain, self.odomain, self.sync_stages + 1) + PulseSynchronizer(self.cd_i, self.cd_o, self.sync_stages + 1) sync_oi = m.submodules.sync_oi = \ - PulseSynchronizer(self.odomain, self.idomain, self.sync_stages) + PulseSynchronizer(self.cd_o, self.cd_i, self.sync_stages) if self.timeout != 0: countdown = Signal(max=self.timeout, reset=self.timeout) with m.If(ack_i | req): - m.d[self.idomain] += countdown.eq(self.timeout) + m.d[self.cd_i] += countdown.eq(self.timeout) with m.Else(): - m.d[self.idomain] += countdown.eq(countdown - countdown.bool()) + m.d[self.cd_i] += countdown.eq(countdown - countdown.bool()) start = Signal(reset=1) - m.d[self.idomain] += start.eq(0) + m.d[self.cd_i] += start.eq(0) m.d.comb += [ req.eq(start | ack_i | (self.timeout != 0 and countdown == 0)), sync_io.i.eq(req), @@ -255,11 +255,11 @@ def elaborate(self, platform): buf_i = Signal(self.width, attrs={"no_retiming": True}) buf_o = Signal(self.width) with m.If(ack_i): - m.d[self.idomain] += buf_i.eq(self.i) + m.d[self.cd_i] += buf_i.eq(self.i) sync_data = m.submodules.sync_data = \ - MultiReg(buf_i, buf_o, odomain=self.odomain, n=self.sync_stages) + MultiReg(buf_i, buf_o, cd_o=self.cd_o, n=self.sync_stages) with m.If(ack_o): - m.d[self.odomain] += self.o.eq(buf_o) + m.d[self.cd_o] += self.o.eq(buf_o) return m @@ -275,9 +275,9 @@ class ElasticBuffer(Elaboratable): Width of databus to be resynchronized depth : int > 1 Number of storage elements in buffer - idomain : str + cd_i : str Name of input clock domain - odomain : str + cd_o : str Name of output clock domain Attributes @@ -287,7 +287,7 @@ class ElasticBuffer(Elaboratable): o : Signal(width) Output data bus """ - def __init__(self, width, depth, idomain, odomain): + def __init__(self, width, depth, cd_i, cd_o): if not isinstance(width, int) or width < 1: raise TypeError("width must be a positive integer, not '{!r}'".format(width)) if not isinstance(depth, int) or depth <= 1: @@ -297,20 +297,20 @@ def __init__(self, width, depth, idomain, odomain): self.o = Signal(width) self.width = width self.depth = depth - self.idomain = idomain - self.odomain = odomain + self.cd_i = cd_i + self.cd_o = cd_o def elaborate(self, platform): m = Module() wptr = Signal(max=self.depth, reset=self.depth // 2) rptr = Signal(max=self.depth) - m.d[self.idomain] += wptr.eq(_incr(wptr, self.depth)) - m.d[self.odomain] += rptr.eq(_incr(rptr, self.depth)) + m.d[self.cd_i] += wptr.eq(_incr(wptr, self.depth)) + m.d[self.cd_o] += rptr.eq(_incr(rptr, self.depth)) storage = Memory(self.width, self.depth) - wport = m.submodules.wport = storage.write_port(domain=self.idomain) - rport = m.submodules.rport = storage.read_port(domain=self.odomain) + wport = m.submodules.wport = storage.write_port(domain=self.cd_i) + rport = m.submodules.rport = storage.read_port(domain=self.cd_o) m.d.comb += [ wport.en.eq(1), @@ -337,11 +337,11 @@ class Gearbox(Elaboratable): ---------- iwidth : int Bit width of the input - idomain : str + cd_i : str Name of input clock domain owidth : int Bit width of the output - odomain : str + cd_o : str Name of output clock domain Attributes @@ -351,7 +351,7 @@ class Gearbox(Elaboratable): o : Signal(owidth), out Output datastream. Transitions on every output clock. """ - def __init__(self, iwidth, idomain, owidth, odomain): + def __init__(self, iwidth, cd_i, owidth, cd_o): if not isinstance(iwidth, int) or iwidth < 1: raise TypeError("iwidth must be a positive integer, not '{!r}'".format(iwidth)) if not isinstance(owidth, int) or owidth < 1: @@ -360,9 +360,9 @@ def __init__(self, iwidth, idomain, owidth, odomain): self.i = Signal(iwidth) self.o = Signal(owidth) self.iwidth = iwidth - self.idomain = idomain + self.cd_i = cd_i self.owidth = owidth - self.odomain = odomain + self.cd_o = cd_o storagesize = iwidth * owidth // gcd(iwidth, owidth) while storagesize // iwidth < 4: @@ -384,19 +384,19 @@ def elaborate(self, platform): iptr = Signal(max=self._ichunks - 1, reset=(self._ichunks // 2 if i_faster else 0)) optr = Signal(max=self._ochunks - 1, reset=(0 if i_faster else self._ochunks // 2)) - m.d[self.idomain] += iptr.eq(_incr(iptr, self._storagesize)) - m.d[self.odomain] += optr.eq(_incr(optr, self._storagesize)) + m.d[self.cd_i] += iptr.eq(_incr(iptr, self._storagesize)) + m.d[self.cd_o] += optr.eq(_incr(optr, self._storagesize)) with m.Switch(iptr): for n in range(self._ichunks): s = slice(n * self.iwidth, (n + 1) * self.iwidth) with m.Case(n): - m.d[self.idomain] += storage[s].eq(self.i) + m.d[self.cd_i] += storage[s].eq(self.i) with m.Switch(optr): for n in range(self._ochunks): s = slice(n * self.owidth, (n + 1) * self.owidth) with m.Case(n): - m.d[self.odomain] += self.o.eq(storage[s]) + m.d[self.cd_o] += self.o.eq(storage[s]) return m diff --git a/nmigen/lib/fifo.py b/nmigen/lib/fifo.py index 2844a058..a0932920 100644 --- a/nmigen/lib/fifo.py +++ b/nmigen/lib/fifo.py @@ -316,7 +316,7 @@ def elaborate(self, platform): produce_enc = m.submodules.produce_enc = \ GrayEncoder(self._ctr_bits) produce_cdc = m.submodules.produce_cdc = \ - MultiReg(produce_w_gry, produce_r_gry, odomain="read") + MultiReg(produce_w_gry, produce_r_gry, cd_o="read") m.d.comb += produce_enc.i.eq(produce_w_nxt), m.d.write += produce_w_gry.eq(produce_enc.o) @@ -325,7 +325,7 @@ def elaborate(self, platform): consume_enc = m.submodules.consume_enc = \ GrayEncoder(self._ctr_bits) consume_cdc = m.submodules.consume_cdc = \ - MultiReg(consume_r_gry, consume_w_gry, odomain="write") + MultiReg(consume_r_gry, consume_w_gry, cd_o="write") m.d.comb += consume_enc.i.eq(consume_r_nxt) m.d.read += consume_r_gry.eq(consume_enc.o) From 2e6cf3c67a6e8699590ef658feda2eeb419b4ac6 Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Fri, 28 Jun 2019 07:04:12 +0100 Subject: [PATCH 9/9] lib.cdc: consistent naming conventions for Gearbox constructor arguments --- nmigen/lib/cdc.py | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index b9bed264..e87248e8 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -335,46 +335,46 @@ class Gearbox(Elaboratable): Parameters ---------- - iwidth : int + width_i : int Bit width of the input cd_i : str Name of input clock domain - owidth : int + width_o : int Bit width of the output cd_o : str Name of output clock domain Attributes ---------- - i : Signal(iwidth), in + i : Signal(width_i), in Input datastream. Sampled on every input clock. - o : Signal(owidth), out + o : Signal(width_o), out Output datastream. Transitions on every output clock. """ - def __init__(self, iwidth, cd_i, owidth, cd_o): - if not isinstance(iwidth, int) or iwidth < 1: - raise TypeError("iwidth must be a positive integer, not '{!r}'".format(iwidth)) - if not isinstance(owidth, int) or owidth < 1: - raise TypeError("owidth must be a positive integer, not '{!r}'".format(owidth)) - - self.i = Signal(iwidth) - self.o = Signal(owidth) - self.iwidth = iwidth + def __init__(self, width_i, cd_i, width_o, cd_o): + if not isinstance(width_i, int) or width_i < 1: + raise TypeError("width_i must be a positive integer, not '{!r}'".format(width_i)) + if not isinstance(width_o, int) or width_o < 1: + raise TypeError("width_o must be a positive integer, not '{!r}'".format(width_o)) + + self.i = Signal(width_i) + self.o = Signal(width_o) + self.width_i = width_i self.cd_i = cd_i - self.owidth = owidth + self.width_o = width_o self.cd_o = cd_o - storagesize = iwidth * owidth // gcd(iwidth, owidth) - while storagesize // iwidth < 4: + storagesize = width_i * width_o // gcd(width_i, width_o) + while storagesize // width_i < 4: storagesize *= 2 - while storagesize // owidth < 4: + while storagesize // width_o < 4: storagesize *= 2 self._storagesize = storagesize - self._ichunks = storagesize // self.iwidth - self._ochunks = storagesize // self.owidth - assert(self._ichunks * self.iwidth == storagesize) - assert(self._ochunks * self.owidth == storagesize) + self._ichunks = storagesize // self.width_i + self._ochunks = storagesize // self.width_o + assert(self._ichunks * self.width_i == storagesize) + assert(self._ochunks * self.width_o == storagesize) def elaborate(self, platform): m = Module() @@ -389,13 +389,13 @@ def elaborate(self, platform): with m.Switch(iptr): for n in range(self._ichunks): - s = slice(n * self.iwidth, (n + 1) * self.iwidth) + s = slice(n * self.width_i, (n + 1) * self.width_i) with m.Case(n): m.d[self.cd_i] += storage[s].eq(self.i) with m.Switch(optr): for n in range(self._ochunks): - s = slice(n * self.owidth, (n + 1) * self.owidth) + s = slice(n * self.width_o, (n + 1) * self.width_o) with m.Case(n): m.d[self.cd_o] += self.o.eq(storage[s])