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

[Corey Minyard]

On 12/18/2016 07:47 PM, Alastair D'Silva wrote:
> 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 :)

I know a little bit and I've implemented some stuff, so I'll try, too :).

> > On 29 November 2016 at 19:30, Fabio Urquiza <address@hidden>
> > wrote:
> <snip>
<snip> some more
> > 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.

Indeed, it is.  On the wire, a NAK after the address bits have been sent 
terminates
the transaction normally.

> > 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.

-corey

> > thanks
> > -- PMM