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

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

[Fabio Estevam]

From: Ezequiel Garcia <ezequiel@collabora.com>

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.

Also improve the warning message to better explain the problem and provide
a hint that the sensor driver needs to be fixed.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
Changes since v2:
- Reduce the warning message (Steve)
Changes since v1:
- Changed the warning message to better explain the problem and provide
a hint that the sensor driver needs to be fixed. (Phillip)
- Keep printing the phy_state information (Phillip)
- Do not change csi2_dphy_wait_clock_lane() (Phillip/Steve)
 drivers/staging/media/imx/imx6-mipi-csi2.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index f29e28df36ed..bfa4b254c4e4 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;
@@ -254,11 +254,9 @@ 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;
+		v4l2_warn(&csi2->sd, "LP-11 wait timeout, likely a sensor driver bug, expect capture failures.\n");
+		v4l2_warn(&csi2->sd, "phy_state = 0x%08x\n", reg);
 	}
-
-	return 0;
 }
 
 /* Wait for active clock on the clock lane. */
@@ -316,9 +314,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);
-- 
2.17.1

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

[Steve Longerbeam]

Thanks Fabio,

Reviewed-by: Steve Longerbeam <slongerbeam@gmail.com>

On 6/27/19 3:29 PM, Fabio Estevam wrote:
> From: Ezequiel Garcia <ezequiel@collabora.com>
>
> 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.
>
> Also improve the warning message to better explain the problem and provide
> a hint that the sensor driver needs to be fixed.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes since v2:
> - Reduce the warning message (Steve)
> Changes since v1:
> - Changed the warning message to better explain the problem and provide
> a hint that the sensor driver needs to be fixed. (Phillip)
> - Keep printing the phy_state information (Phillip)
> - Do not change csi2_dphy_wait_clock_lane() (Phillip/Steve)
>   drivers/staging/media/imx/imx6-mipi-csi2.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index f29e28df36ed..bfa4b254c4e4 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;
> @@ -254,11 +254,9 @@ 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;
> +		v4l2_warn(&csi2->sd, "LP-11 wait timeout, likely a sensor driver bug, expect capture failures.\n");
> +		v4l2_warn(&csi2->sd, "phy_state = 0x%08x\n", reg);
>   	}
> -
> -	return 0;
>   }
>   
>   /* Wait for active clock on the clock lane. */
> @@ -316,9 +314,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);

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

[Philipp Zabel]

On Thu, 2019-06-27 at 19:29 -0300, Fabio Estevam wrote:
> From: Ezequiel Garcia <ezequiel@collabora.com>
> 
> 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.
> 
> Also improve the warning message to better explain the problem and provide
> a hint that the sensor driver needs to be fixed.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

thanks
Philipp

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

[Ezequiel Garcia]

Hey Hans,

On Mon, 2019-07-01 at 08:48 +0200, Philipp Zabel wrote:
> On Thu, 2019-06-27 at 19:29 -0300, Fabio Estevam wrote:
> > From: Ezequiel Garcia <ezequiel@collabora.com>
> > 
> > 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.
> > 
> > Also improve the warning message to better explain the problem and provide
> > a hint that the sensor driver needs to be fixed.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Signed-off-by: Fabio Estevam <festevam@gmail.com>
> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> 

This seems ready to pick and it has Philipp's and Steve's RB.

Thanks,
Eze

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

[Sakari Ailus]

On Tue, Jul 30, 2019 at 05:14:24AM -0300, Ezequiel Garcia wrote:
> Hey Hans,
> 
> On Mon, 2019-07-01 at 08:48 +0200, Philipp Zabel wrote:
> > On Thu, 2019-06-27 at 19:29 -0300, Fabio Estevam wrote:
> > > From: Ezequiel Garcia <ezequiel@collabora.com>
> > > 
> > > 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.
> > > 
> > > Also improve the warning message to better explain the problem and provide
> > > a hint that the sensor driver needs to be fixed.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > Signed-off-by: Fabio Estevam <festevam@gmail.com>
> > 
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> 
> This seems ready to pick and it has Philipp's and Steve's RB.

Hi Ezequiel,

In general the LP-11 condition should be detected by hardware (or firmware)
in such a way that it's detected even if a transmitter that holds the state
just a short period of time. In other words, software is not supposed to be
even testing for it.

Have you checked how it works if you simply leave out this test?

I remember that there has been hardware that does indeed require further
initialisation done only *after* LP-11 has been detected by _software_.

-- 
Regards,

Sakari Ailus

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

[Ezequiel Garcia]

Hi Sakari,

Thanks for reviewing the patch.

On Wed, 2019-08-07 at 15:06 +0300, Sakari Ailus wrote:
> On Tue, Jul 30, 2019 at 05:14:24AM -0300, Ezequiel Garcia wrote:
> > Hey Hans,
> > 
> > On Mon, 2019-07-01 at 08:48 +0200, Philipp Zabel wrote:
> > > On Thu, 2019-06-27 at 19:29 -0300, Fabio Estevam wrote:
> > > > From: Ezequiel Garcia <ezequiel@collabora.com>
> > > > 
> > > > 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.
> > > > 
> > > > Also improve the warning message to better explain the problem and provide
> > > > a hint that the sensor driver needs to be fixed.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > Signed-off-by: Fabio Estevam <festevam@gmail.com>
> > > 
> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > 
> > 
> > This seems ready to pick and it has Philipp's and Steve's RB.
> 
> Hi Ezequiel,
> 
> In general the LP-11 condition should be detected by hardware (or firmware)
> in such a way that it's detected even if a transmitter that holds the state
> just a short period of time. In other words, software is not supposed to be
> even testing for it.
> 
> Have you checked how it works if you simply leave out this test?
> 

The current change relaxes a condition, which we observed was too strict.
Some drivers might be unable to enter LP-11 state, but I don't think
that's a reason to fail capture.

We had to fix at least OV5645 and OV5640 recently because of this,
and I can imagine more drivers will have the same issue.

The way I see it, the driver is imposing a condition that is too strict,
and that's why relaxing it makes sense.

No, regarding removing the test completely, I'm not sure I want to
propose such a change, which would be undoubtfully more invasive.

Thanks,
Ezequiel

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

[Sakari Ailus]

Hi Ezequiel,

On Wed, Aug 07, 2019 at 10:59:22AM -0300, Ezequiel Garcia wrote:
> Hi Sakari,
> 
> Thanks for reviewing the patch.
> 
> On Wed, 2019-08-07 at 15:06 +0300, Sakari Ailus wrote:
> > On Tue, Jul 30, 2019 at 05:14:24AM -0300, Ezequiel Garcia wrote:
> > > Hey Hans,
> > > 
> > > On Mon, 2019-07-01 at 08:48 +0200, Philipp Zabel wrote:
> > > > On Thu, 2019-06-27 at 19:29 -0300, Fabio Estevam wrote:
> > > > > From: Ezequiel Garcia <ezequiel@collabora.com>
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Also improve the warning message to better explain the problem and provide
> > > > > a hint that the sensor driver needs to be fixed.
> > > > > 
> > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > > Signed-off-by: Fabio Estevam <festevam@gmail.com>
> > > > 
> > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > 
> > > 
> > > This seems ready to pick and it has Philipp's and Steve's RB.
> > 
> > Hi Ezequiel,
> > 
> > In general the LP-11 condition should be detected by hardware (or firmware)
> > in such a way that it's detected even if a transmitter that holds the state
> > just a short period of time. In other words, software is not supposed to be
> > even testing for it.
> > 
> > Have you checked how it works if you simply leave out this test?
> > 
> 
> The current change relaxes a condition, which we observed was too strict.
> Some drivers might be unable to enter LP-11 state, but I don't think
> that's a reason to fail capture.

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.

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.

-- 
Regards,

Sakari Ailus

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

[Philipp Zabel]

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

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

[Sakari Ailus]

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

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

[Steve Longerbeam]

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).


>
> 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.

Agreed, I think the warning should remain.

One final note. The CSI-2 receiver in imx6 is a Synopsys DesignWare IP 
core, so these timing issues are likely to be an issue on other SoC's 
that use this core. The CSI-2 chapter in the imx6 reference manual is 
lifted/hacked-up from designware docs. It would be a good idea to get 
the full documentation on the Synopsys DesignWare MIPI CSI-2 Host and 
Device Controller docs at [1] which would probably shed more light.

Steve

[1] https://www.synopsys.com/dw/ipdir.php?ds=mipi_csi2

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

[Steve Longerbeam]

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

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

[Sakari Ailus]

Hi Steve,

On Thu, Aug 08, 2019 at 11:02:29AM -0700, 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.

After powering on the sensor it sensor should stay in (software) standby
mode until streaming is started. It should still have its lanes in LP-11
mode if the sensor supports it (as documented) until the sensor switches to
the streaming mode.

> 
> 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).

Not all sensor drivers power on the sensor before starting streaming.

Perhaps we could add a prepare_streaming() callback (in absence of a better
proposal) to address that? We'd also need a corresponding
unprepare_streaming() callback as well --- to power off the sensor. I think
this only should be done if the sensor can be switched to LP-11 explicitly;
many simply don't support that.

-- 
Regards,

Sakari Ailus

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

[Steve Longerbeam]

Hi Sakari,

On 8/13/19 1:28 AM, Sakari Ailus wrote:
> Hi Steve,
>
> On Thu, Aug 08, 2019 at 11:02:29AM -0700, 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.
> After powering on the sensor it sensor should stay in (software) standby
> mode until streaming is started. It should still have its lanes in LP-11
> mode if the sensor supports it (as documented) until the sensor switches to
> the streaming mode.

Yes that's my understanding too.

>
>> 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).
> Not all sensor drivers power on the sensor before starting streaming.
>
> Perhaps we could add a prepare_streaming() callback (in absence of a better
> proposal) to address that? We'd also need a corresponding
> unprepare_streaming() callback as well --- to power off the sensor. I think
> this only should be done if the sensor can be switched to LP-11 explicitly;
> many simply don't support that.

So you are saying a sensor can power on and place its lanes in LP-11 in 
this new prepare_streaming() callback, the reason being, this can be 
called closer in time, and just before, the receiver DPHY is brought out 
of reset, in order for the receiver DPHY to be able to detect a possible 
early transition to HS mode?

While I don't have any objection to adding a prepare_streaming() 
callback (it could be useful for other reasons), I don't see why the 
sensor could not simply make use of existing s_power to power on and 
place the lanes in LP-11. That is what it's intended for anyway.

There's really no drawback to the patch I posted that moves receiver 
DPHY init and reset-release to s_power time in the receiver driver. If 
given a choice, an isolated fix to the imx mipi csi-2 receiver driver 
should be preferable to implementing a new callback.

But perhaps one advantage to the prepare_streaming() callback idea, is 
that there is more of a guarantee of minimum time between the 
sensor-power-on-and-enter-LP-11 event and receiver-DPHY-reset-release 
event, since s_power is normally called from v4l2_pipeline_pm_use() 
which could add extra overhead between the time s_power is called at the 
transmitter and at the receiver (but not much - the transmitter is 
connected directly to receiver in the graph).

Steve

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

[Sakari Ailus]

Hi Steve,

On Tue, Aug 13, 2019 at 04:27:06PM -0700, Steve Longerbeam wrote:
> Hi Sakari,
> 
> On 8/13/19 1:28 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > On Thu, Aug 08, 2019 at 11:02:29AM -0700, 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.
> > After powering on the sensor it sensor should stay in (software) standby
> > mode until streaming is started. It should still have its lanes in LP-11
> > mode if the sensor supports it (as documented) until the sensor switches to
> > the streaming mode.
> 
> Yes that's my understanding too.
> 
> > 
> > > 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).
> > Not all sensor drivers power on the sensor before starting streaming.
> > 
> > Perhaps we could add a prepare_streaming() callback (in absence of a better
> > proposal) to address that? We'd also need a corresponding
> > unprepare_streaming() callback as well --- to power off the sensor. I think
> > this only should be done if the sensor can be switched to LP-11 explicitly;
> > many simply don't support that.
> 
> So you are saying a sensor can power on and place its lanes in LP-11 in this
> new prepare_streaming() callback, the reason being, this can be called
> closer in time, and just before, the receiver DPHY is brought out of reset,
> in order for the receiver DPHY to be able to detect a possible early
> transition to HS mode?
> 
> While I don't have any objection to adding a prepare_streaming() callback
> (it could be useful for other reasons), I don't see why the sensor could not
> simply make use of existing s_power to power on and place the lanes in
> LP-11. That is what it's intended for anyway.

The vast majority of new drivers no longer have s_power callbacks --- they
use runtime PM instead.

> 
> There's really no drawback to the patch I posted that moves receiver DPHY
> init and reset-release to s_power time in the receiver driver. If given a
> choice, an isolated fix to the imx mipi csi-2 receiver driver should be
> preferable to implementing a new callback.
> 
> But perhaps one advantage to the prepare_streaming() callback idea, is that
> there is more of a guarantee of minimum time between the
> sensor-power-on-and-enter-LP-11 event and receiver-DPHY-reset-release event,
> since s_power is normally called from v4l2_pipeline_pm_use() which could add
> extra overhead between the time s_power is called at the transmitter and at
> the receiver (but not much - the transmitter is connected directly to
> receiver in the graph).

-- 
Regards,

Sakari Ailus

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

[Steve Longerbeam]

On 8/14/19 12:16 AM, Sakari Ailus wrote:
> Hi Steve,
>
> On Tue, Aug 13, 2019 at 04:27:06PM -0700, Steve Longerbeam wrote:
>> Hi Sakari,
>>
>> On 8/13/19 1:28 AM, Sakari Ailus wrote:
>>> Hi Steve,
>>>
>>> On Thu, Aug 08, 2019 at 11:02:29AM -0700, 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.
>>> After powering on the sensor it sensor should stay in (software) standby
>>> mode until streaming is started. It should still have its lanes in LP-11
>>> mode if the sensor supports it (as documented) until the sensor switches to
>>> the streaming mode.
>> Yes that's my understanding too.
>>
>>>> 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).
>>> Not all sensor drivers power on the sensor before starting streaming.
>>>
>>> Perhaps we could add a prepare_streaming() callback (in absence of a better
>>> proposal) to address that? We'd also need a corresponding
>>> unprepare_streaming() callback as well --- to power off the sensor. I think
>>> this only should be done if the sensor can be switched to LP-11 explicitly;
>>> many simply don't support that.
>> So you are saying a sensor can power on and place its lanes in LP-11 in this
>> new prepare_streaming() callback, the reason being, this can be called
>> closer in time, and just before, the receiver DPHY is brought out of reset,
>> in order for the receiver DPHY to be able to detect a possible early
>> transition to HS mode?
>>
>> While I don't have any objection to adding a prepare_streaming() callback
>> (it could be useful for other reasons), I don't see why the sensor could not
>> simply make use of existing s_power to power on and place the lanes in
>> LP-11. That is what it's intended for anyway.
> The vast majority of new drivers no longer have s_power callbacks --- they
> use runtime PM instead.
>

Ok I see that now. Most of them call pm_runtime_get at stream on, but 
some others in an internal_ops open() callback.

So never mind the patch I posted that moves receiver DPHY power on to 
.s_power(), that will actually make things worse for sensors that power 
on via runtime PM in their .s_stream op.

Steve


>> There's really no drawback to the patch I posted that moves receiver DPHY
>> init and reset-release to s_power time in the receiver driver. If given a
>> choice, an isolated fix to the imx mipi csi-2 receiver driver should be
>> preferable to implementing a new callback.
>>
>> But perhaps one advantage to the prepare_streaming() callback idea, is that
>> there is more of a guarantee of minimum time between the
>> sensor-power-on-and-enter-LP-11 event and receiver-DPHY-reset-release event,
>> since s_power is normally called from v4l2_pipeline_pm_use() which could add
>> extra overhead between the time s_power is called at the transmitter and at
>> the receiver (but not much - the transmitter is connected directly to
>> receiver in the graph).