| Message ID | 20211013212132.31519-5-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | q800: GLUE updates for A/UX mode | expand |
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. > + 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. > + > + 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>
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. > Stupid question: why do you use GPIO to pass the auxmode information between VIA and GLUE? Can't we use object_property_set_link() to set a pointer to the GLUE object? Thanks, Laurent
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.
On 15/10/2021 08:17, 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. > > Stupid question: why do you use GPIO to pass the auxmode information between VIA and GLUE? > > Can't we use object_property_set_link() to set a pointer to the GLUE object? For devices that are independent i.e. not contained within others I prefer to restrict the interface to properties that are visible within "info qom-tree" which are MRs and GPIOs. Otherwise GLUE requires knowledge of VIA internals which breaks the device abstraction. ATB, Mark.
Le 15/10/2021 à 21:50, Mark Cave-Ayland a écrit : > 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? Yes > >>> + 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. Up to you, as I said, a matter of taste. > >>> + >>> + 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.
Le 15/10/2021 à 21:59, Mark Cave-Ayland a écrit : > On 15/10/2021 08:17, 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. >> >> Stupid question: why do you use GPIO to pass the auxmode information between VIA and GLUE? >> >> Can't we use object_property_set_link() to set a pointer to the GLUE object? > > For devices that are independent i.e. not contained within others I prefer to restrict the interface > to properties that are visible within "info qom-tree" which are MRs and GPIOs. Otherwise GLUE > requires knowledge of VIA internals which breaks the device abstraction. > OK, makes sense. Thanks, Laurent
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; + + oldirq = (v1s->last_b & VIA1B_vMystery) ? 1 : 0; + irq = (s->b & VIA1B_vMystery) ? 1 : 0; + + 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 */
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(+)