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

[Finn Thain]

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.