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

OPL3 FM Synthesizer Synthesis and Simulation Issues #16

Closed
TarikHamedovic opened this issue Jun 12, 2024 · 33 comments
Closed

OPL3 FM Synthesizer Synthesis and Simulation Issues #16

TarikHamedovic opened this issue Jun 12, 2024 · 33 comments
Labels
help wanted Extra attention is needed

Comments

@TarikHamedovic
Copy link
Collaborator

TarikHamedovic commented Jun 12, 2024

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:

VERILATOR_FLAGS += -Wno-TIMESCALEMOD
VERILATOR_FLAGS += -Wno-LITENDIAN
VERILATOR_FLAGS += -Wno-WIDTH
VERILATOR_FLAGS += -Wno-UNOPTFLAT
VERILATOR_FLAGS += -Wno-CASEINCOMPLETE
VERILATOR_FLAGS += -Wno-UNDRIVEN
VERILATOR_FLAGS += -Wno-VARHIDDEN
VERILATOR_FLAGS += -Wno-IMPORTSTAR
VERILATOR_FLAGS += -Wno-REALCVT
VERILATOR_FLAGS += -Wno-UNUSED

By removing the flags one by one you can see for your self the warnings that Verilator has such as:

%Warning-WIDTH: /home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/3.build/../1.hw/modules/channels/src/channels.sv:212:71: Bit extraction of var[5:0] requires 3 bit index, not 4 bits.
                                                                                                                                  : ... In instance opl3.channels
  212 |                     signals.add_c = !is_new || (chc && !connection_sel[self.channel_num]);
      |                                                                       ^
%Warning-WIDTH: /home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/3.build/../1.hw/modules/channels/src/channels.sv:213:71: Bit extraction of var[5:0] requires 3 bit index, not 4 bits.
                                                                                                                                  : ... In instance opl3.channels
  213 |                     signals.add_d = !is_new || (chd && !connection_sel[self.channel_num]);
      |                                                                       ^
%Warning-WIDTH: /home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/3.build/../1.hw/modules/channels/src/channels.sv:230:90: Operator ADD expects 20 bits on the RHS, but RHS's SEL generates 14 bits.
                                                                                                                                  : ... In instance opl3.channels
  230 |             'b01, 'b10: next_self.channel_l_acc_pre_clamp = self.channel_l_acc_pre_clamp + signals.channel_2_op;
      |                                                                                          ^
%Warning-WIDTH: /home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/3.build/../1.hw/modules/channels/src/channels.sv:236:90: Operator ADD expects 20 bits on the RHS, but RHS's SEL generates 14 bits.
                                                                                                                                  : ... In instance opl3.channels
  236 |             'b01, 'b10: next_self.channel_r_acc_pre_clamp = self.channel_r_acc_pre_clamp + signals.channel_2_op;
      |                                                                                          ^

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:

/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:73: warning: Port 3 (out) of pipeline_sr expects 3 bits, got 2.
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:73:        : Padding 1 high bits of the port.
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:123: error: Index bankb_p[2] is out of range.
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:73: warning: Port 3 (out) of pipeline_sr expects 3 bits, got 2.
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:73:        : Padding 1 high bits of the port.
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/operator/src/phase_generator.sv:282: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/operator/src/phase_generator.sv:302: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/operator/src/phase_generator.sv:302: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/operator/src/phase_generator.sv:314: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/operator/src/phase_generator.sv:314: sorry: constant selects in always_* processes are not currently supported (all bits will be included).
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:73: warning: Port 3 (out) of pipeline_sr expects 3 bits, got 2.
/home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/2.sim/cocotb/../../1.hw/modules/misc/src/mem_multi_bank.sv:73:        : Padding 1 high bits of the port.

And so on... As said I won't list all the listed warnings and errors.

@chili-chips-ba
Copy link
Owner

Hi @gtaylormb. This is essentially:

  • standalone Verilator-based Linter for opl3_fpga RTL
  • Icarus-based Linter for opl3_fpga RTL (within cocotb environment)

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.

@gtaylormb
Copy link
Collaborator

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.

@chili-chips-ba
Copy link
Owner

chili-chips-ba commented Jul 16, 2024

It's wonderful news that RTL Linting is now also a step in the OPL3 QA process!
However, we are still curious about Verilator findings that are over the top? Could you please send a few, say three, most useless warnings.

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...

@chili-chips-ba
Copy link
Owner

@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.

@gtaylormb
Copy link
Collaborator

gtaylormb commented Jul 17, 2024

However, we are still curious about Verilator findings that are over the top? Could you please send a few, say three most useless.

Basically any WIDTH* warning; Verilator is really dumb about these:

%Warning-WIDTHEXPAND: modules/operator/src/calc_envelope_shift.sv:119:31: Operator SHIFTL expects 4 bits on the LHS, but LHS's VARREF 'block' generates 3 bits.
                                                                        : ... note: In instance 'opl3.channels.control_operators.operator.envelope_generator.calc_envelope_shift'
  119 |         block_shifted = block << 1;

Is generated from this code, truncated for brevity:

module calc_envelope_shift
    import opl3_pkg::*;
(
    input wire clk,
    input wire [REG_BLOCK_WIDTH-1:0] block
);
    logic [REG_BLOCK_WIDTH:0] block_shifted;

    always_comb
        block_shifted = block << 1;

This can be eliminated by changing the line to this monstrosity:
block_shifted = (REG_BLOCK_WIDTH+1)'(block << 1);

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.

@gtaylormb
Copy link
Collaborator

@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.

I just added a lint target to the Makefile in my fpga folder:

lint:
	slang $(PKG_SRC) $(RTL_SRC)

@chili-chips-ba
Copy link
Owner

chili-chips-ba commented Jul 17, 2024

image
IEEE.1364-2005.Verilog.pdf

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: block_shifted = {1'b0, (block << 1)};
For better transparency of what is going on: block_shifted = {1'b0, block[REG_BLOCK_WIDTH-2:0], 1'b0};

This fully spelled out form (which we recommend for shifting operations) brings a question: Does block_shifted really need to have one extra MSB bit, which is always and only 0?! Is that bit perhaps getting optimized out in synthesis? Has there been any reference to it in the synth warnings?

@gtaylormb
Copy link
Collaborator

This fully spelled out form (which we recommend for shifting operations) brings a question: Does block_shifted really need to have one extra MSB bit, which is always and only 0?! Is that bit perhaps getting optimized out in synthesis? Has there been any reference to it in the synth warnings?

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.

Waveform from Questa:
image

@gtaylormb
Copy link
Collaborator

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
of the expression and by the fact that it is part of another expression. For example, the bit size of the right-
hand expression of an assignment depends on itself and the size of the left-hand side."

You'll notice for shifts only j is described as self-determined. This implies i is context-determined, thus I believe you have misinterpreted the LRM, and why the original syntax works.

@gtaylormb
Copy link
Collaborator

Another proof i is context-determined:

    initial
        $display("Result: %0x", (1'b1 << 15) >> 15 == 1);

# Result: 1
    initial
        $display("Result: %0x", (1'b1 << 15) >> 15 == 1'b1);

# Result: 0

@gtaylormb
Copy link
Collaborator

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.

@chili-chips-ba
Copy link
Owner

chili-chips-ba commented Jul 17, 2024

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 😉

@gtaylormb
Copy link
Collaborator

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.

@gtaylormb
Copy link
Collaborator

Pretty excited to see your progress

@chili-chips-ba
Copy link
Owner

Stay tuned! @DadoCCAG, since an old timer C64 and SoundBlasterPro adict, is even more excited 😄.

BTW, would something like this block_shifted = {block, SHIFT_NUM'(0)};
be an acceptable alternative to block_shifted = block << SHIFT_NUM ?!

@gtaylormb
Copy link
Collaborator

gtaylormb commented Jul 17, 2024

Stay tuned! @DadoCCAG, since an old timer C64 and SoundBlasterPro adict, is even more excited 😄.

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.

@gtaylormb
Copy link
Collaborator

BTW, would something like this block_shifted = {block, SHIFT_NUM'(0)};
be an acceptable alternative to block_shifted = block << SHIFT_NUM ?!

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.

@chili-chips-ba
Copy link
Owner

chili-chips-ba commented Jul 18, 2024

@wsnyder, given that Verilator %Warning-WIDTHEXPAND got us derailed, and that our Question on Verilator Issues' page is now closed, would it make sense to reopen this Issue as an Enhancement or even Bug, looking to improve Verilator Linting rules, to preempt future derailments?!

@wsnyder
Copy link

wsnyder commented Jul 18, 2024

Yes, feel free to file a bug asking for the specific width shift case get suppressed.

@TarikHamedovic
Copy link
Collaborator Author

TarikHamedovic commented Jul 20, 2024

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

Warning: Wire opl3.\host_if.dout_sync.in [7] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [6] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [5] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [4] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [3] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [2] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [1] is used but has no driver.
Warning: Wire opl3.\host_if.dout_sync.in [0] is used but has no driver.

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 opl3.v:

wire [7:0] status;

host_if host_if(
    .clk(clk),
    .reset(reset),
    .clk_host(clk_host),
    .ic_n(ic_n),
    .cs_n(cs_n),
    .rd_n(rd_n),
    .wr_n(wr_n),
    .address(address),
    .din(din),
    .dout(dout),
    .opl3_reg_wr(opl3_reg_wr),
    .status(status),
    .force_timer_overflow(force_timer_overflow)
);

In host_if.v:

input wire clk_host;
input wire [7:0] status;
wire [7:0] host_status;

synchronizer #(.DATA_WIDTH(opl3_pkg_REG_FILE_DATA_WIDTH)) dout_sync(
    .clk(clk_host),
    .in(status),
    .out(host_status)
);

In synchronizer.v:

input wire [DATA_WIDTH - 1:0] in;

always @(posedge clk) 
    {sync_regs[DATA_WIDTH+:DATA_WIDTH], sync_regs[0+:DATA_WIDTH]} <= {sync_regs[0+:DATA_WIDTH], in};

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

Warning: Resizing cell port opl3.$flatten\channels.\control_operators.\operator.\phase_generator.\exp_lut_inst.$auto$proc_rom.cc:155:do_switch$1939.0.0.A_DO from 10 bits to 20 bits.
Warning: Resizing cell port opl3.host_if.afifo.mem.0.0.A_BM from 10 bits to 20 bits.
Warning: Resizing cell port opl3.host_if.afifo.mem.0.0.A_DI from 10 bits to 20 bits.
Warning: Resizing cell port opl3.host_if.afifo.mem.0.0.B_DO from 10 bits to 20 bits.

Most of these warnings refer to the following line in afifo:

reg [WIDTH-1:0] mem [(1<<LGFIFO)-1:0];

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.

Note

If anyone wants to try the synthesis for themselves you can just type in the 3.build folder:

make synth

That will generate the Verilog files from the SystemVerilog files(assuming you installed sv2v).
If you don't have sv2v you can run these commands instead:

./create_rtl_fileslist -v
make synth_vlog

@chili-chips-ba
Copy link
Owner

Warning Type 1

@gtaylormb, how about tying off status in the generate else branch?!

image

Warning Type 2

@ZipCPU did you know that your AFIFO from 10GE Switch was also used to synthesize FM music 😉
@pu-cc Any tips on this Yosys resizing, from the perspective of CologneChip memory form-factors?

  • Why is the synth choosing larger 20-bit primitive when the 10-bit RAM is also available in GateMate library?

image

@ZipCPU
Copy link

ZipCPU commented Jul 20, 2024

Almost all of my IP has various blocks within it, such as this one (below) in order to avoid the UNUSED variable lint warning.

// Make Verilator happy
// {{{
// Verilator lint_off UNUSED
wire	unused;
assign	unused = &{ 1'b0, S_AXI_AWCACHE, S_AXI_AWPROT, S_AXI_AWQOS,
    S_AXI_ARCACHE, S_AXI_ARPROT, S_AXI_ARQOS };
// Verilator lint_on  UNUSED
// }}}

It's easy enough to do, and it also acts as a documentation that you are aware of the issue.

Dan

@ZipCPU
Copy link

ZipCPU commented Jul 20, 2024

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.

@chili-chips-ba
Copy link
Owner

... 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.

@ZipCPU
Copy link

ZipCPU commented Jul 20, 2024

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.

@chili-chips-ba
Copy link
Owner

chili-chips-ba commented Jul 20, 2024

... you are right, we have updated that subtitle to make it more precise. Looking at Greg's Verilator options
verilator --lint-only -Wno-WIDTHEXPAND -Wno-WIDTHTRUNC --top-module $(TOP) $(VLOG_SRC)

we don't see -Wno-UNDRIVEN. @wsnyder any thoughts on this miss? Interestingly, Slang and Verible have also not caught it...

@wsnyder
Copy link

wsnyder commented Jul 20, 2024

UNDRIVEN isn't enabled by default, see the docs for what is. I recommend -Wall then turn off what you have to.

@TarikHamedovic
Copy link
Collaborator Author

TarikHamedovic commented Jul 20, 2024

When running Verilator linting in our script with the -Wall flag we get the UNDRIVEN warning aswell.

 %Warning-UNDRIVEN: /home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/3.build/../1.hw/modules/top_level/src/opl3.sv:66:37: Signal is not driven: 'status'
                                                                                                                                  : ... note: In instance 'opl3'
    66 |     logic [REG_FILE_DATA_WIDTH-1:0] status;
       |                                     ^~~~~~

But because -Wall flag in Verilator enables all checks there are some that are maybe worth mentioning here.
UNUSEDSIGNAL is being mentioned lots of times.

%Warning-UNUSEDSIGNAL: /home/user/openCologne/4.Advanced--4--Yamaha-OPL3-FM-Synth/3.build/../1.hw/modules/top_level/src/opl3.sv:67:11: Signal is not used: 'force_timer_overflow'
                                                                                                                                     : ... note: In instance 'opl3'
   67 |     logic force_timer_overflow;
      |           ^~~~~~~~~~~~~~~~~~~~

Others are the ones we discussed in WIDTHEXPAND, WIDTHTRUNC. Some that are maybe harmless in GENUNNAMED and PINCONNECTEMPTY aswell as VARHIDDEN and IMPORTSTAR.

@gtaylormb
Copy link
Collaborator

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.

@chili-chips-ba
Copy link
Owner

... 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:

  • set OPL3 into Manta
  • use PC Python script to convert standard soundtrack files to OPL3 programming sequences
  • and transfer them to OPL3 via UART at 115.2kbps
    Would that be too slow? What host_if throughput does OPL3 need to play Doom?

The other option is to use on-chip PicoRV32 RISC-V microcontroller. We are seeking your guidance and tips...

@gtaylormb
Copy link
Collaborator

use PC Python script to convert standard soundtrack files to OPL3 programming sequences

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.

@chili-chips-ba
Copy link
Owner

@TarikHamedovic if you believe that the original topic of this issue is now resolved, and corresponding discussion track exhausted, please proceed with closing it.

@TarikHamedovic
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants