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

[Peter Maydell <peter.maydell@linaro.org>]

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.

> -#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.

>      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 ?

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

thanks
-- PMM