On Fri, Aug 12, 2016 at 04:02:57PM +0100, Jon Hunter wrote:
>
> On 12/08/16 14:46, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Tue, Aug 02, 2016 at 11:34:26AM +0100, Jon Hunter wrote:
> >> Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the
> >> display power partition when probing and this causes the Tegra114
> >> Dalmore to hang during boot.
> >>
> >> The hang occurs when accessing the MIPI calibration registers (which are
> >> accessed during the configuration of the DSI interface). Ideally the
> >> MIPI driver should manage the power partition itself to ensure it is on
> >> when needed. The problem is that the legacy PMC APIs used for managing
> >> the power partitions do not support reference counting and so this
> >> cannot be easily done currently. Long-term we will migrate devices to
> >> use generic PM domains and such scenarios will be easy to support. For
> >> now fix this by removing the code to turn off the display power
> >> partition when probing the DC and always keep the DC on so that the
> >> power partition is not turned off. This is consistent with how the power
> >> partition was managed prior to this commit.
> >>
> >> Please note that for earlier devices such as Tegra114 the MIPI
> >> calibration logic is part of the display power partition, where as for
> >> newer devices, such as Tegra124/210 it is part of the SOR power
> >> partition. Hence, in the long-term is makes more sense to handle such
> >> power partitions via the generic PM domain framework.
> >>
> >> Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")
> >>
> >> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> >> ---
> >>
> >> Please note that the hang is only seen on Tegra114 with v4.8 if the
> >> patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd
> >> patch in series) is applied without this patch. Without the fix for the
> >> PMIC interrupt polarity the Palmas PMIC probe fails and the display
> >> probe also fails because the regulators are not found.
> >>
> >> drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++-------
> >> 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > I don't think this fixes the problem at the root. After looking at the
> > code I think what you're seeing is caused by the tegra_mipi_power_up()
> > call that happens as part of the tegra_mipi_request() from the DSI
> > driver's ->probe().
>
> No it doesn't. It is more of a bandaid. I think that this issue has
> always been there but no exposed until the move to RPM for the DC. I
> can't say I was too happy with it!
>
> > Generally there shouldn't be a problem because the display controller
> > will always get enabled before the encoder (DSI) and hence the power
> > partition should be enabled when the actual calibration happens. The
> > fundamental problem in this case is that we're actually powering up
> > the MIPI calibration logic at the wrong time. So I think what we'll
> > want for a proper fix is to move all register accesses out of the
> > tegra_mipi_request() function and add tegra_mipi_enable() and
> > tegra_mipi_disable() functions that power up and power down the MIPI
> > calibration logic, respectively. That way we can move all the code
> > which relies on the power partition into the tegra_dsi_encoder_enable()
> > and tegra_dsi_encoder_disable() functions.
> >
> > Perhaps an even better place to call these new functions from would be
> > the DSI driver's ->suspend() and ->resume() functions.
> >
> > An added benefit of this will be that the MIPI calibration logic could
> > be powered off when DSI is disabled (provided no other user requires it
> > to be powered on), whereas currently it will remain powered even if the
> > DSI output is off, since the power on happens in ->probe().
> >
> > I'll go write a patch.
>
> Great! Thanks.
I was going to send this out, but EIMT reports that it's still failing.
I'll have to dig out the Dalmore and see what's going on.
Thierry
--- >8 ---
From 325ffd78615e76f04b5225fc1e79921c32037c1f Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@xxxxxxxxxx>
Date: Fri, 12 Aug 2016 16:00:53 +0200
Subject: [PATCH] drm/tegra: dsi: Enhance runtime power management
The MIPI DSI output on Tegra SoCs requires some external logic to
calibrate the MIPI pads before a video signal can be transmitted. This
MIPI calibration logic requires to be powered on while the MIPI pads are
being used, which is currently done as part of the DSI driver's probe
implementation.
This is suboptimal because it will leave the MIPI calibration logic
powered up even if the DSI output is never used.
On Tegra114 and earlier this behaviour also causes the driver to hang
while trying to power up the MIPI calibration logic because the power
partition that contains the MIPI calibration logic will be powered on
by the display controller at output pipeline configuration time. Thus
the power up sequence for the MIPI calibration logic happens before
it's power partition is guaranteed to be enabled.
Fix this by splitting up the API into a request/free pair of functions
that manage the runtime dependency between the DSI and the calibration
modules (no registers are accessed) and a set of enable, calibrate and
disable functions that program the MIPI calibration logic at points in
time where the power partition is really enabled.
Reported-by: Jonathan Hunter <jonathanh@xxxxxxxxxx>
Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
---
drivers/gpu/drm/tegra/dsi.c | 14 ++++++++++
drivers/gpu/host1x/mipi.c | 63 ++++++++++++++++++++++-----------------------
include/linux/host1x.h | 2 ++
3 files changed, 47 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 3d228ad90e0f..39a34d1c4d6e 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -1593,6 +1593,12 @@ static int tegra_dsi_suspend(struct device *dev)
struct tegra_dsi *dsi = dev_get_drvdata(dev);
int err;
+ err = tegra_mipi_disable(dsi->mipi);
+ if (err < 0) {
+ dev_err(dev, "failed to disable MIPI calibration: %d\n", err);
+ return err;
+ }
+
if (dsi->rst) {
err = reset_control_assert(dsi->rst);
if (err < 0) {
@@ -1644,8 +1650,16 @@ static int tegra_dsi_resume(struct device *dev)
}
}
+ err = tegra_mipi_enable(dsi->mipi);
+ if (err < 0) {
+ dev_err(dev, "failed to enable MIPI calibration: %d\n", err);
+ goto assert_reset;
+ }
+
return 0;
+assert_reset:
+ reset_control_assert(dsi->rst);
disable_clk_lp:
clk_disable_unprepare(dsi->clk_lp);
disable_clk:
diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index 52a6fd224127..e00809d996a2 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -242,20 +242,6 @@ struct tegra_mipi_device *tegra_mipi_request(struct device *device)
dev->pads = args.args[0];
dev->device = device;
- mutex_lock(&dev->mipi->lock);
-
- if (dev->mipi->usage_count++ == 0) {
- err = tegra_mipi_power_up(dev->mipi);
- if (err < 0) {
- dev_err(dev->mipi->dev,
- "failed to power up MIPI bricks: %d\n",
- err);
- return ERR_PTR(err);
- }
- }
-
- mutex_unlock(&dev->mipi->lock);
-
return dev;
put:
@@ -270,29 +256,42 @@ EXPORT_SYMBOL(tegra_mipi_request);
void tegra_mipi_free(struct tegra_mipi_device *device)
{
- int err;
+ platform_device_put(device->pdev);
+ kfree(device);
+}
+EXPORT_SYMBOL(tegra_mipi_free);
- mutex_lock(&device->mipi->lock);
+int tegra_mipi_enable(struct tegra_mipi_device *dev)
+{
+ int err = 0;
- if (--device->mipi->usage_count == 0) {
- err = tegra_mipi_power_down(device->mipi);
- if (err < 0) {
- /*
- * Not much that can be done here, so an error message
- * will have to do.
- */
- dev_err(device->mipi->dev,
- "failed to power down MIPI bricks: %d\n",
- err);
- }
- }
+ mutex_lock(&dev->mipi->lock);
- mutex_unlock(&device->mipi->lock);
+ if (dev->mipi->usage_count++ == 0)
+ err = tegra_mipi_power_up(dev->mipi);
+
+ mutex_unlock(&dev->mipi->lock);
+
+ return err;
- platform_device_put(device->pdev);
- kfree(device);
}
-EXPORT_SYMBOL(tegra_mipi_free);
+EXPORT_SYMBOL(tegra_mipi_enable);
+
+int tegra_mipi_disable(struct tegra_mipi_device *dev)
+{
+ int err = 0;
+
+ mutex_lock(&dev->mipi->lock);
+
+ if (--dev->mipi->usage_count == 0)
+ err = tegra_mipi_power_down(dev->mipi);
+
+ mutex_unlock(&dev->mipi->lock);
+
+ return err;
+
+}
+EXPORT_SYMBOL(tegra_mipi_disable);
static int tegra_mipi_wait(struct tegra_mipi *mipi)
{
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index d2ba7d334039..1ffbf2a8cb99 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -304,6 +304,8 @@ struct tegra_mipi_device;
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);
#endif
--
2.9.0
Attachment:
signature.asc
Description: PGP signature
