Re: [PATCH V8 4/5] i2c: tegra: Update transfer timeout

From: Thierry Reding <thierry.reding@gmail.com>
To: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: jonathanh@nvidia.com, mkarthik@nvidia.com, smohammed@nvidia.com,
	talho@nvidia.com, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
Subject: Re: [PATCH V8 4/5] i2c: tegra: Update transfer timeout
Date: Thu, 31 Jan 2019 13:48:13 +0100	[thread overview]
Message-ID: <20190131124813.GJ23438@ulmo> (raw)
In-Reply-To: <1548915387-28826-4-git-send-email-skomatineni@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]

On Wed, Jan 30, 2019 at 10:16:26PM -0800, Sowjanya Komatineni wrote:
> 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>
> ---
>  [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 | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 025d63972e50..435518cd91b6 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -25,7 +25,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>  
> -#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
> +#define TEGRA_I2C_TIMEOUT(ms) (msecs_to_jiffies(ms))

I think I would've just gone with direct usage of msecs_to_jiffies() but
this is also fine.

>  #define BYTES_PER_FIFO_WORD 4
>  
>  #define I2C_CNFG				0x000
> @@ -901,8 +901,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,
> +						TEGRA_I2C_TIMEOUT(1000));
>  		if (time_left == 0) {
>  			dev_err(i2c_dev->dev, "timed out for bus clear\n");
>  			return -ETIMEDOUT;
> @@ -930,6 +931,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	int err = 0;
>  	bool dma = false;
>  	struct dma_chan *chan;
> +	u16 xfer_time = 100;
>  
>  	tegra_i2c_flush_fifos(i2c_dev);
>  
> @@ -945,6 +947,13 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
>  
>  	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> +	/*
> +	 * Transfer time = 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) * 1000,

This doesn't really explain the factor of 1000 in there, which I assume
is just to scale this to milliseconds required by msecs_to_jiffies().
Might be worth to replace it by MSEC_PER_SEC to clarify the purpose.

With that fixed:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]