Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout

[Thierry Reding]

On Tue, Jan 29, 2019 at 11:15:05AM +0100, Thierry Reding wrote:
> On Tue, Jan 29, 2019 at 01:27:09AM +0300, Dmitry Osipenko wrote:
> > 29.01.2019 1:15, Sowjanya Komatineni ÐÐÑÐÑ:
> > > 
> > > 
> > >>>>> Update I2C transfer timeout based on transfer bytes and I2C bus rate 
> > >>>>> to allow enough time during max transfer size based on the speed.
> > >>>>
> > >>>> Could it be that I2C device is busy and just slowly handling the transfer requests? Maybe better to leave the timeout as-is and assume the worst case scenario?
> > >>>>
> > >>> This change includes min transfer time out of 100ms in addition to computed timeout based on transfer bytes and speed which can account in cases of slave devices running at slower speed.
> > >>> Also Tegra I2C Master supports Clock stretching by the slave.
> > >>
> > >> Okay, I suppose in reality this shouldn't break anything.
> > >>
> > >> Please explain what benefits this change brings. Does it fix or improve anything? The commit message only describes changes done in the patch and has no word on justification of those changes. Transfer timeout is an extreme case that doesn't happen often and > > when it happens, usually only the fact of timeout matters. If there is no real value in shortening of the timeout, why bother then?
> > > 
> > > Original transfer timeout in existing driver is 1Sec and incases of transfer size more than 10Kbytes at STD bus rate, timeout is not sufficient.
> > > Also Tegra194 platform supports max of 64K bytes of transfer and to allow full transfer size at lowest bus rate it takes almost ~5.8 Sec.
> > > In cases if large transfer at low bus rates 1 Sec timeout is not enough and in those cases transfers will timeout before it waits for complete transfer to happen.
> > > 
> > > So this patch uses transfer time based on transfer bytes and bus rate.
> > > 
> > 
> > Please add that to the commit message.
> > 
> > And then seems you also need to set I2C adapter timeout to a some
> > larger value. Currently Tegra's I2C doesn't explicitly specify the
> > "adapter.timeout" and I2C core sets it to 1 second if it is 0.
> 
> I don't think adapter.timeout is the same thing as the timeout that
> we're dealing with here. adapter.timeout is only used as a way to retry
> on arbitration lost conditions. So, as far as I understand, if we try to
> send a message to an I2C slave and we loose arbitration, we'll keep
> retrying for one second before finally failing. I wouldn't expect these
> arbitration lost conditions to happen right in the middle of a transfer,
> but rather at the beginning already, so I'm not sure arbitration could
> even be lost after, say, 2 seconds into a very large transfer.
> 
> All of that said, it seems like i2c-tegra doesn't respect the protocol
> for making this arbitration loss retry work in the first place. We
> should be returning -EAGAIN if we detect arbitration loss, but I don't
> see that we ever return that. We should probably fix that in another
> patch.

Scratch that. Sowjanya's "bus clear" patch already takes care of that.

Thierry

Attachment: signature.asc
Description: PGP signature