* [PATCH] mos6522: remove anh register
@ 2019-12-12 19:43 Laurent Vivier
2019-12-13 1:44 ` David Gibson
2019-12-13 9:36 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 6+ messages in thread
From: Laurent Vivier @ 2019-12-12 19:43 UTC (permalink / raw)
To: qemu-devel
Cc: Mark Cave-Ayland, Laurent Vivier, qemu-ppc,
Dr . David Alan Gilbert, David Gibson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] mos6522: remove anh register
2019-12-12 19:43 [PATCH] mos6522: remove anh register Laurent Vivier
@ 2019-12-13 1:44 ` David Gibson
2019-12-13 8:45 ` Laurent Vivier
2019-12-13 9:36 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 6+ messages in thread
From: David Gibson @ 2019-12-13 1:44 UTC (permalink / raw)
To: Laurent Vivier
Cc: Mark Cave-Ayland, qemu-ppc, qemu-devel, Dr . David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 3419 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mos6522: remove anh register
2019-12-13 1:44 ` David Gibson
@ 2019-12-13 8:45 ` Laurent Vivier
2019-12-14 10:47 ` Mark Cave-Ayland
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2019-12-13 8:45 UTC (permalink / raw)
To: David Gibson
Cc: Mark Cave-Ayland, qemu-ppc, qemu-devel, Dr . David Alan Gilbert
[-- Attachment #1.1: Type: text/plain, Size: 867 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mos6522: remove anh register
2019-12-13 8:45 ` Laurent Vivier
@ 2019-12-14 10:47 ` Mark Cave-Ayland
2019-12-14 13:19 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Mark Cave-Ayland @ 2019-12-14 10:47 UTC (permalink / raw)
To: Laurent Vivier, David Gibson
Cc: qemu-ppc, qemu-devel, Dr . David Alan Gilbert
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mos6522: remove anh register
2019-12-14 10:47 ` Mark Cave-Ayland
@ 2019-12-14 13:19 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-14 13:19 UTC (permalink / raw)
To: Mark Cave-Ayland, Laurent Vivier, David Gibson
Cc: qemu-ppc, qemu-devel, Dr . David Alan Gilbert
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mos6522: remove anh register
2019-12-12 19:43 [PATCH] mos6522: remove anh register Laurent Vivier
2019-12-13 1:44 ` David Gibson
@ 2019-12-13 9:36 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-13 9:36 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: qemu-ppc, Mark Cave-Ayland, Dr . David Alan Gilbert, David Gibson
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-14 21:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-12 19:43 [PATCH] mos6522: remove anh register Laurent Vivier
2019-12-13 1:44 ` David Gibson
2019-12-13 8:45 ` Laurent Vivier
2019-12-14 10:47 ` Mark Cave-Ayland
2019-12-14 13:19 ` Philippe Mathieu-Daudé
2019-12-13 9:36 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).