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
signature.asc
Description: PGP signature
