[OpenBIOS] [PATCH RFC] ppc: fix CUDA ADB packet header format

[Laurent Vivier]

Le 12/07/2015 14:34, Mark Cave-Ayland a écrit :
> On 10/07/15 18:49, Cormac O'Brien wrote:
> 
>> Previous versions of QEMU use a 2-byte header for CUDA ADB packets where it
>> should have used a 3-byte one, so this commit allows cuda_adb_req() to
>> differentiate between the two formats and act accordingly.
>>
>> Signed-off-by: Cormac O'Brien <cormac at c-obrien.org>
>> ---
>>  drivers/cuda.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cuda.c b/drivers/cuda.c
>> index 9555dea..deceacf 100644
>> --- a/drivers/cuda.c
>> +++ b/drivers/cuda.c
>> @@ -144,8 +144,18 @@ static int cuda_adb_req (void *host, const uint8_t *snd_buf, int len,
>>   //   CUDA_DPRINTF("len: %d %02x\n", len, snd_buf[0]);
>>      len = cuda_request(host, ADB_PACKET, snd_buf, len, buffer);
>>      if (len > 1 && buffer[0] == ADB_PACKET) {
>> -        pos = buffer + 2;
>> -        len -= 2;
>> +        /* QEMU's previous model used a 2-byte header where it should use a
>> +         * 3-byte one, so we check to see what kind of header we ought to use.
>> +         */
>> +        if (len > 2 && buffer[2] == snd_buf[0]) {
>> +            /* Correct 3-byte header */
>> +            pos = buffer + 3;
>> +            len -= 3;
>> +        } else {
>> +            /* Old 2-byte header */
>> +            pos = buffer + 2;
>> +            len -= 2;
>> +        }
>>      } else {
>>          pos = buffer + 1;
>>          len = -1;
>>
> 
> Technically the patch looks good to me - due to the nature of the
> change, can you confirm that this patch doesn't cause regressions in any
> other OS images during testing? Even better, is there a reference to a
> 3-byte header in any of the open OS sources to confirm that this is
> definitely the correct solution?

Perhaps Alex can explain his changes in QEMU:

https://github.com/agraf/qemu/commit/26a9dfedd107a8716e833f2e959500c0b8a435e7

But in kernel I see only 2-byte header:

http://lxr.free-electrons.com/source/drivers/macintosh/adb.c#L424
http://lxr.free-electrons.com/source/drivers/macintosh/via-cuda.c#L354

The first byte is ADB_PACKET (0) followed by the ADB command.

ADB command is a 8 bit command packet (4 bit address + 2 bit command + 2
bit register), followed by 2 to 8 bytes data packet.

We can find some pseudo-3-bytes header in AppleCuda source, but it's not:

http://www.opensource.apple.com/tarballs/AppleCuda/AppleCuda-100.0.3.tar.gz

IOReturn IOCudaADBController::setAutoPollEnable ( bool enable )
{
    cuda_request_t cmd;

    adb_init_request(&cmd);
    ADB_BUILD_CMD3(&cmd, ADB_PACKET_PSEUDO,
ADB_PSEUDOCMD_START_STOP_AUTO_POLL, (enable ? 1 : 0));

    return CudaDriver->doSyncRequest(&cmd);
}

But ADB_PACKET_PSEUDO is 1,  ADB_PSEUDOCMD_START_STOP_AUTO_POLL is 1.

In Linux we have:

http://lxr.free-electrons.com/source/drivers/macintosh/via-cuda.c#L137

137     cuda_request(&req, NULL, 3, CUDA_PACKET, CUDA_AUTOPOLL, 1);

where CUDA_PACKET is 1, and CUDA_AUTOPOLL is 1.
So Apple ADB_PACKET_PSEUDO == Linux CUDA_PACKET.

IMHO, it's not 3-byte header for ADB, but for the cuda controller.

In AppleCuda source, ADB read and write are only done with 2-byte header:

IOCudaADBController::readFromDevice ()
...
ADB_BUILD_CMD2(&cmd, ADB_PACKET_ADB,
            (ADB_ADBCMD_READ_ADB | (address << 4) | (adbRegister & 3)));
...
IOCudaADBController::writeToDevice ()
...
ADB_BUILD_CMD2(&cmd, ADB_PACKET_ADB,
            (ADB_ADBCMD_WRITE_ADB | (address << 4) | (adbRegister & 3)));
...

Laurent