qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated d


From: Alastair D'Silva
Subject: Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device
Date: Mon, 19 Dec 2016 12:47:06 +1100

On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
> (added a couple of people to cc who might have an opinion on the i2c
> protocol questions below)

I'm certainly no expert, but I'll try :)

> On 29 November 2016 at 19:30, Fabio Urquiza <address@hidden>
> wrote:
<snip>
> > 
> > One of the problems we had to address is regarding the behavior of
> > the
> > ATMEL I2C TPM AT97SC3204T Linux driver. After the driver sends a
> > request
> > to the TPM, it keeps polling the device with I2C read request. The
> > real
> > AT97SC3204T hardware ignore those requests while the response is
> > not ready
> > simply by not ACKing the I2C read on its address. When the response
> > is
> > ready it will ACK the request and proceed writing the response in
> > the wire.
> > 
> > The QEMU I2C API does not provide a way to not ACK I2C requests
> > when the
> > device is not ready to transmit. In fact, if the device has been
> > configured
> > in the virtual machine, QEMU will automatically ACK every request
> > without
> > asking for the device permission for it. Therefore we created a
> > flag in
> > the I2CSlave struct that tells the I2C subsystem that the device is
> > busy
> > and not ready to ACK a I2C transfer. We understand that it could
> > not be
> > the best solution to the problem, but it appears to be the solution
> > that
> > have the least impact in the code overall. Suggestions on a
> > different
> > approach would be welcome.
> 
> I2C slaves definitely ought to be able to NAK I2C requests. This
> is possible for sends, ie data sent from the master to the slave
> (the slave just returns non-zero from its I2CSlaveClass::send
> function).
> In i2c protocol terms, this corresponds to the slave generating a NAK
> (by not taking SDA low) after the master has sent a byte of data.
> The bitbang_i2c code used by versatile was buggy in handling NAKs
> for sends until commit 9706e0162d24.
> 

I agree that they should be able to NAK, it seems that this may be the
first device that actually needs that functionality (or at least, has
implemented it).

The change looks about as minimal as one could make it, so I'm mostly
happy with it.

I think we may need to add the busy field to hw/i2c/core.c:
VMStateDescription vmstate_i2c_slave, if this state should be
persisted.

Also, it looks like the busy element is only ever used in a boolean
context, so a boolean type may be more appropriate.

> For recv I'm less sure how it ought to work, so if you can explain
> in terms of the i2c protocol what slave h/w behaviour we're trying
> to emulate that would help. At what points in the protocol can
> the slave return a NAK?
> 
> Our current API seems to envisage that the slave can return a
> negative value from I2CSlaveClass::recv instead of a data byte,
> but I'm not sure what this means in the i2c protocol.

Negative values are propagated upwards, where they are treated as
errors, eg, in hw/i2c/aspeed_i2c.c:aspeed_i2c_bus_handle_cmd():

int ret = i2c_recv(bus->bus);
if (ret < 0) {
    qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
    ret = 0xff;
}

The call to i2c_recv is too late to issue the NAK, I believe they occur
during the start_transfer() call.


> If I understand your patch correctly, this is adding support
> for the slave refusing to ACK when the master sends out the
> slave address and r/w bit. I think that makes sense, but rather
> than having a state flag in the I2CSlave struct, we should
> change the prototype of the I2CSlaveClass event method so that
> it can return a value indicating ack or nak.
> 

Hmm, this could end up being quite an invasive change, but ultimately
more elegant. I'm not sure which way the community prefers.

> thanks
> -- PMM

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819




reply via email to

[Prev in Thread] Current Thread [Next in Thread]