Codecov Report

Merging #212 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #212   +/-   ##
=======================================
  Coverage   82.82%   82.82%           
=======================================
  Files          33       33           
  Lines        5392     5392           
  Branches     1163     1163           
=======================================
  Hits         4466     4466           
  Misses        798      798           
  Partials      128      128

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4777a7b...139d764. Read the comment docs.

This code is identical for both 7 series and Spartan which I don't like.

This is actually just fine. Duplicating code is cheap, untangling code when you e.g. want to refactor Vivado and have no access to ISE is very expensive, in terms of time and bugs.

Actually, have you checked that this is relevant to ISE? Your commit message only mentions Vivado.

I have not checked on ISE as I don't currently have it installed. The language templates in ISE call for the same thing and I have used it in the past on Spartan 6 (Verilog). I'm happy to remove it from the Spartan parts if you prefer.

I have not checked on ISE as I don't currently have it installed.

I have ISE, but do not know Xilinx tooling well. I can check it if you tell me what to look for to determine whether everything works correctly (ideally an nMigen design I can just run.)

Incidentally, this is exactly why I would like to not have xilinx_common.py. It is much better to have duplicated code that is tested on each platform it is added to, than factored out code which is only tested on a subset of platforms when it is added or changed.

ISE is installing. I'll check it. MultiReg with n=4 was getting turned into an SRL16 with a delay of 3 and a single FDRE.

Your approach to keeping the tools separate makes sense. I've worked on projects in the past that avoided code duplication at all costs and that was stuck in my head.

You might find this essay on the topic insightful. There is place for factoring code out, and there is also place for duplicating it without much concern.

I've confirmed the same behavior on Spartan6/ISE.

Test code:

from nmigen import *
from nmigen.lib.cdc import MultiReg
from nmigen_boards.mercury import *

class Blinky(Elaboratable):
    def elaborate(self, platform):
        clk50   = platform.request("clk50")
        user_led = platform.request("bussw_oe", 0)
        btn = platform.request("user_btn", 0)
        counter  = Signal(20)

        m = Module()
        m.domains.sync = ClockDomain()
        m.d.comb += ClockSignal().eq(clk50.i)
        btn_sync = Signal()
        m.submodules.mreg = MultiReg(i=btn, o=btn_sync, o_domain="sync", n=2, reset=1)
        m.d.sync += counter.eq(Mux(btn_sync, 0, counter + 1))
        m.d.comb += user_led.o.eq(counter[-1])

        return m

if __name__ == "__main__":
    platform = MercuryPlatform()
    platform.build(Blinky(), do_program=False)

With patch, no SRLs are inferred. Without, the MultiReg becomes an SRL16 + FDRE.

Would you like to take a look at ResetSynchronizer too, given that 0.1 is going to be released soon? It's tracked as #87.

ResetSynchronizer has an asynchronous reset on the flip flops which prevents SRL16s from being inferred. I tested on ISE, but it should be the case on Vivado as well as there is no way to reset bits internal to an SRL16. ISE does complain about not being able to use an SRL when n > 2. Giving ResetSynchronizer the same treatment as MultiReg would silence that, but it's probably not worth having the additional code.

Test code:

from nmigen import *
from nmigen.lib.cdc import ResetSynchronizer
from nmigen_boards.mercury import *

class Blinky(Elaboratable):
    def elaborate(self, platform):
        clk50   = platform.request("clk50")
        user_led = platform.request("bussw_oe", 0)
        btn = platform.request("user_btn", 0)
        counter  = Signal(20)

        m = Module()
        m.domains.sync = ClockDomain()
        m.d.comb += ClockSignal().eq(clk50.i)
        btn_sync = Signal()
        m.submodules.rsync = ResetSynchronizer(arst=btn, n=4)
        m.d.sync += counter.eq(counter + 1)
        m.d.comb += user_led.o.eq(counter[-1])

        return m

if __name__ == "__main__":
    platform = MercuryPlatform()
    platform.build(Blinky(), do_program=False)

Thanks for investigating this. What about false paths for both MultiReg and ResetSynchronizer?

I'm happy to work on that and it is something I would benefit from. It's a bit bigger problem and may be a week or two before I get time. I don't use ISE on new designs, so would be tempted to just do it for Vivado and maybe ECP5/nextpnr.

I think that's a reasonable approach.

Looking into timing constraints / false paths I found that ResetSynchronizer would benefit from the ASYNC_REG="TRUE" attribute as well on both Vivado and ISE. It will cause the registers in the chain to be located as close as possible to each other which is beneficial for minimizing metastabilty. I can create a pull request for ISE if desired. Vivado will be done as part of clock domain crossing constraints pull request which is coming shortly.

I can create a pull request for ISE if desired.

Please do.