diff mbox series

[V9,4/5] i2c: tegra: update transfer timeout

Message ID 1549040867-18149-4-git-send-email-skomatineni@nvidia.com
State Superseded
Headers show
Series [V9,1/5] i2c: tegra: sort all the include headers alphabetically | expand

Commit Message

Sowjanya Komatineni Feb. 1, 2019, 5:07 p.m. UTC 
Tegra194 allows max of 64K bytes and Tegra186 and prior allows
max of 4K bytes of transfer per packet.

one sec timeout is not enough for transfers more than 10K bytes
at STD bus rate.

This patch updates I2C transfer timeout based on the transfer size
and I2C bus rate to allow enough time during max transfer size at
lower bus speed.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
 [V9] : Rebased to 5.0-rc4
	Minor updates for readability of xfer time
 [V8] : Added comment with explaination of xfer time calculation
 [V5/V6/V7] : Same as V4
 [V4] : V4 series includes bus clear support and this patch is updated with
	fixed timeout of 1sec for bus clear operation.
 [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 | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Dmitry Osipenko Feb. 1, 2019, 7:16 p.m. UTC | #1 
01.02.2019 20:07, Sowjanya Komatineni пишет:
> Tegra194 allows max of 64K bytes and Tegra186 and prior allows
> max of 4K bytes of transfer per packet.
> 
> one sec timeout is not enough for transfers more than 10K bytes
> at STD bus rate.
> 
> This patch updates I2C transfer timeout based on the transfer size
> and I2C bus rate to allow enough time during max transfer size at
> lower bus speed.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
>  [V9] : Rebased to 5.0-rc4
> 	Minor updates for readability of xfer time
>  [V8] : Added comment with explaination of xfer time calculation
>  [V5/V6/V7] : Same as V4
>  [V4] : V4 series includes bus clear support and this patch is updated with
> 	fixed timeout of 1sec for bus clear operation.
>  [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 | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index ac8009c35863..43449495bfaa 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -25,7 +25,6 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>  
> -#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
>  #define BYTES_PER_FIFO_WORD 4
>  
>  #define I2C_CNFG				0x000
> @@ -893,8 +892,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
>  		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
>  		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>  
> -		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> -							TEGRA_I2C_TIMEOUT);
> +		time_left = wait_for_completion_timeout(
> +						&i2c_dev->msg_complete,
> +						msecs_to_jiffies(1000));

So potentially tegra_i2c_xfer_msg() could take more than 1 second and then fail with -EAGAIN, correct? In that case we should set adapter.timeout in probe to a larger value:

  i2c_dev->adapter.timeout = 3 * HZ;



Otherwise looks good to me:

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Sowjanya Komatineni Feb. 1, 2019, 7:37 p.m. UTC | #2 
> > BYTES_PER_FIFO_WORD 4
> >  
> >  #define I2C_CNFG				0x000
> > @@ -893,8 +892,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
> >  		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> >  		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
> >  
> > -		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > -							TEGRA_I2C_TIMEOUT);
> > +		time_left = wait_for_completion_timeout(
> > +						&i2c_dev->msg_complete,
> > +						msecs_to_jiffies(1000));
>
> So potentially tegra_i2c_xfer_msg() could take more than 1 second and then fail with -EAGAIN, correct? In that case we should set adapter.timeout in probe to a larger value:
>
Bus clear pulse threshold we are setting is 9 (default as per spec) so bus clear should happen after finishing sending of 9 pulses.
So 1sec is very long time for bus to get released and in case of ARB LOST whole transfer to return EAGAIN will be much less then 1sec

>   i2c_dev->adapter.timeout = 3 * HZ;
>
>
> Otherwise looks good to me:
>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Dmitry Osipenko Feb. 1, 2019, 7:41 p.m. UTC | #3 
01.02.2019 22:37, Sowjanya Komatineni пишет:
> 
>>> BYTES_PER_FIFO_WORD 4
>>>  
>>>  #define I2C_CNFG				0x000
>>> @@ -893,8 +892,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
>>>  		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
>>>  		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>>>  
>>> -		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
>>> -							TEGRA_I2C_TIMEOUT);
>>> +		time_left = wait_for_completion_timeout(
>>> +						&i2c_dev->msg_complete,
>>> +						msecs_to_jiffies(1000));
>>
>> So potentially tegra_i2c_xfer_msg() could take more than 1 second and then fail with -EAGAIN, correct? In that case we should set adapter.timeout in probe to a larger value:
>>
> Bus clear pulse threshold we are setting is 9 (default as per spec) so bus clear should happen after finishing sending of 9 pulses.
> So 1sec is very long time for bus to get released and in case of ARB LOST whole transfer to return EAGAIN will be much less then 1sec

Okay, thank you very much for the clarification. What about to set the timeout to a shorter value then? Maybe 10ms.
Thierry Reding Feb. 4, 2019, 7:54 a.m. UTC | #4 
On Fri, Feb 01, 2019 at 07:37:45PM +0000, Sowjanya Komatineni wrote:
> 
> > > BYTES_PER_FIFO_WORD 4
> > >  
> > >  #define I2C_CNFG				0x000
> > > @@ -893,8 +892,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
> > >  		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> > >  		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
> > >  
> > > -		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > > -							TEGRA_I2C_TIMEOUT);
> > > +		time_left = wait_for_completion_timeout(
> > > +						&i2c_dev->msg_complete,
> > > +						msecs_to_jiffies(1000));
> >
> > So potentially tegra_i2c_xfer_msg() could take more than 1 second
> > and then fail with -EAGAIN, correct? In that case we should set
> > adapter.timeout in probe to a larger value:
> >
> Bus clear pulse threshold we are setting is 9 (default as per spec) so
> bus clear should happen after finishing sending of 9 pulses.
> So 1sec is very long time for bus to get released and in case of ARB
> LOST whole transfer to return EAGAIN will be much less then 1sec

What if for example we have a very long transfer, say some 64 KiB on
Tegra194 and about 95% through the transfer something causes the bus
to lock up. The transfer so far would've taken something on the order
of 5 seconds, but we'd still fail, potentially with -EAGAIN, right?

Or is there some other mechanism that would prevent the above from
happening?

Thierry
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index ac8009c35863..43449495bfaa 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,7 +25,6 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
-#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
 #define BYTES_PER_FIFO_WORD 4
 
 #define I2C_CNFG				0x000
@@ -893,8 +892,9 @@  static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
 		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
 		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 
-		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-							TEGRA_I2C_TIMEOUT);
+		time_left = wait_for_completion_timeout(
+						&i2c_dev->msg_complete,
+						msecs_to_jiffies(1000));
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "timed out for bus clear\n");
 			return -ETIMEDOUT;
@@ -921,6 +921,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	u32 *buffer = NULL;
 	int err = 0;
 	bool dma = false;
+	u16 xfer_time = 100;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -942,6 +943,13 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	i2c_dev->is_curr_dma_xfer = dma;
 
+	/*
+	 * Transfer time in mSec = Total bits / transfer rate
+	 * Total bits = 9 bits per byte (including ACK bit) + Start & stop bits
+	 */
+	xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * MSEC_PER_SEC,
+					i2c_dev->bus_clk_rate);
+
 	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
 
 	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
@@ -1059,7 +1067,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 		time_left = wait_for_completion_timeout(
 						&i2c_dev->dma_complete,
-						TEGRA_I2C_TIMEOUT);
+						msecs_to_jiffies(xfer_time));
 
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
@@ -1083,8 +1091,9 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		}
 	}
 
-	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-						TEGRA_I2C_TIMEOUT);
+	time_left = wait_for_completion_timeout(
+					&i2c_dev->msg_complete,
+					msecs_to_jiffies(xfer_time));
 	tegra_i2c_mask_irq(i2c_dev, int_mask);
 
 	if (time_left == 0) {