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
XDR configuration should allow disabling gearboxes #532
Comments
What goes wrong when you just use the normal nmigen-generated output? Such as import nmigen as nm
from nmigen_boards.icebreaker import ICEBreakerPlatform
plat = ICEBreakerPlatform()
m = nm.Module()
plat.add_resources(plat.break_off_pmod)
led = plat.request("led_r", 1, xdr=2)
m.d.comb += led.o_clk.eq(nm.ClockSignal()), led.o0.eq(1), led.o1.eq(0)
plat.build(m) From a quick look at the generated verilog this essentially generates exactly what you've put as your raw example, with the only exception being that |
If I remember correctly, the gearbox in ECP5 is built into the IOB. But I might be wrong.
My view is that:
Pin provides an abstraction, specifically that of a pin with gearbox. Eliminating platform-specific differences is the whole point. If you don't want that, don't use the abstraction! Instantiate the IOB directly. |
Yeah, that's correct, the total latency for ECP5 is 3 cycles as a result, and both inputs are synchronous to the posedge of the clock |
Hmm, I think my understanding of what went wrong is incorrect. Here is my patch: diff --git a/nmigen/vendor/lattice_ice40.py b/nmigen/vendor/lattice_ice40.py
index d5e4dd6..dbddfcc 100644
--- a/nmigen/vendor/lattice_ice40.py
+++ b/nmigen/vendor/lattice_ice40.py
@@ -498,14 +498,14 @@ class LatticeICE40Platform(TemplatedPlatform):
pin_o0 = get_oneg(pin.o0, o_invert)
pin_o1 = get_oneg(pin.o1, o_invert)
- if "i" in pin.dir and pin.xdr == 2:
- i0_ff = Signal.like(pin_i0, name_suffix="_ff")
- i1_ff = Signal.like(pin_i1, name_suffix="_ff")
- get_dff(pin.i_clk, i0_ff, pin_i0)
- get_dff(pin.i_clk, i1_ff, pin_i1)
- if "o" in pin.dir and pin.xdr == 2:
- o1_ff = Signal.like(pin_o1, name_suffix="_ff")
- get_dff(pin.o_clk, pin_o1, o1_ff)
for bit in range(len(port)):
io_args = [
@@ -561,7 +561,7 @@ class LatticeICE40Platform(TemplatedPlatform):
# Re-register negedge output after it leaves fabric. This increases setup time
# to an entire cycle, and doesn't add latency.
io_args.append(("i", "D_OUT_0", pin_o0[bit]))
- io_args.append(("i", "D_OUT_1", o1_ff[bit]))
+ io_args.append(("i", "D_OUT_1", pin_o1[bit]))
if pin.dir in ("oe", "io"):
io_args.append(("i", "OUTPUT_ENABLE", pin.oe)) It removes the automatically generated FF from the o1 datapath. I think I've severely misunderstood the purpose of that FF though. I'll do some more experiments, but in the mean time I do have some followup questions:
diff --git a/nmigen/vendor/lattice_ice40.py b/nmigen/vendor/lattice_ice40.py
index d5e4dd6..c29bdee 100644
--- a/nmigen/vendor/lattice_ice40.py
+++ b/nmigen/vendor/lattice_ice40.py
@@ -504,7 +504,9 @@ class LatticeICE40Platform(TemplatedPlatform):
get_dff(pin.i_clk, i0_ff, pin_i0)
get_dff(pin.i_clk, i1_ff, pin_i1)
if "o" in pin.dir and pin.xdr == 2:
+ o0_ff = Signal.like(pin_o0, name_suffix="_ff")
o1_ff = Signal.like(pin_o1, name_suffix="_ff")
+ get_dff(pin.o_clk, pin.o0, o0_ff)
get_dff(pin.o_clk, pin_o1, o1_ff)
for bit in range(len(port)):
@@ -560,7 +562,7 @@ class LatticeICE40Platform(TemplatedPlatform):
elif pin.xdr == 2:
# Re-register negedge output after it leaves fabric. This increases setup time
# to an entire cycle, and doesn't add latency.
- io_args.append(("i", "D_OUT_0", pin_o0[bit]))
+ io_args.append(("i", "D_OUT_0", o0_ff[bit]))
io_args.append(("i", "D_OUT_1", o1_ff[bit]))
if pin.dir in ("oe", "io"): |
OK, I've thrown together this test design:
Which I think demonstrates the requirement for a FF on (top row is led0, then led1, then led2). Adding an FF to d0 using the following patch: diff --git a/nmigen/vendor/lattice_ice40.py b/nmigen/vendor/lattice_ice40.py
index d5e4dd6..c29bdee 100644
--- a/nmigen/vendor/lattice_ice40.py
+++ b/nmigen/vendor/lattice_ice40.py
@@ -504,7 +504,9 @@ class LatticeICE40Platform(TemplatedPlatform):
get_dff(pin.i_clk, i0_ff, pin_i0)
get_dff(pin.i_clk, i1_ff, pin_i1)
if "o" in pin.dir and pin.xdr == 2:
+ o0_ff = Signal.like(pin_o0, name_suffix="_ff")
o1_ff = Signal.like(pin_o1, name_suffix="_ff")
+ get_dff(pin.o_clk, pin.o0, o0_ff)
get_dff(pin.o_clk, pin_o1, o1_ff)
for bit in range(len(port)):
@@ -560,7 +562,7 @@ class LatticeICE40Platform(TemplatedPlatform):
elif pin.xdr == 2:
# Re-register negedge output after it leaves fabric. This increases setup time
# to an entire cycle, and doesn't add latency.
- io_args.append(("i", "D_OUT_0", pin_o0[bit]))
+ io_args.append(("i", "D_OUT_0", o0_ff[bit]))
io_args.append(("i", "D_OUT_1", o1_ff[bit]))
if pin.dir in ("oe", "io"): "fixes" the signals to be what I would expect: I can submit a PR for the above change if desired, or I can contribute some documentation of this so others don't stub their toe on it =). My question about getting the gearbox latency from above still remains. |
I haven't tried it on hardware, but I think your demo has mixed up o0 and o1 - o0 is the value for the rising edge, o1 for the subsequent falling edge, so you've assigned the LSb to happen first, followed by the MSb. That means your state machine sequences, in time order: If you bit-reverse the values for Meanwhile I'd expect led0 and led2 to be identical (as in your first trace), since both |
I see. I think there's nothing to do here then, I'm just a fool and need to fix my design. Thanks everyone, and sorry for the noise. |
We certainly should have better documentation here.
You're correct in that gearbox latency should be provided but isn't. I have omitted it when I first implemented IO buffer insertion, and no one had enough time and inclination to add them since then. |
I ran in to this while working on my port of Bob Miller's Icebreaker Candy. That project (ab)uses XDR pins to get effectively 2x resolution on a handful of important signals (LED panel blank, latch, and generating SCK). When using the raw Lattice primitives, this is easy:
will effectively automatically generate the required pixel clock (SCK) signal. In nmigen however, the automatically generated gearbox for XDR=2 doesn't allow direct access to
D_OUT_1
. Confusingly the ECP5 platform does allow direct access to these pins (e.g. https://github.com/nmigen/nmigen/blob/c6150d05867bea1a7266e3c950d3d9846ba7ed58/nmigen/vendor/lattice_ecp5.py#L532). From reading the documentation on Pin it seems like ECP5 is "wrong" here -- we should be generating gearboxes there as well.I have a local patch which does the "dumb" thing and just makes ICE40 behave like ECP5 (no gearbox) but I doubt you want that upstream =). Instead, either the
platform.request
API or the resource configuration should allow specifying whether gearboxes are desired. This probably interacts with #458.The text was updated successfully, but these errors were encountered: