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

[Dmitry Osipenko]

Ð Tue, 29 Jan 2019 11:16:52 +0100
Thierry Reding <thierry.reding@xxxxxxxxx> ÐÐÑÐÑ:

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

I'd also suggest to collect those I2C patches into a single patchset.