Re: [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs

[Mark Cave-Ayland]

On 07/02/2022 19:29, Peter Maydell wrote:

> On Thu, 27 Jan 2022 at 21:01, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> > For historical reasons each mos6522 instance implements its own setting and
> > update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
> > of today this is no longer required, and it is now possible to implement
> > the mos6522 IRQs as standard qdev gpios.
> >
> > Switch over to use qdev gpios for the mos6522 device and update all instances
> > accordingly.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/misc/mac_via.c         | 56 +++++++--------------------------------
> >   hw/misc/macio/cuda.c      |  5 ++--
> >   hw/misc/macio/pmu.c       |  4 +--
> >   hw/misc/mos6522.c         | 15 +++++++++++
> >   include/hw/misc/mac_via.h |  6 +----
> >   include/hw/misc/mos6522.h |  2 ++
> >   6 files changed, 32 insertions(+), 56 deletions(-)
>
>
> > -static void via2_nubus_irq_request(void *opaque, int irq, int level)
> > +static void via2_nubus_irq_request(void *opaque, int n, int level)
> >   {
> >       MOS6522Q800VIA2State *v2s = opaque;
> >       MOS6522State *s = MOS6522(v2s);
> > -    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> > +    qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
> >
> >       if (level) {
> >           /* Port A nubus IRQ inputs are active LOW */
> > -        s->a &= ~(1 << irq);
> > -        s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
> > +        s->a &= ~(1 << n);
> >       } else {
> > -        s->a |= (1 << irq);
> > -        s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
> > +        s->a |= (1 << n);
> >       }
> >
> > -    mdc->update_irq(s);
> > +    qemu_set_irq(irq, level);
> >   }
>
> It feels a bit inconsistent here that we're still reaching into
> the MOS6522State to set s->a, but I guess this is still
> better than what we had before.

Yeah it's a little bit messy. On the Mac the Nubus IRQs are ORd to produce the VIA 
interrupt line, but the individual Nubus IRQ lines are wired to the VIA port A to 
allow the state of each line to be determined. The VIA port IO lines are 
bi-directional, so rather than try and implement this with GPIOs it's easiest to have 
a separate VIA2 Nubus GPIOs that also handle the port IO lines as above.

> > -#define VIA1_IRQ_NB             8
> > -
> >   #define VIA1_IRQ_ONE_SECOND     (1 << VIA1_IRQ_ONE_SECOND_BIT)
> >   #define VIA1_IRQ_60HZ           (1 << VIA1_IRQ_60HZ_BIT)
> >   #define VIA1_IRQ_ADB_READY      (1 << VIA1_IRQ_ADB_READY_BIT)
> > @@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
> >
> >       MemoryRegion via_mem;
> >
> > -    qemu_irq irqs[VIA1_IRQ_NB];
> > +    qemu_irq irqs[VIA_NUM_INTS];
>
> This irqs[] array appears to be entirely unused. You could
> delete it as a separate patch before this one.

Ah yes, before this patch each VIA had its own GPIOs which used the methods in 
MOS6522DeviceClass to manipulate the IFR bit. Since the Q800 VIAs have the MOS6522 
device as a parent class, the GPIOs are inherited from that and so this array should 
actually be removed as part of this patch.

> >       qemu_irq auxmode_irq;
> >       uint8_t last_b;
> >
> > @@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
> >   #define VIA2_IRQ_SCSI_BIT       CB2_INT_BIT
> >   #define VIA2_IRQ_ASC_BIT        CB1_INT_BIT
> >
> > -#define VIA2_IRQ_NB             8
> > -
> >   #define VIA2_IRQ_SCSI_DATA      (1 << VIA2_IRQ_SCSI_DATA_BIT)
> >   #define VIA2_IRQ_NUBUS          (1 << VIA2_IRQ_NUBUS_BIT)
> >   #define VIA2_IRQ_UNUSED         (1 << VIA2_IRQ_SCSI_BIT)
> > diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> > index 12abd8b8d2..ced8a670bf 100644
> > --- a/include/hw/misc/mos6522.h
> > +++ b/include/hw/misc/mos6522.h
> > @@ -57,6 +57,8 @@
> >   #define T2_INT             (1 << T2_INT_BIT)
> >   #define T1_INT             (1 << T1_INT_BIT)
> >
> > +#define VIA_NUM_INTS       5
>
> Were we not using 5,6,7 previously ?

5 and 6 are for the in-built timers, and 7 is a set/clear flag so they are not 
externally accessible.

> Anyway,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM


ATB,

Mark.