Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
В Tue, 29 Jan 2019 11:16:52 +0100 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. I'd also suggest to collect those I2C patches into a single patchset.
Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
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
Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
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. Thierry signature.asc Description: PGP signature
RE: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
> > 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. Thanks Dmitry. Will do on next update of this patch...
Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
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.
RE: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
> >>> 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.
Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
28.01.2019 21:28, 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?
RE: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
> > 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.
> >
> > Signed-off-by: Sowjanya Komatineni
> > ---
> > [V3] : Same as V2
> > [V2] : Added this patch in V2 series to allow enough time for data transfer
> > to happen.
> > This patch has dependency with DMA patch as TEGRA_I2C_TIMEOUT define
> > takes argument with this patch.
> >
> > drivers/i2c/busses/i2c-tegra.c | 18 --
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c index aec500ef9a98..3dcbc9960d9d
> > 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -23,7 +23,7 @@
> > #include
> > #include
> >
> > -#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
> > +#define TEGRA_I2C_TIMEOUT(ms) (msecs_to_jiffies(ms))
> > #define BYTES_PER_FIFO_WORD 4
> >
> > #define I2C_CNFG 0x000
> > @@ -116,6 +116,9 @@
> > #define I2C_MST_FIFO_STATUS_TX_MASK0xff
> > #define I2C_MST_FIFO_STATUS_TX_SHIFT 16
> >
> > +/* Packet header size in bytes */
> > +#define I2C_PACKET_HEADER_SIZE 12
> > +
> > /*
> > * msg_end_type: The bus control which need to be send at end of transfer.
> > * @MSG_END_STOP: Send stop pulse at end of transfer.
> > @@ -683,6 +686,17 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> > *i2c_dev,
> > u32 int_mask;
> > unsigned long time_left;
> > unsigned long flags;
> > + u16 xfer_time = 100;
> > + size_t xfer_size = 0;
>
> No need to initialize xfer_size to 0.
>
> > +
> > + if (msg->flags & I2C_M_RD)
> > + xfer_size = ALIGN(msg->len, BYTES_PER_FIFO_WORD);
> > + else
> > + xfer_size = ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> > + I2C_PACKET_HEADER_SIZE;
> > +
> > + xfer_time += DIV_ROUND_CLOSEST((xfer_size * 9) * 1000,
> > + i2c_dev->bus_clk_rate);>
> > tegra_i2c_flush_fifos(i2c_dev);
> >
> > @@ -739,7 +753,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> > *i2c_dev,
> > i2c_readl(i2c_dev, I2C_INT_MASK));
> >
> > time_left = wait_for_completion_timeout(_dev->msg_complete,
> > - TEGRA_I2C_TIMEOUT);
> > + TEGRA_I2C_TIMEOUT(xfer_time));
> > tegra_i2c_mask_irq(i2c_dev, int_mask);
> >
> > if (time_left == 0) {
> >
Re: [PATCH V3 2/3] i2c: tegra: Update transfer timeout
26.01.2019 6:57, 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?
>
> Signed-off-by: Sowjanya Komatineni
> ---
> [V3] : Same as V2
> [V2] : Added this patch in V2 series to allow enough time for data transfer
> to happen.
> This patch has dependency with DMA patch as TEGRA_I2C_TIMEOUT define
> takes argument with this patch.
>
> drivers/i2c/busses/i2c-tegra.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index aec500ef9a98..3dcbc9960d9d 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -23,7 +23,7 @@
> #include
> #include
>
> -#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
> +#define TEGRA_I2C_TIMEOUT(ms) (msecs_to_jiffies(ms))
> #define BYTES_PER_FIFO_WORD 4
>
> #define I2C_CNFG 0x000
> @@ -116,6 +116,9 @@
> #define I2C_MST_FIFO_STATUS_TX_MASK 0xff
> #define I2C_MST_FIFO_STATUS_TX_SHIFT 16
>
> +/* Packet header size in bytes */
> +#define I2C_PACKET_HEADER_SIZE 12
> +
> /*
> * msg_end_type: The bus control which need to be send at end of transfer.
> * @MSG_END_STOP: Send stop pulse at end of transfer.
> @@ -683,6 +686,17 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> *i2c_dev,
> u32 int_mask;
> unsigned long time_left;
> unsigned long flags;
> + u16 xfer_time = 100;
> + size_t xfer_size = 0;
No need to initialize xfer_size to 0.
> +
> + if (msg->flags & I2C_M_RD)
> + xfer_size = ALIGN(msg->len, BYTES_PER_FIFO_WORD);
> + else
> + xfer_size = ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> + I2C_PACKET_HEADER_SIZE;
> +
> + xfer_time += DIV_ROUND_CLOSEST((xfer_size * 9) * 1000,
> + i2c_dev->bus_clk_rate);>
> tegra_i2c_flush_fifos(i2c_dev);
>
> @@ -739,7 +753,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> *i2c_dev,
> i2c_readl(i2c_dev, I2C_INT_MASK));
>
> time_left = wait_for_completion_timeout(_dev->msg_complete,
> - TEGRA_I2C_TIMEOUT);
> + TEGRA_I2C_TIMEOUT(xfer_time));
> tegra_i2c_mask_irq(i2c_dev, int_mask);
>
> if (time_left == 0) {
>
