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