m-labs / migen Goto Github PK

Travis-CI has pretty good support for doing CI on python packages, see http://docs.travis-ci.com/user/languages/python/ -- It would be nice for migen to use this.

I'm guessing that might be a low priority as migen is already tested by misoc?

This isn't necessarily a bug, but I do feel it makes Verilog identifiers that Migen generates more difficult to read.

Migen has a tendency to decorate Verilog identifiers with the __main__ prefix if declared in a top level module. Strictly speaking, these identifiers do not appear to be necessary, and only increase identifier length/verbosity.

The following gist demonstrates the behavior: https://gist.github.com/cr1901/c20cbc5b1eb0d8d43578

In the first .py file of the above gist, ModA() receives no decoration, but signals declared in ModTop() and ModB() receive __main__ decorators. The behavior is not consistent and seems to be related to whether ClockDomains are manually defined. In the second .py file of the above gist, the ModA clock domain is removed, the sys clock domain is inferred in ModTop() and verilog.convert, and the __main__ decoration in the generated Verilog disappears. Except for the always@ for ModA's clock, the two files are functionally identical, but the first generates more verbose identifiers.

Simulating on a Windows machine (Windows XP) seems to randomly trigger the following assertion:
https://github.com/m-labs/migen/blob/master/vpi/ipc.c#L195

This can be reproduced with LiteSATA tests:
https://github.com/m-labs/misoc/tree/master/misoclib/mem/litesata/test

• make scrambler_tb triggers the issue from time to time.
• make link_tb seems to always trigger the issue.

(Note: to run these tests, the following verilog.py file should probably be used to avoid the verilog hanging issue: https://github.com/enjoy-digital/migen/blob/master/migen/fhdl/verilog.py)

Apparently, the proper way to constrain some CDCs (Both AsyncResetSynchronizer and transfer via GreyCounters) should use a max_delay constraint and not a false path.

https://forums.xilinx.com/t5/Vivado-TCL-Community/How-to-set-timing-constraint-in-this-case/m-p/510771/highlight/true#M2049

How would one implement this?

A good entry to the rabbit hole:
copied from http://www.colmryan.org/avrums-clock-domain-crossing-widsom.html#avrums-clock-domain-crossing-widsom :

It seems that MultiReg is not simulated correctly with the new simulator:

from migen import *
from migen.genlib.cdc import MultiReg

class Test(Module):
    def __init__(self):
        self.src = Signal(8, reset=0xA5)
        self.dst_a = Signal(8, reset=0x5A)
        self.dst_b = Signal(8, reset=0x5A)
        # complaining if multireg clock domain not declared in clocks
        self.sync.multireg += self.dst_a.eq(self.src)
        # not complaining if multireg clock domain not declared in clocks
        self.specials += MultiReg(self.src, self.dst_b, "multireg")

def test(dut):
    for i in range(16):
        print("src: %02x, dst_a: %02x, dst_b: %02x"%((yield dut.src), (yield dut.dst_a), (yield dut.dst_b)))
        yield

if __name__ == "__main__":
    dut = Test()
    run_simulation(dut, {"sys": test(dut)}, clocks={"sys": 10, "multireg": 10}, vcd_name="sim.vcd")

reports:

src: a5, dst_a: 5a, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a
src: a5, dst_a: a5, dst_b: 5a

See the following output;

cc -Wall -O2  -fPIC -Wall -Wshadow -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Wformat-security -I/usr/include/iverilog -c  -o ipc.o ipc.c
ipc.c: In function ‘ipc_receive’:
ipc.c:98:17: warning: declaration of ‘index’ shadows a global declaration [-Wshadow]
ipc.c:113:17: warning: declaration of ‘index’ shadows a global declaration [-Wshadow]

NeroJTAG is a fully open specification for FPGA programmers as part of the FPGALink project (https://github.com/makestuff/libfpgalink/wiki/FPGALink) by makestuff (http://github.com/makestuff). There exists both an AVR based and Cypress FX2 based implementations of the firmware.

It would be great if FPGALink was supported as a "programmer" type similar to xc3sprog. (It would also be awesome if there was support for using FPGALink to communicate with the FPGA board but that is probably more a misoc rather than migen thing?)

The Cypress FX2 firmware supports a large number of Digilent's boards, replacing the proprietary "Adept" firmware. (Of interest too myself is the Digilent Atlys board.) From the FPGALink wiki,

Quick-starts for several devkits

This is just a selection; there are many more supported configurations.
Quickstart for Aessent aes220
Quickstart for Canton Electronics LX9 (requires external AVR board)
Quickstart for Digilent Atlys
Quickstart for Digilent FMC Carrier S6
Quickstart for Digilent Nexys2
Quickstart for Digilent Nexys3
Quickstart for MakeStuff LX9R3

I'm curious if I am using the library wrong. I have installed it with python 3.4.3 and python 3.3 and still the same error persists in my project.

ImportError: No module named 'mibuild'

Please can some one tell me what I am doing wrong?

Thanks

A circular shift register:

class T(Module):
    def __init__(self):
        self.sr = sr = Signal(8)
        self.sync += sr[1:].eq(sr)
        # self.comb += sr[0].eq(sr[-1])  # adding this hides sr from the vcd

    def run(self):
        yield self.sr[1:].eq(0x55)  # commenting this hides sr from the vcd
        for i in range(8):
            yield

tb = T()
run_simulation(tb, tb.run(), vcd_name="sr.vcd")

As it is written, it does not do much. But when either of the two changes are done, sr is not handled in the simulation and does not appear in the vcd.

17:07 and something that regularly bothers me about CSR: there is "atomic_write" (write form the bus/cpu perspective) but "w", "r", "we", and "re" are from the gateware perspective. The orientation of the later seems the wrong one to me.
17:22 afaict reads and writes are usually from the perspective of the initiator of the transaction. and since the csr-holding gateware is not the bus master, r/w should be flipped.
17:26 <florent> rjo: I agree on that (even if that's not problematic, the orientation is not natural for me)

At "https://github.com/m-labs/migen/blob/master/migen/build/tools.py#L8" mkdir_noerror() is not capable to make multideph directories. "os.makedirs()" can do this

Hi,

I was trying to simulate a simple Power-on-reset design using Migen (Code: https://paste.debian.net/927041/ )

Issue

When running the above code, Migen generates redundant clock domains for both sys and por, as can be seen in the GTKWave screenshot below:

I traced why this was happening. I found out that this if block in the screenshot was evaluating to True (i.e, the particular clock was not found in the clock_domains list, which is incorrect since the clock_domains list does actually contain a sys clock):

https://github.com/m-labs/migen/blob/master/migen/sim/core.py#L271

Even though clock variable already had value of sys (str), and the self.fragment.clock_domains too had a clock domain named sys, this if condition was still failing, and due to that migen created a new clock domain for sys.

This issue seems to be because _ClockDomainList class doesn't implement its own __contains__ method which takes str-type parameters
https://github.com/m-labs/migen/blob/master/migen/fhdl/structure.py#L688

Possible Fix:

If I implement a __contains__ method for _ClockDomainList class which takes either cd_or_name as parameter, then I get expected output in GTKWave as seen below:

I would be happy to submit a fix for this if it is confirmed as indeed a bug.

https://bugs.python.org/issue27213 breaks fhdl.tracer.

Need ASYNC_REG and a false path to the reset.
https://forums.xilinx.com/t5/Timing-Analysis/set-false-path/m-p/638630/highlight/true#M8189

/home/travis/virtualenv/python3.4.2/lib/python3.4/site-packages/setuptools/dist.py:292: UserWarning: The version specified ('unknown') is an invalid version, this may not work as expected with newer versions of setuptools, pip, and PyPI. Please see PEP 440 for more details.

Any attempt to define a clock domain manually in a Migen source file that uses a simulation will cause simulations to fail. Migen decorates the clk and rst signals with a trailing number, either in the top level simulation module or the DUT module, but does not decorate the clk and rst signals in the other module with the same trailing number. This causes undefined references in simulators such as iverilog.

Migen should recognize in the context of simulations that the clk and rst signals in both the DUT and top level modules are in fact the same signal defined twice.

This bug is demonstrated in the following gist: https://gist.github.com/cr1901/8fb5298fe9982456b9d4

A workaround for the time being is to conditionally define clock domains to be defined depending on whether the user wants to simulate the module, or use the module as part of synthesis (e.g. if __name__ == "__main__").

m-labs/artiq#341

Being able to install migen through PyPi would be a useful for stable configurations. It would also increase the exposure of migen to more of the Python community.

Support for reset-less Signals would avoid having to create a reset_less clock domain just to be able to have a single reset_less register. E.g. for inferring shift registers in spartan 6.

When defining a record with no fields, as in
_layout = []
and then using raw_bits(), it will generate invalid verilog, e.g.:
assign {} = x;

Dual-port block RAMs in some FPGAs can have ports with different sizes. This feature cannot currently be used with Migen.

(From #72)

Currently a reset from either the read or from the write CD resets that side's (and only that side's) address (Gray) counter.

This is probably wrong as it can lead to spurious data becoming appearing on the read side or/and data written on the write side being lost. The latter is probably less dangerous though. Instead we probably should

1. not reset it by default and instead provide an explicit (async) reset signal to be used from either CD or both
2. reset the counters to the other side's counter (but that's a race)
3. reset both when there is a reset from either CD (similarly to what ElasticBuffer does)
4. choose some asymmetric behavior, i.e. have only the read side reset drop all data but the write side reset do nothing

I am leaning towards (1) and changing the behavior of ElasticBuffer and GearBox in the same way..

@sbourdeauducq @enjoy-digital

The following code, with x a n-bit signal:
Cat(x+1, 1)
should produce a n+2 bit value (carry from the addition, plus 1 added by Cat).

The simulator implements this behavior, but the Verilog output is {1'b1, x+1}, which is only n+1 bits (the carry bit from the addition is lost).

I've been mostly using VHDL, so I'm in the habit of assigning to slices which is causing me some issues with migen.

class Minimal(Module):

    def __init__(self):
        # Ports  
        i = Signal()
        o = Signal(2)
        self.ios = {i, o}
        self.comb += (o[0].eq(i))
        self.comb += (o[1].eq(~i))

produces the following verilog

/* Machine-generated using Migen */
module top(
    input i,
    output reg [1:0] o
);


// synthesis translate_off
reg dummy_s;
initial dummy_s <= 1'd0;
// synthesis translate_on

// synthesis translate_off
reg dummy_d;
// synthesis translate_on
always @(*) begin
    o <= 2'd0;
    o[0] <= i;
    o[1] <= (~i);
// synthesis translate_off
    dummy_d <= dummy_s;
// synthesis translate_on
end

endmodule

When simulated in Vivado this gets stuck in a loop. Removing "o <+ 2'd0" fixes the problem.

It feels like there are two issues there:

1. The unnecessary assigning of the reset value in the @(*) block.
2. The incorrect use of "reg" for the output rather than "wire".

See m-labs/artiq#296.

from migen import *

@FullMemoryWE()
class Example(Module):
    def __init__(self):
        self.specials.mem = Memory(32, 100)
        self.specials.p1 = self.mem.get_port(we_granularity=8)

run_simulation(Example(), iter([]))

Traceback (most recent call last):
  File "examples/basic/memory.py", line 9, in <module>
    run_simulation(Example(), iter([]))
  File "/home/rj/work/software/migen/migen/migen/sim/core.py", line 364, in run_simulation
    with Simulator(*args, **kwargs) as s:
  File "/home/rj/work/software/migen/migen/migen/sim/core.py", line 235, in __init__
    raise ValueError("Could not lower all specials", self.fragment.specials)
ValueError: ('Could not lower all specials', {<migen.fhdl.specials._MemoryPort object at 0x7fa5f451b630>})

FullMemoryWE leaves the old "primary" ports hanging around.
Same is true for SplitMemory because it uses the same design.
We could just ignore Specials that emit empty verilog. There is no need to lower them.

What is the reason for it? Maybe it could at least have a default value?

Looks like when my readme-update changes were merged the following was changed;

++<<<<<<< HEAD
 +http://m-labs.hk/gateware.html
++=======
+ http://migen.readthedocs.org/en/latest/
+ 
+ Install instructions:
+ https://migen.readthedocs.org/en/latest/introduction.html#installing-migen
++>>>>>>> 62d264e... Adding links to the readthedocs.org documentation.

Adding the http://m-labs.hk/gateware.html link is a good idea (the user guide, tutorial and lecture slide stuff is pretty awesome!)

However, I was wondering why the links to readthedocs documentation was dropped? I was also unable to find any links to the readthedocs page on the gateware page. Is there something wrong with the documentation at migen.readthedocs.org?

I/O names can change when little modifications are done to the code.

We should to to use name_override on I/Os to get consistent interfaces (useful when doing top level simulation with an external simulator or when building a core).

Here is a quick and dirty implementation:
enjoy-digital/litex@a716c56

Do you see a better way to do this?

Because ISE is retarded, if you have anything other than period constraints on a TNM_NET (such as a TIG), then it is unable to trace the constraints through any of the clock elements like PLLs and DCMs. See timvideos/HDMI2USB-litex-firmware#210 for a lot more info.

The solution for ISE is to generate two TMN with different names on the same signals and use one only for period constraints and another for TIG constraints.

This means that cd55748#diff-15950acffd0d546d5f0722a73827c20aR203 should be changed so it uses a name other then GRP. Will send a patch soon.

Patch below is an incomplete first implementation.

diff --git a/examples/basic/fsm.py b/examples/basic/fsm.py
index 7692467..970567e 100644
--- a/examples/basic/fsm.py
+++ b/examples/basic/fsm.py
@@ -6,6 +6,7 @@ class Example(Module):
    def __init__(self):
        self.s = Signal()
        self.counter = Signal(8)
+       x = Array([Signal(name="a") for i in range(7)])

        myfsm = FSM()
        self.submodules += myfsm
@@ -17,6 +18,7 @@ class Example(Module):
        myfsm.act("BAR",
            self.s.eq(0),
            NextValue(self.counter, self.counter + 1),
+           NextValue(x[self.counter], 89),
            NextState("FOO")
        )

diff --git a/migen/fhdl/structure.py b/migen/fhdl/structure.py
index 071ae23..a05ce9d 100644
--- a/migen/fhdl/structure.py
+++ b/migen/fhdl/structure.py
@@ -446,14 +446,14 @@ class _ArrayProxy(Value):
        self.key = key

    def __getattr__(self, attr):
-       return _ArrayProxy([getattr(choice, attr) for choice in self.choices],
+       return _ArrayProxy(tuple(getattr(choice, attr) for choice in self.choices),
            self.key)

    def __getitem__(self, key):
-       return _ArrayProxy([choice.__getitem__(key) for choice in self.choices],
+       return _ArrayProxy(tuple(choice.__getitem__(key) for choice in self.choices),
            self.key)

-class Array(list):
+class Array(tuple):
    """Addressable multiplexer

    An array is created from an iterable of values and indexed using the
@@ -486,7 +486,7 @@ class Array(list):
        if isinstance(key, Value):
            return _ArrayProxy(self, key)
        else:
-           return list.__getitem__(self, key)
+           return tuple.__getitem__(self, key)

 class ClockDomain:
    """Synchronous domain
diff --git a/migen/fhdl/tools.py b/migen/fhdl/tools.py
index ed74ab0..4823176 100644
--- a/migen/fhdl/tools.py
+++ b/migen/fhdl/tools.py
@@ -1,5 +1,5 @@
 from migen.fhdl.structure import *
-from migen.fhdl.structure import _Slice, _Assign
+from migen.fhdl.structure import _Slice, _Assign, _ArrayProxy
 from migen.fhdl.visit import NodeVisitor, NodeTransformer
 from migen.fhdl.bitcontainer import value_bits_sign
 from migen.util.misc import flat_iteration
@@ -247,3 +247,28 @@ def rename_clock_domain(f, old, new):
        pass
    else:
        cd.rename(new)
+
+class FHDLComparer:
+   def __init__(self, obj):
+       self.obj = obj
+
+   def __eq__(self, other):
+       if not isinstance(other, FHDLComparer):
+           raise TypeError
+       if not isinstance(other.obj, type(self.obj)):
+           return False
+       if isinstance(self.obj, Signal):
+           return self.obj is other.obj
+       elif isinstance(self.obj, _ArrayProxy):
+           return list(map(FHDLComparer, self.obj.choices)) == list(map(FHDLComparer, other.obj.choices)) \
+               and FHDLComparer(self.obj.key) == FHDLComparer(other.obj.key)
+       else:
+           raise NotImplementedError
+
+   def __hash__(self):
+       if isinstance(self.obj, Signal):
+           return hash(self.obj)
+       elif isinstance(self.obj, _ArrayProxy):
+           return hash((tuple(map(FHDLComparer, self.obj.choices)), FHDLComparer(self.obj.key)))
+       else:
+           raise NotImplementedError
diff --git a/migen/genlib/fsm.py b/migen/genlib/fsm.py
index 44a1be3..40adc91 100644
--- a/migen/genlib/fsm.py
+++ b/migen/genlib/fsm.py
@@ -3,6 +3,7 @@ from collections import OrderedDict
 from migen.fhdl.std import *
 from migen.fhdl.module import FinalizeError
 from migen.fhdl.visit import NodeTransformer
+from migen.fhdl.tools import FHDLComparer
 from migen.fhdl.bitcontainer import value_bits_sign

 class AnonymousState:
@@ -24,7 +25,7 @@ class _LowerNext(NodeTransformer):
        self.next_state_signal = next_state_signal
        self.encoding = encoding
        self.aliases = aliases
-       # register -> next_value_ce, next_value
+       # FHDLComparer(register) -> next_value_ce, next_value
        self.registers = OrderedDict()

    def visit_unknown(self, node):
@@ -36,12 +37,12 @@ class _LowerNext(NodeTransformer):
            return self.next_state_signal.eq(self.encoding[actual_state])
        elif isinstance(node, NextValue):
            try:
-               next_value_ce, next_value = self.registers[node.register]
+               next_value_ce, next_value = self.registers[FHDLComparer(node.register)]
            except KeyError:
                related = node.register if isinstance(node.register, Signal) else None
                next_value = Signal(bits_sign=value_bits_sign(node.register), related=related)
                next_value_ce = Signal(related=related)
-               self.registers[node.register] = next_value_ce, next_value
+               self.registers[FHDLComparer(node.register)] = next_value_ce, next_value
            return next_value.eq(node.value), next_value_ce.eq(1)
        else:
            return node
@@ -126,7 +127,7 @@ class FSM(Module):
        ]
        self.sync += self.state.eq(self.next_state)
        for register, (next_value_ce, next_value) in ln.registers.items():
-           self.sync += If(next_value_ce, register.eq(next_value))
+           self.sync += If(next_value_ce, register.obj.eq(next_value))

        # drive entering/leaving signals
        for state, signal in self.before_leaving_signals.items():

Since FullMemoryWE and SplitMemory replace/remove the old memories, they can't be accessed directly in simulation. Both decorators should provide transparent access to the (new) underlying memories.

e.g. platform.add_platform_command(" ... {clk}", clk=ClockSignal())

I'd like to be able to translate the migen Module hierarchy into the exported HDL hierarchy at least to some extent.

• This would allow estimating resource usage at some useful granularity.
• It would allow easier exporting and re-use of migen gateware in verilog/vhdl projects.
• This would help with floor planning if one wants to do that.
• Vivado at least seems to have some options that allow some synthesis to be hierarchy-constrained and therefore potentially scaling better.
• I don't expect this to be able to map any of the metaprogramming and parametrization in migen to parametrization in verilog. That won't work. I am fine with exporting module parametrizations as separate and fully specialized modules.

Implementation idea:

• Module would receive a list of ios
• There would need to be some way to check/enforce/verify the interface, i.e. ensure that module inputs are not also assigned to from within a module and that internal Signals within a module are not accessed from the outside.
• Some way to substitute a Module by an Instance.
• build() would export specialized modules.

e.g.
https://travis-ci.org/m-labs/migen/builds/73124713#L1218

Same question for https://github.com/m-labs/migen/blob/master/setup.py#L18 as in m-labs/artiq#131

from migen import *

class TB(Module):
    def __init__(self, n):
        self.n = n
        self.r = Signal(n, reset=0b10)
        self.comb += self.r[0].eq(self.r[-1])
        self.sync += self.r[1:].eq(self.r)
        self.out = []

    def run(self):
        for i in range(2*self.n):
            self.out.append((yield self.r))
            yield

n = 3
tb = TB(n)
run_simulation(tb, tb.run(), vcd_name="t.vcd")
assert tb.out == [2, 4, 1, 2, 4, 1], tb.out
# AssertionError: [2, 2, 2, 2, 2, 2]

(originally #36)

NOOB QUESTION.

Is there a way to spit the following verilog

always @ ( negedge clk) ....

Or a more complicated sensitivity list?

When attempting to use Record.raw_bits() to create a bus to manipulate in further Migen expressions, Migen will generate the concatenation "in place" in the generated Verilog code for each use of the bus instead of assigning to a temporary bus.

E.g. Assuming a record with fields a, b, c, d of varying widths and used as inouts, Migen will generate:

 {a, b, c, d}[0] = my_wire;

for each use of the Signal created from Record.raw_bits(), as opposed to:

my_bus = {a,b,c,d}; 
assign my_bus[0] = my_wire;

While the generated Verilog code in the former may be syntactically valid, some vendors may reject such assignments anyway- Xilinx's compiler (when use_new_parser=YES, as it should be for Migen) is known to choke on these assignments.

See the following gist for the code I was trying to synthesize when I ran into this issue: https://gist.github.com/cr1901/c994d2d307a35f1344ac

The following migen.genlib modules already have inline documentation!

• coding - Good!
• sort - Good!

The following migen.genlib modules are missing docstrings or other form of inline documentation;

• cdc - BusSynchronizer has a dockstring, but others don't.
• complex - This is just a complex number? - What makes it different from Python's inbuilt complex type?
• divider - Whats "w"? The width? What is it actually dividing?
• fsm - Lots of undocumented stuff here.
• io - These are mostly self explanatory, but still could use information about why they exist?
• misc - Lots of undocumented stuff here.
• record - Lots of undocumented stuff here.
• resetsync - Why does the AsyncResetSynchronizer exist?
• rob - Lots of undocumented stuff here.
• roundrobin - switch_policy?

These last two modules are mostly good but need some fixes.

• mhamgen - The documentation is all in comments rather than in Python doc strings.
• fifo - Mostly good, but how is SyncFIFOBuffered different?

As I figure out these modules I'll try and start sending patches with documentation myself if no one else has gotten to it first.

The extension is .rst, but the content is Markdown.

https://irclog.whitequark.org/m-labs/2017-06-09#1496995548-1497020136;
https://github.com/m-labs/migen/blob/master/migen/sim/core.py#L308

I am encountering problems of GUI docking location does not restored to the last state upon restarting. I noticed this problem occurs after I undock certain widgets from the main dock before restarting.

The error message I received was: artiq_gui.state: Failed to restore state for object 'DockArea'

Upon multiple restart, sometimes certain widgets are missing, this issues is not repeatable upon multiple restart.

The iverilog-vpi tool can be used to find this path using iverilog-vpi --install-dir

From man iverilog-vpi;

iverilog-vpi is a tool to simplify the compilation of VPI modules for use
with Icarus Verilog. It takes on the command line a list of C or C++ source
files, and generates as output a linked VPI module.

Vivado fails to generate the .bin raw binary bitstream file which is used for flashing, and seems to lack an option to do so.
Maybe the flasher should support loading .bit files.

From http://www.scarabhardware.com/minispartan6/

The Spartan6 LX25 FPGA, or the Spartan 6 LX9 FPGA from Xilinx two of the best FPGAs on the market.

Currently the device is set to xc6slx9 in https://github.com/m-labs/migen/blob/master/mibuild/platforms/minispartan6.py#L115

What is the correct way to support the two different variants of this board?