emu-russia / dmgcpu

DMG CPU Reverse Engineering
Creative Commons Zero v1.0 Universal
29 stars 4 forks source link

`CLK_ENA` should only go high after `OSC_STABLE` #219

Closed Rodrigodd closed 6 months ago

Rodrigodd commented 6 months ago

Noticed that when integrating this CPU into the complete Game Boy system made by msinger. That work is displayed at https://github.com/msinger/dmg-sim/pull/1.

From msinger research, CLK_ENA should only go high after OSC_STABLE, otherwise the OSC_STABLE never goes up (this is better explained in the link above).

Extracting the logic of CLK_ENA from Seq.v, we can see it never interacts with OSC_STABLE, and goes up immediately after RESET:

RESET SYNC_RESET CLK_ENA
0 0 ~(d[100] \| IR[4] & d[101]) \| SeqControl_1
0 1 ~(d[100] \| IR[4] & d[101])
1 x 0
ogamespec commented 6 months ago

Hi, and I was just checking a couple days ago to see if anyone was interested in this repository in 2024. And then a day later, you write. Coincidence? I don't think so, hehe.

I've realized the problem, I'll see how the wires go and report back what I find. I had to spend some time just to remember the context (wires, signal assignments, etc.) 😅

ogamespec commented 6 months ago

There is a "big picture" derived from PlanAhead by digesting the Seq.v: https://raw.githubusercontent.com/emu-russia/dmgcpu/main/HDL/Design/Seq.png

I have traced how OSC_STABLE connects through all the gates to CLK_ENA. Here is the path: OSC_STABLE -> g59 -> g58 -> g57 -> g44 -> g45 -> g43 -> g49 -> g31 -> CLK_ENA.

It's about the "never interacts" thesis 😃

msinger commented 6 months ago

@ogamespec, did you automatically generate the HDL from this circuit diagram or vice versa? Or is it possible that there was a mistake made by hand? I mean, does it make sense to double check that the logic in the diagram matches the logic implemented in HDL?

Rodrigodd commented 6 months ago

"never interact"

I think I exagerated here. There is a path between OSC_STABLE and CLK_ENA, but when you trace the signals through all gates, it nevers changes the final result.

I confirm this a couple of times by tracing by hand, and I also convert the logic to SymPy (see logic.py in my PR), from which I derived the truth table I showed above.

ogamespec commented 6 months ago

@ogamespec, did you automatically generate the HDL from this circuit diagram or vice versa? Or is it possible that there was a mistake made by hand? I mean, does it make sense to double check that the logic in the diagram matches the logic implemented in HDL?

If to speak about Seq.v, there the most part is made automatically, a little bit corrected by hands (renaming of signals) (See https://github.com/emu-russia/dmgcpu/tree/main/netlist); The rest part is also partly automatic, partly manual, I don't remember exactly, but I think decoders are made automatically and corrected by hands.

There is a path between OSC_ENA and CLK_ENA

I'm confused. The description of the Issue includes OSC_STABLE.

There can certainly be errors in HDL (any chip study is prone to this), my main concern was to reconstruct the topology and transistor circuits. HDL is just a "bonus". 😄

Rodrigodd commented 6 months ago

I'm confused. The description of the Issue includes OSC_STABLE.

Sorry, was a typo. I edit it to fix it.

ogamespec commented 6 months ago

Looks like I should make a sequencer in Logisim so you can poke around :) I'll be back later, see what I can do.

ogamespec commented 6 months ago

Started analyzing the sequencer in Logisim, so far it's going well:

image

The whole left part is occupied by 2 circuits: the beginning and the end of the interrupt sequence.

On the right side is a more confusing instruction clocking sequencer schematic, continued...

Rodrigodd commented 6 months ago

When I was investigating I generated the following schematic using yosys and https://github.com/nturley/netlistsvg. Here, in case it is useful:

sequencer

I used yosys's connect -unset and optimization passes to only show the gates connected to CLK_ENA:

```sh yosys -p "\ read_verilog _GekkioNames.v SeqCells.v Seq.v; \ prep -top Sequencer; \ select Sequencer; \ connect -unset a; \ connect -unset OSC_ENA; \ connect -unset RD; \ connect -unset MREQ; \ connect -unset SeqOut_1; \ connect -unset SeqOut_2; \ connect -unset SeqOut_3; \ select -clear; \ prep -top Sequencer; \ write_json sequencer.json" # prep -top Sequencer -flatten; \ netlistsvg sequencer.json -o sequencer.svg ```
ogamespec commented 6 months ago

Big picture from Logisim:

image

We have the same subcircuit for CLK_ENA, but I haven't figured out if that's good or bad yet.

I'll do some more tinkering and check the topology photos and the restored netlist for the SYNC_RESET and RESET signals. I take it you expect the circuit to behave differently?

EDIT: Oh lol read the title of the issue again 😄

ogamespec commented 6 months ago

image

hmmm... g49 is seq_rs_latch2. I may have incorrectly reconstructed the circuit logic of this gate.

ogamespec commented 6 months ago

I traced the RESET and SYNC_RESET signals again, there is no problem with them. They go where they need to go. Most likely the nr and s inputs for cell g49 (module4_2) are just mixed up. I will check it now.

msinger commented 6 months ago

Here is the circuit outside of the CPU, which requires CLK_ENA to go high after OSC_STABLE: image The magenta circle encloses the relevant circuit and the smaller red circle marks the important part which is the reason for this "CLK_ENA after OSC_STABLE" constraint. We have a latch TUBO to the left of the red circle. This latch gets reset to 0 at system reset (RESET) or when the crystal oscillator gets disabled (OSC_ENA=0), so it starts out at 0. It's ~Q output is 1 then. It goes into the AND-gate UNUT (the part that I have encircled red). The other input of this AND-gate comes from the DIV-register. The wire marked "16Hz" will get high after around 32 milliseconds after reset. So only when TUBO is reset and after these 32 milliseconds, UNUT's output can be high. UNUT outputting high will raise OSC_STABLE and it will deassert reset signals to most peripherals including the SYNC_RESET of the CPU. If the CPU raises CLK_ENA before the 32 milliseconds have passed (which is signaled by a raising OSC_STABLE), then the ~Q output of TUBO will be low and UNUT is blocked from ever outputting high, therefore AFER will never release the SYNC_RESET.

Just wanted to put this out there so that the statement of "CLK_ENA has to go high after OSC_STABLE" is not just floating there in the air.

ogamespec commented 6 months ago

Thanks for the detailed explanation of what's out there, but I confirm - the problem was a trivial swap of the s and nr inputs for cell g49 (which is special and occurs in a single instance). I didn't notice it, it's my bad! The fix is already uploaded.

ogamespec commented 6 months ago

@Rodrigodd image

msinger commented 6 months ago

Nice catch! Those unique instances are evil. :)

Rodrigodd commented 6 months ago

Great work! But does this fix the problem? When I was investigating I remember that the OSC_STABLE signal was being lost much earlier.

Below is the output of logic.py. Here dff is modeled as a direct connection, and latch as an and, which is commutative, so that error was not affecting this script at least.

w108    = RESET
w132    = d101
w133    = ~IR_4 | ~d101
w134    = IR_4 & d101
w109    = IR_4 & d101
w107    = d100
w120    = d100
w121    = (~IR_4 & ~RESET & ~d100) | (~RESET & ~d100 & ~d101)
w122    = RESET | d100 | (IR_4 & d101)
w112    = SeqControl_1
w113    = SeqControl_1
w118    = OSC_STABLE
w16     = SYNC_RESET
w15     = ~SYNC_RESET
w119    = SYNC_RESET | ~OSC_STABLE
w117    = SYNC_RESET | ~OSC_STABLE
w115    = SYNC_RESET | ~OSC_STABLE
w116    = OSC_STABLE & ~SYNC_RESET
w114    = False
w14     = ~RESET & ~SYNC_RESET
w27     = RESET | SYNC_RESET | ~SeqControl_1
w28     = RESET | (SYNC_RESET & d100) | (d100 & ~SeqControl_1) | (IR_4 & SYNC_RESET & d101) | (IR_4 & d101 & ~SeqControl_1)
w66     = (SeqControl_1 & ~RESET & ~SYNC_RESET) | (~IR_4 & ~RESET & ~d100) | (~RESET & ~d100 & ~d101)
CLK_ENA = ~((RESET | d100 | (IR_4 & d101)) & ~(SeqControl_1 & (~(RESET | SYNC_RESET) | ~((OSC_STABLE & ~SYNC_RESET) | ~(OSC_STABLE & ~SYNC_RESET)))))

CLK_ENA = ~RESET & (SeqControl_1 | ~d100) & (~SYNC_RESET | ~d100) & (SeqControl_1 | ~IR_4 | ~d101) & (~IR_4 | ~SYNC_RESET | ~d101)
CLK_ENA = (SeqControl_1 & ~RESET & ~SYNC_RESET) | (~IR_4 & ~RESET & ~d100) | (~RESET & ~d100 & ~d101)

[0, 0] : (SeqControl_1 | ~d100) & (SeqControl_1 | ~IR_4 | ~d101)
[0, 1] : ~d100 & (~IR_4 | ~d101)
[1, 0] : False
[1, 1] : False

It is hard to follow without the Verilog (I should have copied the module names in the script), but w114, from module seq_nor g45.x to seq_oai_21 g43.a1, is already False. It comes from w116 and w117, where w116 is the last signal that changes with OSC_STABLE.

But this script doesn't take into account the transient state of the circuit, and there may be errors. When I reach home, I will run dmg-sim to confirm.

The new version of logic.py:

```python from sympy import symbols, Not, And, Or, Symbol, simplify from sympy.logic import SOPform from sympy.logic.boolalg import truth_table, to_cnf, to_dnf # Define symbols RESET, SYNC_RESET, OSC_STABLE = symbols('RESET SYNC_RESET OSC_STABLE') SeqControl_1, d101, d100 = symbols('SeqControl_1 d101 d100') IR_4 = symbols('IR_4') def dff(d): return d def latch(d, enable): return And(d, enable) # SYNC_RESET = 1 # RESET = 1 # OSC_STABLE = 0 # SeqControl_1 = 0 # d100 = 0 # d101 = 0 # IR_4 = 0 # Given expressions w108 = RESET w132 = d101 w133 = Not(And(IR_4, w132)) w134 = Not(w133) w109 = dff(w134) w107 = d100 w120 = dff(w107) w121 = Not(Or(w120, w109, w108)) w122 = Not(w121) w112 = SeqControl_1 w113 = dff(w112) w118 = OSC_STABLE w16 = SYNC_RESET w15 = Not(w16) w119 = Not(And(w15, w118)) w117 = dff(w119) w115 = dff(w117) w116 = Not(w115) w114 = Not(Or(w116, w117)) w108 = RESET w16 = SYNC_RESET w14 = Not(Or(w16, w108)) w27 = Not(And(Or(w14, w114), w113)) w28 = latch(d=w122, enable=w27) w66 = Not(w28) CLK_ENA = w66 def simpl(expr): return to_dnf(expr, simplify=True) # Print the converted expressions print("w108 =", simpl(w108)) print("w132 =", simpl(w132)) print("w133 =", simpl(w133)) print("w134 =", simpl(w134)) print("w109 =", simpl(w109)) print("w107 =", simpl(w107)) print("w120 =", simpl(w120)) print("w121 =", simpl(w121)) print("w122 =", simpl(w122)) print("w112 =", simpl(w112)) print("w113 =", simpl(w113)) print("w118 =", simpl(w118)) print("w16 =", simpl(w16)) print("w15 =", simpl(w15)) print("w119 =", simpl(w119)) print("w117 =", simpl(w117)) print("w115 =", simpl(w115)) print("w116 =", simpl(w116)) print("w114 =", simpl(w114)) print("w14 =", simpl(w14)) print("w27 =", simpl(w27)) print("w28 =", simpl(w28)) print("w66 =", simpl(w66)) print("CLK_ENA =", CLK_ENA) print() print("CLK_ENA =", to_cnf(CLK_ENA, simplify=True)) print("CLK_ENA =", to_dnf(CLK_ENA, simplify=True)) print() table = truth_table(CLK_ENA, [RESET, SYNC_RESET]) for t in table: print(t[0], ':', to_cnf(t[1], simplify=True)) ``` <\details>
ogamespec commented 6 months ago

Polished sequencer for Logisim (Evo 3.8.0). You can try how it works there too.

seq

https://github.com/emu-russia/dmgcpu/tree/main/logisim -> seq.circ

ogamespec commented 6 months ago

I won't close the issue until you confirm that the error is gone. If you find something else strange, it is better to create a new Issue. Thank you!

Rodrigodd commented 6 months ago

CLK_ENA still goes high immediately after RESET is low.

image

ogamespec commented 6 months ago

From HDL/Icarus/run.bat:

image

It seems that OSC_STABLE is in inverse polarity (Active-low).

ogamespec commented 6 months ago

Here is the module to simulate the external binding of the CLK: https://github.com/emu-russia/dmgcpu/blob/main/HDL/Icarus/external_clk.v

Rodrigodd commented 6 months ago

The simulation from Icarus/run.v works because the signal from UPOF (referred as SixteenHz here) is hardcoded as high:

https://github.com/emu-russia/dmgcpu/blob/a716cfcd23e45db20b855e4859df6bfd1150c11b/HDL/Icarus/external_clk.v#L87-L101

If you replaced it with a signal that goes high only after a while you can reproduce this issue:

    wire TUBO_q;
    wire TUBO_nq;
    wire ASOL_q;
    wire ASOL_nq;
    reg SixteenHz;
    wire AFER_nq;

    initial SixteenHz = 1'b0;
    always begin
        // wait 4 cycles after RESET
        repeat (12) @(posedge CLK);
        SixteenHz <= ~SixteenHz;
    end

    NOR_LATCH TUBO (.set(CLK_ENA), .res(RESET | ~OSC_ENA), .q(TUBO_q), .nq(TUBO_nq));
    assign OSC_STABLE = (T1_nT2 | nT1_T2 | (TUBO_nq & SixteenHz));
    NOR_LATCH ASOL (.set(~(~OSC_STABLE | RESET)), .res(RESET), .q(ASOL_q), .nq(ASOL_nq));
    DFFR_B AFER (.clk(MAIN_CLK_P), .nres(T1T2), .d(ASOL_nq), .q(SYNC_RESET), .nq(AFER_nq) );

image

Rodrigodd commented 6 months ago

It is hard to follow without the Verilog [...] but w114, from module seq_nor g45.x to seq_oai_21 g43.a1, is already False.

I said that earlier but when you take into account the transient behavior of the DFFs, w114 is actually a OSC_STABLE positive edge detector. ogamespec has put a note about that in the logisim, I didn't see it earlier.

More precisely, it is a negative edge detection for ~OSC_STABLE | SYNC_RESET, or a positive edge detection for OSC_STABLE & ~SYNC_RESET

Including that in the logic (still ignoring the remaining DFF, but I think they are less important), the logic of CLK_ENA becomes:

RESET SYNC_RESET CLK_ENA
0 0 SeqControl_1 \| (~IR_4 & ~d100) \| (~d100 & ~d101)
0 1 (OSC_STABLE_EDGE & SeqControl_1) \| (~IR_4 & ~d100) \| (~d100 & ~d101)
1 x OSC_STABLE_EDGE & SeqControl_1

But still, CLK_ENA goes up immediately after RESET, because IR and d and zeroed out.

ogamespec commented 6 months ago

I have more to think about and study the materials. I don't see yet what I can do from inside SM83 to solve the problem.

ogamespec commented 6 months ago

I think I found another strange thing. In the big picture, CLK8 = CLK:

image

And on the wiki, the sequencer description says this:

image

CLK polarity is definitely messed up somewhere, I'll look into it.

ogamespec commented 6 months ago

After #226, I get a picture like this:

image

Is that a good thing or a bad? :)

Rodrigodd commented 6 months ago

That picture looks fantastic!

Can also confirm that it now works in dmg-sim:

image

Was #227 that finally fixed it? Can't tell what changed exactly, only g33 appears to have it's clks effectively swapped.

I looked into why my logic.py didn't work very well. It has a error in the w27 logic, but even so it was not capturing how the last latch is working, holding CLK_ENA low until the OSC_STABLE edge detection.

But in any case, thank you very much for your work!

The CPU is still not working, in the picture above you can see that JP 0003 is jumping to FFFC, but I was able to fix that by inverting the zbus and wbus. It still failed some instructions later, but I believe I can manage for now. After fixing a couple more instructions I'll make a PR here.

If I become stuck again I'll let you know!

ogamespec commented 6 months ago

The main edit is here:

https://github.com/emu-russia/dmgcpu/pull/227/files#diff-c025afa1f2b555e1b11f5ba3973d1acd61e44d03d0376c6b04a733c561eac0b2 (mixed up clk/cclk ports)

And this one: https://github.com/emu-russia/dmgcpu/pull/227/files#diff-121501a027f0c874bbf57c4ba47bba71b8e3edff89a1d96e1c849642203d9251 (related to mixed up CLKs)

I'm glad it's more lively now, we'll see what happens next. I think this Issue is done, since I see that OSC_STABLE and CLK_ENA seem to interact properly :). I'll keep the issue open a bit longer for aftershocks. Thank you!

ogamespec commented 6 months ago

only g33 appears to have it's clks effectively swapped

Both latches used in single instances have a special connection to the CLK.

g55 (NMI Edge detect), opposite of DFFs that use CLK9/8: image

g33 (CB opcode related): image

I also want to mention one particular DFF that uses CLK2/1 (g84): image It's not clear why it's there yet, but it's there :)

I have tidied up the schematic for Logisim to match all the edits. To analyze the sequencer work it is the easiest to use, I added a lot of notes there. But I can't update HDL/Design/Seq.pdf yet, I don't have access to PlanAhead, so it's not fixed yet.

Rodrigodd commented 6 months ago

only g33 appears to have it's clks effectively swapped

Sorry, I meant g84. Because you inverted the clock in the module definition, and then for each instantiation, minus g84. So only g84 appear to change. I see know it uses different clocks.

But g84 appear to not contribuite to the CLK_ENA signal, so I am a little confused. But this is just curiosity, I can figure it out later what I am missing, don't need to worry about that.

ogamespec commented 6 months ago

Sorry, I meant g84. Because you inverted the clock in the module definition, and then for each instantiation, minus g84. So only g84 appear to change. I see know it uses different clocks.

Nice one :) I'll check again and write.

ogamespec commented 6 months ago

I confirm, for g84 the clk and cclk ports are also mixed up :( clk should be CLK1; cclk should be CLK2.

image

I'll fix that!

It turns out that g84 can be interpreted as negedge DFF: CLK=CLK2 (positive polarity), CCLK=CLK1 (negative polarity).

ogamespec commented 6 months ago

image

ogamespec commented 6 months ago

@Rodrigodd I've updated all the materials with the latest actual fixes (including HDL sources, design pdf, sequencer schematic for Logisim, netlist for the Deroute program, and description on the wiki). Now things seem to have cleared up on SM83's side on this Issue. If nothing else comes up, you can close this Issue permanently :) Thanks a lot again, everyone!

Rodrigodd commented 6 months ago

Was https://github.com/emu-russia/dmgcpu/pull/227 that finally fixed it? Can't tell what changed exactly, only g33 appears to have it's clks effectively swapped.

I was testing it again to check what exactly was the fix. Actually, it was https://github.com/emu-russia/dmgcpu/pull/222, about the swapped ports of the latch, that fixed this issue. I must have confused my two clones of the repository when I did the testing before, I'm not sure. Sorry for the confusion.

So this issue is more than fixed, I will close it now. Thank you @ogamespec for thoroughly investigating the issue!