Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Mark Cave-Ayland]

On 15/10/2021 07:58, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> > Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
> > whether the hardware is configured for A/UX mode.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/misc/mac_via.c         | 18 ++++++++++++++++++
> >   hw/misc/trace-events      |  1 +
> >   include/hw/misc/mac_via.h |  1 +
> >   3 files changed, 20 insertions(+)
> >
> > diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> > index 7a53a8b4c0..a08ffbcd88 100644
> > --- a/hw/misc/mac_via.c
> > +++ b/hw/misc/mac_via.c
> > @@ -880,6 +880,20 @@ static void via1_adb_update(MOS6522Q800VIA1State *v1s)
> >       }
> >   }
> >   
> > +static void via1_auxmode_update(MOS6522Q800VIA1State *v1s)
> > +{
> > +    MOS6522State *s = MOS6522(v1s);
> > +    int oldirq, irq;
> > +
>
> Please, add a comment to explain what happens here as "vMystery" is not 
> self-explicit.

Would something simple like:

/* Check to see if the A/UX mode bit has changed */

suffice here?

> > +    oldirq = (v1s->last_b & VIA1B_vMystery) ? 1 : 0;
> > +    irq = (s->b & VIA1B_vMystery) ? 1 : 0;
>
> For me, it would be clearer with:
>
>      oldirq = !!(v1s->last_b & VIA1B_vMystery);
>      irq = !!(s->b & VIA1B_vMystery);
>
> but it's a matter of taste.

I had to think carefully about that one :)  If you're fine with the existing version 
I'd prefer to keep it as I find it easier to read.

> > +
> > +    if (irq != oldirq) {
> > +        trace_via1_auxmode(irq);
> > +        qemu_set_irq(v1s->auxmode_irq, irq);
> > +    }
> > +}
> > +
> >   static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned 
> > size)
> >   {
> >       MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
> > @@ -902,6 +916,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr 
> > addr, uint64_t val,
> >       case VIA_REG_B:
> >           via1_rtc_update(v1s);
> >           via1_adb_update(v1s);
> > +        via1_auxmode_update(v1s);
> >   
> >           v1s->last_b = ms->b;
> >           break;
> > @@ -1046,6 +1061,9 @@ static void mos6522_q800_via1_init(Object *obj)
> >                 TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
> >   
> >       qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
> > +
> > +    /* A/UX mode */
> > +    qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
> >   }
> >   
> >   static const VMStateDescription vmstate_q800_via1 = {
> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> > index ede413965b..2da96d167a 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -228,6 +228,7 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, 
> > int value) "secto
> >   via1_adb_send(const char *state, uint8_t data, const char *vadbint) "state %s 
> > data=0x%02x vADBInt=%s"
> >   via1_adb_receive(const char *state, uint8_t data, const char *vadbint, int status, int 
> > index, int size) "state %s data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
> >   via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size) 
> > "data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
> > +via1_auxmode(int mode) "setting auxmode to %d"
> >   
> >   # grlib_ahb_apb_pnp.c
> >   grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" 
> > data:0x%08x"
> > diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
> > index 4506abe5d0..b445565866 100644
> > --- a/include/hw/misc/mac_via.h
> > +++ b/include/hw/misc/mac_via.h
> > @@ -43,6 +43,7 @@ struct MOS6522Q800VIA1State {
> >       MemoryRegion via_mem;
> >   
> >       qemu_irq irqs[VIA1_IRQ_NB];
> > +    qemu_irq auxmode_irq;
> >       uint8_t last_b;
> >   
> >       /* RTC */
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>


ATB,

Mark.