-
Notifications
You must be signed in to change notification settings - Fork 3
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
OPL3 FM Synthesizer Synthesis and Simulation Issues #16
Comments
Hi @gtaylormb. This is essentially:
In a follow up to @hzeller recommendation, we are also planning to bring up Verible-based Linter for opl3_fpga RTL. Please consider using these flows to make the opl3_fpga RTL even better than it already is. |
Verilator's linting is way over the top, and satisfying all the warnings would lead to messier, more verbose, and harder to read code that's not necessary for synthesis. Icarus does not fully support SystemVerilog and probably can't be used for this project. I suggest switching to https://github.com/MikePopoloski/slang for sane linting. I have found a few minor things running slang and will try to update https://github.com/gtaylormb/opl3_fpga shortly. |
It's wonderful news that RTL Linting is now also a step in the OPL3 QA process! As we have in the meantime become more familiar with Verible, we are starting to put it to good use, not only for Linting, but also for the unified code look-and-feel, i.e. formatting. We therefore highly recommend taking a closer look at it... |
@gtaylormb could you please share the script (or workflow) you are using for your Slang-based OPL3 RTL linting? That's to bootstrap our effort in that direction. |
Basically any WIDTH* warning; Verilator is really dumb about these:
Is generated from this code, truncated for brevity:
This can be eliminated by changing the line to this monstrosity: Terrible--there's hundreds of these in opl3_fpga. But if you turn off the WIDTH* warnings it may provide some useful stuff. Mostly not though. I tried Verible. Seems okay. It mostly complained about missing newline characters at the end of files, tabs instead of spaces in some files, and using always instead of always_ff or always_comb in a Verilog (non-SystemVerilog) .v file that I didn't write. Which I suspect I could probably fix if I separate out my .v and .sv files and run them with different parameters to Verible. This is probably a step above Verilator. Unified formatting of files is also good. I will explore it some more. |
I just added a lint target to the Makefile in my fpga folder:
|
Regarding Verilator linting, it seems excruciatingly precise -- Verilog shift operators indeed don't change operand width. While VHDL would error-out on that line, Verilog is silently zero-padding the extra MSB. That's exactly what Verilator is warning about (and rightly so). As shown by @gtaylormb above, a way to silence it is by explicitly declaring the zero-padding intent. Here is a bit less verbose form of the same: This fully spelled out form (which we recommend for shifting operations) brings a question: Does |
That's wrong, and both spelled out forms are IMHO a bad way to code due to the verbosity which I was highlighting, but also highlight a misunderstanding of how Verilog works. If REG_BLOCK_WIDTH is 3, and block is 'b111, block_shifted gets 'b1110. There are no synthesis warnings and no other tools complain because the original syntax is correct. This type of shift is all over the code and proven in both Xilinx and Altera FPGAs. Verbosity = more potential for bugs both when coding and maintaining. The verilator warning is counterproductive. |
Regarding the language in the LRM, you left out some info just above the chart you pasted (I'm looking at IEEE Std 1800-2012 for reference): "A context-determined expression is one where the bit length of the expression is determined by the bit length You'll notice for shifts only |
Another proof
|
Anyway, I've merged slang and Verilator linting to opl3_fpga: gtaylormb/opl3_fpga#51 Of course I've turned off the WIDTH* warnings in Verilator. I didn't find any others to be as blatantly misguided. |
Great that OPL3 RTL is now cleaner👍 We are soon to take a fresh new copy of it and try porting it again to GateMate, pending PNR improvements that CologneChip has in the pipe. As for the width question, it seems that the very fact we are having a lengthy discussion on it, resorting to LRM quotes, and pulling in other folks for their expert opinion, supports the statement that explicit expression of designer intent is not without virtues 😉 |
I mean, I would argue it was the misguided Verilator warning that threw us into this discussion in the first place. The original code is quite clear IMO. This point was illustrated by the fact that he (I think unintentionally) said the code is quite clear--move the bits over 1 into the wider vector (even though that is the code that throws the warning) :) I would still further argue against more verbose syntax. Part selects to perform shifts are harder to read and more error prone than the shift operator IMO. E.g. try changing it to shift 5 instead of 1, or a parameterized X amount. |
Pretty excited to see your progress |
Stay tuned! @DadoCCAG, since an old timer C64 and SoundBlasterPro adict, is even more excited 😄. BTW, would something like this |
That's awesome. I actually had a C64 at one point that was my grandfather's back in the day. He actually coded music on it, saved on a bunch of tapes, but I believe those are long gone I'm afraid. In the 90s my family bought a 486 with a PAS-16; that was my exposure to the OPL3. A lot of nostalgia. BTW have you guys checked out https://github.com/MiSTer-devel/ao486_MiSTer? A really, really cool project. OPL3_FPGA was ported to it, so you can play all those old DOS games using this code on an FPGA. Edit: it's what got me diving back into opl3_fpga after 9 years or so. That and being on garden leave. |
I mean, I think you can guess my preference. Nothing conveys design intent for a shift better than using the shift provided with the language. When you're scanning through code debugging or whatever, it is advantageous to quickly understand the code without having to stare at it, and move on--hence my preference for concise, readable code, and why I've been using the more advanced features of SystemVerilog in design for the past 15 years. You used SHIFT in the parameter name, but otherwise I'd have to stare at that line for a bit to understand, "oh, this guy's doing a shift here." In my brain I'd literally replace the concatenation/bit extension code with a << operator. I'd also wonder why a shift operator wasn't used, and probably stare at it a bit longer, thinking I'm missing something. In complex designs this becomes really tedious and distracts from real problems. This is why higher-level languages were designed. Of course in using the features of a language, it helps to have extensive knowledge of the language and the trust that it does what you want which happens with experience (and test benches), so coders that are unsure or mistrustful of the language tend to produce worse looking code to avoid imagined problems--sort of, go with what you know or think you know. And not just newer coders--I've seen older, more experienced engineers still code in old school Verilog and explicitly define things down to the finest granularity, because they never allowed that trust to occur or bothered to try or learn new ways of doing things (e.g. SystemVerilog) even though they have years and years of coding experience. This is actually extremely common. Anyway, I think we got a bit derailed (no pun intended) with the Verilator warning. No other tool gives that warning that I'm aware of, and I think (I hope) you understand now why I don't agree with it and think it's detrimental. It should not scare you from using the shift operator--it's really quite simple and clean. Linting and good coding style cannot replace a good test bench and testing of course. People still make mistakes, and there are tradeoffs with every approach. |
@wsnyder, given that Verilator |
Yes, feel free to file a bug asking for the specific width shift case get suppressed. |
After updating the OPL3 FM Synthesizer to the latest version, where Greg added Verilator and Slang linting as a step in the workflow, I tried the GateMate synthesis after converting the SystemVerilog files to Verilog as discussed here. The output now has much fewer warnings, but there are still a few that I would like to address. Warning Type 1: Undriven Input
Going through all the SystemVerilog and generated Verilog files in question I don't see a problem. I am going to go through the generated Verilog files because those are the ones synthesized. Code ommited for brevity and clarity.
In host_if.v:
In synchronizer.v:
The warnings indicate that the dout_sync.in wire in the host_if module is not driven. This is suspicious because dout_sync should be connected to status, which should be driven by the opl3 module. Warning Type 2: Port Resizing
Most of these warnings refer to the following line in afifo:
In afifo.v, the tool is resizing the WIDTH from 10 bits to 20 bits. Similar warning is also present in the control_operators file. NoteIf anyone wants to try the synthesis for themselves you can just type in the 3.build folder:
That will generate the Verilog files from the SystemVerilog files(assuming you installed sv2v).
|
Warning Type 1@gtaylormb, how about tying off Warning Type 2@ZipCPU did you know that your AFIFO from 10GE Switch was also used to synthesize FM music 😉
|
Almost all of my IP has various blocks within it, such as this one (below) in order to avoid the UNUSED variable lint warning.
It's easy enough to do, and it also acts as a documentation that you are aware of the issue. Dan |
Also, one other note: beware that Verilator tends to only generate warnings for parameter paths chosen during the lint step. A full Verilator lint requires running lint with all potential parameter settings. |
... good tips. However, the warnings from this, second round of scrutiny, are from Yosys synthesis (GateMate fork) -- Greg has in the first cleanup round already taken care of Verilator and Slang linting. |
Well ... I tried. I read "Warning Type 1: Unused Wires" and figured that's what was going on. After a second look, the issue appears to be used but not driven wires. Verilator can typically detect that sort of thing. I'm not sure why Verilator hasn't flagged these issues then. |
... you are right, we have updated that subtitle to make it more precise. Looking at Greg's Verilator options we don't see |
UNDRIVEN isn't enabled by default, see the docs for what is. I recommend |
When running Verilator linting in our script with the
But because
Others are the ones we discussed in |
UNDRIVEN and UNUSEDSIGNAL are useful warnings. I can fix in opl3_fpga. Note these occur if the timers are turned off via the INSTANTIATE_TIMERS parameter in opl3_pkg.sv. I'm not sure how you guys are planning on using opl3_fpga, but if you want the full chip (as used in ao486 MiSTer), you may want these on. If timers are turned off these signals just get trimmed out. Status is useless in this case. I didn't use them for the base opl3_fpga implementation because I just use an internal timer in the Zynq PS to control writes from .dro files. |
... please, send a note when you want us to refresh the OPL3 RTL again. Given the resolution of the last GateMate build issue, we are ready to try the OPL3 core. We are looking for a featherweight demo SOC to put the OPL3 into, one that can only read and play the soundtracks. Basically, something on par with your Xilinx BD example, but portable to all FPGA vendors. We are also curious on your take on the following idea:
The other option is to use on-chip PicoRV32 RISC-V microcontroller. We are seeking your guidance and tips... |
I'd probably start with porting imfplay over to your platform and highjacking the OPL3 writes. This is what I did for opl3_fpga and my modified version is here. You won't need the timers and won't need to read anything back. DRO files are already in the format of OPL3 programming sequences with 1us timer gaps essentially. I suspect that UART speed should be fast enough but I've never characterized the actual write speeds. |
@TarikHamedovic if you believe that the original topic of this issue is now resolved, and corresponding discussion track exhausted, please proceed with closing it. |
Greg added Linting, we also fixed some GateMate synthesis and implementation issues and now the sythesis and implementation of the OPL3 are okay, so I think the original issue is resolved. |
Verilator Simulation Warnings
While running the synthesis for the OPL3 FM Synthesizer, I encountered several warnings related to the RTL code. Although the synthesis completes successfully due to the Makefile being configured to ignore these warnings, it's crucial to address these warnings for cleaner code and synthesis. Other tools like yosys might treat these warnings as errors, potentially causing issues in the future. To run the Makefile go to 2.sim/ folder and run
make
. The following flags are currently set to ignore warnings in the Makefile:By removing the flags one by one you can see for your self the warnings that Verilator has such as:
I won't list all the warnings here due to readabilty but there are 108 WIDTH warnings, 37 UNUSED warnings, and several minor warnings.
Cocotb Simulation Warnings
Running
make
in the 2.sim/cocoTB folder also results in warnings related to the RTL code:And so on... As said I won't list all the listed warnings and errors.
The text was updated successfully, but these errors were encountered: