Re: [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
> >
> > 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/26a9dfedd107a8716e833f2e959500c0b8a435e...

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