Hi Tomi,

On Thu, Apr 29, 2021 at 02:57:23AM +0300, Laurent Pinchart wrote:

On Mon, Apr 19, 2021 at 02:29:20PM +0300, Tomi Valkeinen wrote:

quoted

On 18/04/2021 15:46, Laurent Pinchart wrote:

quoted

On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote:

quoted

Asserting ComplexIO reset seems to affect the HW (ie. asserting reset
will break an active capture), but the RESET_DONE bit never changes to
"reset is ongoing" state. Thus we always get a timeout.

Drop the wait, as it seems to achieve nothing.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
  drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++-------------
  1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c
index 0354f311c5d2..245c601b992c 100644
--- a/drivers/media/platform/ti-vpe/cal-camerarx.c
+++ b/drivers/media/platform/ti-vpe/cal-camerarx.c

@@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
  
  static void cal_camerarx_stop(struct cal_camerarx *phy)
  {
-	unsigned int i;
  	int ret;
  
  	cal_camerarx_ppi_disable(phy);

@@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy)
  			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL,
  			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
  
-	/* Wait for power down completion */
-	for (i = 0; i < 10; i++) {
-		if (cal_read_field(phy->cal,
-				   CAL_CSI2_COMPLEXIO_CFG(phy->instance),
-				   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) ==
-		    CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING)

Isn't this the wrong condition ? I would have expected
CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not
CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why
you always get a timeout.

No, I don't think so. The complexio reset is set active just before the 
wait. So the reset status should show reset ongoing, until at some point 
we release the reset (we do that when starting the PHY again).

The TRM doesn't talk about this, though. So, I guess the status might go 
to RESETONGOING for a very short time and back to RESETCOMPLETED before 
the code can see the RESETONGOING. But I suspect the status just stays 
at RESETCOMPLETED.

The TRM is indeed not very clear. My understanding was that asserting
RESET_CTRL initiates the reset, and RESET_DONE switches to 1 once the
reset completes. There's however a note in the initialization sequence
that states

a. Deassert the CAMERARX reset:
i. Set CAL_CSI2_COMPLEXIO_CFG_j[30] RESET_CTRL to 0x1.
CAUTION
For the CAL_CSI2_COMPLEXIO_CFG_j[29] RESET_DONE bit to be set to 0x1
(reset completed), the external sensor must to be active and sending the
MIPI HS BYTECLK (that is, RXBYTECLKHS clock).

The RESET_DONE bit may thus only toggle when de-asserting the reset
signal (by setting RESET_CTRL to 1). It would be useful to test that
hypothesis by reading RESET_DONE just before setting RESET_CTRL to 1,
and right after. I'd expect the values to be 0 and 1 respectively. If
that's the case, then this patch is likely correct, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


The register macros are quite confusing by the way. We have

#define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK          BIT(30)
#define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL                       0
#define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL           1

When reading the code, I thought

        cal_write_field(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance),
                        CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL,
                        CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK)

meant that we were setting the reset bit to 1. I would personally get
rid of the _MASK suffixes for single bits, and use 0 and 1 in the code
instead of CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL and
CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL.

Do you think this would be a good idea ? It can be done in a follow-up
patch.

-- 
Regards,

Laurent Pinchart