-
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
Xilinx specific MultiReg #212
Conversation
Vivado was inferring an SRL16 from a MultiReg in some cases
Codecov Report
@@ 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.
|
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 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 |
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 |
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 |
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 |
Please do. |
Vivado was inferring an SRL16 from a MultiReg in some cases. This simply adds an
ASYNC_REG="TRUE"
attribute to the registers.This code is identical for both 7 series and Spartan which I don't like. I see two possibilities:
Add something like async_reg_attrs to the platform file and if present apply it to the regs in the MultiReg. It could also be applied to ResetSynchronizer, etc.
A new file, vendor/xilinx_common.py containing code common to all supported Xilinx parts.
I'm happy to do either.