hw/misc/mos6522.c | 12 ++++-------- include/hw/misc/mos6522.h | 1 - 2 files changed, 4 insertions(+), 9 deletions(-)
Register addr 1 is defined as buffer A with handshake (vBufAH),
register addr 15 is also defined as buffer A without handshake (vBufA).
Linux kernel has a big "DON'T USE!" next to the register 1 addr
definition (vBufAH), and only uses register 15 (vBufA).
So remove the definition of 'anh' and use only 'a' (with VIA_REG_ANH and
VIA_REG_A).
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
hw/misc/mos6522.c | 12 ++++--------
include/hw/misc/mos6522.h | 1 -
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index cecf0be59e..86ede4005c 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -244,6 +244,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
val = s->b;
break;
case VIA_REG_A:
+ case VIA_REG_ANH:
val = s->a;
break;
case VIA_REG_DIRB:
@@ -297,9 +298,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
val = s->ier | 0x80;
break;
default:
- case VIA_REG_ANH:
- val = s->anh;
- break;
+ g_assert_not_reached();
}
if (addr != VIA_REG_IFR || val != 0) {
@@ -322,6 +321,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
mdc->portB_write(s);
break;
case VIA_REG_A:
+ case VIA_REG_ANH:
s->a = (s->a & ~s->dira) | (val & s->dira);
mdc->portA_write(s);
break;
@@ -395,9 +395,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
break;
default:
- case VIA_REG_ANH:
- s->anh = val;
- break;
+ g_assert_not_reached();
}
}
@@ -439,7 +437,6 @@ const VMStateDescription vmstate_mos6522 = {
VMSTATE_UINT8(pcr, MOS6522State),
VMSTATE_UINT8(ifr, MOS6522State),
VMSTATE_UINT8(ier, MOS6522State),
- VMSTATE_UINT8(anh, MOS6522State),
VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 0,
vmstate_mos6522_timer, MOS6522Timer),
VMSTATE_END_OF_LIST()
@@ -460,7 +457,6 @@ static void mos6522_reset(DeviceState *dev)
s->ifr = 0;
s->ier = 0;
/* s->ier = T1_INT | SR_INT; */
- s->anh = 0;
s->timers[0].frequency = s->frequency;
s->timers[0].latch = 0xffff;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 493c907537..97384c6e02 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -115,7 +115,6 @@ typedef struct MOS6522State {
uint8_t pcr;
uint8_t ifr;
uint8_t ier;
- uint8_t anh;
MOS6522Timer timers[2];
uint64_t frequency;
--
2.23.0
On Thu, Dec 12, 2019 at 08:43:59PM +0100, Laurent Vivier wrote: > Register addr 1 is defined as buffer A with handshake (vBufAH), > register addr 15 is also defined as buffer A without handshake (vBufA). > > Linux kernel has a big "DON'T USE!" next to the register 1 addr > definition (vBufAH), and only uses register 15 (vBufA). > > So remove the definition of 'anh' and use only 'a' (with VIA_REG_ANH and > VIA_REG_A). I'm not actually following the rationale for removing the register. Linux doesn't use it, but if it's part of the real hardware model we should keep it, no? > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > hw/misc/mos6522.c | 12 ++++-------- > include/hw/misc/mos6522.h | 1 - > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index cecf0be59e..86ede4005c 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -244,6 +244,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > val = s->b; > break; > case VIA_REG_A: > + case VIA_REG_ANH: > val = s->a; > break; > case VIA_REG_DIRB: > @@ -297,9 +298,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > val = s->ier | 0x80; > break; > default: > - case VIA_REG_ANH: > - val = s->anh; > - break; > + g_assert_not_reached(); > } > > if (addr != VIA_REG_IFR || val != 0) { > @@ -322,6 +321,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > mdc->portB_write(s); > break; > case VIA_REG_A: > + case VIA_REG_ANH: > s->a = (s->a & ~s->dira) | (val & s->dira); > mdc->portA_write(s); > break; > @@ -395,9 +395,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > break; > default: > - case VIA_REG_ANH: > - s->anh = val; > - break; > + g_assert_not_reached(); > } > } > > @@ -439,7 +437,6 @@ const VMStateDescription vmstate_mos6522 = { > VMSTATE_UINT8(pcr, MOS6522State), > VMSTATE_UINT8(ifr, MOS6522State), > VMSTATE_UINT8(ier, MOS6522State), > - VMSTATE_UINT8(anh, MOS6522State), > VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 0, > vmstate_mos6522_timer, MOS6522Timer), > VMSTATE_END_OF_LIST() > @@ -460,7 +457,6 @@ static void mos6522_reset(DeviceState *dev) > s->ifr = 0; > s->ier = 0; > /* s->ier = T1_INT | SR_INT; */ > - s->anh = 0; > > s->timers[0].frequency = s->frequency; > s->timers[0].latch = 0xffff; > diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h > index 493c907537..97384c6e02 100644 > --- a/include/hw/misc/mos6522.h > +++ b/include/hw/misc/mos6522.h > @@ -115,7 +115,6 @@ typedef struct MOS6522State { > uint8_t pcr; > uint8_t ifr; > uint8_t ier; > - uint8_t anh; > > MOS6522Timer timers[2]; > uint64_t frequency; -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Le 13/12/2019 à 02:44, David Gibson a écrit : > On Thu, Dec 12, 2019 at 08:43:59PM +0100, Laurent Vivier wrote: >> Register addr 1 is defined as buffer A with handshake (vBufAH), >> register addr 15 is also defined as buffer A without handshake (vBufA). >> >> Linux kernel has a big "DON'T USE!" next to the register 1 addr >> definition (vBufAH), and only uses register 15 (vBufA). >> >> So remove the definition of 'anh' and use only 'a' (with VIA_REG_ANH and >> VIA_REG_A). > > I'm not actually following the rationale for removing the register. > Linux doesn't use it, but if it's part of the real hardware model we > should keep it, no? It's actually two methods to access the same register (with handshake, without handshake). In the datashit Register 15 is described as "Same as register 1 except no handshake". Thanks, Laurent
On 13/12/2019 08:45, Laurent Vivier wrote: > Le 13/12/2019 à 02:44, David Gibson a écrit : >> On Thu, Dec 12, 2019 at 08:43:59PM +0100, Laurent Vivier wrote: >>> Register addr 1 is defined as buffer A with handshake (vBufAH), >>> register addr 15 is also defined as buffer A without handshake (vBufA). >>> >>> Linux kernel has a big "DON'T USE!" next to the register 1 addr >>> definition (vBufAH), and only uses register 15 (vBufA). >>> >>> So remove the definition of 'anh' and use only 'a' (with VIA_REG_ANH and >>> VIA_REG_A). >> >> I'm not actually following the rationale for removing the register. >> Linux doesn't use it, but if it's part of the real hardware model we >> should keep it, no? > > It's actually two methods to access the same register (with handshake, > without handshake). > > In the datashit Register 15 is described as "Same as register 1 except > no handshake". Indeed, certainly my reading of the specification was that it was the same register. I'd be inclined to add qemu_log() at unimp for the handshake registers so at least we can easily spot if anything is using them. ATB, Mark.
On 12/14/19 11:47 AM, Mark Cave-Ayland wrote: > On 13/12/2019 08:45, Laurent Vivier wrote: >> Le 13/12/2019 à 02:44, David Gibson a écrit : >>> On Thu, Dec 12, 2019 at 08:43:59PM +0100, Laurent Vivier wrote: >>>> Register addr 1 is defined as buffer A with handshake (vBufAH), >>>> register addr 15 is also defined as buffer A without handshake (vBufA). >>>> >>>> Linux kernel has a big "DON'T USE!" next to the register 1 addr >>>> definition (vBufAH), and only uses register 15 (vBufA). >>>> >>>> So remove the definition of 'anh' and use only 'a' (with VIA_REG_ANH and >>>> VIA_REG_A). >>> >>> I'm not actually following the rationale for removing the register. >>> Linux doesn't use it, but if it's part of the real hardware model we >>> should keep it, no? >> >> It's actually two methods to access the same register (with handshake, >> without handshake). >> >> In the datashit Register 15 is described as "Same as register 1 except >> no handshake". > > Indeed, certainly my reading of the specification was that it was the same register. > I'd be inclined to add qemu_log() at unimp for the handshake registers so at least we > can easily spot if anything is using them. Agreed, clever than my review suggestion. case VIA_REG_A: + qemu_log_mask(LOG_UNIMP, ...); + /* fall through */ + case VIA_REG_ANH: val = s->a; break;
On 12/12/19 8:43 PM, Laurent Vivier wrote: > Register addr 1 is defined as buffer A with handshake (vBufAH), > register addr 15 is also defined as buffer A without handshake (vBufA). Maybe add "IOW both addresses access the same register." > > Linux kernel has a big "DON'T USE!" next to the register 1 addr > definition (vBufAH), and only uses register 15 (vBufA). I agree with David, the Linux reference is confusing. I'd omit it. > > So remove the definition of 'anh' and use only 'a' (with VIA_REG_ANH and > VIA_REG_A). > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > hw/misc/mos6522.c | 12 ++++-------- > include/hw/misc/mos6522.h | 1 - > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index cecf0be59e..86ede4005c 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -244,6 +244,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > val = s->b; > break; > case VIA_REG_A: Maybe add: /* As we do not model handshake, fall through no handshake. */ > + case VIA_REG_ANH: > val = s->a; > break; > case VIA_REG_DIRB: > @@ -297,9 +298,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) > val = s->ier | 0x80; > break; > default: > - case VIA_REG_ANH: > - val = s->anh; Oops, default was buggy. Maybe worth: Fixes: 51f233ec92c > - break; > + g_assert_not_reached(); > } > > if (addr != VIA_REG_IFR || val != 0) { > @@ -322,6 +321,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > mdc->portB_write(s); > break; > case VIA_REG_A: /* As we do not model handshake, fall through no handshake. */ > + case VIA_REG_ANH: > s->a = (s->a & ~s->dira) | (val & s->dira); > mdc->portA_write(s); > break; > @@ -395,9 +395,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); > break; > default: > - case VIA_REG_ANH: > - s->anh = val; Buggy again. > - break; > + g_assert_not_reached(); > } > } > > @@ -439,7 +437,6 @@ const VMStateDescription vmstate_mos6522 = { > VMSTATE_UINT8(pcr, MOS6522State), > VMSTATE_UINT8(ifr, MOS6522State), > VMSTATE_UINT8(ier, MOS6522State), > - VMSTATE_UINT8(anh, MOS6522State), Don't you need to increase .version_id? > VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 0, > vmstate_mos6522_timer, MOS6522Timer), > VMSTATE_END_OF_LIST() > @@ -460,7 +457,6 @@ static void mos6522_reset(DeviceState *dev) > s->ifr = 0; > s->ier = 0; > /* s->ier = T1_INT | SR_INT; */ > - s->anh = 0; > > s->timers[0].frequency = s->frequency; > s->timers[0].latch = 0xffff; > diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h > index 493c907537..97384c6e02 100644 > --- a/include/hw/misc/mos6522.h > +++ b/include/hw/misc/mos6522.h > @@ -115,7 +115,6 @@ typedef struct MOS6522State { > uint8_t pcr; > uint8_t ifr; > uint8_t ier; > - uint8_t anh; > > MOS6522Timer timers[2]; > uint64_t frequency; >
© 2016 - 2024 Red Hat, Inc.