cores.litedram: move name conflict detection to the builder.
authorJean-François Nguyen <jf@lambdaconcept.com>
Tue, 29 Jun 2021 16:37:35 +0000 (18:37 +0200)
committerJean-François Nguyen <jf@lambdaconcept.com>
Tue, 29 Jun 2021 16:38:24 +0000 (18:38 +0200)
examples/sdram_soc.py
lambdasoc/cores/litedram.py
lambdasoc/test/test_cores_litedram.py

index 2be6bfcae47ce4dff66dd585a7eedcddfa72b250..613a52d70e80bde8f91bf046edd6c504a6d5ef95 100644 (file)
@@ -140,10 +140,11 @@ if __name__ == "__main__":
         pins = litedram_cfg.request_pins(platform, "ddr3", 0),
     )
 
-    litedram_products = litedram_core.build(do_build=True)
-    with litedram_products.extract(f"{litedram_core.name}/{litedram_core.name}.v") as litedram_v:
-        with open(litedram_v, "r") as litedram_v_contents:
-            platform.add_file(litedram_v, litedram_v_contents)
+    litedram_builder  = litedram.Builder()
+    litedram_products = litedram_core.build(litedram_builder, do_build=True)
+
+    litedram_core_v = f"{litedram_core.name}/{litedram_core.name}.v"
+    platform.add_file(litedram_core_v, litedram_products.get(litedram_core_v, mode="t"))
 
     soc = SDRAMSoC(
          reset_addr=0x30000000, clk_freq=litedram_cfg.user_clk_freq,
index 9936b857bceee0eb482d05780231e16c70ce8b0f..c5a7c601b2290c6806c1e9860aebf84678b3b3e4 100644 (file)
@@ -374,26 +374,6 @@ class NativePort(Record):
 
 
 class Core(Elaboratable):
-    _namespace = set()
-
-    @classmethod
-    def clear_namespace(cls):
-        """Clear private namespace.
-
-        Every time an instance of :class:`litedram.Core` is created, its name is stored in a
-        private namespace. This allows us to detect name collisions, which are problematic for at
-        least two reasons:
-            * by default, a sub-directory named after the instance is created at build-time in
-            order to isolate it from other LiteDRAM builds. A collision would overwrite previous
-            build products.
-            * the instance name becomes the name of its top-level Verilog module. Importing two
-            modules with the same name will cause a toolchain error.
-
-        :meth:`litedram.Core.clear_namespace` resets this namespace. It is intended for cases where
-        stateless class instantiations are desirable, such as unit testing.
-        """
-        cls._namespace.clear()
-
     """An nMigen wrapper for a standalone LiteDRAM core.
 
     Parameters
@@ -432,16 +412,7 @@ class Core(Elaboratable):
 
         if name is not None and not isinstance(name, str):
             raise TypeError("Name must be a string, not {!r}".format(name))
-        name = name or tracer.get_var_name(depth=2 + src_loc_at)
-
-        if not name_force and name in Core._namespace:
-            raise ValueError(
-                "Name '{}' has already been used for a previous litedram.Core instance. Building "
-                "this instance may overwrite previous build products. Passing `name_force=True` "
-                "will disable this check.".format(name)
-            )
-        Core._namespace.add(name)
-        self.name = name
+        self.name = name or tracer.get_var_name(depth=2 + src_loc_at)
 
         module = config.get_module()
         size   = config.module_bytes \
@@ -490,24 +461,34 @@ class Core(Elaboratable):
                                  "Core.ctrl_bus")
         return self._ctrl_bus
 
-    def build(self, do_build=True, build_dir="build/litedram", sim=False):
+    def build(self, builder, *, do_build=True, build_dir="build/litedram", sim=False,
+            name_force=False):
         """Build the LiteDRAM core.
 
         Arguments
         ---------
+        builder: :class:`litedram.Builder`
+            Builder instance.
         do_build : bool
-            Build the LiteDRAM core. Defaults to `True`.
+            Execute the build locally. Defaults to ``True``.
         build_dir : str
-            Build directory.
+            Root build directory. Defaults to ``"build/litedram"``.
         sim : bool
-            Do the build in simulation mode (i.e. with a PHY model). Defaults to `False`.
+            Do the build in simulation mode (i.e. by replacing the PHY with a model). Defaults to
+            ``False``.
+        name_force : bool
+            Ignore builder name conflicts. Defaults to ``False``.
 
         Return value
         ------------
         An instance of :class:`nmigen.build.run.LocalBuildProducts` if ``do_build`` is ``True``.
         Otherwise, an instance of :class:``nmigen.build.run.BuildPlan``.
         """
-        plan = Builder().prepare(self, build_dir, sim)
+        if not isinstance(builder, Builder):
+            raise TypeError("Builder must be an instance of litedram.Builder, not {!r}"
+                            .format(builder))
+
+        plan = builder.prepare(self, sim=sim, name_force=name_force)
         if not do_build:
             return plan
 
@@ -611,29 +592,6 @@ class Core(Elaboratable):
 
 
 class Builder:
-    """
-    LiteDRAM builder
-    ----------------
-
-    Build products (any):
-        * ``{{top.name}}_csr.csv`` : CSR listing.
-        * ``{{top.name}}/build_{{top.name}}.sh``: LiteDRAM build script.
-        * ``{{top.name}}/{{top.name}}.v`` : LiteDRAM core.
-        * ``{{top.name}}/software/include/generated/csr.h`` : CSR accessors.
-        * ``{{top.name}}/software/include/generated/git.h`` : Git version.
-        * ``{{top.name}}/software/include/generated/mem.h`` : Memory regions.
-        * ``{{top.name}}/software/include/generated/sdram_phy.h`` : SDRAM initialization sequence.
-        * ``{{top.name}}/software/include/generated/soc.h`` : SoC constants.
-
-    Build products (ECP5):
-        * ``{{top.name}}/{{top.name}}.lpf`` : Constraints file.
-        * ``{{top.name}}/{{top.name}}.ys`` : Yosys script.
-
-    Build products (Artix 7):
-        * ``{{top.name}}/{{top.name}}.xdc`` : Constraints file
-        * ``{{top.name}}/{{top.name}}.tcl`` : Vivado script.
-    """
-
     file_templates = {
         "build_{{top.name}}.sh": r"""
             # {{autogenerated}}
@@ -703,10 +661,81 @@ class Builder:
         """,
     ]
 
-    def prepare(self, top, build_dir, sim):
-        if not isinstance(top, Core):
-            raise TypeError("Top module must be an instance of litedram.Core, not {!r}"
-                            .format(top))
+    """LiteDRAM builder.
+
+    Build products
+    --------------
+
+    Any platform:
+        * ``{{top.name}}_csr.csv`` : CSR listing.
+        * ``{{top.name}}/build_{{top.name}}.sh``: LiteDRAM build script.
+        * ``{{top.name}}/{{top.name}}.v`` : LiteDRAM core.
+        * ``{{top.name}}/software/include/generated/csr.h`` : CSR accessors.
+        * ``{{top.name}}/software/include/generated/git.h`` : Git version.
+        * ``{{top.name}}/software/include/generated/mem.h`` : Memory regions.
+        * ``{{top.name}}/software/include/generated/sdram_phy.h`` : SDRAM initialization sequence.
+        * ``{{top.name}}/software/include/generated/soc.h`` : SoC constants.
+
+    Lattice ECP5 platform:
+        * ``{{top.name}}/{{top.name}}.lpf`` : Constraints file.
+        * ``{{top.name}}/{{top.name}}.ys`` : Yosys script.
+
+    Xilinx Artix7 platform:
+        * ``{{top.name}}/{{top.name}}.xdc`` : Constraints file
+        * ``{{top.name}}/{{top.name}}.tcl`` : Vivado script.
+
+    Name conflict avoidance
+    -----------------------
+
+    Every time :meth:`litedram.Builder.prepare` is called, the name of the :class:`litedram.Core`
+    instance is added to ``namespace`. This allows the detection of name conflicts, which are
+    problematic for the following reasons:
+        * if two build plans are executed locally within the same root directory, the latter could
+          overwrite the products of the former.
+        * the LiteDRAM instance name becomes the name of its top-level Verilog module; importing
+          two modules with the same name will cause a toolchain error.
+
+    Attributes
+    ----------
+    namespace : set(str)
+        Builder namespace.
+    """
+    def __init__(self):
+        self.namespace = set()
+
+    def prepare(self, core, *, sim=False, name_force=False):
+        """Prepare a build plan.
+
+        Arguments
+        ---------
+        core : :class:`litedram.Core`
+            The LiteDRAM instance to be built.
+        sim : bool
+            Do the build in simulation mode (i.e. by replacing the PHY with a model).
+        name_force : bool
+            Force name. If ``True``, no exception will be raised in case of a name conflict with a
+            previous LiteDRAM instance.
+
+        Return value
+        ------------
+        A :class:`nmigen.build.run.BuildPlan` for this LiteDRAM instance.
+
+        Exceptions
+        ----------
+        Raises a :exn:`ValueError` if ``core.name`` conflicts with a previous build plan and
+        ``name_force`` is ``False``.
+        """
+        if not isinstance(core, Core):
+            raise TypeError("LiteDRAM core must be an instance of litedram.Core, not {!r}"
+                            .format(core))
+
+        if core.name in self.namespace and not name_force:
+            raise ValueError(
+                "LiteDRAM core name '{}' has already been used for a previous build. Building "
+                "this instance may overwrite previous build products. Passing `name_force=True` "
+                "will disable this check".format(core.name)
+            )
+        self.namespace.add(core.name)
 
         autogenerated = f"Automatically generated by LambdaSoC {__version__}. Do not edit."
 
@@ -727,13 +756,12 @@ class Builder:
                 raise
             return compiled.render({
                 "autogenerated": autogenerated,
-                "build_dir": os.path.abspath(build_dir),
                 "emit_commands": emit_commands,
                 "sim": sim,
-                "top": top,
+                "top": core,
             })
 
-        plan = BuildPlan(script=f"build_{top.name}")
+        plan = BuildPlan(script=f"build_{core.name}")
         for filename_tpl, content_tpl in self.file_templates.items():
             plan.add_file(render(filename_tpl, origin=filename_tpl),
                           render(content_tpl,  origin=content_tpl))
index 58c189359b5bad7d6ae2ca920371a56d064dbd23..aabf6f8abf9ab285194b1cc4a7cbe204863ef826 100644 (file)
@@ -414,12 +414,6 @@ class CoreTestCase(unittest.TestCase):
             init_clk_freq  = int(25e6),
         )
 
-    def setUp(self):
-        litedram.Core.clear_namespace()
-
-    def tearDown(self):
-        litedram.Core.clear_namespace()
-
     def test_simple(self):
         core = litedram.Core(self._cfg)
         self.assertIs(core.config, self._cfg)
@@ -452,10 +446,44 @@ class CoreTestCase(unittest.TestCase):
                 r"Name must be a string, not 42"):
             core = litedram.Core(self._cfg, name=42)
 
-    def test_wrong_name_collision(self):
-        core_1 = litedram.Core(self._cfg, name="core")
+
+class BuilderTestCase(unittest.TestCase):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self._cfg = litedram.ECP5Config(
+            memtype        = "DDR3",
+            module_name    = "MT41K256M16",
+            module_bytes   = 2,
+            module_ranks   = 1,
+            input_clk_freq = int(100e6),
+            user_clk_freq  = int(70e6),
+            init_clk_freq  = int(25e6),
+        )
+
+    def test_prepare(self):
+        core = litedram.Core(self._cfg)
+        builder = litedram.Builder()
+        builder.prepare(core)
+        self.assertEqual(list(builder.namespace), ["core"])
+
+    def test_prepare_name_conflict(self):
+        core = litedram.Core(self._cfg)
+        builder = litedram.Builder()
+        builder.prepare(core)
         with self.assertRaisesRegex(ValueError,
-                r"Name 'core' has already been used for a previous litedram\.Core instance\. "
-                r"Building this instance may overwrite previous build products. Passing "
-                r"`name_force=True` will disable this check."):
-            core_2 = litedram.Core(self._cfg, name="core")
+                r"LiteDRAM core name 'core' has already been used for a previous build\. Building "
+                r"this instance may overwrite previous build products\. Passing `name_force=True` "
+                r"will disable this check"):
+            builder.prepare(core)
+
+    def test_prepare_name_force(self):
+        core = litedram.Core(self._cfg)
+        builder = litedram.Builder()
+        builder.prepare(core)
+        builder.prepare(core, name_force=True)
+
+    def test_prepare_wrong_core(self):
+        builder = litedram.Builder()
+        with self.assertRaisesRegex(TypeError,
+                r"LiteDRAM core must be an instance of litedram.Core, not 'foo'"):
+            builder.prepare("foo")