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