From 7668d2cc69bb49d1e77ea9701779451224ca0b92 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jean-Fran=C3=A7ois=20Nguyen?= Date: Tue, 29 Jun 2021 18:37:35 +0200 Subject: [PATCH] cores.litedram: move name conflict detection to the builder. --- examples/sdram_soc.py | 9 +- lambdasoc/cores/litedram.py | 158 +++++++++++++++----------- lambdasoc/test/test_cores_litedram.py | 52 +++++++-- 3 files changed, 138 insertions(+), 81 deletions(-) diff --git a/examples/sdram_soc.py b/examples/sdram_soc.py index 2be6bfc..613a52d 100644 --- a/examples/sdram_soc.py +++ b/examples/sdram_soc.py @@ -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, diff --git a/lambdasoc/cores/litedram.py b/lambdasoc/cores/litedram.py index 9936b85..c5a7c60 100644 --- a/lambdasoc/cores/litedram.py +++ b/lambdasoc/cores/litedram.py @@ -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)) diff --git a/lambdasoc/test/test_cores_litedram.py b/lambdasoc/test/test_cores_litedram.py index 58c1893..aabf6f8 100644 --- a/lambdasoc/test/test_cores_litedram.py +++ b/lambdasoc/test/test_cores_litedram.py @@ -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") -- 2.30.2