Re: [PATCH 00/22] ADB: fix autopoll issues and rework mac_via state machine - Mark Cave-Ayland

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, laurent@vivier.eu
Subject: Re: [PATCH 00/22] ADB: fix autopoll issues and rework mac_via state machine
Date: Sat, 20 Jun 2020 13:10:08 +0100	[thread overview]
Message-ID: <4867f0f6-c011-ff3f-9a6b-98daee72e025@ilande.co.uk> (raw)
In-Reply-To: <alpine.LNX.2.22.394.2006162015310.11@nippy.intranet>

On 16/06/2020 11:24, Finn Thain wrote:

> Tested-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Thanks for all your work on this.
> 
> I've just noticed a discrepancy between the traces from an ADB bus scan on 
> Laurent's Apple Quadra and an ADB bus scan on your patched QEMU machine.
> 
> Apple Q800:
> 
> [C1f][s   ][Rff-][Rff ][rff-]
> [C2f][s   ][R61 ][R05 ][r00-]
> [C3f][s   ][R79 ][R01 ][r00-]
> [C4f][s   ][Rff-][Rff ][rff-]
> [C5f][s   ][Rff-][Rff ][rff-]
> [C6f][s   ][Rff-][Rff ][rff-]
> [C7f][s   ][Rff-][Rff ][rff-]
> [C8f][s   ][Rff-][Rff ][rff-]
> [C9f][s   ][Rff-][Rff ][rff-]
> [Caf][s   ][Rff-][Rff ][rff-]
> [Cbf][s   ][Rff-][Rff ][rff-]
> [Ccf][s   ][Rff-][Rff ][rff-]
> [Cdf][s   ][Rff-][Rff ][rff-]
> [Cef][s   ][Rff-][Rff ][rff-]
> [Cff][s   ][Rff-][Rff ][rff-]
> 
> QEMU Q800:
> 
> [C1f][s   ][Rff-][Rff ][rff-]
> [C2f][s   ][R02 ][R01 ][r00-]
> [C3f][s   ][R03 ][R02 ][r00-]
> [C4f][s   ][R03-][R02 ][rff-]
> [C5f][s   ][R03-][R02 ][rff-]
> [C6f][s   ][R03-][R02 ][rff-]
> [C7f][s   ][R03-][R02 ][rff-]
> [C8f][s   ][R03-][R02 ][rff-]
> [C9f][s   ][R03-][R02 ][rff-]
> [Caf][s   ][R03-][R02 ][rff-]
> [Cbf][s   ][R03-][R02 ][rff-]
> [Ccf][s   ][R03-][R02 ][rff-]
> [Cdf][s   ][R03-][R02 ][rff-]
> [Cef][s   ][R03-][R02 ][rff-]
> [Cff][s   ][R03-][R02 ][rff-]
> 
> I think this could be easy to fix; it's probably just an uninitialized 
> packet buffer. When you come to submit v2, you may want to look into this.

Thanks for testing again! I think the issue here is simply that I've been doing my
testing on mailine which still has to extra switch to IDLE state before reading the
response.

Can you test with the following patch on top of the series to see if this works for you?

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b3fb710f7f..362878ca06 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -745,6 +745,11 @@ static void adb_via_send(MacVIAState *s, int state, uint8_t data)
                                           s->adb_data_out,
                                           s->adb_data_out_index);
         s->adb_data_in_index = 0;
+        if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
+            s->adb_data_in[0] = 0xff;
+            s->adb_data_in[1] = 0xff;
+            s->adb_data_in_size = 2;
+        }

         /*
          * If last command is TALK, store it for use by autopoll and adjust
@@ -782,9 +787,6 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
             if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
                 *data = 0xff;
                 ms->b |= VIA1B_vADBInt;
-                s->adb_data_in[0] = 0xff;
-                s->adb_data_in[1] = 0xff;
-                s->adb_data_in_size = 2;
                 qemu_irq_raise(s->adb_data_ready);
             } else if (s->adb_data_in_size > 0) {
                 adb_bus->status = ADB_STATUS_POLLREPLY;


This should correctly initialise the buffer in both cases. If it works then I'll
squash it into the original patch for v2.


ATB,

Mark.

