Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
From: Sowjanya Komatineni
Date: Wed Aug 05 2020 - 13:36:39 EST
On 8/5/20 10:23 AM, Dmitry Osipenko wrote:
05.08.2020 20:04, Sowjanya Komatineni пишет:
On 8/5/20 9:57 AM, Dmitry Osipenko wrote:
05.08.2020 19:50, Sowjanya Komatineni пишет:
On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
05.08.2020 19:33, Sowjanya Komatineni пишет:
On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
05.08.2020 17:05, Dmitry Osipenko пишет:
05.08.2020 16:46, Thierry Reding пишет:
On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni
wrote:
With the split of MIPI calibration into tegra_mipi_calibrate() and
tegra_mipi_wait(), MIPI clock is not kept enabled till the
calibration
is done.
So, this patch skips disabling MIPI clock after triggering
start of
calibration and disables it only after waiting for done status
from
the calibration logic.
This patch renames tegra_mipi_calibrate() as
tegra_mipi_start_calibration()
and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
inline
with their usage.
As MIPI clock is left enabled and in case of any failures with CSI
input
streaming tegra_mipi_finish_calibration() will not get invoked.
So added new API tegra_mipi_cancel_calibration() which disables
MIPI clock
and consumer drivers can call this in such cases.
Reviewed-by: Dmitry Osipenko <digetx@xxxxxxxxx>
Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
---
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.