Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 8/6/20 10:44 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 10:27 AM, Dmitry Osipenko wrote:
>> 06.08.2020 20:12, Sowjanya Komatineni пишет:
>>> On 8/6/20 9:41 AM, Sowjanya Komatineni wrote:
>>>> On 8/6/20 9:10 AM, Dmitry Osipenko wrote:
>>>>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>>>> Confirmed from HW designer, calibration FSM to finish takes worst
>>>>>>>> case
>>>>>>>> 72uS so by the time it gets to sensor stream it will be done its
>>>>>>>> sequence and will be waiting for DONE bit.
>>>>>>>>
>>>>>>>> So disabling MIPI CAL clock on sensor stream fails is safe.
>>>>>>> 72us is quite a lot of time, what will happen if LP-11 happens 
>>>>>>> before
>>>>>>> FSM finished calibration?
>>>>>>>
>>>>>>> Maybe the finish_calibration() needs to split into two parts:
>>>>>>>
>>>>>>>     1. wait for CAL_STATUS_ACTIVE before enabling sensor
>>>>>>>     2. wait for CAL_STATUS_DONE after enabling sensor
>>>>>> I don't think we need to split for active and done. Active will 
>>>>>> be 1 as
>>>>>> long as other pads are in calibration as well.
>>>>>>
>>>>>> We cant use active status check for specific pads under calibration.
>>>>>> This is common bit for all pads.
>>>>> Does hardware have a single FSM block shared by all pads or there 
>>>>> is FSM
>>>>> per group of pads?
>>>> MIPI CAL status register has DONE bits for individual pads status and
>>>> single ACTIVE bit.
>>>>
>>>> ACTIVE bit set to 1 indicates auto calibration is active which is the
>>>> case even when other pads (other CSI pads from other ports streaming
>>>> in case of parallel stream) are under calibration. Also DSI is shared
>>>> as well.
>>>>
>>>> We do calibration for individual pads. So, we should not rely on
>>>> ACTIVE bit.
>>>>
>>>>
>>>> MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the
>>>> beginning.
>>>>
>>>> But I think this also should be fixed as in case of parallel streams
>>>> calibration can happen in parallel waiting for ACTIVE to be cleared
>>>> makes all calibration callers to wait for longer than needed as ACTIVE
>>>> is common for all pads.
>>>>
>>>>>> Unfortunately HW don't have separate status indicating when 
>>>>>> sequence is
>>>>>> done to indicate its waiting for LP11.
>>>>>>
>>>>>>
>>>>>> To avoid all this, will remove cancel_calibration() totally and use
>>>>>> same
>>>>>> finish calibration even in case of stream failure then.
>>>>>>
>>>>> What about to add 72us delay to the end of start_calibration() in 
>>>>> order
>>>>> to ensure that FSM is finished before LP-11?
>>>> Why we should add 72uS in start_calibration() when can use same
>>>> finish_calibration() for both pass/fail cases?
>>>>
>>>> Only timing loose we see is in case of failure we still wait for 250ms
>>>> and as this is failing case I hope should be ok.
>>>>
>>> Also as we don't need cancel_calibration(), keeping tegra_mipi_wait()
>>> like earlier makes sense I believe as we are letting it finish going
>>> thru sequence.
>>>
>>> So I think below are fixes,
>>>
>>> 1. Existing MIPI driver, tegra_mipi_wait() to not use status ACTIVE bit
>>> to be 0 and use only DONE bit to be 1 for wait condition  as we are
>>> calibrating separately for individual pads and this ACTIVE bit is 
>>> common
>>> for all pads where it will not be 0 in case of other parallel streams
>>> which may also be under calibration.
>> Yes, looks like it's a mistake of the current MIPI driver that it polls
>> the ACTIVE bit.
>>
>>> 2. No need for separate cancel_calibration. So, probably earlier names
>>> tegra_mipi_calibrate() and tegra_mipi_wait() hols good as we are 
>>> waiting
>>> for calibration sequence to finish irrespective of fail/pass.
>> The new names reflect better what those functions actually do, IMO.
> ok Will keep same names.
>>
>> What about to make finish_calibration() to take an additional argument
>> which corresponds to the awaited HW bits? For example if it's CSIA, then
>> it could be:
>>
>>    tegra_mipi_finish_calibration(csi_chan->mipi, MIPI_CAL_CSIA);
> MIPI device is separate for each stream so waiting for only those 
> corresponding DONE bits happen currently and no need to pass argument.
>>
>>
>> Also, is it okay that DSI and CSI could change MIPI_CAL_CTRL after DSI
>> or CSI already started calibration?
>>
>> Looking at the current start_calibration(), I think the mutex should be
>> kept locked and then finish_calibration() should unlock it.

Right mutex_unlock should happen at end of finish_calibration.

With keeping mutex locked in start, we dont have to check for active to 
be 0 to issue start as mutex will keep it locked and other pads 
calibration can only go thru when current one is done.

So instead of below sequence, its simpler to do this way?

start_calibration()

- mutex_lock

- wait for 72uS after start

finish_calibration()

- keep check for ACTIVE = 0 and DONE = 1

- mutex_unlock()

>
> Confirmed with HW designer.
>
> ACTIVE is common bit for all pads where we see it 1 as long as all 
> pads (DSI + all CSI Pads) are under calibration.
>
> While MIPI CAL is doing calibration for certain pads, before issuing 
> other start it has to wait for ACTIVE to be 0.
>
>
> Earlier driver (before split) checks for ACTIVE to be 0 along with 
> DONE bit to be 1 as it does both calibrate and wait in same API.
>
> With the split, looks like we need below sequence to be safe.
>
> 1. tegra_mipi_start_calibration(): wait for ACTIVE to be 0 before 
> issuing START and after issuing start wait for 72uS to let calibration 
> code sequence finish so it will be ready to see LP-11 after that.
>
> In case of parallel streams, call to start_calibration can happen when 
> pads of other stream are under calibration.
>
> 2. tegra_mipi_finish_calibration(): check for DONE bit to be 1
>
>
>