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 <[email protected]>
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
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);
+}
+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
--
2.7.4
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 <[email protected]>
>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>> ---
>>> 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?
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 <[email protected]>
>>>>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>>>>> ---
>>>>>>>> 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.
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 <[email protected]>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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.
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 <[email protected]>
>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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.
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.
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.
>
>
>
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.
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 <[email protected]>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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.
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 <[email protected]>
>>>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>>>> ---
>>>>>>> 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.
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 <[email protected]>
>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>> ---
>>>>> 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.
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 <[email protected]>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
> ---
> 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
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 <[email protected]>
>>>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>>>> ---
>>>>>> 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
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 <[email protected]>
>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>> ---
>> 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?
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 <[email protected]>
>>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>>> ---
>>>> 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.
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.
>>
>>
>>
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.
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?
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()
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.
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.
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.
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?
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.
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.
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.
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
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.
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
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.
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
>
>
>
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.
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.
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.
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.
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.
>