Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Steve Longerbeam <slongerbeam@gmail.com>]

Hi Laurent,

On 6/26/19 1:00 AM, Laurent Pinchart wrote:
> Hi Ezequiel,
>
> Thank you for the patch.
>
> On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
>> Not all sensors will be able to guarantee a proper initial state.
>> This may be either because the driver is not properly written,
>> or (probably unlikely) because the hardware won't support it.
>>
>> While the right solution in the former case is to fix the sensor
>> driver, the real world not always allows right solutions, due to lack
>> of available documentation and support on these sensors.
>>
>> Let's relax this requirement, and allow the driver to support stream start,
>> even if the sensor initial sequence wasn't the expected.
>> A warning is still emitted, so users should be hinted that something is off.
> I'm not sure this is a very good idea. Failure to detect the LP-11 state
> may mean that the sensor is completely powered off, but it may also mean
> that it is already streaming data. I don't know how the CSI-2 receiver
> state machine will operate in the first case, but in the second case it
> will not be able to synchronise to the incoming stream, so it won't work
> anyway.

 From my experience, at least with the OV5640 and the DS90Ux940, it can 
be difficult to coax some CSI-2 transmitters into a quiescent LP-11 bus 
state after power on, and yet the CSI-2 receiver state machine, at least 
in the imx6, is able to move forward to stream on anyway. So I think it 
makes sense to at least relax the LP-11 requirement at stream on for the 
imx6 receiver driver, and print a warning. But on second thought I agree 
the active clock lane requirement before stream on needs to remain.

In the second case you point out above (sensor is already actively 
streaming at stream on), why do you think that the receiver will not be 
able to synchronize?


>
> I think you should instead fix the problem in the sensor driver, as you
> hinted. Relaxing the requirement here will only make it more confusing,
> it's a hack, and isn't portable across CSI-2 receivers. The same buggy
> sensor driver won't work with other CSI-2 receivers whose internal state
> machine require starting in the LP-11 state.

Agreed, if it's possible the sensor driver should be fixed to enter 
LP-11 at power on.

Steve

> Which sensor are you using ?
>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>>   drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
>>   1 file changed, 9 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
>> index f29e28df36ed..10342434e797 100644
>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
>> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>>   }
>>   
>>   /* Waits for low-power LP-11 state on data and clock lanes. */
>> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>>   {
>>   	u32 mask, reg;
>>   	int ret;
>> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>>   
>>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>>   				 (reg & mask) == mask, 0, 500000);
>> -	if (ret) {
>> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	if (ret)
>> +		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>>   }
>>   
>>   /* Wait for active clock on the clock lane. */
>> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>>   {
>>   	u32 reg;
>>   	int ret;
>>   
>>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>>   				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
>> -	if (ret) {
>> -		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
>> -			 reg);
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	if (ret)
>> +		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
>> +			  reg);
>>   }
>>   
>>   /* Setup the i.MX CSI2IPU Gasket */
>> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
>>   	csi2_enable(csi2, true);
>>   
>>   	/* Step 5 */
>> -	ret = csi2_dphy_wait_stopstate(csi2);
>> -	if (ret)
>> -		goto err_assert_reset;
>> +	csi2_dphy_wait_stopstate(csi2);
>>   
>>   	/* Step 6 */
>>   	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
>> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
>>   		goto err_assert_reset;
>>   
>>   	/* Step 7 */
>> -	ret = csi2_dphy_wait_clock_lane(csi2);
>> -	if (ret)
>> -		goto err_stop_upstream;
>> -
>> +	csi2_dphy_wait_clock_lane(csi2);
>>   	return 0;
>>   
>> -err_stop_upstream:
>> -	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
>>   err_assert_reset:
>>   	csi2_enable(csi2, false);
>>   err_disable_clk: