Re: [PATCH v2 00/22] ADB: fix autopoll issues and rework mac_via state machine

[Mark Cave-Ayland]

On 26/06/2020 08:14, Laurent Vivier wrote:

> Le 23/06/2020 à 22:49, Mark Cave-Ayland a écrit :
>> 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>
>>
>>
>> v2:
>> - Rebased onto master
>> - Added R-B tags from Philippe
>> - Fixed byte discrepency at end of bus timeout spotted by Finn
>> - Added Tested-by tag from Finn
>>
>>
>> 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               | 210 ++++++++++++++++--
>>  hw/input/trace-events        |  27 ++-
>>  hw/misc/mac_via.c            | 411 +++++++++++++++++++++++------------
>>  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, 620 insertions(+), 283 deletions(-)
>>
> 
> Acked-by: Laurent Vivier <laurent@vivier.eu>

Thanks Laurent! I know David is busy at the moment, but he's fine with the Mac 
PPC
changes so I'll add your A-B tag and send a PR shortly.


ATB,

Mark.