| Message ID | 20191212194359.13508-1-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mos6522: remove anh register | expand |
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;
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 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; >
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;
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;
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(-)