Repository: amaranth-lang/amaranth
Pull request #1442 (2024-06-30 00:43:19)

build: add clock constraints on `IOPort` instead of buffered `Signal`. [0.5 backport]


Patch:
diff --git a/amaranth/build/plat.py b/amaranth/build/plat.py
index d5b02ab..5332f57 100644
--- a/amaranth/build/plat.py
+++ b/amaranth/build/plat.py
@@ -198,14 +198,14 @@ class TemplatedPlatform(Platform):
         """,
     }
 
-    def iter_clock_constraints(self):
-        for net_signal, port_signal, frequency in super().iter_clock_constraints():
+    def iter_signal_clock_constraints(self):
+        for signal, frequency in super().iter_signal_clock_constraints():
             # Skip any clock constraints placed on signals that are never used in the design.
             # Otherwise, it will cause a crash in the vendor platform if it supports clock
             # constraints on non-port nets.
-            if net_signal not in self._name_map:
+            if signal not in self._name_map:
                 continue
-            yield net_signal, port_signal, frequency
+            yield signal, frequency
 
     def toolchain_prepare(self, fragment, name, *, emit_src=True, **kwargs):
         # Restrict the name of the design to a strict alphanumeric character set. Platforms will
@@ -327,8 +327,11 @@ class TemplatedPlatform(Platform):
             else:
                 return " ".join(opts)
 
-        def hierarchy(signal, separator):
-            return separator.join(self._name_map[signal][1:])
+        def hierarchy(net, separator):
+            if isinstance(net, IOPort):
+                return net.name
+            else:
+                return separator.join(self._name_map[net][1:])
 
         def ascii_escape(string):
             def escape_one(match):
diff --git a/amaranth/build/res.py b/amaranth/build/res.py
index 0744ef0..f6d3c93 100644
--- a/amaranth/build/res.py
+++ b/amaranth/build/res.py
@@ -109,8 +109,9 @@ class ResourceManager:
 
         # List of (pin, port, buffer) pairs for non-dir="-" requests.
         self._pins      = []
-        # Constraint list
+        # Constraint lists
         self._clocks    = SignalDict()
+        self._io_clocks = {}
 
         self.add_resources(resources)
         self.add_connectors(connectors)
@@ -219,6 +220,8 @@ class ResourceManager:
                         for name in phys_names
                     ])
                     port = io.SingleEndedPort(iop, invert=phys.invert, direction=direction)
+                    if resource.clock is not None:
+                        self.add_clock_constraint(iop, resource.clock.frequency)
                 if isinstance(phys, DiffPairs):
                     phys_names_p = phys.p.map_names(self._conn_pins, resource)
                     phys_names_n = phys.n.map_names(self._conn_pins, resource)
@@ -232,7 +235,8 @@ class ResourceManager:
                         for name in phys_names_n
                     ])
                     port = io.DifferentialPort(p, n, invert=phys.invert, direction=direction)
-
+                    if resource.clock is not None:
+                        self.add_clock_constraint(p, resource.clock.frequency)
                 for phys_name in phys_names:
                     if phys_name in self._phys_reqd:
                         raise ResourceError("Resource component {} uses physical pin {}, but it "
@@ -253,8 +257,6 @@ class ResourceManager:
                     buffer = PinBuffer(pin, port)
                     self._pins.append((pin, port, buffer))
 
-                    if resource.clock is not None:
-                        self.add_clock_constraint(pin.i, resource.clock.frequency)
                     return pin
 
             else:
@@ -275,38 +277,28 @@ class ResourceManager:
             raise TypeError(f"A clock constraint can only be applied to a Signal, but a "
                             f"ClockSignal is provided; assign the ClockSignal to an "
                             f"intermediate signal and constrain the latter instead.")
-        elif not isinstance(clock, Signal):
-            raise TypeError(f"Object {clock!r} is not a Signal")
+        elif not isinstance(clock, (Signal, IOPort)):
+            raise TypeError(f"Object {clock!r} is not a Signal or IOPort")
         if not isinstance(frequency, (int, float)):
             raise TypeError(f"Frequency must be a number, not {frequency!r}")
 
-        if clock in self._clocks:
+        if isinstance(clock, IOPort):
+            clocks = self._io_clocks
+        else:
+            clocks = self._clocks
+
+        frequency = float(frequency)
+        if clock in clocks and clocks[clock] != frequency:
             raise ValueError("Cannot add clock constraint on {!r}, which is already constrained "
                              "to {} Hz"
-                             .format(clock, self._clocks[clock]))
+                             .format(clock, clocks[clock]))
         else:
-            self._clocks[clock] = float(frequency)
-
-    def iter_clock_constraints(self):
-        # Back-propagate constraints through the input buffer. For clock constraints on pins
-        # (the majority of cases), toolchains work better if the constraint is defined on the pin
-        # and not on the buffered internal net; and if the toolchain is advanced enough that
-        # it considers clock phase and delay of the input buffer, it is *necessary* to define
-        # the constraint on the pin to match the designer's expectation of phase being referenced
-        # to the pin.
-        #
-        # Constraints on nets with no corresponding input pin (e.g. PLL or SERDES outputs) are not
-        # affected.
-        pin_i_to_port = SignalDict()
-        for pin, port, _fragment in self._pins:
-            if hasattr(pin, "i"):
-                if isinstance(port, io.SingleEndedPort):
-                    pin_i_to_port[pin.i] = port.io
-                elif isinstance(port, io.DifferentialPort):
-                    pin_i_to_port[pin.i] = port.p
-                else:
-                    assert False
+            clocks[clock] = frequency
+
+    def iter_signal_clock_constraints(self):
+        for signal, frequency in self._clocks.items():
+            yield signal, frequency
 
-        for net_signal, frequency in self._clocks.items():
-            port_signal = pin_i_to_port.get(net_signal)
-            yield net_signal, port_signal, frequency
+    def iter_port_clock_constraints(self):
+        for port, frequency in self._io_clocks.items():
+            yield port, frequency
diff --git a/amaranth/vendor/_altera.py b/amaranth/vendor/_altera.py
index 8da033e..21e66e8 100644
--- a/amaranth/vendor/_altera.py
+++ b/amaranth/vendor/_altera.py
@@ -310,12 +310,11 @@ class AlteraPlatform(TemplatedPlatform):
             {{get_override("add_settings")|default("# (add_settings placeholder)")}}
         """,
         "{{name}}.sdc": r"""
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -name {{port_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port_signal.name|tcl_quote}}]
-                {% else -%}
-                    create_clock -name {{net_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{net_signal|hierarchy("|")|tcl_quote}}]
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -name {{signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("|")|tcl_quote}}]
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -name {{port.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port.name|tcl_quote}}]
             {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """,
diff --git a/amaranth/vendor/_gowin.py b/amaranth/vendor/_gowin.py
index 11c4fbf..280d009 100644
--- a/amaranth/vendor/_gowin.py
+++ b/amaranth/vendor/_gowin.py
@@ -439,8 +439,11 @@ class GowinPlatform(TemplatedPlatform):
         """,
         "{{name}}.sdc": r"""
         // {{autogenerated}}
-        {% for net_signal,port_signal,frequency in platform.iter_clock_constraints() -%}
-            create_clock -name {{port_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port_signal.name|tcl_quote}}]
+        {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+            create_clock -name {{signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")|tcl_quote}}]
+        {% endfor %}
+        {% for port, frequency in platform.iter_port_clock_constraints() -%}
+            create_clock -name {{port.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port.name|tcl_quote}}]
         {% endfor %}
         {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """,
diff --git a/amaranth/vendor/_lattice.py b/amaranth/vendor/_lattice.py
index 834926f..0514605 100644
--- a/amaranth/vendor/_lattice.py
+++ b/amaranth/vendor/_lattice.py
@@ -497,12 +497,11 @@ class LatticePlatform(TemplatedPlatform):
                     {%- for key, value in attrs.items() %} {{key}}={{value}}{% endfor %};
                 {% endif %}
             {% endfor %}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    FREQUENCY PORT "{{port_signal.name}}" {{frequency}} HZ;
-                {% else -%}
-                    FREQUENCY NET "{{net_signal|hierarchy(".")}}" {{frequency}} HZ;
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                FREQUENCY NET "{{signals|hierarchy(".")}}" {{frequency}} HZ;
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                FREQUENCY PORT "{{port.name}}" {{frequency}} HZ;
             {% endfor %}
             {{get_override("add_preferences")|default("# (add_preferences placeholder)")}}
         """
@@ -584,12 +583,11 @@ class LatticePlatform(TemplatedPlatform):
                 ldc_set_port -iobuf {{ '{' }}{%- for key, value in attrs.items() %}{{key}}={{value}} {% endfor %}{{ '}' }} {{'['}}get_ports {{port_name}}{{']'}}
                 {% endif %}
             {% endfor %}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -name {{port_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port_signal.name|tcl_quote}}]
-                {% else -%}
-                    create_clock -name {{net_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{net_signal|hierarchy("/")|tcl_quote}}]
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -name {{signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")|tcl_quote}}]
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -name {{port.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port.name|tcl_quote}}]
             {% endfor %}
             {{get_override("add_preferences")|default("# (add_preferences placeholder)")}}
         """
@@ -684,12 +682,11 @@ class LatticePlatform(TemplatedPlatform):
         """,
         "{{name}}.sdc": r"""
             set_hierarchy_separator {/}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -name {{port_signal.name|tcl_quote("Diamond")}} -period {{1000000000/frequency}} [get_ports {{port_signal.name|tcl_quote("Diamond")}}]
-                {% else -%}
-                    create_clock -name {{net_signal.name|tcl_quote("Diamond")}} -period {{1000000000/frequency}} [get_nets {{net_signal|hierarchy("/")|tcl_quote("Diamond")}}]
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -name {{signal.name|tcl_quote("Diamond")}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")|tcl_quote("Diamond")}}]
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -name {{port.name|tcl_quote("Diamond")}} -period {{1000000000/frequency}} [get_ports {{port.name|tcl_quote("Diamond")}}]
             {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """,
@@ -782,12 +779,11 @@ class LatticePlatform(TemplatedPlatform):
         """,
         # Pre-synthesis SDC constraints
         "{{name}}.sdc": r"""
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -name {{port_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port_signal.name}}]
-                {% else -%}
-                    create_clock -name {{net_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{net_signal|hierarchy("/")}}]
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -name {{signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")|tcl_quote}}]
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -name {{port.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port.name|tcl_quote}}]
             {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """,
diff --git a/amaranth/vendor/_quicklogic.py b/amaranth/vendor/_quicklogic.py
index 33a490e..02d5af9 100644
--- a/amaranth/vendor/_quicklogic.py
+++ b/amaranth/vendor/_quicklogic.py
@@ -71,10 +71,11 @@ class QuicklogicPlatform(TemplatedPlatform):
         """,
         "{{name}}.sdc": r"""
             # {{autogenerated}}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -period {{100000000/frequency}} {{port_signal.name|ascii_escape}}
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -period {{100000000/frequency}} {{signal.name|ascii_escape}}
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -period {{100000000/frequency}} {{port.name|ascii_escape}}
             {% endfor %}
         """
     }
diff --git a/amaranth/vendor/_siliconblue.py b/amaranth/vendor/_siliconblue.py
index 0db8380..690d7fb 100644
--- a/amaranth/vendor/_siliconblue.py
+++ b/amaranth/vendor/_siliconblue.py
@@ -134,9 +134,12 @@ class SiliconBluePlatform(TemplatedPlatform):
             {% for port_name, pin_name, attrs in platform.iter_port_constraints_bits() -%}
                 set_io {{port_name}} {{pin_name}}
             {% endfor %}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                set_frequency {{net_signal|hierarchy(".")}} {{frequency/1000000}}
-            {% endfor%}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                set_frequency {{signal|hierarchy(".")}} {{frequency/1000000}}
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                set_frequency {{port.name}} {{frequency/1000000}}
+            {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """,
     }
@@ -242,12 +245,11 @@ class SiliconBluePlatform(TemplatedPlatform):
         "{{name}}.sdc": r"""
             # {{autogenerated}}
             set_hierarchy_separator {/}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -name {{port_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port_signal.name|tcl_quote}}]
-                {% else -%}
-                    create_clock -name {{net_signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{net_signal|hierarchy("/")|tcl_quote}}]
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -name {{signal.name|tcl_quote}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")|tcl_quote}}]
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -name {{port.name|tcl_quote}} -period {{1000000000/frequency}} [get_ports {{port.name|tcl_quote}}]
             {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """,
diff --git a/amaranth/vendor/_xilinx.py b/amaranth/vendor/_xilinx.py
index 9d0cbf2..df73129 100644
--- a/amaranth/vendor/_xilinx.py
+++ b/amaranth/vendor/_xilinx.py
@@ -647,12 +647,11 @@ class XilinxPlatform(TemplatedPlatform):
                     set_property {{attr_name}} {{attr_value|tcl_quote}} [get_ports {{port_name|tcl_quote}}]
                 {% endfor %}
             {% endfor %}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is not none -%}
-                    create_clock -name {{port_signal.name|ascii_escape}} -period {{1000000000/frequency}} [get_ports {{port_signal.name|tcl_quote}}]
-                {% else -%}
-                    create_clock -name {{net_signal.name|ascii_escape}} -period {{1000000000/frequency}} [get_nets {{net_signal|hierarchy("/")|tcl_quote}}]
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -name {{signal.name|ascii_escape}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")|tcl_quote}}]
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -name {{port.name|ascii_escape}} -period {{1000000000/frequency}} [get_nets {{port|hierarchy("/")|tcl_quote}}]
             {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """
@@ -723,9 +722,13 @@ class XilinxPlatform(TemplatedPlatform):
                     NET "{{port_name}}" {{attr_name}}={{attr_value}};
                 {% endfor %}
             {% endfor %}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                NET "{{net_signal|hierarchy("/")}}" TNM_NET="PRD{{net_signal|hierarchy("/")}}";
-                TIMESPEC "TS{{net_signal|hierarchy("__")}}"=PERIOD "PRD{{net_signal|hierarchy("/")}}" {{1000000000/frequency}} ns HIGH 50%;
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                NET "{{signal|hierarchy("/")}}" TNM_NET="PRD{{signal|hierarchy("/")}}";
+                TIMESPEC "TS{{signal|hierarchy("__")}}"=PERIOD "PRD{{signal|hierarchy("/")}}" {{1000000000/frequency}} ns HIGH 50%;
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                NET "{{port|hierarchy("/")}}" TNM_NET="PRD{{port|hierarchy("/")}}";
+                TIMESPEC "TS{{port|hierarchy("__")}}"=PERIOD "PRD{{port|hierarchy("/")}}" {{1000000000/frequency}} ns HIGH 50%;
             {% endfor %}
             {{get_override("add_constraints")|default("# (add_constraints placeholder)")}}
         """
@@ -848,10 +851,11 @@ class XilinxPlatform(TemplatedPlatform):
         """,
         "{{name}}.sdc": r"""
             # {{autogenerated}}
-            {% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%}
-                {% if port_signal is none -%}
-                    create_clock -period {{1000000000/frequency}} {{net_signal.name|ascii_escape}}
-                {% endif %}
+            {% for signal, frequency in platform.iter_signal_clock_constraints() -%}
+                create_clock -period {{1000000000/frequency}} {{signal.name|ascii_escape}}
+            {% endfor %}
+            {% for port, frequency in platform.iter_port_clock_constraints() -%}
+                create_clock -period {{1000000000/frequency}} {{port.name|ascii_escape}}
             {% endfor %}
         """
     }
@@ -1173,7 +1177,8 @@ class XilinxPlatform(TemplatedPlatform):
 
     def add_clock_constraint(self, clock, frequency):
         super().add_clock_constraint(clock, frequency)
-        clock.attrs["keep"] = "TRUE"
+        if not isinstance(clock, IOPort):
+            clock.attrs["keep"] = "TRUE"
 
     def get_io_buffer(self, buffer):
         if isinstance(buffer, io.Buffer):


Test patch:
diff --git a/tests/test_build_res.py b/tests/test_build_res.py
index bda61b4..a0dbc55 100644
--- a/tests/test_build_res.py
+++ b/tests/test_build_res.py
@@ -226,17 +226,17 @@ class ResourceManagerTestCase(FHDLTestCase):
         ) = self.cm.iter_pins()
         clk100_buffer._MustUse__silence = True
         clk50_buffer._MustUse__silence = True
-        self.assertEqual(list(self.cm.iter_clock_constraints()), [
-            (clk100.i, clk100_port.p, 100e6),
-            (clk50.i, clk50_port.io, 50e6)
+        self.assertEqual(list(self.cm.iter_port_clock_constraints()), [
+            (clk100_port.p, 100e6),
+            (clk50_port.io, 50e6)
         ])
 
     def test_add_clock(self):
         with _ignore_deprecated():
             i2c = self.cm.request("i2c")
         self.cm.add_clock_constraint(i2c.scl.o, 100e3)
-        self.assertEqual(list(self.cm.iter_clock_constraints()), [
-            (i2c.scl.o, None, 100e3)
+        self.assertEqual(list(self.cm.iter_signal_clock_constraints()), [
+            (i2c.scl.o, 100e3)
         ])
         ((_, _, scl_buffer), (_, _, sda_buffer)) = self.cm.iter_pins()
         scl_buffer._MustUse__silence = True
@@ -269,7 +269,7 @@ class ResourceManagerTestCase(FHDLTestCase):
 
     def test_wrong_clock_signal(self):
         with self.assertRaisesRegex(TypeError,
-                r"^Object None is not a Signal$"):
+                r"^Object None is not a Signal or IOPort$"):
             self.cm.add_clock_constraint(None, 10e6)
 
     def test_wrong_clock_frequency(self):
@@ -335,10 +335,8 @@ class ResourceManagerTestCase(FHDLTestCase):
 
     def test_wrong_clock_constraint_twice(self):
         with _ignore_deprecated():
-            clk100 = self.cm.request("clk100")
-        (pin, port, buffer), = self.cm.iter_pins()
-        buffer._MustUse__silence = True
+            clk100 = self.cm.request("clk100", dir="-")
         with self.assertRaisesRegex(ValueError,
-                (r"^Cannot add clock constraint on \(sig clk100_0__i\), which is already "
+                (r"^Cannot add clock constraint on \(io-port clk100_0__p\), which is already "
                     r"constrained to 100000000\.0 Hz$")):
-            self.cm.add_clock_constraint(clk100.i, 1e6)
+            self.cm.add_clock_constraint(clk100.p, 1e6)