Hi Philipp, On Thu, Aug 08, 2019 at 10:53:25AM +0200, 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. > > 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. Thanks for sharing your insights. So basically the driver tries to wait for LP-11 and if it doesn't observe it, then it tries to proceed nevertheless. I'll take the patch. -- Kind regards, Sakari Ailus
