mos6522: remove anh register
diff mbox series

Message ID 20191212194359.13508-1-laurent@vivier.eu
State New
Headers show
Series
  • mos6522: remove anh register
Related show

Commit Message

Laurent Vivier Dec. 12, 2019, 7:43 p.m. UTC
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(-)

Comments

David Gibson Dec. 13, 2019, 1:44 a.m. UTC | #1
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;
Laurent Vivier Dec. 13, 2019, 8:45 a.m. UTC | #2
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
Philippe Mathieu-Daudé Dec. 13, 2019, 9:36 a.m. UTC | #3
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;
>
Mark Cave-Ayland Dec. 14, 2019, 10:47 a.m. UTC | #4
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.
Philippe Mathieu-Daudé Dec. 14, 2019, 1:19 p.m. UTC | #5
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;

Patch
diff mbox series

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;