Skip to content
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

wrong type of buffer primitive used in series 7 #438

Closed
DaKnig opened this issue Jul 18, 2020 · 13 comments · Fixed by #444, #460 or #490
Closed

wrong type of buffer primitive used in series 7 #438

DaKnig opened this issue Jul 18, 2020 · 13 comments · Fixed by #444, #460 or #490

Comments

@DaKnig
Copy link

@DaKnig DaKnig commented Jul 18, 2020

when I try to elaborate a very simple blinking-led example I wrote, I get this from vivado (2019.2):

WARNING: [Netlist 29-345] The value of SIM_DEVICE on instance 'cd_sync/U$$1' of type 'BUFGCTRL' is 'ULTRASCALE'; it is being changed to match the current FPGA architecture, '7SERIES'. For functional simulation to match hardware behavior, the value of SIM_DEVICE should be changed in the source netlist. 

it looks to me like nMigen instantiates the wrong primitive for some buffer

@whitequark whitequark transferred this issue from amaranth-lang/amaranth-boards Jul 19, 2020
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 19, 2020

Here's some Xilinx forum post claiming it's harmless. I think we should just add the SIM_DEVICE parameter. I have no special insight on why this is an appropriate solution but it seems unlikely to break anything.

@daveshah1
Copy link

@daveshah1 daveshah1 commented Jul 21, 2020

SIM_DEVICE enables the correct sim model for the target device as there have been slight behaviour changes, it doesn't actually change anything in synthesis but creates a warning due to being a possible sim/synth mismatch.

whitequark added a commit that referenced this issue Jul 22, 2020
The parameter defaults to "ULTRASCALE", even when synthesizing for
7-series devices. This could lead to a simulation/synthesis mismatch,
and causes a warning.

Fixes #438.
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 22, 2020

My ancient Vivado version doesn't have the warning, can you check that #444 fixes this for you?

@whitequark whitequark added this to the 0.3 milestone Jul 22, 2020
@DaKnig
Copy link
Author

@DaKnig DaKnig commented Jul 23, 2020

it does not; I used this guide https://nmigen.info/nmigen/latest/install.html#editable-development-snapshot to setup a newer version of nmigen, still nothing. furthermore , I discovered that this is not the only primitive it got wrong: here are a few more primitives it got wrong:

WARNING: [Netlist 29-345] The value of SIM_DEVICE on instance 'cd_sync/U$$1' of type 'BUFGCTRL' is 'ULTRASCALE'; it is being changed to match the current FPGA architecture, '7SERIES'. For functional simulation to match hardware behavior, the value of SIM_DEVICE should be changed in the source netlist. 
WARNING: [Netlist 29-345] The value of SIM_DEVICE on instance 'cd_sync/U$$1' of type 'BUFGCE' is 'ULTRASCALE'; it is being changed to match the current FPGA architecture, '7SERIES'. For functional simulation to match hardware behavior, the value of SIM_DEVICE should be changed in the source netlist. 
WARNING: [Netlist 29-345] The value of SIM_DEVICE on instance 'cd_sync/U$$1' of type 'BUFGCTRL' is 'ULTRASCALE'; it is being changed to match the current FPGA architecture, '7SERIES'. For functional simulation to match hardware behavior, the value of SIM_DEVICE should be changed in the source netlist. 

@rroohhh
Copy link
Contributor

@rroohhh rroohhh commented Jul 23, 2020

#444 not working kinda seems like a vivado bug, looking at the log, it correctly reads in the SIM_DEVICE attribute of the BUFGCE as 7SERIES:

INFO: [Synth 8-6157] synthesizing module 'BUFGCE' [/data/opt/Xilinx/Vivado/2019.2/scripts/rt/data/unisim_comp.v:1085]
        Parameter CE_TYPE bound to: SYNC - type: string
        Parameter IS_CE_INVERTED bound to: 1'b0
        Parameter IS_I_INVERTED bound to: 1'b0
        Parameter SIM_DEVICE bound to: 7SERIES - type: string
        Parameter STARTUP_SYNC bound to: FALSE - type: string

but then transforms the BUFGCE into a BUFGCTRL during which the attribute seems to get lost:

WARNING: [Netlist 29-345] The value of SIM_DEVICE on instance 'cd_sync/U$$1' of type 'BUFGCTRL' is 'ULTRASCALE'; it is being changed to match the current FPGA architecture, '7SERIES'. For functional simulation to match hardware behavior, the value of SIM_DEVICE should be changed in the source netlist.
[...]
INFO: [Project 1-111] Unisim Transformation Summary:
  A total of 1 instances were transformed.
  BUFGCE => BUFGCTRL: 1 instance

Full build directory: build.zip

This is for the following design:

from arty_z7 import ArtyZ720Platform

from nmigen import *
from nmigen.back.rtlil import convert

class Blinky(Elaboratable):
    def __init__(self):
        self.led = Signal(reset=0)

    def elaborate(self, platform):
        m = Module()

        counter = Signal(range(int(platform.default_clk_frequency+1)))

        m.d.sync += counter.eq(
            Mux(counter==int(platform.default_clk_frequency),
                0, counter+1))

        m.d.sync += self.led.eq(self.led ^ (counter==0))
        # if counter is zero, flip the led

        return m


class top_level(Elaboratable):
    def elaborate(self, platform):
        m=Module()
        m.submodules.blink = Blinky()

        led = platform.request("led", 2)
        m.d.comb += led.eq(m.submodules.blink.led)
        return m

ArtyZ720Platform().build(top_level(), do_program=True)

This is using vivado 2019.2, so it could have been fixed in the newest version of vivado (2020.1).

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 23, 2020

@rroohhh Thanks for investigation. I think I'm going to merge the PR, since it seems to be a clear improvement, even if it doesn't fully fix the problem at the moment.

whitequark added a commit that referenced this issue Jul 23, 2020
The parameter defaults to "ULTRASCALE", even when synthesizing for
7-series devices. This could lead to a simulation/synthesis mismatch,
and causes a warning.

Fixes #438.
@whitequark whitequark reopened this Jul 23, 2020
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 23, 2020

(Reopening as upstream bug.)

@rroohhh
Copy link
Contributor

@rroohhh rroohhh commented Jul 23, 2020

One way to avoid this would be using BUFGCTRL directly instead of BUFGCE. Going after UG472 BUFGCE is implemented in terms of BUFGCTRL anyways:

Unlike BUFG, BUFGCE is a clock buffer with one clock input, one clock output and a clock
enable line. This primitive is based on BUFGCTRL with some pins connected to logic High
or Low.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 23, 2020

Ah, I'm fine with that solution too.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 31, 2020

@rroohhh Something like this? #460

@DaKnig Could you check if this fixes the issue with your Vivado version?

@rroohhh
Copy link
Contributor

@rroohhh rroohhh commented Jul 31, 2020

Technically switching to BUFGCTRL is only needed for 7series (as SIM_DEVICE defaults to ultrascale (atleast currently)).
Furthermore on ultrascale BUFGCE and BUFGCTRL are actually two different primitives. The equivalent to BUFGCE on 7series seems to be BUFGCE_1 which is implemented in terms of BUFGCTRL on ultrascale. Now which one to prefer I don't know.

Also this is missing setting SIM_DEVICE = "7SERIES" again, so it doesn't remove the warning.

Sorry if what I wrote was misleading, I wanted to say if you use BUFGCTRL and set SIM_DEVICE="7SERIES it actually works, as then vivado doesn't perform the transformation of BUFGCE to BUFGCTRL which seems to internally loose the SIM_DEVICE attribute.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 26, 2020

@rroohhh Does this work? #490

@rroohhh
Copy link
Contributor

@rroohhh rroohhh commented Aug 26, 2020

It works for me for 7series on vivado 2019.2. (Can't test ultrascale as I don't have the ultrascale device support installed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment