Re: [PATCH 15/28] media: ti-vpe: cal: remove wait when stopping camerarx

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 29/04/2021 02:57, Laurent Pinchart wrote:
> Hi Tomi,
> 
> 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@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

No, it's a bit to keep (or release) camerarx in reset. When the camerarx 
is being started, both reset ctrl and done are 0. The driver then 
releases the reset by setting ctrl to 1. Nothing happens at this time, 
as the camerarx needs the byteclk from the sensor to finish the reset. 
Later, when the sensor has been started, done changes to 1. This works fine.

The problem is on the stop side. Setting ctrl back to 0 does something, 
as the capture stops, so presumably the camerarx goes into reset state. 
But the done bit does not change.

The done bit is back to 0 when we start the camerarx again. My guess is 
that it's reset when the CAL module is turned off via runtime PM. This 
is not good, as there's no strict rule that says CAL will be turned off 
by runtime PM. However, I have not found any other way to reset the done 
bit. And in the case that we don't get a CAL reset, I guess we're still 
fine, as the camerarx is just already out of reset, and works.

  Tomi