MiSTer-devel / ao486_MiSTer

ao486 port for MiSTer
Other
252 stars 69 forks source link

Drive OPL3 off audio clock, fix SB DSP clock domain crossing #158

Closed gtaylormb closed 3 weeks ago

gtaylormb commented 3 weeks ago

Use clk_audio to drive OPL3 as this is what the final clock domain is anyway (removes clk_opl entirely for better overall stability as discussed with @sorgelig here https://github.com/MiSTer-devel/ao486_MiSTer/pull/157), and we already have an existing sample clock enable implemented. Using divider of 494 gets us to 49.74899KHz sample freq, +0.07% away from perfect, closer than we were after removal of OPL3 PLL.

Also fixed the SB DSP & CMS clock domain crossing from clk_sys->clk_audio. Possible reduction in noise.

sorgelig commented 3 weeks ago

There is a simpler way to pass vector across clocks. Just pass it when 2 consecutive readings of sample are the same. For clock enable - you can use polynomial enabler, then resulting clock will be exactly the same as original.

gtaylormb commented 3 weeks ago

There is a simpler way to pass vector across clocks. Just pass it when 2 consecutive readings of sample are the same.

There are some issues with this. Let's look at the old code:

    old_l0 <= sb_out_l;
    old_l1 <= old_l0;
    if(old_l0 == old_l1) sb_l <= {old_l1[15],old_l1};

sb_out_l is in the clk_sys domain, old_* are in the clk_audio domain. old_l0 is quite likely to be metastable as transitions on sb_out_l can occur during within the setup and hold time of clk_audio, so it cannot be used for the comparison as this value may be metastable and invalid, and the metastability can propagate down the line (sb_l can go metastable, tmp_l as well, etc,)

We could register the vector a 3rd time and use the 2nd and 3rd flops for the comparison. That would filter out the metastability assuming sb_out_l is held stable for at least 2 clk_audio cycles, but since sb_out_l contains samples generated from 2 different sources, SB DSP and CMS, that might not be true. But let's assume sb_out_l is held and we properly filter out the metastability, there is a remaining issue.

Because sb_out_l is a vector, we cannot simply pass each bit through a 2 flop synchronizer circuit as these bits may resolve in different clock cycles of clk_audio due to routing and propagation delay differences between each bit. This is the issue of data coherency. Multiple registering won't fix this as the incorrect value simply propagates to the proceeding flops.

My fixed code using a single bit 2-flop synchronized handshaking signal addresses both of these issues. Another common method would be to pass the vector through an asynchronous FIFO. Metastability and data coherency on the vector are addressed through a combination of dual clock RAM and grey-code counters on the address pointers to pass clock domains (only 1 bit changes at a time). For this application (very few transitions compared to clock frequencies) the single bit handshaking signal is simpler and uses less resources. Even 2-flop synchronizers fail but the MTBF is extremely high. If this were an ultra-high reliability application (e.g. space, human safety, nuclear) or GHz frequencies I might use at least a 3-flop synchronizer.

Synchronizers get picked up by Quartus and it generates a MTBF calculation for you:

-------------------------
; Metastability Summary ;
-------------------------
Worst-Case MTBF of Design is 1e+09 years or 3.15e+16 seconds.
Typical MTBF of Design is 1e+09 years or 3.15e+16 seconds.

Number of Synchronizer Chains Found: 513
Shortest Synchronizer Chain: 2 Registers
Fraction of Chains for which MTBFs Could Not be Calculated: 0.992
Worst Case Available Settling Time: 2.800 ns

This is assuming you 1: have a valid synchronizer circuit between clock domains 2: you've used the synchronizers properly and inputs are held for the proper number of cycles in the receiving clock domain, and 3: static timing analysis passes.

gtaylormb commented 3 weeks ago

For clock enable - you can use polynomial enabler, then resulting clock will be exactly the same as original.

Do you have an example of this? I would be interested. It's probably not necessary as we're very close using an integer divider, but I'd like to see one of these.

sorgelig commented 3 weeks ago

Do you have an example of this? I would be interested. It's probably not necessary as we're very close using an integer divider, but I'd like to see one of these.

something like this: https://github.com/MiSTer-devel/C64_MiSTer/blob/8504b93e9324ef2d44f64b47317781699a2e2503/c64.sv#L1137-L1150