diff mbox series

drm: mxsfb: Simplify LCDIF clock handling

Message ID 20220206185555.275768-1-marex@denx.de (mailing list archive)
State New, archived
Headers show
Series drm: mxsfb: Simplify LCDIF clock handling | expand

Commit Message

Marek Vasut Feb. 6, 2022, 6:55 p.m. UTC
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 <marex@denx.de>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Robby Cai <robby.cai@nxp.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 85 ++++++++++++++++++-------------
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 18 ++-----
 2 files changed, 54 insertions(+), 49 deletions(-)

Comments

Alexander Stein Feb. 10, 2022, 8:11 a.m. UTC | #1
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 <marex@denx.de>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Robby Cai <robby.cai@nxp.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Stefan Agner <stefan@agner.ch>
> ---
>  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 *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 mxsfb_drm_private *mxsfb = drm->dev_private;
> +
> +	/* These clock supply the Control Bus, APB, APBH Ctrl Registers */
> +	clk_prepare_enable(mxsfb->clk_axi);
> +	/* These clock supply the System Bus, AXI, Write Path, LFIFO */
> +	clk_prepare_enable(mxsfb->clk_disp_axi);
> +	/* These clock supply the DISPLAY CLOCK Domain */
> +	clk_prepare_enable(mxsfb->clk);
> +
> +	return 0;
> +}
> +
>  static int mxsfb_suspend(struct device *dev)
>  {
>  	struct drm_device *drm = dev_get_drvdata(dev);
> +	int ret;
> 
> -	return drm_mode_config_helper_suspend(drm);
> +	ret = drm_mode_config_helper_suspend(drm);
> +	if (ret)
> +		return ret;
> +
> +	return mxsfb_rpm_suspend(dev);
>  }
> 
>  static int mxsfb_resume(struct device *dev)
>  {
>  	struct drm_device *drm = dev_get_drvdata(dev);
> 
> +	mxsfb_rpm_resume(dev);
> +
>  	return drm_mode_config_helper_resume(drm);
>  }
> -#endif
> 
>  static const struct dev_pm_ops mxsfb_pm_ops = {
> +	.runtime_suspend = mxsfb_rpm_suspend,
> +	.runtime_resume = mxsfb_rpm_resume,
>  	SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume)
>  };
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 0655582ae8ed6..03743a84c8e79
> 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -100,10 +100,6 @@ static void mxsfb_enable_controller(struct
> mxsfb_drm_private *mxsfb) {
>  	u32 reg;
> 
> -	if (mxsfb->clk_disp_axi)
> -		clk_prepare_enable(mxsfb->clk_disp_axi);
> -	clk_prepare_enable(mxsfb->clk);
> -
>  	/* Increase number of outstanding requests on all supported IPs */
>  	if (mxsfb->devdata->has_ctrl2) {
>  		reg = readl(mxsfb->base + LCDC_V4_CTRL2);
> @@ -168,10 +164,6 @@ static void mxsfb_disable_controller(struct
> mxsfb_drm_private *mxsfb) reg = readl(mxsfb->base + LCDC_VDCTRL4);
>  	reg &= ~VDCTRL4_SYNC_SIGNALS_ON;
>  	writel(reg, mxsfb->base + LCDC_VDCTRL4);
> -
> -	clk_disable_unprepare(mxsfb->clk);
> -	if (mxsfb->clk_disp_axi)
> -		clk_disable_unprepare(mxsfb->clk_disp_axi);
>  }
> 
>  /*
> @@ -352,9 +344,6 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc
> *crtc, dma_addr_t paddr;
> 
>  	pm_runtime_get_sync(drm->dev);
> -	mxsfb_enable_axi_clk(mxsfb);
> -
> -	drm_crtc_vblank_on(crtc);
> 
>  	/* If there is a bridge attached to the LCDIF, use its bus format 
*/
>  	if (mxsfb->bridge) {
> @@ -388,6 +377,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc
> *crtc, }
> 
>  	mxsfb_enable_controller(mxsfb);
> +
> +	drm_crtc_vblank_on(crtc);
>  }
> 
>  static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
> @@ -397,6 +388,8 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc
> *crtc, struct drm_device *drm = mxsfb->drm;
>  	struct drm_pending_vblank_event *event;
> 
> +	drm_crtc_vblank_off(crtc);
> +
>  	mxsfb_disable_controller(mxsfb);
> 
>  	spin_lock_irq(&drm->event_lock);
> @@ -407,9 +400,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc
> *crtc, }
>  	spin_unlock_irq(&drm->event_lock);
> 
> -	drm_crtc_vblank_off(crtc);
> -
> -	mxsfb_disable_axi_clk(mxsfb);
>  	pm_runtime_put_sync(drm->dev);
>  }
diff mbox series

Patch

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 mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	/* These clock supply the Control Bus, APB, APBH Ctrl Registers */
+	clk_prepare_enable(mxsfb->clk_axi);
+	/* These clock supply the System Bus, AXI, Write Path, LFIFO */
+	clk_prepare_enable(mxsfb->clk_disp_axi);
+	/* These clock supply the DISPLAY CLOCK Domain */
+	clk_prepare_enable(mxsfb->clk);
+
+	return 0;
+}
+
 static int mxsfb_suspend(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
+	int ret;
 
-	return drm_mode_config_helper_suspend(drm);
+	ret = drm_mode_config_helper_suspend(drm);
+	if (ret)
+		return ret;
+
+	return mxsfb_rpm_suspend(dev);
 }
 
 static int mxsfb_resume(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
 
+	mxsfb_rpm_resume(dev);
+
 	return drm_mode_config_helper_resume(drm);
 }
-#endif
 
 static const struct dev_pm_ops mxsfb_pm_ops = {
+	.runtime_suspend = mxsfb_rpm_suspend,
+	.runtime_resume = mxsfb_rpm_resume,
 	SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume)
 };
 
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 0655582ae8ed6..03743a84c8e79 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -100,10 +100,6 @@  static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 {
 	u32 reg;
 
-	if (mxsfb->clk_disp_axi)
-		clk_prepare_enable(mxsfb->clk_disp_axi);
-	clk_prepare_enable(mxsfb->clk);
-
 	/* Increase number of outstanding requests on all supported IPs */
 	if (mxsfb->devdata->has_ctrl2) {
 		reg = readl(mxsfb->base + LCDC_V4_CTRL2);
@@ -168,10 +164,6 @@  static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
 	reg = readl(mxsfb->base + LCDC_VDCTRL4);
 	reg &= ~VDCTRL4_SYNC_SIGNALS_ON;
 	writel(reg, mxsfb->base + LCDC_VDCTRL4);
-
-	clk_disable_unprepare(mxsfb->clk);
-	if (mxsfb->clk_disp_axi)
-		clk_disable_unprepare(mxsfb->clk_disp_axi);
 }
 
 /*
@@ -352,9 +344,6 @@  static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	dma_addr_t paddr;
 
 	pm_runtime_get_sync(drm->dev);
-	mxsfb_enable_axi_clk(mxsfb);
-
-	drm_crtc_vblank_on(crtc);
 
 	/* If there is a bridge attached to the LCDIF, use its bus format */
 	if (mxsfb->bridge) {
@@ -388,6 +377,8 @@  static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	}
 
 	mxsfb_enable_controller(mxsfb);
+
+	drm_crtc_vblank_on(crtc);
 }
 
 static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
@@ -397,6 +388,8 @@  static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
 	struct drm_device *drm = mxsfb->drm;
 	struct drm_pending_vblank_event *event;
 
+	drm_crtc_vblank_off(crtc);
+
 	mxsfb_disable_controller(mxsfb);
 
 	spin_lock_irq(&drm->event_lock);
@@ -407,9 +400,6 @@  static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
 	}
 	spin_unlock_irq(&drm->event_lock);
 
-	drm_crtc_vblank_off(crtc);
-
-	mxsfb_disable_axi_clk(mxsfb);
 	pm_runtime_put_sync(drm->dev);
 }