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

[Corey Minyard]

On 12/19/2016 09:31 AM, Peter Maydell wrote:
> On 19 December 2016 at 13:55, Corey Minyard <address@hidden> wrote:
> > On 12/18/2016 07:47 PM, Alastair D'Silva wrote:
> > > On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
> > > > 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.
> OK, so if returning negative values from i2c_recv() isn't
> the device saying "I am NAKing this", what *do* they mean?

It actually makes no sense.  In real I2C hardware, the receiver of the 
byte always
does the ACK/NAK.  The NAK is sent by the receiver of the data to signal 
that it
has finished the transfer.

So when i2c_recv() is called, it's actually the I2c device doing a 
transmit and the
i2c master receiving the data.  So the device cannot send a NAK in that 
scenario.

The start conditions and address are always send by the master to the 
device, so
it makes sense for the start events to be able to return a NAK.

And for i2c_send(), the device should respond with a NAK to terminate the
transfer.  So it makes sense for i2c_send() to be able to return a NAK.  
This
doesn't appear to be properly implemented in a number of places.

> > > > 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.
> >
> > I have a patch that adds a check_event() handler along side the event()
> > handler.
> > If a device wants to send a NAK, it can implement check_event() instead of
> > event() and return non-zero to NAK.
> >
> > I toyed with just changing all the event() calls, but there are a bunch.
> > This seemed
> > like the better approach.  I can send if you like.
> It looks like there are only a dozen or so. I think it would
> be better for the long term just to change the event calls.
> We should also document in the comments in the I2CSlaveClass
> struct definition exactly what the semantics of the various
> functions are.

Ok, my memory didn't serve me correctly.  I'll rework my patch for that.

Thanks,

-corey

> thanks
> -- PMM