Message ID | 20200614142840.10245-1-mark.cave-ayland@ilande.co.uk (mailing list archive) |
---|---|
Headers | show |
Series | ADB: fix autopoll issues and rework mac_via state machine | expand |
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.
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.
This patchset is something I have been chipping away at for a while since spending some time over the Christmas holidays trying to boot the MacOS toolbox ROM on the new q800 machine. Initially I discovered that there were some problems when the MacOS ROM was enumerating ADB devices due to multiple meanings of the vADBInt bit. After fixing this there were still issues with keys being dropped during autopoll which were eventually traced back to the autopoll timer re-firing before the host had managed to read back the previous response. At this point I noticed that CUDA/PMU/mac_via all had their own implementations of ADB autopoll, and that it would make sense to consolidate the autopoll timer, mask, interval and locking into the ADB bus. This would allow the logic to be removed from each separate device and managed in just one place. Finally I updated the trace-events to allow separate tracing of bus requests and device responses which makes it easier to follow the ADB enumeration process. The breakdown of the patchset is as follows: - Patch 1 keeps checkpatch happy for the remainder of the patchset whilst patch 2 is the proper fix for a spurious ADB register 3 write during enumeration caused by ignoring the request length which I had tried to work around earlier. - Patches 3 to 10 are part of the autopoll consolidation process which moves the separate autopoll implementations into a single implementation within ADBBusState. - Patches 11 to 13 update the ADB implementation to hold a status variable indicating the result of the last request and allow devices to indicate whether they have data to send. This extra information is required by the upcoming mac_via state machine changes. - Patches 14 to 17 add a variable and functions to block and unblock ADB autopoll at bus level, adding the functions at the correct places within CUDA and PMU. - Patches 18 and 19 rework the mac_via ADB state machine so that the bus can be enumerated correctly, and both explicit and autopoll requests work under both MacOS and Linux. - Patch 20 enforces the blocking and unblocking of autopoll at the ADB level, including adding an assert() to prevent developers from trying to make an ADB request whilst autopoll is in progress. - Patches 21 and 22 update the trace-events to separate out ADB device and ADB bus events. The patch has been tested by myself and a couple of others during the development process across the PPC g3beige/mac99 and 68K q800 machine so it should be quite solid. One thing to indicate is that the patchset bumps the VMState versions for the affected devices but does not allow older versions to load. This is a conscious decision given that for the mac_via device used in the q800 machine it would be just about impossible to map this in a way that would work for all cases. Similarly for the Mac PPC machines migration is already hit/miss due to timebase issues so I don't see this as being a big loss. To finish off I'd also like to say a big thank-you to both Laurent Vivier and Finn Thain who both took time to answer my questions, dump information from a real q800, and analyse it in very fine detail. Without them this patchset would still be several months away. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Mark Cave-Ayland (22): adb: coding style update to fix checkpatch errors adb: fix adb-mouse read length and revert disable-reg3-direct-writes workaround cuda: convert ADB autopoll timer from ns to ms pmu: fix duplicate autopoll mask variable pmu: honour autopoll_rate_ms when rearming the ADB autopoll timer adb: introduce realize/unrealize and VMStateDescription for ADB bus adb: create autopoll variables directly within ADBBusState cuda: convert to use ADBBusState internal autopoll variables pmu: convert to use ADBBusState internal autopoll variables mac_via: convert to use ADBBusState internal autopoll variables adb: introduce new ADBDeviceHasData method to ADBDeviceClass adb: keep track of devices with pending data adb: add status field for holding information about the last ADB request adb: use adb_request() only for explicit requests adb: add autopoll_blocked variable to block autopoll cuda: add adb_autopoll_block() and adb_autopoll_unblock() functions pmu: add adb_autopoll_block() and adb_autopoll_unblock() functions mac_via: move VIA1 portB write logic into mos6522_q800_via1_write() mac_via: rework ADB state machine to be compatible with both MacOS and Linux adb: only call autopoll callbacks when autopoll is not blocked adb: use adb_device prefix for ADB device trace events adb: add ADB bus trace events hw/input/adb-kbd.c | 42 ++-- hw/input/adb-mouse.c | 65 +++--- hw/input/adb.c | 212 +++++++++++++++++-- hw/input/trace-events | 27 ++- hw/misc/mac_via.c | 399 ++++++++++++++++++++++------------- hw/misc/macio/cuda.c | 60 +++--- hw/misc/macio/pmu.c | 47 ++--- hw/misc/trace-events | 3 + hw/ppc/mac_newworld.c | 2 - include/hw/input/adb.h | 26 ++- include/hw/misc/mac_via.h | 2 +- include/hw/misc/macio/cuda.h | 4 - include/hw/misc/macio/pmu.h | 4 - 13 files changed, 608 insertions(+), 285 deletions(-)