[15/16] media: ti-vpe: cal: improve wait for stop-state

  Sometimes waiting for stop-state timeouts. Testing shows that sometimes
we need to wait more than what the current code does. It is not clear
how long this wait can be, but it is based on how quickly the sensor
provides a valid clock, and how quickly CAL syncs to it.

Change the code to make it more obvious how long we'll wait, and set a
wider range for usleep_range. Increase the timeout to 750ms.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
  

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:20PM +0200, Tomi Valkeinen wrote:
> Sometimes waiting for stop-state timeouts. Testing shows that sometimes
> we need to wait more than what the current code does. It is not clear
> how long this wait can be, but it is based on how quickly the sensor
> provides a valid clock, and how quickly CAL syncs to it.
> 
> Change the code to make it more obvious how long we'll wait, and set a
> wider range for usleep_range. Increase the timeout to 750ms.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 929f9b3ca4f9..df5a4281838b 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -849,19 +849,19 @@ static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>  
>  static void csi2_wait_stop_state(struct cal_ctx *ctx)
>  {
> -	int i;
> +	unsigned long timeout;
>  
> -	for (i = 0; i < 10; i++) {
> +	timeout = jiffies + msecs_to_jiffies(750);
> +	while (time_before(jiffies, timeout)) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_TIMING(ctx->csi2_port),
>  				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) == 0)
>  			break;
> -		usleep_range(1000, 1100);
> +		usleep_range(500, 5000);
>  	}

Same here, readx_poll_timeout() ?

> -	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached %s\n",
> +	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached\n",
>  		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
> -		(i >= 10) ? "(timeout)" : "");
> +		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));

That's not correct anymore, if the condition is false then the stop
state hasn't been reached.

Is this debug statement really useful ?

>  
>  	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
>  			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
  

On 16/03/2020 14:45, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Mar 13, 2020 at 01:41:20PM +0200, Tomi Valkeinen wrote:
>> Sometimes waiting for stop-state timeouts. Testing shows that sometimes
>> we need to wait more than what the current code does. It is not clear
>> how long this wait can be, but it is based on how quickly the sensor
>> provides a valid clock, and how quickly CAL syncs to it.
>>
>> Change the code to make it more obvious how long we'll wait, and set a
>> wider range for usleep_range. Increase the timeout to 750ms.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/media/platform/ti-vpe/cal.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index 929f9b3ca4f9..df5a4281838b 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -849,19 +849,19 @@ static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>>   
>>   static void csi2_wait_stop_state(struct cal_ctx *ctx)
>>   {
>> -	int i;
>> +	unsigned long timeout;
>>   
>> -	for (i = 0; i < 10; i++) {
>> +	timeout = jiffies + msecs_to_jiffies(750);
>> +	while (time_before(jiffies, timeout)) {
>>   		if (reg_read_field(ctx->dev,
>>   				   CAL_CSI2_TIMING(ctx->csi2_port),
>>   				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) == 0)
>>   			break;
>> -		usleep_range(1000, 1100);
>> +		usleep_range(500, 5000);
>>   	}
> 
> Same here, readx_poll_timeout() ?
> 
>> -	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached %s\n",
>> +	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached\n",
>>   		ctx->csi2_port,
>> -		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
>> -		(i >= 10) ? "(timeout)" : "");
>> +		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
> 
> That's not correct anymore, if the condition is false then the stop
> state hasn't been reached.

It wasn't exactly correct earlier either, as it always said "stop state reached", although it had 
"(timeout)" too.

> Is this debug statement really useful ?

I did not find any of these debug prints useful in my debugging, but as this is not "my" driver, I 
didn't want to drop them.

But I'll drop the debug print here and in the previous patch. Benoit can comment if he wants them back.

  Tomi