diff mbox series

[RFC,07/10] hw/mos6522: Fix initial timer counter reload

Message ID ae3528be683e131503ea272847a4490d505739ca.1629799776.git.fthain@linux-m68k.org
State New
Headers show
Series [RFC,01/10] hw/mos6522: Remove get_load_time() methods and functions | expand

Commit Message

Finn Thain Aug. 24, 2021, 10:09 a.m. UTC
The first reload of timer 1 is early by half of a clock cycle as it gets
measured from a falling edge. By contrast, the succeeding reloads are
measured from rising edge to rising edge.

Neglecting that complication, the behaviour of the counter should be the
same from one reload to the next. The sequence is always:

N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...

But at the first reload, the present driver does this instead:

N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...

Fix this deviation for both timer 1 and timer 2, and allow for the fact
that on a real 6522 the timer 2 counter is not reloaded when it wraps.

Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
 hw/misc/mos6522.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Comments

Mark Cave-Ayland Aug. 25, 2021, 8:23 a.m. UTC | #1
On 24/08/2021 11:09, Finn Thain wrote:

> The first reload of timer 1 is early by half of a clock cycle as it gets
> measured from a falling edge. By contrast, the succeeding reloads are
> measured from rising edge to rising edge.
> 
> Neglecting that complication, the behaviour of the counter should be the
> same from one reload to the next. The sequence is always:
> 
> N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
> 
> But at the first reload, the present driver does this instead:
> 
> N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
> 
> Fix this deviation for both timer 1 and timer 2, and allow for the fact
> that on a real 6522 the timer 2 counter is not reloaded when it wraps.
> 
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
>   hw/misc/mos6522.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 5b1657ac0d..0a241fe9f8 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
>       if (ti->index == 0) {
>           /* the timer goes down from latch to -1 (period of latch + 2) */
>           if (d <= (ti->counter_value + 1)) {
> -            counter = (ti->counter_value - d) & 0xffff;
> +            counter = ti->counter_value - d;
>           } else {
> -            counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> -            counter = (ti->latch - counter) & 0xffff;
> +            int64_t d_post_reload = d - (ti->counter_value + 2);
> +            /* XXX this calculation assumes that ti->latch has not changed */
> +            counter = ti->latch - (d_post_reload % (ti->latch + 2));
>           }
>       } else {
> -        counter = (ti->counter_value - d) & 0xffff;
> +        counter = ti->counter_value - d;
>       }
> -    return counter;
> +    return counter & 0xffff;
>   }
>   
>   static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
> @@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
>   
>       /* the timer goes down from latch to -1 (period of latch + 2) */
>       if (d <= (ti->counter_value + 1)) {
> -        counter = (ti->counter_value - d) & 0xffff;
> +        counter = ti->counter_value - d;
>       } else {
> -        counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> -        counter = (ti->latch - counter) & 0xffff;
> +        int64_t d_post_reload = d - (ti->counter_value + 2);
> +        /* XXX this calculation assumes that ti->latch has not changed */
> +        counter = ti->latch - (d_post_reload % (ti->latch + 2));
>       }
> +    counter &= 0xffff;
>   
>       /* Note: we consider the irq is raised on 0 */
>       if (counter == 0xffff) {

I think the code looks right, but I couldn't see an explicit reference to this 
behaviour in http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf. 
Presumably this matches what you've observed on real hardware?


ATB,

Mark.
Finn Thain Aug. 28, 2021, 12:46 a.m. UTC | #2
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:

> On 24/08/2021 11:09, Finn Thain wrote:
> 
> > The first reload of timer 1 is early by half of a clock cycle as it gets
> > measured from a falling edge. By contrast, the succeeding reloads are
> > measured from rising edge to rising edge.
> > 
> > Neglecting that complication, the behaviour of the counter should be the
> > same from one reload to the next. The sequence is always:
> > 
> > N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
> > 
> > But at the first reload, the present driver does this instead:
> > 
> > N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
> > 
> > Fix this deviation for both timer 1 and timer 2, and allow for the fact
> > that on a real 6522 the timer 2 counter is not reloaded when it wraps.
> > 
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> >   hw/misc/mos6522.c | 19 +++++++++++--------
> >   1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 5b1657ac0d..0a241fe9f8 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s,
> > MOS6522Timer *ti)
> >       if (ti->index == 0) {
> >           /* the timer goes down from latch to -1 (period of latch + 2) */
> >           if (d <= (ti->counter_value + 1)) {
> > -            counter = (ti->counter_value - d) & 0xffff;
> > +            counter = ti->counter_value - d;
> >           } else {
> > -            counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> > -            counter = (ti->latch - counter) & 0xffff;
> > +            int64_t d_post_reload = d - (ti->counter_value + 2);
> > +            /* XXX this calculation assumes that ti->latch has not changed
> > */
> > +            counter = ti->latch - (d_post_reload % (ti->latch + 2));
> >           }
> >       } else {
> > -        counter = (ti->counter_value - d) & 0xffff;
> > +        counter = ti->counter_value - d;
> >       }
> > -    return counter;
> > +    return counter & 0xffff;
> >   }
> >     static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int
> > val)
> > @@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> >         /* the timer goes down from latch to -1 (period of latch + 2) */
> >       if (d <= (ti->counter_value + 1)) {
> > -        counter = (ti->counter_value - d) & 0xffff;
> > +        counter = ti->counter_value - d;
> >       } else {
> > -        counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> > -        counter = (ti->latch - counter) & 0xffff;
> > +        int64_t d_post_reload = d - (ti->counter_value + 2);
> > +        /* XXX this calculation assumes that ti->latch has not changed */
> > +        counter = ti->latch - (d_post_reload % (ti->latch + 2));
> >       }
> > +    counter &= 0xffff;
> >         /* Note: we consider the irq is raised on 0 */
> >       if (counter == 0xffff) {
> 
> I think the code looks right, but I couldn't see an explicit reference to this
> behaviour in
> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf.

Yes, that datasheet is missing a lot of information.

> Presumably this matches what you've observed on real hardware?
> 

Yes.
diff mbox series

Patch

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 5b1657ac0d..0a241fe9f8 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,15 +63,16 @@  static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
     if (ti->index == 0) {
         /* the timer goes down from latch to -1 (period of latch + 2) */
         if (d <= (ti->counter_value + 1)) {
-            counter = (ti->counter_value - d) & 0xffff;
+            counter = ti->counter_value - d;
         } else {
-            counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
-            counter = (ti->latch - counter) & 0xffff;
+            int64_t d_post_reload = d - (ti->counter_value + 2);
+            /* XXX this calculation assumes that ti->latch has not changed */
+            counter = ti->latch - (d_post_reload % (ti->latch + 2));
         }
     } else {
-        counter = (ti->counter_value - d) & 0xffff;
+        counter = ti->counter_value - d;
     }
-    return counter;
+    return counter & 0xffff;
 }
 
 static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
@@ -103,11 +104,13 @@  static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
 
     /* the timer goes down from latch to -1 (period of latch + 2) */
     if (d <= (ti->counter_value + 1)) {
-        counter = (ti->counter_value - d) & 0xffff;
+        counter = ti->counter_value - d;
     } else {
-        counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
-        counter = (ti->latch - counter) & 0xffff;
+        int64_t d_post_reload = d - (ti->counter_value + 2);
+        /* XXX this calculation assumes that ti->latch has not changed */
+        counter = ti->latch - (d_post_reload % (ti->latch + 2));
     }
+    counter &= 0xffff;
 
     /* Note: we consider the irq is raised on 0 */
     if (counter == 0xffff) {