MI_INIT_MODE bug ?

[bsmiles32]

Hi,

I was reading the source code, and I think thre might be mistake in vr4300/interface.c :

Code: Select all

75    result = word & 0x7F;
76
77     if (word & 0x0080)
78       result &= ~MI_INIT_MODE;
79     else if (word & 0x0100)
80       result |= MI_INIT_MODE;
81
82     if (word & 0x0200)
83       result &= ~MI_EBUS_TEST_MODE;
84     else if (word & 0x0400)
85       result |= MI_EBUS_TEST_MODE;
86
87     if (word & 0x0800) {
88       vr4300->mi_regs[MI_INTR_REG] &= ~MI_INTR_DP;
89       check_for_interrupts(vr4300); // TODO/FIXME: ???
90     }
91
92     if (word & 0x1000)
93       result &= ~MI_RDRAM_REG_MODE;
94     else if (word & 0x2000)
95       result |= MI_RDRAM_REG_MODE;
96
97     vr4300->mi_regs[MI_INIT_MODE_REG] = result;

From that code, bits MI_INIT_MODE, MI_EBUS_TEST_MODE and MI_RDRAM_REG_MODE are always cleared, whatever value "word" has.
I guess, not using the temporary "result" would fix it.

Anyway, congratulations for this very clean code base... I'm kind of jealous, because the emulator I normally contribute to doesn't have this well designed architecture.
Keep up the good work !

[beannaich]

My reading of that code doesn't give me the same impression at all.

[MarathonMan]

My reading of that code doesn't give me the same impression at all.

Same here... scratching my head.

Thanks for the kudos though.

[bsmiles32]

Sorry, bad explanation.
What I meant was that MI_INIT_MODE, MI_EBUS_TEST_MODE and MI_RDRAM_REG_MODE bits can be cleared even when not explicitly asked.

Example:
* initial value: MI_INIT_MODE_REG = 0x80; // MI_INIT_MODE set
* input : word = 0x00; // ie should change nothing

the current code ends up with MI_INIT_MODE_REG = 0x00 instead of 0x80

The same apply to MI_EBUS_TEST_MODE and MI_RDRAM_REG_MODE.

[grivy]

With word = 0x00, the last statement would become:
vr4300->mi_regs[MI_INIT_MODE_REG] = 0x00;
But I can't say whether that be good or bad

[beannaich]

I think I see what you're seeing:

Code: Select all

result = (vr4300->mi_regs[MI_INIT_MODE_REG] & ~0x7f) | (word & 0x7f); // <-- is this a fix to the problem you noticed?

if (word & 0x0080)
  result &= ~MI_INIT_MODE;
else if (word & 0x0100)
  result |= MI_INIT_MODE;

if (word & 0x0200)
  result &= ~MI_EBUS_TEST_MODE;
else if (word & 0x0400)
  result |= MI_EBUS_TEST_MODE;

if (word & 0x0800) {
  vr4300->mi_regs[MI_INTR_REG] &= ~MI_INTR_DP;
  check_for_interrupts(vr4300); // TODO/FIXME: ???
}

if (word & 0x1000)
  result &= ~MI_RDRAM_REG_MODE;
else if (word & 0x2000)
  result |= MI_RDRAM_REG_MODE;

vr4300->mi_regs[MI_INIT_MODE_REG] = result;

@marathonman: is there a name associated with the bottom 7 bits of MI_INIT_MODE_REG? If so, can they be #defined? I'd look into it myself and pull request, but, you know.. I can't.
@krom: test ROM time?

[krom]

@krom: test ROM time?

I think I'll wait to see what MarathonMan thinks before I jump into making a test ROM for this, but it should not be too hard to cook up something to check out the correct behavior =D

[bsmiles32]

is this a fix to the problem you noticed?

Yes

[MarathonMan]

Ahh, I see... I'll try to look at this in the next day or so and patch as required.

Thanks for the report!