From 028d5d80736151dbe6f9d35d35544cbfff6bef87 Mon Sep 17 00:00:00 2001 From: Wanda Date: Sun, 28 Apr 2024 20:04:16 +0200 Subject: [PATCH] hdl._ir, lib, vendor: add `RequirePosedge`, use it whenever required. --- amaranth/hdl/_ir.py | 40 +++++++++++++++++++++++++++++++-- amaranth/hdl/_xfrm.py | 10 +++++++++ amaranth/lib/cdc.py | 2 ++ amaranth/vendor/_altera.py | 4 ++++ amaranth/vendor/_gowin.py | 3 +++ amaranth/vendor/_lattice.py | 11 +++++++++ amaranth/vendor/_siliconblue.py | 5 +++++ amaranth/vendor/_xilinx.py | 12 ++++++++++ tests/test_hdl_ir.py | 25 +++++++++++++++++++++ 9 files changed, 110 insertions(+), 2 deletions(-) diff --git a/amaranth/hdl/_ir.py b/amaranth/hdl/_ir.py index 66750c1..f24cd2e 100644 --- a/amaranth/hdl/_ir.py +++ b/amaranth/hdl/_ir.py @@ -10,8 +10,9 @@ from . import _ast, _cd, _ir, _nir __all__ = [ "AlreadyElaborated", "UnusedElaboratable", "Elaboratable", "DuplicateElaboratable", - "DriverConflict", "Fragment", "Instance", "IOBufferInstance", "PortDirection", "Design", - "build_netlist", + "DomainRequirementFailed", "DriverConflict", + "Fragment", "Instance", "IOBufferInstance", "RequirePosedge", "PortDirection", + "Design", "build_netlist", ] @@ -40,6 +41,11 @@ class DuplicateElaboratable(Exception): pass +class DomainRequirementFailed(Exception): + """Raised when a module has unsatisfied requirements about a clock domain, such as getting + a negedge domain when only posedge domains are supported.""" + + class Fragment: @staticmethod def get(obj, platform): @@ -385,6 +391,19 @@ class IOBufferInstance(Fragment): self.src_loc = src_loc or tracer.get_src_loc(src_loc_at) +class RequirePosedge(Fragment): + """A special fragment that requires a given domain to have :py:`clk_edge="pos"`, failing + elaboration otherwise. + + This is a private interface, without a stability guarantee. + """ + + def __init__(self, domain, *, src_loc_at=0, src_loc=None): + super().__init__() + self._domain = domain + self.src_loc = src_loc or tracer.get_src_loc(src_loc_at) + + def _add_name(assigned_names, name): if name in assigned_names: name = f"{name}${len(assigned_names)}" @@ -429,6 +448,7 @@ class Design: else: self._use_signal(fragment, conn) self._assign_names(fragment, hierarchy) + self._check_domain_requires() def _compute_fragment_depth_parent(self, fragment: Fragment, parent: "Fragment | None", depth: int): """Recursively computes every fragment's depth and parent.""" @@ -576,6 +596,8 @@ class Design: self._use_signal(fragment, domain.clk) if domain.rst is not None: self._use_signal(fragment, domain.rst) + elif isinstance(fragment, _ir.RequirePosedge): + pass else: for domain_name, statements in fragment.statements.items(): if domain_name != "comb": @@ -662,6 +684,18 @@ class Design: subfragment_name = _add_name(frag_info.assigned_names, subfragment_name) self._assign_names(subfragment, hierarchy=(*hierarchy, subfragment_name)) + def _check_domain_requires(self): + for fragment, fragment_info in self.fragments.items(): + if isinstance(fragment, RequirePosedge): + domain = fragment.domains[fragment._domain] + if domain.clk_edge != "pos": + if fragment.src_loc is None: + src_loc = ":0" + else: + src_loc = f"{fragment.src_loc[0]}:{fragment.src_loc[1]}" + fragment_name = ".".join(fragment_info.name) + raise DomainRequirementFailed(f"Domain {domain.name} has a negedge clock, but posedge clock is required by {fragment_name} at {src_loc}") + def lookup_domain(self, domain, context): if domain == "comb": raise KeyError("comb") @@ -1491,6 +1525,8 @@ class NetlistEmitter: assert parent_module_idx is not None self.emit_iobuffer(parent_module_idx, fragment) self.fragment_module_idx[fragment] = parent_module_idx + elif isinstance(fragment, _ir.RequirePosedge): + pass elif type(fragment) is _ir.Fragment: module_idx = self.netlist.add_module(parent_module_idx, fragment_name, src_loc=fragment.src_loc, cell_src_loc=cell_src_loc) self.fragment_module_idx[fragment] = module_idx diff --git a/amaranth/hdl/_xfrm.py b/amaranth/hdl/_xfrm.py index 062cddc..92d0705 100644 --- a/amaranth/hdl/_xfrm.py +++ b/amaranth/hdl/_xfrm.py @@ -314,6 +314,8 @@ class FragmentTransformer: oe=fragment.oe, src_loc=fragment.src_loc, ) + elif isinstance(fragment, RequirePosedge): + new_fragment = RequirePosedge(fragment._domain, src_loc=fragment.src_loc) else: new_fragment = Fragment(src_loc=fragment.src_loc) new_fragment.attrs = OrderedDict(fragment.attrs) @@ -445,6 +447,8 @@ class DomainCollector(ValueVisitor, StatementVisitor): self.on_value(port._data) self.on_value(port._en) self._add_used_domain(port._domain) + if isinstance(fragment, RequirePosedge): + self._add_used_domain(fragment._domain) if isinstance(fragment, Instance): for name, (value, dir) in fragment.ports.items(): @@ -535,6 +539,12 @@ class DomainRenamer(FragmentTransformer, ValueTransformer, StatementTransformer) if port._domain in self.domain_map: port._domain = self.domain_map[port._domain] + def on_fragment(self, fragment): + new_fragment = super().on_fragment(fragment) + if isinstance(new_fragment, RequirePosedge) and new_fragment._domain in self.domain_map: + new_fragment._domain = self.domain_map[new_fragment._domain] + return new_fragment + class DomainLowerer(FragmentTransformer, ValueTransformer, StatementTransformer): def __init__(self, domains=None): diff --git a/amaranth/lib/cdc.py b/amaranth/lib/cdc.py index b2a7cdd..1afcc76 100644 --- a/amaranth/lib/cdc.py +++ b/amaranth/lib/cdc.py @@ -1,6 +1,7 @@ import warnings from .. import * +from ..hdl._ir import RequirePosedge __all__ = ["FFSynchronizer", "AsyncFFSynchronizer", "ResetSynchronizer", "PulseSynchronizer"] @@ -187,6 +188,7 @@ class AsyncFFSynchronizer(Elaboratable): ClockSignal("async_ff").eq(ClockSignal(self._o_domain)), self.o.eq(flops[-1]) ] + m.submodules += RequirePosedge(self._o_domain) return m diff --git a/amaranth/vendor/_altera.py b/amaranth/vendor/_altera.py index 0b206a5..8da033e 100644 --- a/amaranth/vendor/_altera.py +++ b/amaranth/vendor/_altera.py @@ -2,6 +2,7 @@ from abc import abstractmethod from ..hdl import * from ..hdl import _ast +from ..hdl._ir import RequirePosedge from ..lib import io, wiring from ..build import * @@ -152,6 +153,7 @@ class DDRBuffer(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_reg = Signal(len(self.port)) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) @@ -167,6 +169,7 @@ class DDRBuffer(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) m.submodules.o_ddr = Instance("altddio_out", p_width=len(self.port), o_dataout=buf.o, @@ -507,6 +510,7 @@ class AlteraPlatform(TemplatedPlatform): def get_async_ff_sync(self, async_ff_sync): m = Module() sync_output = Signal() + m.submodules += RequirePosedge(async_ff_sync._o_domain) if async_ff_sync._edge == "pos": m.submodules += Instance("altera_std_synchronizer", p_depth=async_ff_sync._stages, diff --git a/amaranth/vendor/_gowin.py b/amaranth/vendor/_gowin.py index beae85a..11c4fbf 100644 --- a/amaranth/vendor/_gowin.py +++ b/amaranth/vendor/_gowin.py @@ -4,6 +4,7 @@ import math import re from ..hdl import * +from ..hdl._ir import RequirePosedge from ..lib import io, wiring from ..lib.cdc import ResetSynchronizer from ..build import * @@ -131,6 +132,7 @@ class DDRBuffer(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) for bit in range(len(self.port)): @@ -144,6 +146,7 @@ class DDRBuffer(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = self.o[0] ^ inv_mask o1_inv = self.o[1] ^ inv_mask for bit in range(len(self.port)): diff --git a/amaranth/vendor/_lattice.py b/amaranth/vendor/_lattice.py index 94d03d7..d926c0c 100644 --- a/amaranth/vendor/_lattice.py +++ b/amaranth/vendor/_lattice.py @@ -1,6 +1,7 @@ from abc import abstractmethod from ..hdl import * +from ..hdl._ir import RequirePosedge from ..lib import io, wiring from ..build import * @@ -107,6 +108,7 @@ class FFBufferECP5(io.FFBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i_inv = Signal.like(self.i) for bit in range(len(self.port)): m.submodules[f"i_ff{bit}"] = Instance("IFS1P3DX", @@ -119,6 +121,7 @@ class FFBufferECP5(io.FFBuffer): m.d.comb += self.i.eq(i_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o_inv = Signal.like(self.o) m.d.comb += o_inv.eq(self.o ^ inv_mask) for bit in range(len(self.port)): @@ -142,6 +145,7 @@ class FFBufferNexus(io.FFBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i_inv = Signal.like(self.i) for bit in range(len(self.port)): m.submodules[f"i_ff{bit}"] = Instance("IFD1P3DX", @@ -154,6 +158,7 @@ class FFBufferNexus(io.FFBuffer): m.d.comb += self.i.eq(i_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o_inv = Signal.like(self.o) m.d.comb += o_inv.eq(self.o ^ inv_mask) for bit in range(len(self.port)): @@ -177,6 +182,7 @@ class DDRBufferECP5(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) for bit in range(len(self.port)): @@ -191,6 +197,7 @@ class DDRBufferECP5(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) m.d.comb += [ @@ -218,6 +225,7 @@ class DDRBufferMachXO2(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) for bit in range(len(self.port)): @@ -232,6 +240,7 @@ class DDRBufferMachXO2(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) m.d.comb += [ @@ -259,6 +268,7 @@ class DDRBufferNexus(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) for bit in range(len(self.port)): @@ -273,6 +283,7 @@ class DDRBufferNexus(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) m.d.comb += [ diff --git a/amaranth/vendor/_siliconblue.py b/amaranth/vendor/_siliconblue.py index 36c1501..08cc966 100644 --- a/amaranth/vendor/_siliconblue.py +++ b/amaranth/vendor/_siliconblue.py @@ -4,6 +4,7 @@ from abc import abstractmethod from ..hdl import * +from ..hdl._ir import RequirePosedge from ..lib.cdc import ResetSynchronizer from ..lib import io from ..build import * @@ -506,10 +507,12 @@ class SiliconBluePlatform(TemplatedPlatform): else: io_args.append(("o", "D_IN_0", i[bit])) elif isinstance(buffer, io.FFBuffer): + m.submodules += RequirePosedge(self.i_domain) i_type = 0b00 # PIN_INPUT_REGISTERED aka PIN_INPUT_DDR io_args.append(("i", "INPUT_CLK", ClockSignal(buffer.i_domain))) io_args.append(("o", "D_IN_0", i[bit])) elif isinstance(buffer, io.DDRBuffer): + m.submodules += RequirePosedge(self.i_domain) i_type = 0b00 # PIN_INPUT_REGISTERED aka PIN_INPUT_DDR io_args.append(("i", "INPUT_CLK", ClockSignal(buffer.i_domain))) io_args.append(("o", "D_IN_0", i0[bit])) @@ -521,10 +524,12 @@ class SiliconBluePlatform(TemplatedPlatform): o_type = 0b1010 # PIN_OUTPUT_TRISTATE io_args.append(("i", "D_OUT_0", o[bit])) elif isinstance(buffer, io.FFBuffer): + m.submodules += RequirePosedge(self.o_domain) o_type = 0b1101 # PIN_OUTPUT_REGISTERED_ENABLE_REGISTERED io_args.append(("i", "OUTPUT_CLK", ClockSignal(buffer.o_domain))) io_args.append(("i", "D_OUT_0", o[bit])) elif isinstance(buffer, io.DDRBuffer): + m.submodules += RequirePosedge(self.o_domain) o_type = 0b1100 # PIN_OUTPUT_DDR_ENABLE_REGISTERED io_args.append(("i", "OUTPUT_CLK", ClockSignal(buffer.o_domain))) io_args.append(("i", "D_OUT_0", o0[bit])) diff --git a/amaranth/vendor/_xilinx.py b/amaranth/vendor/_xilinx.py index e5dec54..9a44c62 100644 --- a/amaranth/vendor/_xilinx.py +++ b/amaranth/vendor/_xilinx.py @@ -2,6 +2,7 @@ import re from abc import abstractmethod from ..hdl import * +from ..hdl._ir import RequirePosedge from ..lib.cdc import ResetSynchronizer from ..lib import io, wiring from ..build import * @@ -125,11 +126,13 @@ class FFBuffer(io.FFBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i_inv = Signal.like(self.i) _make_dff(m, "i", self.i_domain, buf.i, i_inv, iob=True) m.d.comb += self.i.eq(i_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o_inv = Signal.like(self.o) m.d.comb += o_inv.eq(self.o ^ inv_mask) _make_dff(m, "o", self.o_domain, o_inv, buf.o, iob=True) @@ -146,6 +149,7 @@ class DDRBufferVirtex2(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) # First-generation input DDR register: basically just two FFs with opposite @@ -161,6 +165,7 @@ class DDRBufferVirtex2(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) o1_ff = Signal(len(self.port)) @@ -210,6 +215,7 @@ class DDRBufferSpartan3E(io.DDRBuffer): } if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) if platform.family == "spartan6" or isinstance(self.port, io.DifferentialPort): @@ -257,6 +263,7 @@ class DDRBufferSpartan3E(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) m.d.comb += [ @@ -333,6 +340,7 @@ class DDRBufferVirtex4(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) for bit in range(len(self.port)): @@ -352,6 +360,7 @@ class DDRBufferVirtex4(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) m.d.comb += [ @@ -384,6 +393,7 @@ class DDRBufferUltrascale(io.DDRBuffer): inv_mask = sum(inv << bit for bit, inv in enumerate(self.port.invert)) if self.direction is not io.Direction.Output: + m.submodules += RequirePosedge(self.i_domain) i0_inv = Signal(len(self.port)) i1_inv = Signal(len(self.port)) for bit in range(len(self.port)): @@ -402,6 +412,7 @@ class DDRBufferUltrascale(io.DDRBuffer): m.d.comb += self.i[1].eq(i1_inv ^ inv_mask) if self.direction is not io.Direction.Input: + m.submodules += RequirePosedge(self.o_domain) o0_inv = Signal(len(self.port)) o1_inv = Signal(len(self.port)) m.d.comb += [ @@ -1234,6 +1245,7 @@ class XilinxPlatform(TemplatedPlatform): def get_async_ff_sync(self, async_ff_sync): m = Module() + m.submodules += RequirePosedge(async_ff_sync._o_domain) m.domains += ClockDomain("async_ff", async_reset=True, local=True) # Instantiate a chain of async_ff_sync._stages FDPEs with all # their PRE pins connected to either async_ff_sync.i or diff --git a/tests/test_hdl_ir.py b/tests/test_hdl_ir.py index 34d5987..5b6a682 100644 --- a/tests/test_hdl_ir.py +++ b/tests/test_hdl_ir.py @@ -3598,3 +3598,28 @@ class DomainLookupTestCase(FHDLTestCase): self.assertIs(design.lookup_domain("sync", xm5), m1_c) self.assertIs(design.lookup_domain("d", xm4), m4_d) self.assertIs(design.lookup_domain("d", xm5), m5_d) + + +class RequirePosedgeTestCase(FHDLTestCase): + def test_require_ok(self): + m = Module() + m.domains.sync = ClockDomain() + m.submodules += RequirePosedge("sync") + Fragment.get(m, None).prepare() + + def test_require_fail(self): + m = Module() + m.domains.sync = ClockDomain(clk_edge="neg") + m.submodules += RequirePosedge("sync") + with self.assertRaisesRegex(DomainRequirementFailed, + r"^Domain sync has a negedge clock, but posedge clock is required by top.U\$0 at .*$"): + Fragment.get(m, None).prepare() + + def test_require_renamed(self): + m = Module() + m.domains.sync = ClockDomain(clk_edge="pos") + m.domains.test = ClockDomain(clk_edge="neg") + m.submodules += DomainRenamer("test")(RequirePosedge("sync")) + with self.assertRaisesRegex(DomainRequirementFailed, + r"^Domain test has a negedge clock, but posedge clock is required by top.U\$0 at .*$"): + Fragment.get(m, None).prepare() \ No newline at end of file