diff mbox series

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

Message ID 1596469346-937-9-git-send-email-skomatineni@nvidia.com
State Changes Requested
Headers show
Series Support for Tegra video capture from external sensor | expand

Commit Message

Sowjanya Komatineni Aug. 3, 2020, 3:42 p.m. UTC 
With the split of MIPI calibration into tegra_mipi_calibrate() and
tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
is done.

So, this patch skips disabling MIPI clock after triggering start of
calibration and disables it only after waiting for done status from
the calibration logic.

This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
with their usage.

As MIPI clock is left enabled and in case of any failures with CSI input
streaming tegra_mipi_finish_calibration() will not get invoked.
So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
and consumer drivers can call this in such cases.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/gpu/drm/tegra/dsi.c |  4 ++--
 drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
 include/linux/host1x.h      |  5 +++--
 3 files changed, 15 insertions(+), 13 deletions(-)

Comments

Thierry Reding Aug. 5, 2020, 1:46 p.m. UTC | #1 
On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
> With the split of MIPI calibration into tegra_mipi_calibrate() and
> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
> is done.
> 
> So, this patch skips disabling MIPI clock after triggering start of
> calibration and disables it only after waiting for done status from
> the calibration logic.
> 
> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
> with their usage.
> 
> As MIPI clock is left enabled and in case of any failures with CSI input
> streaming tegra_mipi_finish_calibration() will not get invoked.
> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
> and consumer drivers can call this in such cases.
> 
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c |  4 ++--
>  drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>  include/linux/host1x.h      |  5 +++--
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 3820e8d..a7864e9 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>  		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>  	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>  
> -	err = tegra_mipi_calibrate(dsi->mipi);
> +	err = tegra_mipi_start_calibration(dsi->mipi);
>  	if (err < 0)
>  		return err;
>  
> -	return tegra_mipi_wait(dsi->mipi);
> +	return tegra_mipi_finish_calibration(dsi->mipi);
>  }
>  
>  static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> index e606464..b15ab6e 100644
> --- a/drivers/gpu/host1x/mipi.c
> +++ b/drivers/gpu/host1x/mipi.c
> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>  }
>  EXPORT_SYMBOL(tegra_mipi_disable);
>  
> -int tegra_mipi_wait(struct tegra_mipi_device *device)
> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
> +{
> +	clk_disable(device->mipi->clk);

Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
registers here? We don't clear the START bit in the former when the
calibration has successfully finished, but I suspect that's because
the bit is self-clearing. But I wonder if we still need to clear it
upon cancellation to make sure the calibration does indeed stop.

Thierry
Dmitry Osipenko Aug. 5, 2020, 2:05 p.m. UTC | #2 
05.08.2020 16:46, Thierry Reding пишет:
> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>> is done.
>>
>> So, this patch skips disabling MIPI clock after triggering start of
>> calibration and disables it only after waiting for done status from
>> the calibration logic.
>>
>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>> with their usage.
>>
>> As MIPI clock is left enabled and in case of any failures with CSI input
>> streaming tegra_mipi_finish_calibration() will not get invoked.
>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>> and consumer drivers can call this in such cases.
>>
>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>  drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>  include/linux/host1x.h      |  5 +++--
>>  3 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>> index 3820e8d..a7864e9 100644
>> --- a/drivers/gpu/drm/tegra/dsi.c
>> +++ b/drivers/gpu/drm/tegra/dsi.c
>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>>  		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>  	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>  
>> -	err = tegra_mipi_calibrate(dsi->mipi);
>> +	err = tegra_mipi_start_calibration(dsi->mipi);
>>  	if (err < 0)
>>  		return err;
>>  
>> -	return tegra_mipi_wait(dsi->mipi);
>> +	return tegra_mipi_finish_calibration(dsi->mipi);
>>  }
>>  
>>  static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>> index e606464..b15ab6e 100644
>> --- a/drivers/gpu/host1x/mipi.c
>> +++ b/drivers/gpu/host1x/mipi.c
>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>  }
>>  EXPORT_SYMBOL(tegra_mipi_disable);
>>  
>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>> +{
>> +	clk_disable(device->mipi->clk);
> 
> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
> registers here? We don't clear the START bit in the former when the
> calibration has successfully finished, but I suspect that's because
> the bit is self-clearing. But I wonder if we still need to clear it
> upon cancellation to make sure the calibration does indeed stop.

Apparently there is no way to explicitly stop calibration other than to
reset MIPI calibration block, but Sowjanya says this is unnecessary.

Perhaps having a fixed delay before disabling clock could be enough to
ensure that calibration is stopped before the clock is disabled?
Dmitry Osipenko Aug. 5, 2020, 2:19 p.m. UTC | #3 
05.08.2020 17:05, Dmitry Osipenko пишет:
> 05.08.2020 16:46, Thierry Reding пишет:
>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>> is done.
>>>
>>> So, this patch skips disabling MIPI clock after triggering start of
>>> calibration and disables it only after waiting for done status from
>>> the calibration logic.
>>>
>>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>> with their usage.
>>>
>>> As MIPI clock is left enabled and in case of any failures with CSI input
>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>>> and consumer drivers can call this in such cases.
>>>
>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>  drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>  drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>  include/linux/host1x.h      |  5 +++--
>>>  3 files changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>> index 3820e8d..a7864e9 100644
>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>>>  		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>  	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>  
>>> -	err = tegra_mipi_calibrate(dsi->mipi);
>>> +	err = tegra_mipi_start_calibration(dsi->mipi);
>>>  	if (err < 0)
>>>  		return err;
>>>  
>>> -	return tegra_mipi_wait(dsi->mipi);
>>> +	return tegra_mipi_finish_calibration(dsi->mipi);
>>>  }
>>>  
>>>  static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>> index e606464..b15ab6e 100644
>>> --- a/drivers/gpu/host1x/mipi.c
>>> +++ b/drivers/gpu/host1x/mipi.c
>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>>  }
>>>  EXPORT_SYMBOL(tegra_mipi_disable);
>>>  
>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>> +{
>>> +	clk_disable(device->mipi->clk);
>>
>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>> registers here? We don't clear the START bit in the former when the
>> calibration has successfully finished, but I suspect that's because
>> the bit is self-clearing. But I wonder if we still need to clear it
>> upon cancellation to make sure the calibration does indeed stop.
> 
> Apparently there is no way to explicitly stop calibration other than to
> reset MIPI calibration block, but Sowjanya says this is unnecessary.
> 
> Perhaps having a fixed delay before disabling clock could be enough to
> ensure that calibration is stopped before the clock is disabled?
> 

Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
it needs to be polled until it's unset?
Sowjanya Komatineni Aug. 5, 2020, 4:33 p.m. UTC | #4 
On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
> 05.08.2020 17:05, Dmitry Osipenko пишет:
>> 05.08.2020 16:46, Thierry Reding пишет:
>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>> is done.
>>>>
>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>> calibration and disables it only after waiting for done status from
>>>> the calibration logic.
>>>>
>>>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>> with their usage.
>>>>
>>>> As MIPI clock is left enabled and in case of any failures with CSI input
>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>>>> and consumer drivers can call this in such cases.
>>>>
>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>   drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>   drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>   include/linux/host1x.h      |  5 +++--
>>>>   3 files changed, 15 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>> index 3820e8d..a7864e9 100644
>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
>>>>   		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>   	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>   
>>>> -	err = tegra_mipi_calibrate(dsi->mipi);
>>>> +	err = tegra_mipi_start_calibration(dsi->mipi);
>>>>   	if (err < 0)
>>>>   		return err;
>>>>   
>>>> -	return tegra_mipi_wait(dsi->mipi);
>>>> +	return tegra_mipi_finish_calibration(dsi->mipi);
>>>>   }
>>>>   
>>>>   static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>> index e606464..b15ab6e 100644
>>>> --- a/drivers/gpu/host1x/mipi.c
>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>>>   }
>>>>   EXPORT_SYMBOL(tegra_mipi_disable);
>>>>   
>>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>> +{
>>>> +	clk_disable(device->mipi->clk);
>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>> registers here? We don't clear the START bit in the former when the
>>> calibration has successfully finished, but I suspect that's because
>>> the bit is self-clearing. But I wonder if we still need to clear it
>>> upon cancellation to make sure the calibration does indeed stop.
>> Apparently there is no way to explicitly stop calibration other than to
>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>
>> Perhaps having a fixed delay before disabling clock could be enough to
>> ensure that calibration is stopped before the clock is disabled?
>>
> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
> it needs to be polled until it's unset?

Confirmed with HW design team during this patch update.

SW does not need to clear START bit and only write 1 takes effect to 
that bit.

Also, no need to have delay or do any other register settings unclear as 
its FSM and there's nothing to get stuck.

Also it goes thru small finite set of codes and by the time sensor 
programming happens for enabling streaming FSM will finish its 
calibration sequence much early and it will only wait for pads LP-11.

So, during cancel we only need disable MIPI clock.
Dmitry Osipenko Aug. 5, 2020, 4:47 p.m. UTC | #5 
05.08.2020 19:33, Sowjanya Komatineni пишет:
> 
> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>>> is done.
>>>>>
>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>> calibration and disables it only after waiting for done status from
>>>>> the calibration logic.
>>>>>
>>>>> This patch renames tegra_mipi_calibrate() as
>>>>> tegra_mipi_start_calibration()
>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>>> with their usage.
>>>>>
>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>> input
>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>> MIPI clock
>>>>> and consumer drivers can call this in such cases.
>>>>>
>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>> ---
>>>>>   drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>   drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>   include/linux/host1x.h      |  5 +++--
>>>>>   3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>>> index 3820e8d..a7864e9 100644
>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>> tegra_dsi *dsi)
>>>>>           DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>       tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>   -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>       if (err < 0)
>>>>>           return err;
>>>>>   -    return tegra_mipi_wait(dsi->mipi);
>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>   }
>>>>>     static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>> unsigned long bclk,
>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>> index e606464..b15ab6e 100644
>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>> tegra_mipi_device *dev)
>>>>>   }
>>>>>   EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>   -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>> +{
>>>>> +    clk_disable(device->mipi->clk);
>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>> registers here? We don't clear the START bit in the former when the
>>>> calibration has successfully finished, but I suspect that's because
>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>> upon cancellation to make sure the calibration does indeed stop.
>>> Apparently there is no way to explicitly stop calibration other than to
>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>
>>> Perhaps having a fixed delay before disabling clock could be enough to
>>> ensure that calibration is stopped before the clock is disabled?
>>>
>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>> it needs to be polled until it's unset?
> 
> Confirmed with HW design team during this patch update.
> 
> SW does not need to clear START bit and only write 1 takes effect to
> that bit.
> 
> Also, no need to have delay or do any other register settings unclear as
> its FSM and there's nothing to get stuck.
> 
> Also it goes thru small finite set of codes and by the time sensor
> programming happens for enabling streaming FSM will finish its
> calibration sequence much early and it will only wait for pads LP-11.
> 
> So, during cancel we only need disable MIPI clock.
> 

But there is no guarantee that cancel_calibration() couldn't be invoked
in the middle of the calibration process, hence FSM could freeze in an
intermediate state if it's running on the disabled MIPI clock, this
doesn't sound good.
Sowjanya Komatineni Aug. 5, 2020, 4:50 p.m. UTC | #6 
On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>>>> is done.
>>>>>>
>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>> calibration and disables it only after waiting for done status from
>>>>>> the calibration logic.
>>>>>>
>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>> tegra_mipi_start_calibration()
>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>>>> with their usage.
>>>>>>
>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>> input
>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>> MIPI clock
>>>>>> and consumer drivers can call this in such cases.
>>>>>>
>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>    drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>    include/linux/host1x.h      |  5 +++--
>>>>>>    3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>>>> index 3820e8d..a7864e9 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>> tegra_dsi *dsi)
>>>>>>            DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>        tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>    -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>        if (err < 0)
>>>>>>            return err;
>>>>>>    -    return tegra_mipi_wait(dsi->mipi);
>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>    }
>>>>>>      static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>> unsigned long bclk,
>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>> index e606464..b15ab6e 100644
>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>> tegra_mipi_device *dev)
>>>>>>    }
>>>>>>    EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>    -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>>> +{
>>>>>> +    clk_disable(device->mipi->clk);
>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>> registers here? We don't clear the START bit in the former when the
>>>>> calibration has successfully finished, but I suspect that's because
>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>> Apparently there is no way to explicitly stop calibration other than to
>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>
>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>> ensure that calibration is stopped before the clock is disabled?
>>>>
>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>> it needs to be polled until it's unset?
>> Confirmed with HW design team during this patch update.
>>
>> SW does not need to clear START bit and only write 1 takes effect to
>> that bit.
>>
>> Also, no need to have delay or do any other register settings unclear as
>> its FSM and there's nothing to get stuck.
>>
>> Also it goes thru small finite set of codes and by the time sensor
>> programming happens for enabling streaming FSM will finish its
>> calibration sequence much early and it will only wait for pads LP-11.
>>
>> So, during cancel we only need disable MIPI clock.
>>
> But there is no guarantee that cancel_calibration() couldn't be invoked
> in the middle of the calibration process, hence FSM could freeze in an
> intermediate state if it's running on the disabled MIPI clock, this
> doesn't sound good.
Actual calibration logic uses UART_FST_CAL clock which is always enabled
Dmitry Osipenko Aug. 5, 2020, 4:57 p.m. UTC | #7 
05.08.2020 19:50, Sowjanya Komatineni пишет:
> 
> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>> calibration
>>>>>>> is done.
>>>>>>>
>>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>>> calibration and disables it only after waiting for done status from
>>>>>>> the calibration logic.
>>>>>>>
>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>> tegra_mipi_start_calibration()
>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>> inline
>>>>>>> with their usage.
>>>>>>>
>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>> input
>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>> MIPI clock
>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>    drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>    include/linux/host1x.h      |  5 +++--
>>>>>>>    3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>> tegra_dsi *dsi)
>>>>>>>            DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>        tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>    -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>        if (err < 0)
>>>>>>>            return err;
>>>>>>>    -    return tegra_mipi_wait(dsi->mipi);
>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>    }
>>>>>>>      static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>> unsigned long bclk,
>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>> index e606464..b15ab6e 100644
>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>> tegra_mipi_device *dev)
>>>>>>>    }
>>>>>>>    EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>    -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>> *device)
>>>>>>> +{
>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>> Apparently there is no way to explicitly stop calibration other
>>>>> than to
>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>
>>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>
>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>>> it needs to be polled until it's unset?
>>> Confirmed with HW design team during this patch update.
>>>
>>> SW does not need to clear START bit and only write 1 takes effect to
>>> that bit.
>>>
>>> Also, no need to have delay or do any other register settings unclear as
>>> its FSM and there's nothing to get stuck.
>>>
>>> Also it goes thru small finite set of codes and by the time sensor
>>> programming happens for enabling streaming FSM will finish its
>>> calibration sequence much early and it will only wait for pads LP-11.
>>>
>>> So, during cancel we only need disable MIPI clock.
>>>
>> But there is no guarantee that cancel_calibration() couldn't be invoked
>> in the middle of the calibration process, hence FSM could freeze in an
>> intermediate state if it's running on the disabled MIPI clock, this
>> doesn't sound good.
> Actual calibration logic uses UART_FST_CAL clock which is always enabled

What enables the UART_FST_CAL clock? I don't see this clock used anywhere.
Sowjanya Komatineni Aug. 5, 2020, 5:04 p.m. UTC | #8 
On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>> calibration
>>>>>>>> is done.
>>>>>>>>
>>>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>>>> calibration and disables it only after waiting for done status from
>>>>>>>> the calibration logic.
>>>>>>>>
>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>> inline
>>>>>>>> with their usage.
>>>>>>>>
>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>>> input
>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>> MIPI clock
>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>
>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>     drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>     include/linux/host1x.h      |  5 +++--
>>>>>>>>     3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>> tegra_dsi *dsi)
>>>>>>>>             DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>         tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>     -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>         if (err < 0)
>>>>>>>>             return err;
>>>>>>>>     -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>     }
>>>>>>>>       static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>> unsigned long bclk,
>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>     }
>>>>>>>>     EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>     -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>> *device)
>>>>>>>> +{
>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>> than to
>>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>>
>>>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>
>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>>>> it needs to be polled until it's unset?
>>>> Confirmed with HW design team during this patch update.
>>>>
>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>> that bit.
>>>>
>>>> Also, no need to have delay or do any other register settings unclear as
>>>> its FSM and there's nothing to get stuck.
>>>>
>>>> Also it goes thru small finite set of codes and by the time sensor
>>>> programming happens for enabling streaming FSM will finish its
>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>
>>>> So, during cancel we only need disable MIPI clock.
>>>>
>>> But there is no guarantee that cancel_calibration() couldn't be invoked
>>> in the middle of the calibration process, hence FSM could freeze in an
>>> intermediate state if it's running on the disabled MIPI clock, this
>>> doesn't sound good.
>> Actual calibration logic uses UART_FST_CAL clock which is always enabled
> What enables the UART_FST_CAL clock? I don't see this clock used anywhere.

UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.

I don't see mipi driver handling this and I think that's because this 
clock is enabled all the time as its used for UART as well. Probably 
thierry can comment on this clock.

Also regarding cancel calibration, as FSM goes thru only finite sequence 
codes by the time csi stream and sensor stream happens which is where we 
check for calibration to complete for sure calibration will be finished 
and calibration logic will only wait for pads to be in LP-11 to apply 
results. So nothing special needed during cancel except to turn clock 
off to balance its usage count.
Dmitry Osipenko Aug. 5, 2020, 5:23 p.m. UTC | #9 
05.08.2020 20:04, Sowjanya Komatineni пишет:
> 
> On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni
>>>>>>>> wrote:
>>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>>> calibration
>>>>>>>>> is done.
>>>>>>>>>
>>>>>>>>> So, this patch skips disabling MIPI clock after triggering
>>>>>>>>> start of
>>>>>>>>> calibration and disables it only after waiting for done status
>>>>>>>>> from
>>>>>>>>> the calibration logic.
>>>>>>>>>
>>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>>> inline
>>>>>>>>> with their usage.
>>>>>>>>>
>>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>>>> input
>>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>>> MIPI clock
>>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>>     drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>>     include/linux/host1x.h      |  5 +++--
>>>>>>>>>     3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>>> tegra_dsi *dsi)
>>>>>>>>>             DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>>         tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>>     -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>>         if (err < 0)
>>>>>>>>>             return err;
>>>>>>>>>     -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>>     }
>>>>>>>>>       static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>>> unsigned long bclk,
>>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>>     }
>>>>>>>>>     EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>>     -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>>> *device)
>>>>>>>>> +{
>>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and
>>>>>>>> MIPI_CAL_STATUS
>>>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>>> than to
>>>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>>>
>>>>>>> Perhaps having a fixed delay before disabling clock could be
>>>>>>> enough to
>>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>>
>>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register.
>>>>>> Maybe
>>>>>> it needs to be polled until it's unset?
>>>>> Confirmed with HW design team during this patch update.
>>>>>
>>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>>> that bit.
>>>>>
>>>>> Also, no need to have delay or do any other register settings
>>>>> unclear as
>>>>> its FSM and there's nothing to get stuck.
>>>>>
>>>>> Also it goes thru small finite set of codes and by the time sensor
>>>>> programming happens for enabling streaming FSM will finish its
>>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>>
>>>>> So, during cancel we only need disable MIPI clock.
>>>>>
>>>> But there is no guarantee that cancel_calibration() couldn't be invoked
>>>> in the middle of the calibration process, hence FSM could freeze in an
>>>> intermediate state if it's running on the disabled MIPI clock, this
>>>> doesn't sound good.
>>> Actual calibration logic uses UART_FST_CAL clock which is always enabled
>> What enables the UART_FST_CAL clock? I don't see this clock used
>> anywhere.
> 
> UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.
> 
> I don't see mipi driver handling this and I think that's because this
> clock is enabled all the time as its used for UART as well. Probably
> thierry can comment on this clock.

It's not only the MIPI driver, the clock isn't defined at all neither in
the clk driver, nor in clk DT bindings.

It could be fragile to assume that it's always enabled.

> Also regarding cancel calibration, as FSM goes thru only finite sequence
> codes by the time csi stream and sensor stream happens which is where we
> check for calibration to complete for sure calibration will be finished
> and calibration logic will only wait for pads to be in LP-11 to apply
> results. So nothing special needed during cancel except to turn clock
> off to balance its usage count.

I guess it should be okay for the case of the VI driver, but
in general please don't assume that code can't change in the future. The
common API always should be made reliable for all possible situations.
Sowjanya Komatineni Aug. 5, 2020, 5:29 p.m. UTC | #10 
On 8/5/20 10:04 AM, Sowjanya Komatineni wrote:
>
> On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni 
>>>>>>>> wrote:
>>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() 
>>>>>>>>> and
>>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>>> calibration
>>>>>>>>> is done.
>>>>>>>>>
>>>>>>>>> So, this patch skips disabling MIPI clock after triggering 
>>>>>>>>> start of
>>>>>>>>> calibration and disables it only after waiting for done status 
>>>>>>>>> from
>>>>>>>>> the calibration logic.
>>>>>>>>>
>>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>>> inline
>>>>>>>>> with their usage.
>>>>>>>>>
>>>>>>>>> As MIPI clock is left enabled and in case of any failures with 
>>>>>>>>> CSI
>>>>>>>>> input
>>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>>> MIPI clock
>>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>>     drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>>     include/linux/host1x.h      |  5 +++--
>>>>>>>>>     3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>>> tegra_dsi *dsi)
>>>>>>>>>             DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>>         tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>>     -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>>         if (err < 0)
>>>>>>>>>             return err;
>>>>>>>>>     -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>>     }
>>>>>>>>>       static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>>> unsigned long bclk,
>>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c 
>>>>>>>>> b/drivers/gpu/host1x/mipi.c
>>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>>     }
>>>>>>>>>     EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>>     -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>>> *device)
>>>>>>>>> +{
>>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and 
>>>>>>>> MIPI_CAL_STATUS
>>>>>>>> registers here? We don't clear the START bit in the former when 
>>>>>>>> the
>>>>>>>> calibration has successfully finished, but I suspect that's 
>>>>>>>> because
>>>>>>>> the bit is self-clearing. But I wonder if we still need to 
>>>>>>>> clear it
>>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>>> than to
>>>>>>> reset MIPI calibration block, but Sowjanya says this is 
>>>>>>> unnecessary.
>>>>>>>
>>>>>>> Perhaps having a fixed delay before disabling clock could be 
>>>>>>> enough to
>>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>>
>>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. 
>>>>>> Maybe
>>>>>> it needs to be polled until it's unset?
>>>>> Confirmed with HW design team during this patch update.
>>>>>
>>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>>> that bit.
>>>>>
>>>>> Also, no need to have delay or do any other register settings 
>>>>> unclear as
>>>>> its FSM and there's nothing to get stuck.
>>>>>
>>>>> Also it goes thru small finite set of codes and by the time sensor
>>>>> programming happens for enabling streaming FSM will finish its
>>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>>
>>>>> So, during cancel we only need disable MIPI clock.
>>>>>
>>>> But there is no guarantee that cancel_calibration() couldn't be 
>>>> invoked
>>>> in the middle of the calibration process, hence FSM could freeze in an
>>>> intermediate state if it's running on the disabled MIPI clock, this
>>>> doesn't sound good.
>>> Actual calibration logic uses UART_FST_CAL clock which is always 
>>> enabled
>> What enables the UART_FST_CAL clock? I don't see this clock used 
>> anywhere.
>
> UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.
>
> I don't see mipi driver handling this and I think that's because this 
> clock is enabled all the time as its used for UART as well. Probably 
> thierry can comment on this clock.
>
> Also regarding cancel calibration, as FSM goes thru only finite 
> sequence codes by the time csi stream and sensor stream happens which 
> is where we check for calibration to complete for sure calibration 
> will be finished and calibration logic will only wait for pads to be 
> in LP-11 to apply results. So nothing special needed during cancel 
> except to turn clock off to balance its usage count.
>
>
UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM 
that goes thru sequence codes and when done waits for pads to be in 
LP-11 to apply results.

MIPI_CLK is controller gate clock which is also need to be kept enabled 
as incase if it sees LP-11 it updates registers so its recommended to 
have this clock enabled.

We can cancel_calibration() in CSI only when csi/sensor stream on fails 
and in which case there will be no LP-11 so we can unconditionally 
disable MIPI_CLK.
Sowjanya Komatineni Aug. 5, 2020, 5:32 p.m. UTC | #11 
On 8/5/20 10:23 AM, Dmitry Osipenko wrote:
> 05.08.2020 20:04, Sowjanya Komatineni пишет:
>> On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 19:50, Sowjanya Komatineni пишет:
>>>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>>>>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni
>>>>>>>>> wrote:
>>>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>>>>> calibration
>>>>>>>>>> is done.
>>>>>>>>>>
>>>>>>>>>> So, this patch skips disabling MIPI clock after triggering
>>>>>>>>>> start of
>>>>>>>>>> calibration and disables it only after waiting for done status
>>>>>>>>>> from
>>>>>>>>>> the calibration logic.
>>>>>>>>>>
>>>>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>>>>> tegra_mipi_start_calibration()
>>>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>>>>> inline
>>>>>>>>>> with their usage.
>>>>>>>>>>
>>>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>>>>> input
>>>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>>>>> MIPI clock
>>>>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/gpu/drm/tegra/dsi.c |  4 ++--
>>>>>>>>>>      drivers/gpu/host1x/mipi.c   | 19 ++++++++++---------
>>>>>>>>>>      include/linux/host1x.h      |  5 +++--
>>>>>>>>>>      3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>>>>> tegra_dsi *dsi)
>>>>>>>>>>              DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>>>>>          tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>>>>>      -    err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>>>>> +    err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>>>>>          if (err < 0)
>>>>>>>>>>              return err;
>>>>>>>>>>      -    return tegra_mipi_wait(dsi->mipi);
>>>>>>>>>> +    return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>>>>>      }
>>>>>>>>>>        static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>>>>> unsigned long bclk,
>>>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>>>>> index e606464..b15ab6e 100644
>>>>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>>>>> tegra_mipi_device *dev)
>>>>>>>>>>      }
>>>>>>>>>>      EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>>>>>      -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>>>>> *device)
>>>>>>>>>> +{
>>>>>>>>>> +    clk_disable(device->mipi->clk);
>>>>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and
>>>>>>>>> MIPI_CAL_STATUS
>>>>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>>>>> Apparently there is no way to explicitly stop calibration other
>>>>>>>> than to
>>>>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>>>>
>>>>>>>> Perhaps having a fixed delay before disabling clock could be
>>>>>>>> enough to
>>>>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>>>>
>>>>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register.
>>>>>>> Maybe
>>>>>>> it needs to be polled until it's unset?
>>>>>> Confirmed with HW design team during this patch update.
>>>>>>
>>>>>> SW does not need to clear START bit and only write 1 takes effect to
>>>>>> that bit.
>>>>>>
>>>>>> Also, no need to have delay or do any other register settings
>>>>>> unclear as
>>>>>> its FSM and there's nothing to get stuck.
>>>>>>
>>>>>> Also it goes thru small finite set of codes and by the time sensor
>>>>>> programming happens for enabling streaming FSM will finish its
>>>>>> calibration sequence much early and it will only wait for pads LP-11.
>>>>>>
>>>>>> So, during cancel we only need disable MIPI clock.
>>>>>>
>>>>> But there is no guarantee that cancel_calibration() couldn't be invoked
>>>>> in the middle of the calibration process, hence FSM could freeze in an
>>>>> intermediate state if it's running on the disabled MIPI clock, this
>>>>> doesn't sound good.
>>>> Actual calibration logic uses UART_FST_CAL clock which is always enabled
>>> What enables the UART_FST_CAL clock? I don't see this clock used
>>> anywhere.
>> UART_FST_MIPI_CAL is shared with uart and is always enabled all the time.
>>
>> I don't see mipi driver handling this and I think that's because this
>> clock is enabled all the time as its used for UART as well. Probably
>> thierry can comment on this clock.
> It's not only the MIPI driver, the clock isn't defined at all neither in
> the clk driver, nor in clk DT bindings.
>
> It could be fragile to assume that it's always enabled.

 From SW clock architecture this clock is always kept programmed to 
68Mhz and enabled.

Its shared clock for UART and I see it enabled.

Thierry can comment more on where this clock is being handled? prior to 
kernel?

>
>> Also regarding cancel calibration, as FSM goes thru only finite sequence
>> codes by the time csi stream and sensor stream happens which is where we
>> check for calibration to complete for sure calibration will be finished
>> and calibration logic will only wait for pads to be in LP-11 to apply
>> results. So nothing special needed during cancel except to turn clock
>> off to balance its usage count.
> I guess it should be okay for the case of the VI driver, but
> in general please don't assume that code can't change in the future. The
> common API always should be made reliable for all possible situations.

Yes, There is no assumption here.

Just to be clear, UART_FST_MIPI_CAL is the clock used for calibration 
logic which is FSM that goes thru sequence codes and when done waits for 
pads to be in LP-11 to apply results.

MIPI_CLK is controller gate clock which is also need to be kept enabled 
as in case if it sees LP-11 it updates registers so its recommended to 
have this clock enabled during calibration process.

In case of CSI pads calibration, we call cancel_calibration() only when 
csi/sensor stream on fails and in which case there will be no LP-11 so 
we can unconditionally disable MIPI_CLK.
Dmitry Osipenko Aug. 5, 2020, 5:34 p.m. UTC | #12 
05.08.2020 20:29, Sowjanya Komatineni пишет:
...
> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
> that goes thru sequence codes and when done waits for pads to be in
> LP-11 to apply results.
> 
> MIPI_CLK is controller gate clock which is also need to be kept enabled
> as incase if it sees LP-11 it updates registers so its recommended to
> have this clock enabled.
> 
> We can cancel_calibration() in CSI only when csi/sensor stream on fails
> and in which case there will be no LP-11 so we can unconditionally
> disable MIPI_CLK.
> 

There is no guarantee that the fail comes before the LP-11. For example,
some odd camera driver may have a complicated enable sequence which may
fail after enabling the hardware streaming.
Sowjanya Komatineni Aug. 5, 2020, 5:46 p.m. UTC | #13 
On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
> 05.08.2020 20:29, Sowjanya Komatineni пишет:
> ...
>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>> that goes thru sequence codes and when done waits for pads to be in
>> LP-11 to apply results.
>>
>> MIPI_CLK is controller gate clock which is also need to be kept enabled
>> as incase if it sees LP-11 it updates registers so its recommended to
>> have this clock enabled.
>>
>> We can cancel_calibration() in CSI only when csi/sensor stream on fails
>> and in which case there will be no LP-11 so we can unconditionally
>> disable MIPI_CLK.
>>
> There is no guarantee that the fail comes before the LP-11. For example,
> some odd camera driver may have a complicated enable sequence which may
> fail after enabling the hardware streaming.

MIPI_CLK to keep enable is for calibration logic to update results, but 
like I said calibration logic uses UART_FST_MIPI_CAL clock. So even in 
case if fail happens from sensor after having pads in LP-11 then, 
calibration logic will still be running but result update will not 
happen with clock disabled. But HW will not stuck as this is confirmed 
from HW designer.
Sowjanya Komatineni Aug. 5, 2020, 6:06 p.m. UTC | #14 
On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>
> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>> ...
>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>> that goes thru sequence codes and when done waits for pads to be in
>>> LP-11 to apply results.
>>>
>>> MIPI_CLK is controller gate clock which is also need to be kept enabled
>>> as incase if it sees LP-11 it updates registers so its recommended to
>>> have this clock enabled.
>>>
>>> We can cancel_calibration() in CSI only when csi/sensor stream on fails
>>> and in which case there will be no LP-11 so we can unconditionally
>>> disable MIPI_CLK.
>>>
>> There is no guarantee that the fail comes before the LP-11. For example,
>> some odd camera driver may have a complicated enable sequence which may
>> fail after enabling the hardware streaming.
>
> MIPI_CLK to keep enable is for calibration logic to update results, 
> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So 
> even in case if fail happens from sensor after having pads in LP-11 
> then, calibration logic will still be running but result update will 
> not happen with clock disabled. But HW will not stuck as this is 
> confirmed from HW designer.

If LP-11 happens from sensor stream (followed by fail) and by that time 
if calibration FSM is done and if calibration logic sees LP-11 then 
results will be applied to pads.

We did start of calibration before CSI stream so by the time we do 
sensor stream enable, calibration logic might have done with FSM and 
waiting for LP-11

Also if we see any special case, we always can use finish_calibration() 
instead of cancel_calibration() as well.

finish_calibration() has extra 250ms wait time polling done bit and we 
can ignore its return code during fail pathway.

>
>
>
Sowjanya Komatineni Aug. 6, 2020, 12:47 a.m. UTC | #15 
On 8/5/20 11:06 AM, Sowjanya Komatineni wrote:
>
> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>>
>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>>> ...
>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>>> that goes thru sequence codes and when done waits for pads to be in
>>>> LP-11 to apply results.
>>>>
>>>> MIPI_CLK is controller gate clock which is also need to be kept 
>>>> enabled
>>>> as incase if it sees LP-11 it updates registers so its recommended to
>>>> have this clock enabled.
>>>>
>>>> We can cancel_calibration() in CSI only when csi/sensor stream on 
>>>> fails
>>>> and in which case there will be no LP-11 so we can unconditionally
>>>> disable MIPI_CLK.
>>>>
>>> There is no guarantee that the fail comes before the LP-11. For 
>>> example,
>>> some odd camera driver may have a complicated enable sequence which may
>>> fail after enabling the hardware streaming.
>>
>> MIPI_CLK to keep enable is for calibration logic to update results, 
>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So 
>> even in case if fail happens from sensor after having pads in LP-11 
>> then, calibration logic will still be running but result update will 
>> not happen with clock disabled. But HW will not stuck as this is 
>> confirmed from HW designer.
>
> If LP-11 happens from sensor stream (followed by fail) and by that 
> time if calibration FSM is done and if calibration logic sees LP-11 
> then results will be applied to pads.
>
> We did start of calibration before CSI stream so by the time we do 
> sensor stream enable, calibration logic might have done with FSM and 
> waiting for LP-11
>
> Also if we see any special case, we always can use 
> finish_calibration() instead of cancel_calibration() as well.
>
> finish_calibration() has extra 250ms wait time polling done bit and we 
> can ignore its return code during fail pathway.
>
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.

>>
>>
>>
Dmitry Osipenko Aug. 6, 2020, 1:32 p.m. UTC | #16 
06.08.2020 03:47, Sowjanya Komatineni пишет:
> 
> On 8/5/20 11:06 AM, Sowjanya Komatineni wrote:
>>
>> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>>>
>>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>>>> ...
>>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>>>> that goes thru sequence codes and when done waits for pads to be in
>>>>> LP-11 to apply results.
>>>>>
>>>>> MIPI_CLK is controller gate clock which is also need to be kept
>>>>> enabled
>>>>> as incase if it sees LP-11 it updates registers so its recommended to
>>>>> have this clock enabled.
>>>>>
>>>>> We can cancel_calibration() in CSI only when csi/sensor stream on
>>>>> fails
>>>>> and in which case there will be no LP-11 so we can unconditionally
>>>>> disable MIPI_CLK.
>>>>>
>>>> There is no guarantee that the fail comes before the LP-11. For
>>>> example,
>>>> some odd camera driver may have a complicated enable sequence which may
>>>> fail after enabling the hardware streaming.
>>>
>>> MIPI_CLK to keep enable is for calibration logic to update results,
>>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So
>>> even in case if fail happens from sensor after having pads in LP-11
>>> then, calibration logic will still be running but result update will
>>> not happen with clock disabled. But HW will not stuck as this is
>>> confirmed from HW designer.
>>
>> If LP-11 happens from sensor stream (followed by fail) and by that
>> time if calibration FSM is done and if calibration logic sees LP-11
>> then results will be applied to pads.
>>
>> We did start of calibration before CSI stream so by the time we do
>> sensor stream enable, calibration logic might have done with FSM and
>> waiting for LP-11
>>
>> Also if we see any special case, we always can use
>> finish_calibration() instead of cancel_calibration() as well.

Why not to do it right now?

Then the code could look like this:

src_subdev = tegra_channel_get_remote_source_subdev(chan);
ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
err = tegra_mipi_finish_calibration(csi_chan->mipi);

if (ret < 0 && ret != -ENOIOCTLCMD)
	goto err_disable_csi_stream;

if (err < 0)
	dev_warn(csi_chan->csi->dev,
		 "MIPI calibration failed: %d\n", err);

>> finish_calibration() has extra 250ms wait time polling done bit and we
>> can ignore its return code during fail pathway.
>>
> 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
Sowjanya Komatineni Aug. 6, 2020, 3:59 p.m. UTC | #17 
On 8/6/20 6:32 AM, Dmitry Osipenko wrote:
> 06.08.2020 03:47, Sowjanya Komatineni пишет:
>> On 8/5/20 11:06 AM, Sowjanya Komatineni wrote:
>>> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote:
>>>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote:
>>>>> 05.08.2020 20:29, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM
>>>>>> that goes thru sequence codes and when done waits for pads to be in
>>>>>> LP-11 to apply results.
>>>>>>
>>>>>> MIPI_CLK is controller gate clock which is also need to be kept
>>>>>> enabled
>>>>>> as incase if it sees LP-11 it updates registers so its recommended to
>>>>>> have this clock enabled.
>>>>>>
>>>>>> We can cancel_calibration() in CSI only when csi/sensor stream on
>>>>>> fails
>>>>>> and in which case there will be no LP-11 so we can unconditionally
>>>>>> disable MIPI_CLK.
>>>>>>
>>>>> There is no guarantee that the fail comes before the LP-11. For
>>>>> example,
>>>>> some odd camera driver may have a complicated enable sequence which may
>>>>> fail after enabling the hardware streaming.
>>>> MIPI_CLK to keep enable is for calibration logic to update results,
>>>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So
>>>> even in case if fail happens from sensor after having pads in LP-11
>>>> then, calibration logic will still be running but result update will
>>>> not happen with clock disabled. But HW will not stuck as this is
>>>> confirmed from HW designer.
>>> If LP-11 happens from sensor stream (followed by fail) and by that
>>> time if calibration FSM is done and if calibration logic sees LP-11
>>> then results will be applied to pads.
>>>
>>> We did start of calibration before CSI stream so by the time we do
>>> sensor stream enable, calibration logic might have done with FSM and
>>> waiting for LP-11
>>>
>>> Also if we see any special case, we always can use
>>> finish_calibration() instead of cancel_calibration() as well.
> Why not to do it right now?

> Then the code could look like this:
>
> src_subdev = tegra_channel_get_remote_source_subdev(chan);
> ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
> err = tegra_mipi_finish_calibration(csi_chan->mipi);
>
> if (ret < 0 && ret != -ENOIOCTLCMD)
> 	goto err_disable_csi_stream;
>
> if (err < 0)
> 	dev_warn(csi_chan->csi->dev,
> 		 "MIPI calibration failed: %d\n", err);
>
>>> finish_calibration() has extra 250ms wait time polling done bit and we
>>> can ignore its return code during fail pathway.
>>>
>> 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.

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.
Dmitry Osipenko Aug. 6, 2020, 4:10 p.m. UTC | #18 
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?

> 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?
Dmitry Osipenko Aug. 6, 2020, 4:13 p.m. UTC | #19 
06.08.2020 18:59, Sowjanya Komatineni пишет:
..
> We cant use active status check for specific pads under calibration.
> This is common bit for all pads.

I'm not sure why this is a problem.
Dmitry Osipenko Aug. 6, 2020, 4:37 p.m. UTC | #20 
06.08.2020 19:13, Dmitry Osipenko пишет:
> 06.08.2020 18:59, Sowjanya Komatineni пишет:
> ..
>> We cant use active status check for specific pads under calibration.
>> This is common bit for all pads.
> 
> I'm not sure why this is a problem.
> 

IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE
and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.
Sowjanya Komatineni Aug. 6, 2020, 4:41 p.m. UTC | #21 
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.
Sowjanya Komatineni Aug. 6, 2020, 4:42 p.m. UTC | #22 
On 8/6/20 9:37 AM, Dmitry Osipenko wrote:
> 06.08.2020 19:13, Dmitry Osipenko пишет:
>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>> ..
>>> We cant use active status check for specific pads under calibration.
>>> This is common bit for all pads.
>> I'm not sure why this is a problem.
>>
> IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE
> and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.

As soon as START bit it set, FSM will set ACTIVE = 1

There is no added advantage of waiting for ACTIVE to be in 
start_calibration()
Sowjanya Komatineni Aug. 6, 2020, 4:43 p.m. UTC | #23 
On 8/6/20 9:42 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 9:37 AM, Dmitry Osipenko wrote:
>> 06.08.2020 19:13, Dmitry Osipenko пишет:
>>> 06.08.2020 18:59, Sowjanya Komatineni пишет:
>>> ..
>>>> We cant use active status check for specific pads under calibration.
>>>> This is common bit for all pads.
>>> I'm not sure why this is a problem.
>>>
>> IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE
>> and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.
>
> As soon as START bit it set, FSM will set ACTIVE = 1
>
> There is no added advantage of waiting for ACTIVE to be in 
> start_calibration()
Also like I explained in other post of same discussion, ACTIVE we will 
be 1 even when other parallel pads are under calibration.
Dmitry Osipenko Aug. 6, 2020, 4:45 p.m. UTC | #24 
06.08.2020 19:41, Sowjanya Komatineni пишет:
...
>> 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.
> 

You said that calibration settings are applied to pads on LP-11, but if
LP-11 happens before FSM is finished, then what values will be applied
if any?
Sowjanya Komatineni Aug. 6, 2020, 4:51 p.m. UTC | #25 
On 8/6/20 9:45 AM, Dmitry Osipenko wrote:
> 06.08.2020 19:41, Sowjanya Komatineni пишет:
> ...
>>> 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.
>>
> You said that calibration settings are applied to pads on LP-11, but if
> LP-11 happens before FSM is finished, then what values will be applied
> if any?

No calibration logic will check for LP-11 only after finishing 
calibration sequence codes.

After that if it sees LP-11, it will apply results to pads and DONE bit 
will then be set to 1 indication pad results update.

Unfortunately like I said we don't have status indication for 
calibrating finished before waiting for LP-11.

ACTIVE bit is common for all PADS. If multiple 6 streams are happening 
in parallel, ACTIVE will be 1 as long as its calibrating any of the pads 
and its not for individual pads.
Sowjanya Komatineni Aug. 6, 2020, 5:12 p.m. UTC | #26 
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.

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.
Dmitry Osipenko Aug. 6, 2020, 5:15 p.m. UTC | #27 
06.08.2020 19:51, Sowjanya Komatineni пишет:
>>>> 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.
>>>
>> You said that calibration settings are applied to pads on LP-11, but if
>> LP-11 happens before FSM is finished, then what values will be applied
>> if any?
> 
> No calibration logic will check for LP-11 only after finishing
> calibration sequence codes.
> 
> After that if it sees LP-11, it will apply results to pads and DONE bit
> will then be set to 1 indication pad results update.

Are you sure that HW doesn't use level-triggered logic for LP-11 signal?

> Unfortunately like I said we don't have status indication for
> calibrating finished before waiting for LP-11.

This is not a problem if hardware can cope with LP-11 happened at the
time of calibration. If hardware can't cope with that, then somethings
needs to be done about it.
Dmitry Osipenko Aug. 6, 2020, 5:27 p.m. UTC | #28 
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.

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);


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.
Sowjanya Komatineni Aug. 6, 2020, 5:44 p.m. UTC | #29 
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.

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
Sowjanya Komatineni Aug. 6, 2020, 5:52 p.m. UTC | #30 
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
>
>
>
Dmitry Osipenko Aug. 6, 2020, 6:01 p.m. UTC | #31 
06.08.2020 20:52, Sowjanya Komatineni пишет:
...
> 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

I think only the DONE bits which correspond to the mipi_device->pads
bitmask should be awaited.

> - mutex_unlock()

Perhaps the start_calibration() also needs to be changed to not touch
the MIPI_CAL_CONFIG bits of the unrelated pads?

Otherwise sounds good to me.
Sowjanya Komatineni Aug. 6, 2020, 6:07 p.m. UTC | #32 
On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
> 06.08.2020 20:52, Sowjanya Komatineni пишет:
> ...
>> 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
> I think only the DONE bits which correspond to the mipi_device->pads
> bitmask should be awaited.

As next START can't be triggered when auto cal is ACTIVE, we should keep 
this in finish.

As we do mutex_unlock only at end of finish, other pads calibrations 
dont go thru till the one in process is finished.

So in this case ACTIVE applies to current selected pads that are under 
calibration.

>
>> - mutex_unlock()
> Perhaps the start_calibration() also needs to be changed to not touch
> the MIPI_CAL_CONFIG bits of the unrelated pads?
Driver already takes care of programming corresponding pads config only.
>
> Otherwise sounds good to me.
Dmitry Osipenko Aug. 6, 2020, 6:18 p.m. UTC | #33 
06.08.2020 21:07, Sowjanya Komatineni пишет:
> 
> On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
>> 06.08.2020 20:52, Sowjanya Komatineni пишет:
>> ...
>>> 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
>> I think only the DONE bits which correspond to the mipi_device->pads
>> bitmask should be awaited.
> 
> As next START can't be triggered when auto cal is ACTIVE, we should keep
> this in finish.
> 
> As we do mutex_unlock only at end of finish, other pads calibrations
> dont go thru till the one in process is finished.
> 
> So in this case ACTIVE applies to current selected pads that are under
> calibration.

Should be better to check only the relevant bits in order to catch bugs,
otherwise you may get a DONE status from the irrelevant pads.

>>> - mutex_unlock()
>> Perhaps the start_calibration() also needs to be changed to not touch
>> the MIPI_CAL_CONFIG bits of the unrelated pads?
> Driver already takes care of programming corresponding pads config only.

It writes 0 to the config of the unrelated pads, which probably isn't
nice if some pads use periodic auto-calibration.

https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350

Although looks like auto-calibration isn't supported by the current driver.
Sowjanya Komatineni Aug. 6, 2020, 6:44 p.m. UTC | #34 
On 8/6/20 11:18 AM, Dmitry Osipenko wrote:
> 06.08.2020 21:07, Sowjanya Komatineni пишет:
>> On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
>>> 06.08.2020 20:52, Sowjanya Komatineni пишет:
>>> ...
>>>> 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
>>> I think only the DONE bits which correspond to the mipi_device->pads
>>> bitmask should be awaited.
>> As next START can't be triggered when auto cal is ACTIVE, we should keep
>> this in finish.
>>
>> As we do mutex_unlock only at end of finish, other pads calibrations
>> dont go thru till the one in process is finished.
>>
>> So in this case ACTIVE applies to current selected pads that are under
>> calibration.
> Should be better to check only the relevant bits in order to catch bugs,
> otherwise you may get a DONE status from the irrelevant pads.
tegra_mipi_device is separate for DSI and CSI channels. mutex lock used 
during calibrate is device specific lock.
So, it will not prevent other devices to hold till on going calibration 
is done unless we add wait for active bit before triggering start.

Currently we wait for active bit at end during calibration done check 
after start trigger. But when other devices go thru calibration in 
parallel as lock is device specific and not common lock for all devices 
it will trigger start but MIPI calibration logic ignore if previous 
calibration is still in progress.

Need to serialize calibration start requests from different devices 
based on ACTIVE bit.

>>> Perhaps the start_calibration() also needs to be changed to not touch
>>> the MIPI_CAL_CONFIG bits of the unrelated pads?
>> Driver already takes care of programming corresponding pads config only.
> It writes 0 to the config of the unrelated pads, which probably isn't
> nice if some pads use periodic auto-calibration.
>
> https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350
>
> Although looks like auto-calibration isn't supported by the current driver.

Yes we don't use auto-calibration.

Only common bit here is MIPI_CAL_CTRL start. All others are pad specific 
currently.
Sowjanya Komatineni Aug. 6, 2020, 6:51 p.m. UTC | #35 
On 8/6/20 11:44 AM, Sowjanya Komatineni wrote:
>
> On 8/6/20 11:18 AM, Dmitry Osipenko wrote:
>> 06.08.2020 21:07, Sowjanya Komatineni пишет:
>>> On 8/6/20 11:01 AM, Dmitry Osipenko wrote:
>>>> 06.08.2020 20:52, Sowjanya Komatineni пишет:
>>>> ...
>>>>> 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
>>>> I think only the DONE bits which correspond to the mipi_device->pads
>>>> bitmask should be awaited.
>>> As next START can't be triggered when auto cal is ACTIVE, we should 
>>> keep
>>> this in finish.
>>>
>>> As we do mutex_unlock only at end of finish, other pads calibrations
>>> dont go thru till the one in process is finished.
>>>
>>> So in this case ACTIVE applies to current selected pads that are under
>>> calibration.
>> Should be better to check only the relevant bits in order to catch bugs,
>> otherwise you may get a DONE status from the irrelevant pads.
> tegra_mipi_device is separate for DSI and CSI channels. mutex lock 
> used during calibrate is device specific lock.
> So, it will not prevent other devices to hold till on going 
> calibration is done unless we add wait for active bit before 
> triggering start.
>
> Currently we wait for active bit at end during calibration done check 
> after start trigger. But when other devices go thru calibration in 
> parallel as lock is device specific and not common lock for all 
> devices it will trigger start but MIPI calibration logic ignore if 
> previous calibration is still in progress.
>
> Need to serialize calibration start requests from different devices 
> based on ACTIVE bit.

Sorry confused. MIPI driver is using common lock from tegra_mipi for all 
tegra_mipi_device

So should be ok as only one device start_calibration can happen at a time.

But its still good to check ACTIVE is cleared and to report as error if 
not as ACTIVE will be cleared once set of pads enabled for this calibration.

Eg: CSI port can be 4 lane and all 4 pads gets enabled at same time and 
with this ACTIVE should still be verified to be 0 to make sure all pads 
calibration is done

>
>>>> Perhaps the start_calibration() also needs to be changed to not touch
>>>> the MIPI_CAL_CONFIG bits of the unrelated pads?
>>> Driver already takes care of programming corresponding pads config 
>>> only.
>> It writes 0 to the config of the unrelated pads, which probably isn't
>> nice if some pads use periodic auto-calibration.
>>
>> https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350 
>>
>>
>> Although looks like auto-calibration isn't supported by the current 
>> driver.
>
> Yes we don't use auto-calibration.
>
> Only common bit here is MIPI_CAL_CTRL start. All others are pad 
> specific currently.
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 3820e8d..a7864e9 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -694,11 +694,11 @@  static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi)
 		DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
 	tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
 
-	err = tegra_mipi_calibrate(dsi->mipi);
+	err = tegra_mipi_start_calibration(dsi->mipi);
 	if (err < 0)
 		return err;
 
-	return tegra_mipi_wait(dsi->mipi);
+	return tegra_mipi_finish_calibration(dsi->mipi);
 }
 
 static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index e606464..b15ab6e 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -293,17 +293,19 @@  int tegra_mipi_disable(struct tegra_mipi_device *dev)
 }
 EXPORT_SYMBOL(tegra_mipi_disable);
 
-int tegra_mipi_wait(struct tegra_mipi_device *device)
+void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
+{
+	clk_disable(device->mipi->clk);
+}
+EXPORT_SYMBOL(tegra_mipi_cancel_calibration);
+
+int tegra_mipi_finish_calibration(struct tegra_mipi_device *device)
 {
 	struct tegra_mipi *mipi = device->mipi;
 	void __iomem *status_reg = mipi->regs + (MIPI_CAL_STATUS << 2);
 	u32 value;
 	int err;
 
-	err = clk_enable(device->mipi->clk);
-	if (err < 0)
-		return err;
-
 	mutex_lock(&device->mipi->lock);
 
 	err = readl_relaxed_poll_timeout(status_reg, value,
@@ -315,9 +317,9 @@  int tegra_mipi_wait(struct tegra_mipi_device *device)
 
 	return err;
 }
-EXPORT_SYMBOL(tegra_mipi_wait);
+EXPORT_SYMBOL(tegra_mipi_finish_calibration);
 
-int tegra_mipi_calibrate(struct tegra_mipi_device *device)
+int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
 {
 	const struct tegra_mipi_soc *soc = device->mipi->soc;
 	unsigned int i;
@@ -382,11 +384,10 @@  int tegra_mipi_calibrate(struct tegra_mipi_device *device)
 	tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL);
 
 	mutex_unlock(&device->mipi->lock);
-	clk_disable(device->mipi->clk);
 
 	return 0;
 }
-EXPORT_SYMBOL(tegra_mipi_calibrate);
+EXPORT_SYMBOL(tegra_mipi_start_calibration);
 
 static const struct tegra_mipi_pad tegra114_mipi_pads[] = {
 	{ .data = MIPI_CAL_CONFIG_CSIA },
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 20c885d..b490dda 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -333,7 +333,8 @@  struct tegra_mipi_device *tegra_mipi_request(struct device *device,
 void tegra_mipi_free(struct tegra_mipi_device *device);
 int tegra_mipi_enable(struct tegra_mipi_device *device);
 int tegra_mipi_disable(struct tegra_mipi_device *device);
-int tegra_mipi_calibrate(struct tegra_mipi_device *device);
-int tegra_mipi_wait(struct tegra_mipi_device *device);
+int tegra_mipi_start_calibration(struct tegra_mipi_device *device);
+int tegra_mipi_finish_calibration(struct tegra_mipi_device *device);
+void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device);
 
 #endif