From: Samuel Holland <samuel@sholland.org>
To: Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
dri-devel@lists.freedesktop.org
Cc: stable@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Samuel Holland <samuel@sholland.org>
Subject: [PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM
Date: Tue, 11 Feb 2020 01:28:58 -0600 [thread overview]
Message-ID: <20200211072858.30784-4-samuel@sholland.org> (raw)
In-Reply-To: <20200211072858.30784-1-samuel@sholland.org>
The driver currently uses runtime PM to perform some of the module
initialization and cleanup. This has three problems:
1) There is no Kconfig dependency on CONFIG_PM, so if runtime PM is
disabled, the driver will not work at all, since the module will
never be initialized.
2) The driver does not ensure that the device is suspended when
sun6i_dsi_probe() fails or when sun6i_dsi_remove() is called. It
simply disables runtime PM. From the docs of pm_runtime_disable():
The device can be either active or suspended after its runtime PM
has been disabled.
And indeed, the device will likely still be active if sun6i_dsi_probe
fails. For example, if the panel driver is not yet loaded, we have
the following sequence:
sun6i_dsi_probe()
pm_runtime_enable()
mipi_dsi_host_register()
of_mipi_dsi_device_add(child)
...device_add()...
__device_attach()
pm_runtime_get_sync(dev->parent) -> Causes resume
bus_for_each_drv()
__device_attach_driver() -> No match for panel
pm_runtime_put(dev->parent) -> Async idle request
component_add()
__component_add()
try_to_bring_up_masters()
try_to_bring_up_master()
sun4i_drv_bind()
component_bind_all()
component_bind()
sun6i_dsi_bind() -> Fails with -EPROBE_DEFER
mipi_dsi_host_unregister()
pm_runtime_disable()
__pm_runtime_disable()
__pm_runtime_barrier() -> Idle request is still pending
cancel_work_sync() -> DSI host is *not* suspended!
Since the device is not suspended, the clock and regulator are never
disabled. The imbalance causes a WARN at devres free time.
3) The driver relies on being suspended when sun6i_dsi_encoder_enable()
is called. The resume callback has a comment that says:
Some part of it can only be done once we get a number of
lanes, see sun6i_dsi_inst_init
And then part of the resume callback only runs if dsi->device is not
NULL (that is, if sun6i_dsi_attach() has been called). However, as
the above call graph shows, the resume callback is guaranteed to be
called before sun6i_dsi_attach(); it is called before child devices
get their drivers attached.
Therefore, part of the controller initialization will only run if the
device is suspended between the calls to mipi_dsi_host_register() and
component_add() (which ends up calling sun6i_dsi_encoder_enable()).
Again, as shown by the above call graph, this is not the case. It
appears that the controller happens to work because it is still
initialized by the bootloader.
Because the connector is hardcoded to always be connected, the
device's runtime PM reference is not dropped until system suspend,
when sun4i_drv_drm_sys_suspend() ends up calling
sun6i_dsi_encoder_disable(). However, that is done as a system sleep
PM hook, and at that point the system PM core has already taken
another runtime PM reference, so sun6i_dsi_runtime_suspend() is
not called. Likewise, by the time the PM core releases its reference,
sun4i_drv_drm_sys_resume() has already re-enabled the encoder.
So after system suspend and resume, we have *still never called*
sun6i_dsi_inst_init(), and now that the rest of the display pipeline
has been reset, the DSI host is unable to communicate with the panel,
causing VBLANK timeouts.
Fix all of these issues by inlining the runtime PM hooks into the
encoder enable/disable functions, which are guaranteed to run after a
panel is attached. This allows sun6i_dsi_inst_init() to be called
unconditionally. Furthermore, this causes the hardware to be turned off
during system suspend and reinitialized on resume, which was not
happening before.
Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 89 ++++++++------------------
1 file changed, 26 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index ef35ce5a9bb0..8c2e326d8e16 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -14,7 +14,6 @@
#include <linux/phy/phy-mipi-dphy.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
@@ -721,10 +720,31 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
union phy_configure_opts opts = { 0 };
struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
u16 delay;
+ int err;
DRM_DEBUG_DRIVER("Enabling DSI output\n");
- pm_runtime_get_sync(dsi->dev);
+ err = regulator_enable(dsi->regulator);
+ if (err)
+ dev_warn(dsi->dev, "failed to enable VCC-DSI supply: %d\n", err);
+
+ reset_control_deassert(dsi->reset);
+ clk_prepare_enable(dsi->mod_clk);
+
+ /*
+ * Enable the DSI block.
+ */
+ regmap_write(dsi->regs, SUN6I_DSI_CTL_REG, SUN6I_DSI_CTL_EN);
+
+ regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL0_REG,
+ SUN6I_DSI_BASIC_CTL0_ECC_EN | SUN6I_DSI_BASIC_CTL0_CRC_EN);
+
+ regmap_write(dsi->regs, SUN6I_DSI_TRANS_START_REG, 10);
+ regmap_write(dsi->regs, SUN6I_DSI_TRANS_ZERO_REG, 0);
+
+ sun6i_dsi_inst_init(dsi, dsi->device);
+
+ regmap_write(dsi->regs, SUN6I_DSI_DEBUG_DATA_REG, 0xff);
delay = sun6i_dsi_get_video_start_delay(dsi, mode);
regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL1_REG,
@@ -787,7 +807,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
phy_power_off(dsi->dphy);
phy_exit(dsi->dphy);
- pm_runtime_put(dsi->dev);
+ clk_disable_unprepare(dsi->mod_clk);
+ reset_control_assert(dsi->reset);
+ regulator_disable(dsi->regulator);
}
static int sun6i_dsi_get_modes(struct drm_connector *connector)
@@ -1147,12 +1169,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
goto err_unprotect_clk;
}
- pm_runtime_enable(dev);
-
ret = mipi_dsi_host_register(&dsi->host);
if (ret) {
dev_err(dev, "Couldn't register MIPI-DSI host\n");
- goto err_pm_disable;
+ goto err_unprotect_clk;
}
ret = component_add(&pdev->dev, &sun6i_dsi_ops);
@@ -1165,8 +1185,6 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
err_remove_dsi_host:
mipi_dsi_host_unregister(&dsi->host);
-err_pm_disable:
- pm_runtime_disable(dev);
err_unprotect_clk:
clk_rate_exclusive_put(dsi->mod_clk);
return ret;
@@ -1179,65 +1197,11 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
component_del(&pdev->dev, &sun6i_dsi_ops);
mipi_dsi_host_unregister(&dsi->host);
- pm_runtime_disable(dev);
clk_rate_exclusive_put(dsi->mod_clk);
return 0;
}
-static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev)
-{
- struct sun6i_dsi *dsi = dev_get_drvdata(dev);
- int err;
-
- err = regulator_enable(dsi->regulator);
- if (err) {
- dev_err(dsi->dev, "failed to enable VCC-DSI supply: %d\n", err);
- return err;
- }
-
- reset_control_deassert(dsi->reset);
- clk_prepare_enable(dsi->mod_clk);
-
- /*
- * Enable the DSI block.
- *
- * Some part of it can only be done once we get a number of
- * lanes, see sun6i_dsi_inst_init
- */
- regmap_write(dsi->regs, SUN6I_DSI_CTL_REG, SUN6I_DSI_CTL_EN);
-
- regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL0_REG,
- SUN6I_DSI_BASIC_CTL0_ECC_EN | SUN6I_DSI_BASIC_CTL0_CRC_EN);
-
- regmap_write(dsi->regs, SUN6I_DSI_TRANS_START_REG, 10);
- regmap_write(dsi->regs, SUN6I_DSI_TRANS_ZERO_REG, 0);
-
- if (dsi->device)
- sun6i_dsi_inst_init(dsi, dsi->device);
-
- regmap_write(dsi->regs, SUN6I_DSI_DEBUG_DATA_REG, 0xff);
-
- return 0;
-}
-
-static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev)
-{
- struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-
- clk_disable_unprepare(dsi->mod_clk);
- reset_control_assert(dsi->reset);
- regulator_disable(dsi->regulator);
-
- return 0;
-}
-
-static const struct dev_pm_ops sun6i_dsi_pm_ops = {
- SET_RUNTIME_PM_OPS(sun6i_dsi_runtime_suspend,
- sun6i_dsi_runtime_resume,
- NULL)
-};
-
static const struct of_device_id sun6i_dsi_of_table[] = {
{ .compatible = "allwinner,sun6i-a31-mipi-dsi" },
{ }
@@ -1250,7 +1214,6 @@ static struct platform_driver sun6i_dsi_platform_driver = {
.driver = {
.name = "sun6i-mipi-dsi",
.of_match_table = sun6i_dsi_of_table,
- .pm = &sun6i_dsi_pm_ops,
},
};
module_platform_driver(sun6i_dsi_platform_driver);
--
2.24.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-02-11 7:29 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-11 7:28 [PATCH 1/4] drm/sun4i: dsi: Remove unused drv from driver context Samuel Holland
2020-02-11 7:28 ` [PATCH 2/4] drm/sun4i: dsi: Use NULL to signify "no panel" Samuel Holland
2020-02-11 7:28 ` [PATCH 3/4] drm/sun4i: dsi: Allow binding the host without a panel Samuel Holland
2020-02-15 2:24 ` Samuel Holland
2020-02-11 7:28 ` Samuel Holland [this message]
2020-02-11 8:26 ` [PATCH 4/4] drm/sun4i: dsi: Remove incorrect use of runtime PM Maxime Ripard
2020-02-11 15:39 ` Samuel Holland
2020-02-15 2:37 ` Samuel Holland
2020-02-14 15:39 ` Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200211072858.30784-4-samuel@sholland.org \
--to=samuel@sholland.org \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mripard@kernel.org \
--cc=stable@vger.kernel.org \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).