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

[Steve Longerbeam <slongerbeam@gmail.com>]

On 8/8/19 11:02 AM, Steve Longerbeam wrote:
>
>
> On 8/8/19 1:53 AM, Philipp Zabel wrote:
>> Hi Sakari,
>>
>> On Thu, 2019-08-08 at 11:26 +0300, Sakari Ailus wrote:
>> [...]
>>>>> Have you checked how it works if you simply leave out this test?
>> Whether this works or not depends on the sensor used, and for some
>> sensor/drivers may depend on timing (or random factors influencing it).
>> See below.
>>
>> [...]
>>> Some devices can be commanded to enter LP-11 state but some will just
>>> briefly visit that state. The LP-11 state is mandatory but software 
>>> should
>>> not be involved in detecting it if at all possible.
>> This is a good point. Devices that can be set to LP-11 state
>> immediately, but that don't stay there long enough (either because they
>> wait for less than the required by spec 100µs, or because system load
>> causes this check to be executed too late) may end up working reliably
>> even though the warning fires.
>
>>> So if the hardware does not require further initialisation to be 
>>> done in
>>> LP-11 state, you should remove the check.
>>>
>>>> We had to fix at least OV5645 and OV5640 recently because of this,
>>>> and I can imagine more drivers will have the same issue.
>>> This is actually an issue in the IMX driver (or hardware), not in the
>>> sensor driver. It may be that sometimes it's easier to work around 
>>> it in
>>> the sensor driver.
>>>
>>> So, I'd like to know whether the check itself is a driver bug, or 
>>> something
>>> that the hardware requires. The fact that you're sending this patch
>>> suggests the former.
>> This is something that the hardware requires, according to the reference
>> manual. See the comment in drivers/staging/media/imx/imx6-mipi-csi2.c,
>> especially step 5.:
>>
>> /*
>>   * The required sequence of MIPI CSI-2 startup as specified in the 
>> i.MX6
>>   * reference manual is as follows:
>>   *
>>   * 1. Deassert presetn signal (global reset).
>>   *        It's not clear what this "global reset" signal is (maybe APB
>>   *        global reset), but in any case this step would be probably
>>   *        be carried out during driver load in csi2_probe().
>>   *
>>   * 2. Configure MIPI Camera Sensor to put all Tx lanes in LP-11 state.
>>   *        This must be carried out by the MIPI sensor's s_power(ON) 
>> subdev
>>   *        op.
>>   *
>>   * 3. D-PHY initialization.
>>   * 4. CSI2 Controller programming (Set N_LANES, deassert PHY_SHUTDOWNZ,
>>   *    deassert PHY_RSTZ, deassert CSI2_RESETN).
>>   * 5. Read the PHY status register (PHY_STATE) to confirm that all 
>> data and
>>   *    clock lanes of the D-PHY are in LP-11 state.
>>   * 6. Configure the MIPI Camera Sensor to start transmitting a clock 
>> on the
>>   *    D-PHY clock lane.
>>   * 7. CSI2 Controller programming - Read the PHY status register 
>> (PHY_STATE)
>>   *    to confirm that the D-PHY is receiving a clock on the D-PHY 
>> clock lane.
>>   */
>>
>> I read this as the hardware needing to see the LP-11 -> HS transition
>> after the DPHY reset has been released, and before the CSI2 RX
>> controller is programmed.
>
> I think that's a fair assumption, and there's another paragraph at the 
> end of that comment above that adds more evidence to that:
>
> ...
> * All steps 3 through 7 are carried out by csi2_s_stream(ON) here. Step
> * 6 is accomplished by calling the source subdev's s_stream(ON) between
> * steps 5 and 7.
> */
>
>
> So the driver is expecting that the LP-11 state persists until step 6, 
> at which the LP-11 -> HS transition occurs when streaming is started 
> at the transmitter.
>
> But if the transmitter only stays in LP-11 state for the minimum 100 
> usec after it is powered on, and then _automatically_ transitions to 
> HS, it's quite possible the LP-11 -> HS transition will happen long 
> before the DPHY is taken out of reset. That's because the 
> transmitter's s_power(ON) is called when the capture device is opened 
> (via v4l2_pipeline_pm_use()), but the steps above are carried out when 
> streaming starts, so userland would have to issue VIDIOC_STREAMON well 
> within the 100 usec expires after open()'ing the device, for the 
> receiver hardware to see the transition.
>
> Perhaps that would be an argument for moving steps 1 - 5 into the 
> driver's s_power(ON) call, which would first call s_power(ON) to the 
> transmitter and then immediately go through steps 1 - 5. Steps 6,7 
> would then remain in s_stream(ON).

Well, after more testing on the i.MX6q SabreSD with the MIPI CSI-2 
OV5640, this looks like an advisable thing to do. I will submit a patch.

Steve