Skip to contentSkip to conversation

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

PATCH Superseded Fabio Estevam 2 participants 2 messages Jun 27, 2019

Subsystems (4)
Media (V4L/DVB) Arm i.MX and Layerscape Staging Media drivers for Freescale i.MX

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Fabio Estevam

author reviewer
#0 lore raw

PATCH v2 1/1 media: imx: mipi csi-2: Don't fail if initial state times-out

From: Ezequiel Garcia <[email protected]>

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 <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>

---
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 | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/staging/media/imx/imx6-mipi-csi2.c +6 -8
index f29e28df36ed..240f992ad2ef 100644
@@ -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,11 @@ 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, "Timeout waiting for LP-11 state on all active lanes.\n");
+		v4l2_warn(&csi2->sd, "This is most likely caused by a bug in the sensor driver.\n");
+		v4l2_warn(&csi2->sd, "Capture might fail or contain visual artifacts.\n");
+		v4l2_warn(&csi2->sd, "phy_state = 0x%08x\n", reg);
 	}
-
-	return 0;
 }
 
 /* Wait for active clock on the clock lane. */

@@ -316,9 +316,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);
Signature · 1 line
2.17.1
maintainer
#1 lore raw
Quoted 42 lines · Fabio Estevam

From: Ezequiel Garcia <[email protected]>

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 <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>

---
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 | 14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)
drivers/staging/media/imx/imx6-mipi-csi2.c +1 -1
index f29e28df36ed..240f992ad2ef 100644
@@ -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;

drivers/staging/media/imx/imx6-mipi-csi2.c static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2) · line 259

Quoted 8 lines
drivers/staging/media/imx/imx6-mipi-csi2.c +3 -2
@@ -254,11 +254,11 @@ 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, "Timeout waiting for LP-11 state on all active lanes.\n");
+		v4l2_warn(&csi2->sd, "This is most likely caused by a bug in the sensor driver.\n");
+		v4l2_warn(&csi2->sd, "Capture might fail or contain visual artifacts.\n");

I think this should be reduced, something like:

"LP-11 wait timeout, likely a sensor driver bug, expect capture failures"

Steve

Quoted 18 lines
drivers/staging/media/imx/imx6-mipi-csi2.c +2 -5
+		v4l2_warn(&csi2->sd, "phy_state = 0x%08x\n", reg);
  	}
-
-	return 0;
  }
 
  /* Wait for active clock on the clock lane. */

@@ -316,9 +316,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);