On 04/06/2021 16:44, Laurent Pinchart wrote:
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:
On 18/04/2021 15:46, Laurent Pinchart wrote:
On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote:
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@xxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxx>
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.
I'd rather keep the MASK prefix as it's used for multiple bit masks too.
I think the problem here is the define for 0. The define should tell
what the bit value does, but here it's just the field name. The value
defines could perhaps be:
#define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_ASSERT 0
#define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_DEASSERT 1
Using just 0 or 1 may work fine at times, but often you don't know what
they mean. You set 1 to RESET_CTRL. Does it put the IP into reset? Or
release the reset?
Some other bits in cal_regs.h are fine, like:
#define CAL_CTRL_PWRSCPCLK_MASK BIT(21)
#define CAL_CTRL_PWRSCPCLK_AUTO 0
#define CAL_CTRL_PWRSCPCLK_FORCE 1
But some have this same issue:
#define CAL_CTRL_POSTED_WRITES_MASK BIT(0)
#define CAL_CTRL_POSTED_WRITES_NONPOSTED 0
#define CAL_CTRL_POSTED_WRITES 1
Tomi
