Ok managed to look into this a bit further, it looks to me like a actual bug.

This patch

--- a/nmigen/hdl/xfrm.py
+++ b/nmigen/hdl/xfrm.py
@@ -305,7 +305,7 @@ class TransformedElaboratable:
         return getattr(self._elaboratable_, attr)

     def elaborate(self, platform):
-        fragment = Fragment.get(self._elaboratable_, platform)
+        fragment = Fragment.get(self._elaboratable_, platform).prepare()
         for transform in self._transforms_:
             fragment = transform(fragment)
         return fragment

makes the tests work, after removing the manual addition of the other ClockDomain

however the third test still fails, when not adding the sync clock domain manually.

No, this patch is clearly wrong. prepare should never be called until the fragment reaches the backend.

I agree.

The problem seems to be, that map_domains of the DomainRenamer is called before the sync clock domain actually exists, as it is only created by fragment._propagate_domains in prepare.

However I don't understand enough of the nmigen internals to properly fix that (yet).

Let me take a look at this now.

    m = Module()
    m.d.comb += ClockSignal().eq(0)
    m1 = DomainRenamer("other")(m)
    main(m1)

So, this is supposed to fail. There is one somewhat unintuitive aspect of clock domains. If there is at least one clock domain in the system--defined by m.domains += ..., then prepare does nothing, but if there are no defined domains, sync is created by default.

    m = Module()
    m.d.comb += ClockSignal().eq(0)

    m = DomainRenamer("other")(m)

    m.domains += [ClockDomain("other")]
    main(m)

This (I modified your example a bit) is supposed to work. It asserts and this is a bug.

    m = Module()
    m.d.comb += ClockSignal().eq(0)

    # this testcase doesn't fail if these two lines are omitted
    a = Signal()
    m.d.sync += a.eq(0)

    m_super = Module()
    m_super.domains += [ClockDomain("sync"), ClockDomain("other")]
    m_super.submodules.m = DomainRenamer("other")(m)

    main(m_super)

This should just work, so it's also clearly a bug.

    m = Module()
    m.d.comb += ClockSignal().eq(0)
    m1 = DomainRenamer("other")(m)
    main(m1)

So, this is supposed to fail. There is one somewhat unintuitive aspect of clock domains. If there is at least one clock domain in the system--defined by m.domains += ..., then prepare does nothing, but if there are no defined domains, sync is created by default.

Yes, I noticed that, what's wrong with making DomainRenamer create the clock domain?

    m = Module()
    m.d.comb += ClockSignal().eq(0)

    m = DomainRenamer("other")(m)

    m.domains += [ClockDomain("other")]
    main(m)

This (I modified your example a bit) is supposed to work. It asserts and this is a bug.

Ok, the original example was only that way, because without ClockDomain("sync") it produced Signal (clk sync) refers to nonexistent domain 'sync'.

    m = Module()
    m.d.comb += ClockSignal().eq(0)

    # this testcase doesn't fail if these two lines are omitted
    a = Signal()
    m.d.sync += a.eq(0)

    m_super = Module()
    m_super.domains += [ClockDomain("sync"), ClockDomain("other")]
    m_super.submodules.m = DomainRenamer("other")(m)

    main(m_super)

This should just work, so it's also clearly a bug.

Yes, I noticed that, what's wrong with making DomainRenamer create the clock domain?

There should only be a single ClockDomain object for a given clock domain name in the entire design (since that's where the actual clock signal lives). Consider that most uses of DomainRenamer would be something like:

self.submodules += DomainRenamer("pix")(self.pixel_dac)
# in the same module, or maybe way above in the hierarchy
self.domains += ClockDomain("pix")

so if DomainRenamer created the domain it would lead to a conflict then.

Ah, I see, had too much migen and its handling of clock domains left in my head ;)

I actually haven't changed that part, or at least haven't intended to... Did I miss something?

well something like this

from migen import *
from migen.fhdl import verilog

class A(Module):
    def __init__(self, width):
        a = Signal(width)
        self.sync += a.eq(0)

class B(Module):
    def __init__(self):
        self.submodules += ClockDomainsRenamer("domain_a")(A(1))
        self.submodules += ClockDomainsRenamer("domain_b")(A(2))
        self.submodules += ClockDomainsRenamer("domain_a")(A(3))

if __name__ == "__main__":
    print(verilog.convert(B()))

certainly works in migen

Ah and of course this also works

from migen import *
from migen.fhdl import verilog

class Test(Module):
    def __init__(self):
        a = Signal(1)
        b = Signal(2)

        self.comb += ClockSignal("a").eq(1)

        self.sync.a += a.eq(1)
        self.sync.b += b.eq(2)


if __name__ == "__main__":
    print(verilog.convert(Test()))

So, your first example should work in nmigen (if it doesn't it's a bug), and I was under impression that the second one doesn't work in migen. But it does. So maybe we should fix that, too.

okay, so

def A(width):
    m = Module()
    a = Signal(width)
    m.d.sync += a.eq(0)
    return m

m = Module()
m.submodules += DomainRenamer("a")(A(1))
m.submodules += DomainRenamer("b")(A(2))
m.submodules += DomainRenamer("a")(A(3))

main(m)

would be my translation of the first example to nmigen and that does not work:

  File "cdc.py", line 17, in <module>
    main(m)
  File "/home/robin/projects/nmigen_test/nmigen/cli.py", line 76, in main
    main_runner(parser, parser.parse_args(), *args, **kwargs)
  File "/home/robin/projects/nmigen_test/nmigen/cli.py", line 58, in main_runner
    output = verilog.convert(fragment, name=name, ports=ports)
  File "/home/robin/projects/nmigen_test/nmigen/back/verilog.py", line 29, in convert
    il_text = rtlil.convert(*args, **kwargs)
  File "/home/robin/projects/nmigen_test/nmigen/back/rtlil.py", line 852, in convert
    fragment = ir.Fragment.get(fragment, platform=None).prepare(**kwargs)
  File "/home/robin/projects/nmigen_test/nmigen/hdl/ir.py", line 383, in prepare
    fragment._propagate_ports(ports)
  File "/home/robin/projects/nmigen_test/nmigen/hdl/ir.py", line 350, in _propagate_ports
    sub_ins, sub_outs, sub_inouts = subfrag._propagate_ports(ports=())
  File "/home/robin/projects/nmigen_test/nmigen/hdl/ir.py", line 334, in _propagate_ports
    cd = self.domains[domain]
KeyError: 'a'

adding m.domains += [ClockDomain("a"), ClockDomain("b")] makes it work.

Also if the first example should work, I don't understand why

m = Module()
m.d.comb += ClockSignal().eq(0)
m1 = DomainRenamer("other")(m)
main(m1)

should fail.

So, I thought about this and here's what I am thinking we should do:

1. In Fragment.prepare, instead of the current ensure_sync_exists hack, add a callback that fires for each clock domain that is not explicitly defined in the design. If not specified, this callback will silently create a fresh domain with a synchronous reset, and (if ports are explicitly specified) its clock and reset signals will be added to the ports list. This would replace the oMigen functionality that made examples convenient.
2. In the platform layer, add two overridable functions: create_domain and create_sync_domain. By default, create_domain will raise an error for every domain other than sync, and it will try to use create_sync_domain (if available) to create sync. This would replace the oMigen default_clk_* functionality by allowing each platform to perform the most appropriate actions; most will probably instantiate a CRG with nothing but a clock, but for example Versa ECP5 has a reset button, which ought to be hooked up to the CRG. Also, complex boards with many domains can still be handled, unlike with the primitive default_clk_* mechanism.

@sbourdeauducq @jordens Thoughts?

@cr1901 This should handle your troublesome MachXO2 board without an external oscillator very nicely.