Bug in TAC counter rate calculation

[ullenius]

From the 1.04u spec (url )

  TAC Field Bits:

            Bit 1 & 0, counter rate
                00: 4096 Hz
                01: 262144 Hz
                10: 65536 Hz
                11: 16384 Hz

            Bit 2, interrupt type
                0: Use v-blank
                1: Use timer

I believe there's a bug in this line in gbs.c

const long timertc = (256 - tma) * (16 << (((tac) & 3) << 1));

We extract this code to a separate function. And then compare it to a more explicit mapping we get different results.

#include <stdio.h>

void counter_rate(const unsigned int);
const unsigned int tac_rate(const unsigned int);

int main() {

    printf("00: %d\n", tac_rate(0 + 4) * 256);
    printf("01: %d\n", tac_rate(1 + 4) * 256);
    printf("10: %d\n", tac_rate(2 + 4) * 256);
    printf("11: %d\n", tac_rate(3 + 4) * 256);

    printf("----current code---\n");
    counter_rate(0 + 4);
    counter_rate(1 + 4);
    counter_rate(2 + 4);
    counter_rate(3 + 4);

    return 0;
}

void counter_rate(const unsigned int tac) {
    const unsigned tma = 0; // we set modulo to 0

    printf("tac: %d\n", tac & 3);
    const long timertc = (256 - tma) * (16 << (((tac) & 3) << 1));
    printf("timertc: %ld\n", timertc);

}

const unsigned int tac_rate(const unsigned int tac) { // tma is also 0 here

    const unsigned int result = tac & 0x3;

    if (result == 0) {
        return 16; // 16 * 256 = 4096
    }

    else if (result == 1) {
        return 16 << 6; // 256 * 1024 = 262144
    }
    else if (result == 2) {
        return 16 << 4; // 256^2 = 65536
    }
    else if (result == 3) {
        return 16 << 2; // 256 * 64 = 16384
    }

}

Running this results in:

$ cc -o foobar tac.c
./foobar

00: 4096
01: 262144
10: 65536
11: 16384

----current code---
tac: 0
timertc: 4096
tac: 1
timertc: 16384
tac: 2
timertc: 65536
tac: 3
timertc: 262144

TL;DR

The current code has swapped the mappings for tac 01 and 11 (see results above).

[ranma]

const long timertc = (256 - tma) * (16 << (((tac) & 3) << 1));

Thats not actually the code in gbs.c, which version of the code were you looking at?

The actual code in gbs.c is:

long timertc = (256-gbs->tma) * (16 << (((gbs->tac+3) & 3) << 1));

which also matches gbhw.c:

gbhw->timertc = 16 << (((val+3) & 3) << 1);

The first thing it does is rotate the value by three, so the table (for modified val) becomes regular:

00: 262144 01: 65536 10: 16384 11: 4096

The actual timer base Hz is the GBHW_CLOCK / timertc, so the formula (ignoring TMA for now) then becomes:

const long timerbase = (4194304 / (16 << (((tac + 3) & 3) << 1)));
printf("timerbase: %ld\n", timerbase);

Adjusting your code for that I get:

$ gcc -o /tmp/tac /tmp/tac.c
$ /tmp/tac
00: 4096
01: 262144
10: 65536
11: 16384
----current code---
tac: 0
timerbase: 4096
tac: 1
timerbase: 262144
tac: 2
timerbase: 65536
tac: 3
timerbase: 16384

Which matches (so the code in ghbw.c is correct) and is the rate at which TIMA increases. Now TMA is reload value for TIMA when it overflows, so effectively the divider: TMA 0 -> 256 base clocks per interrupt, TIMA restarts at 0 TMA 1 -> 255 base clocks per interrupt, TIMA restarts at 1 TMA 255 -> 1 base clock per interrupt, TIMA restarts at 255

So the correct full formula should be: const double timerirqhz = (4194304.0 / (16 << (((tac + 3) & 3) << 1))) / (256 - tma)

What gbs.c currently does is:

long timertc = (256-gbs->tma) * (16 << (((gbs->tac+3) & 3) << 1));
float hz = GBHW_CLOCK / (float)timertc;

So that is 4194304.0 / ((256 - tma) * (16 << (((tac+3) & 3) << 1). I can reorder the multiplication: 4194304.0 / ((16 << (((tac+3) & 3) << 1)) * (256 - tma)) And move it out of the brackets: 4194304.0 / (16 << (((tac+3) & 3) << 1) / (256 - tma) And now we can see that it should be correct already (same as the "correct full formula" above). (Unless I made a mistake here somewhere)

Granted, the existing code is a bit hard to follow...