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

[Philipp Zabel <p.zabel@pengutronix.de>]

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.

If not all lanes are in LP-11 state at step 5., we can't guarantee that
the DPHY has already seen this transition nor that it will see it in
time before we start enabling the CSI2 RX.
Since this can lead to anything from sporadic to 100% startup failure,
depending on timing or whether the sensor is configured correctly to
produce this transition at all, I'd like this warning to stay.
It has been helpful before when developing sensor drivers.

regards
Philipp