-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fragment.prepare should allow caller to handle nonexistent clock domains #57
Comments
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 however the third test still fails, when not adding the |
No, this patch is clearly wrong. |
I agree. The problem seems to be, that However I don't understand enough of the |
Let me take a look at this now. |
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
This (I modified your example a bit) is supposed to work. It asserts and this is a bug.
This should just work, so it's also clearly a bug. |
Yes, I noticed that, what's wrong with making
Ok, the original example was only that way, because without
|
There should only be a single
so if |
Ah, I see, had too much |
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 |
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
adding 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:
@sbourdeauducq @jordens Thoughts? @cr1901 This should handle your troublesome MachXO2 board without an external oscillator very nicely. |
This allows e.g. injecting a clock/reset generator in platform build code on demand (i.e. if the domain is not instantiated manually). See #57.
I am not sure if I am using
DomainRenamer
orClockSignal
wrong but I can't get them to work together.prints this
The text was updated successfully, but these errors were encountered: