jotego / jt12

FM sound source written in Verilog, fully compatible with YM2612, YM3438 (JT12), YM2203 (JT03) and YM2610 (JT10)
GNU General Public License v3.0
116 stars 18 forks source link

Wrong initial values #29

Closed jotego closed 5 years ago

jotego commented 5 years ago

Some games have wrong sound for a while or forever depending on initial value of registers.

Note that initial values of jt12_opram may not get implemented correctly when using initial. There is a warning from Xilinx ISE:

WARNING:PhysDesignRules:2410 - This design is using one or more 9K Block RAMs (RAMB8BWER). 9K Block RAM initialization data, both user defined and default, may be incorrect and should not be used. For more information, please reference Xilinx Answer Record 39999.

I don't know if Quartus also has a similar problem.

sorgelig commented 5 years ago

Initial block for RAM works fine in Quartus. I've already used such construction to implement the ROM.

jotego commented 5 years ago

The issue with ISE seems to be that opram uses two different block RAMs to compose the word width. So the initial values do not seem to break apart well. Do get any warning related to RAM initial values in the log files?

jotego commented 5 years ago

When opram had this initial value, we had the SEGA sound of Sonic:

{ ~7'd0, 37'd0 };

Could you try with this?

jt12_opram u_opram(
    .clk    ( clk       ),
    .clk_en ( clk_en    ),
    .wr_addr( cur       ),
    .rd_addr( next      ),
    .data   ( rst ? { ~7'd0, 37'd0 } : regop_in ),
    .q      ( regop_out )
);
sorgelig commented 5 years ago

nope... btw, your current code keeps ch/op = 0 when rst is active. So that line has no meaning.

gyurco commented 5 years ago

Can you put { ~7'd0, 37'd0 }; to the initialization code at least until a common solution with Xilinx is found? So at least fpgagen on MiST can use your repo.

gyurco commented 5 years ago

Here's a small reset code (I can send a PR if you prefer, but small enough to just copy-paste into jt12_reg.v), so you can even delete the whole initial block from opram.v

// memory for OP registers
parameter regop_width=44;

wire [regop_width-1:0] regop_in, regop_out;
reg opram_rst = 0;
reg [4:0] opram_reset_addr;

always @(posedge clk) begin
    reg old_rst;
    old_rst <= rst;
    if (~old_rst & rst) begin
        opram_reset_addr <= 0;
        opram_rst <= 1;
    end;
    if (clk_en) begin
        if (opram_rst) opram_reset_addr <= opram_reset_addr + 1'd1;
        if (opram_reset_addr == 5'd31) opram_rst <= 0;
    end
end

jt12_opram u_opram(
    .clk    ( clk       ),
    .clk_en ( clk_en    ),
    .wr_addr( opram_rst ? opram_reset_addr : cur ),
    .rd_addr( next      ),
    .data   ( opram_rst ? { ~7'd0, 37'd0 } : regop_in ),
    .q      ( regop_out )
);
sorgelig commented 5 years ago

The reset of opram is implemented in my repository. It's smaller.

jotego commented 5 years ago

@gyurco I have added { ~7'd0, 37'd0 } as you requested. How does it make a difference for MiST? @sorgelig , there was still a bug in jt12_sh_rst. The reset value had to wide enough:

if( rst ) begin
    bits[i] <= {stages{rstval}};

I caught this by running verilator in lint mode at the jt12_mmr level. I am going to run it at the top level. I have pushed these changes although I have not synthesized with them yet.

gyurco commented 5 years ago

Tested two problematic games with the latest commit, they're all OK!

sorgelig commented 5 years ago

is there a real purpose to init by { ~7'd0, 37'd0 } instead of simple 0 for whole size?

jotego commented 5 years ago

The first 7 bits are those of TL, which is volume. all set at one mean no sound.

jotego commented 5 years ago

I think initial values are good now. Closing this issue.

sorgelig commented 5 years ago

Nope. They aren't good. I know you don't like Titan 2 demo, but it clearly shows the initial values are wrong. If you load Titan 2 as the first title right after core loading, then you won't get the music. It will be muted. For quick test: the scene with ping-pong should have background noise, but it's also missing.

How i solve it currently: Launch Sonic 1 and wait for music to start and then load Titan 2 demo. Music and pingpong background noise will work. Btw, if you won't wait Sonic music to start and launch Titan 2 earlier (like after SEGA sound) then it won't have the music and noise.

This clearly shows that initial values of YM2612 aren't all zeros. Change ~7d'0 to 7'd0 doesn't fix the problem. I vote for re-opening this issue.

sorgelig commented 5 years ago

i want to add: since jt12_opram is erased upon reset, the problem is not in its initial values but in some other registers which may be modified through jt12 writes but not reset.

jotego commented 5 years ago

The developer of Titan 2 reported that the problem came from Z80, and not the YM2612 so I think we can discard that one.

Registers outside jt12_opram are also being reset to a proper value when there is a meaning to doing that. Some registers in the pipeline will get cleared away after a full FM cycle (24 FM clock cycles) so there is no need for reset.

So I think we can say that initial values are fine.

sorgelig commented 5 years ago

I reworked ZBUS access, so now Titan 2 starts with music always.