Re: (EXT) [PATCH] drm: mxsfb: Simplify LCDIF clock handling
Hi Marek,
I like the overall idea. Thanks for the effort.
Am Sonntag, 6. Februar 2022, 19:55:55 CET schrieb Marek Vasut:
> The current clock handling in the LCDIF driver is a convoluted mess.
> Implement runtime PM ops which turn the clock ON and OFF and let the
> pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable
> and .atomic_disable callbacks turn the clock ON and OFF at the right
> time.
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 85 ++-
> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 18 ++-
> 2 files changed, 54 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 375f26d4a4172..4ff3c6195dd0c
> 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -72,18 +72,6 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
> },
> };
>
> -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb)
> -{
> - if (mxsfb->clk_axi)
> - clk_prepare_enable(mxsfb->clk_axi);
> -}
> -
> -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb)
> -{
> - if (mxsfb->clk_axi)
> - clk_disable_unprepare(mxsfb->clk_axi);
> -}
> -
The declarations for mxsfb_enable_axi_clk() and mxsfb_disable_axi_clk() are
still in drivers/gpu/drm/mxsfb/mxsfb_drv.h. Please remove them as well.
You will then notice that they are still used at some places.
> static struct drm_framebuffer *
> mxsfb_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> const struct drm_mode_fb_cmd2 *mode_cmd)
> @@ -224,33 +212,31 @@ static int mxsfb_load(struct drm_device *drm,
> if (IS_ERR(mxsfb->clk))
> return PTR_ERR(mxsfb->clk);
>
> - mxsfb->clk_axi = devm_clk_get(drm->dev, "axi");
> + mxsfb->clk_axi = devm_clk_get_optional(drm->dev, "axi");
> if (IS_ERR(mxsfb->clk_axi))
> - mxsfb->clk_axi = NULL;
> + return PTR_ERR(mxsfb->clk_axi);
>
> - mxsfb->clk_disp_axi = devm_clk_get(drm->dev, "disp_axi");
> + mxsfb->clk_disp_axi = devm_clk_get_optional(drm->dev, "disp_axi");
> if (IS_ERR(mxsfb->clk_disp_axi))
> - mxsfb->clk_disp_axi = NULL;
> + return PTR_ERR(mxsfb->clk_disp_axi);
>
> ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
> if (ret)
> return ret;
>
> - pm_runtime_enable(drm->dev);
> -
> /* Modeset init */
> drm_mode_config_init(drm);
>
> ret = mxsfb_kms_init(mxsfb);
> if (ret < 0) {
> dev_err(drm->dev, "Failed to initialize KMS
pipeline\n");
> - goto err_vblank;
> + return ret;
> }
>
> ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> if (ret < 0) {
> dev_err(drm->dev, "Failed to initialise vblank\n");
> - goto err_vblank;
> + return ret;
> }
>
> /* Start with vertical blanking interrupt reporting disabled. */
> @@ -260,7 +246,7 @@ static int mxsfb_load(struct drm_device *drm,
> if (ret) {
> if (ret != -EPROBE_DEFER)
> dev_err(drm->dev, "Cannot connect bridge:
%d\n", ret);
> - goto err_vblank;
> + return ret;
> }
>
> drm->mode_config.min_width = MXSFB_MIN_XRES;
> @@ -277,13 +263,10 @@ static int mxsfb_load(struct drm_device *drm,
> goto err_vblank;
You are still using err_vblank here which gets removed below.
Alexander
> mxsfb->irq = ret;
>
> - pm_runtime_get_sync(drm->dev);
> ret = mxsfb_irq_install(drm, mxsfb->irq);
> - pm_runtime_put_sync(drm->dev);
> -
> if (ret < 0) {
> dev_err(drm->dev, "Failed to install IRQ handler\n");
> - goto err_vblank;
> + return ret;
> }
>
> drm_kms_helper_poll_init(drm);
> @@ -292,12 +275,9 @@ static int mxsfb_load(struct drm_device *drm,
>
> drm_helper_hpd_irq_event(drm);
>
> - return 0;
> -
> -err_vblank:
> - pm_runtime_disable(drm->dev);
> + pm_runtime_enable(drm->dev);
>
> - return ret;
> + return 0;
> }
>
> static void mxsfb_unload(struct drm_device *drm)
> @@ -305,9 +285,7 @@ static void mxsfb_unload(struct drm_device *drm)
> drm_kms_helper_poll_fini(drm);
> drm_mode_config_cleanup(drm);
>
> - pm_runtime_get_sync(drm->dev);
> mxsfb_irq_uninstall(drm);
> - pm_runtime_put_sync(drm->dev);
>
> drm->dev_private = NULL;
>
> @@ -388,23 +366,60 @@ static void mxsfb_shutdown(struct platform_device
> *pdev) drm_atomic_helper_shutdown(drm);
> }
>
> -#ifdef CONFIG_PM_SLEEP
> +static int mxsfb_rpm_suspend(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> + struct mxsfb_drm_private
[PATCH] drm: mxsfb: Simplify LCDIF clock handling
The current clock handling in the LCDIF driver is a convoluted mess.
Implement runtime PM ops which turn the clock ON and OFF and let the
pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable
and .atomic_disable callbacks turn the clock ON and OFF at the right
time.
Signed-off-by: Marek Vasut
Cc: Alexander Stein
Cc: Laurent Pinchart
Cc: Lucas Stach
Cc: Peng Fan
Cc: Robby Cai
Cc: Sam Ravnborg
Cc: Stefan Agner
---
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 85 ++-
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 18 ++-
2 files changed, 54 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 375f26d4a4172..4ff3c6195dd0c 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -72,18 +72,6 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
},
};
-void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb)
-{
- if (mxsfb->clk_axi)
- clk_prepare_enable(mxsfb->clk_axi);
-}
-
-void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb)
-{
- if (mxsfb->clk_axi)
- clk_disable_unprepare(mxsfb->clk_axi);
-}
-
static struct drm_framebuffer *
mxsfb_fb_create(struct drm_device *dev, struct drm_file *file_priv,
const struct drm_mode_fb_cmd2 *mode_cmd)
@@ -224,33 +212,31 @@ static int mxsfb_load(struct drm_device *drm,
if (IS_ERR(mxsfb->clk))
return PTR_ERR(mxsfb->clk);
- mxsfb->clk_axi = devm_clk_get(drm->dev, "axi");
+ mxsfb->clk_axi = devm_clk_get_optional(drm->dev, "axi");
if (IS_ERR(mxsfb->clk_axi))
- mxsfb->clk_axi = NULL;
+ return PTR_ERR(mxsfb->clk_axi);
- mxsfb->clk_disp_axi = devm_clk_get(drm->dev, "disp_axi");
+ mxsfb->clk_disp_axi = devm_clk_get_optional(drm->dev, "disp_axi");
if (IS_ERR(mxsfb->clk_disp_axi))
- mxsfb->clk_disp_axi = NULL;
+ return PTR_ERR(mxsfb->clk_disp_axi);
ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
if (ret)
return ret;
- pm_runtime_enable(drm->dev);
-
/* Modeset init */
drm_mode_config_init(drm);
ret = mxsfb_kms_init(mxsfb);
if (ret < 0) {
dev_err(drm->dev, "Failed to initialize KMS pipeline\n");
- goto err_vblank;
+ return ret;
}
ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
if (ret < 0) {
dev_err(drm->dev, "Failed to initialise vblank\n");
- goto err_vblank;
+ return ret;
}
/* Start with vertical blanking interrupt reporting disabled. */
@@ -260,7 +246,7 @@ static int mxsfb_load(struct drm_device *drm,
if (ret) {
if (ret != -EPROBE_DEFER)
dev_err(drm->dev, "Cannot connect bridge: %d\n", ret);
- goto err_vblank;
+ return ret;
}
drm->mode_config.min_width = MXSFB_MIN_XRES;
@@ -277,13 +263,10 @@ static int mxsfb_load(struct drm_device *drm,
goto err_vblank;
mxsfb->irq = ret;
- pm_runtime_get_sync(drm->dev);
ret = mxsfb_irq_install(drm, mxsfb->irq);
- pm_runtime_put_sync(drm->dev);
-
if (ret < 0) {
dev_err(drm->dev, "Failed to install IRQ handler\n");
- goto err_vblank;
+ return ret;
}
drm_kms_helper_poll_init(drm);
@@ -292,12 +275,9 @@ static int mxsfb_load(struct drm_device *drm,
drm_helper_hpd_irq_event(drm);
- return 0;
-
-err_vblank:
- pm_runtime_disable(drm->dev);
+ pm_runtime_enable(drm->dev);
- return ret;
+ return 0;
}
static void mxsfb_unload(struct drm_device *drm)
@@ -305,9 +285,7 @@ static void mxsfb_unload(struct drm_device *drm)
drm_kms_helper_poll_fini(drm);
drm_mode_config_cleanup(drm);
- pm_runtime_get_sync(drm->dev);
mxsfb_irq_uninstall(drm);
- pm_runtime_put_sync(drm->dev);
drm->dev_private = NULL;
@@ -388,23 +366,60 @@ static void mxsfb_shutdown(struct platform_device *pdev)
drm_atomic_helper_shutdown(drm);
}
-#ifdef CONFIG_PM_SLEEP
+static int mxsfb_rpm_suspend(struct device *dev)
+{
+ struct drm_device *drm = dev_get_drvdata(dev);
+ struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+ /* These clock supply the DISPLAY CLOCK Domain */
+ clk_disable_unprepare(mxsfb->clk);
+ /* These clock supply the System Bus, AXI, Write Path, LFIFO */
+ clk_disable_unprepare(mxsfb->clk_disp_axi);
+ /* These clock supply the Control Bus, APB, APBH Ctrl Registers */
+ clk_disable_unprepare(mxsfb->clk_axi);
+
+ return 0;
+}
+
+static int mxsfb_rpm_resume(struct device *dev)
+{
+ struct drm_device *drm = dev_get_drvdata(dev);
+ struct
