-
Notifications
You must be signed in to change notification settings - Fork 195
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
Use SB_GB_IO instead of SB_IO+SB_GB #89
Comments
I've been looking into the timings between the FX2 and ICE interface. ICE to FX2The ICE has a clock-to-out somewhere between 9 and 10 ns. (relative to the So this works just fine. The data valid window of the ICE outputs entirely cover the data capture window of the FX2 with plenty of margins. Whatever the ICE outputs will be captured on the next rising clk edge by the FX2. See diagram below : FX2 to ICEThe FX2 is a bit under-specified in its clock-to-out. There is only a max value of 11 ns but no minimum value, so we have to assume that the data isn't valid at all from the clock edge up to 11 ns after it. In the ICE40 it takes a long time for the clock to go from the pad to the actual So this means that the ICE40 data capture window is right in the middle of the unstable part of the FX2 output :/ See diagram below. The best way to solve this would be to use the DDR registers in the DetailsAttached the timing report from icecube to get details on the IO timings. Also attached the sources for the wavedrom drawings. |
Thank you for your analysis again! I'm not entirely sure about one facet here:
Do I understand it correctly that this adds another cycle of latency with regards to the current arrangement with |
Unfortunately, this takes liberties with the FX2 interface; the data is now captured on falling edge, so instead of one full cycle of data validity there is only a half cycle. However, the FX2 interface was certainly broken before, and it seems less broken to me now, so I am committing this anyway. Ideally, the FX2 interface would register the input on the next rising edge. However, this would add two registers in the input path, and we are already using the INFM1 bit in the FX2. Therefore, adding this register would require a radical rethinking of the entire FX2 interface. See #89.
Looks like my understanding is correct. Therefore, there is a problem with this solution. Namely, what happens when we are filling an IN FIFO and it becomes full? With one register, FX2 has an (I'm not sure how |
Mmm, that's weird I would have thought that the latency would be the same. If we look at the 'orange' data cycle for the fx2_out in the diagram above. In the previous implementation, I would have assumed that what the ice40 captured during the invalid window was that orange cycle data like I put on the diagram. And so if you capture on the falling edge, you essentially move that capture window half a cycle earlier than currently, and then re-registering it once on the rising edge in the fabric would bring it "in-alignement" with what the |
@smunaut I'd like to ask you for a favor. Can you please take another look at the timings of this interface, but this time, assuming externally sourced (in the FPGA) CLKIF, where the CLKIF output is produced using a DDR register with |
@smunaut Bad news: this doesn't help. With the video-rgb-input applet, there is digital snow on the output, which I know for sure is caused by timings mismatch between ICE and FX2. I have a bitstream I made half a year ago that doesn't have this problem but I have no idea how to reproduce it. There's still something else wrong with the arbiter. |
Looks like ae5cbc1 is the good commit. Bisecting now. |
I've bisected this down to... 630b524. Which is not really of help. |
I've made a standalone way to reproduce the glitches. First, apply this patch: diff --git a/software/glasgow/applet/video/rgb_input/__init__.py b/software/glasgow/applet/video/rgb_input/__init__.py
index aa8ee9d..f472b19 100644
--- a/software/glasgow/applet/video/rgb_input/__init__.py
+++ b/software/glasgow/applet/video/rgb_input/__init__.py
@@ -3,6 +3,7 @@ import math
from migen import *
from migen.genlib.cdc import *
+from ....gateware.clockgen import *
from ... import *
@@ -12,12 +13,15 @@ class VideoRGBInputSubtarget(Module):
gx = Signal(5)
bx = Signal(5)
dck = Signal()
- self.specials += [
- MultiReg(pads.r_t.i, rx),
- MultiReg(pads.g_t.i, gx),
- MultiReg(pads.b_t.i, bx),
- MultiReg(pads.dck_t.i, dck)
- ]
+ # self.specials += [
+ # MultiReg(pads.r_t.i, rx),
+ # MultiReg(pads.g_t.i, gx),
+ # MultiReg(pads.b_t.i, bx),
+ # MultiReg(pads.dck_t.i, dck)
+ # ]
+
+ self.submodules.clockgen = ClockGen(ClockGen.derive(sys_clk_freq, 1.4e6))
+ self.comb += dck.eq(self.clockgen.clk)
dck_r = Signal()
stb = Signal()
@@ -59,7 +63,7 @@ class VideoRGBInputSubtarget(Module):
NextState("SKIP-FIRST-PIXEL")
).Elif(row == rows,
NextValue(row, 0),
- NextState("REPORT-FRAME")
+ NextState("CAPTURE-ROW")
).Else(
NextState("REPORT-FRAME")
)
@@ -86,7 +90,7 @@ class VideoRGBInputSubtarget(Module):
("REPORT-3", 10, "CAPTURE-PIXEL")
):
self.fsm.act(state,
- din.eq((pixel >> offset) & 0x1f),
+ din.eq(col & 0x1f),#(pixel >> offset) & 0x1f),
we.eq(1),
NextState(nextstate)
) Second, run:
Third, run:
This is what it should display: This is what it displays: |
My question on the Cypress forum: https://community.cypress.com/message/191835 |
Have you tried eliminating the FIFO datapath from the equation? (If the corruption is on the first valid cycle after interruption, it also means it's the first data outputed by the fifo after being empty, which is also a corner case). I would personally do a test by keeping the exact same control flow and just replace the FIFO data that is going to the FX2 with a counter and see if you also have the corruption. (but maybe you already verified that). |
@enjoy-digital You are right! There's no corruption if I replace the FIFO with a counter. But... the FIFO is just migen's SyncFIFOBuffered... |
Then it seems there is a mismatch between the control/data path for this corner case on the FIFO. I'm using the same FIFO on several designs in situations that are very similar (mostly on Xilinx devices). Maybe something is not handled correctly in the toolchain. At least it shows the issue is probably not on the FX2 side. |
Yeah... |
Do you have YosysHQ/yosys#896 in your Yosys tree? |
@daveshah1 If I use Yosys master instead of whichever old commit I've had, it segfaults. |
@gregdavill Repro: bug.zip |
Hmm, can't reproduce and Valgrind shows nothing either over here. But it doesn't hit the #896 case either, so that's not related anyway. |
@daveshah1 Valgrind log of the crash:
|
@daveshah1 Nevermind, I rebuilt Yosys and the crash came away. |
SB_GB_IO has a much lower (and completely predictable!) delay than SB_IO + SB_GB pair we are using now. In fact, using the latter pair has been observed to (rarely) lead to selftest failures due to setup/hold violations on FD pins.
Unfortunately, simply switching to SB_GB_IO completely breaks FX2 timing. I have adjusted the strobes to work (without actually trying to think of a proper solution) in the
sb_gb_io
branch, but really, a model of the FX2 bus and tests are necessary to fix this properly. See #44.The text was updated successfully, but these errors were encountered: