[PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[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.

This requires slight reordering in mxsfb_crtc_atomic_enable() and
mxsfb_crtc_atomic_disable(), since all the register accesses must
happen only with clock enabled and clock frequency configuration
must happen with clock disabled.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 104 +++++++++++++++++-------------
 drivers/gpu/drm/mxsfb/mxsfb_kms.c |  27 +++-----
 2 files changed, 67 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 9d71c55a31c07..c790aeff0a6f0 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -73,18 +73,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)
@@ -173,13 +161,9 @@ static void mxsfb_irq_disable(struct drm_device *drm)
 {
 	struct mxsfb_drm_private *mxsfb = drm->dev_private;
 
-	mxsfb_enable_axi_clk(mxsfb);
-
 	/* Disable and clear VBLANK IRQ */
 	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
 	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-
-	mxsfb_disable_axi_clk(mxsfb);
 }
 
 static int mxsfb_irq_install(struct drm_device *dev, int irq)
@@ -225,43 +209,41 @@ 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);
+
+	platform_set_drvdata(pdev, drm);
 
 	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. */
 	drm_crtc_vblank_off(&mxsfb->crtc);
 
 	ret = mxsfb_attach_bridge(mxsfb);
-	if (ret) {
-		dev_err_probe(drm->dev, ret, "Cannot connect bridge\n");
-		goto err_vblank;
-	}
+	if (ret)
+		return dev_err_probe(drm->dev, ret, "Cannot connect bridge\n");
 
 	drm->mode_config.min_width	= MXSFB_MIN_XRES;
 	drm->mode_config.min_height	= MXSFB_MIN_YRES;
@@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm,
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_vblank;
+		return ret;
 	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);
 
-	platform_set_drvdata(pdev, 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)
 {
+	pm_runtime_get_sync(drm->dev);
+
 	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);
+	pm_runtime_disable(drm->dev);
 
 	drm->dev_private = NULL;
-
-	pm_runtime_disable(drm->dev);
 }
 
 DEFINE_DRM_GEM_CMA_FOPS(fops);
@@ -388,23 +363,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 4cfb6c0016799..657b6afbbf1f9 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);
 }
 
 /*
@@ -250,8 +242,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 
 	mxsfb_set_formats(mxsfb, bus_format);
 
-	clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
-
 	if (mxsfb->bridge && mxsfb->bridge->timings)
 		bus_flags = mxsfb->bridge->timings->input_bus_flags;
 
@@ -346,16 +336,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_state *state)
 {
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
+	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
 	struct drm_bridge_state *bridge_state;
 	struct drm_device *drm = mxsfb->drm;
 	u32 bus_format = 0;
 	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) {
 		bridge_state =
@@ -382,6 +368,10 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	if (!bus_format)
 		bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 
+	clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
+
+	pm_runtime_get_sync(drm->dev);
+
 	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
 
 	/* Write cur_buf as well to avoid an initial corrupt frame */
@@ -392,6 +382,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,
@@ -401,6 +393,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);
@@ -411,9 +405,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);
 }
 
-- 
2.34.1

[PATCH v2 2/7] drm: mxsfb: Simplify LCDIF IRQ handling

[Marek Vasut]

The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation
from the LCDIF block already and this is called in mxsfb_load() before
request_irq(), so explicitly disabling IRQ using custom function like
mxsfb_irq_disable() is not needed, remove it. The request_irq() call
would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove
the unnecessary check. Finally, remove both mxsfb_irq_install() and
mxsfb_irq_uninstall() as well, since they are no longer useful.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 38 +++++++------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index c790aeff0a6f0..94cafff7f68d5 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -157,33 +157,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void mxsfb_irq_disable(struct drm_device *drm)
-{
-	struct mxsfb_drm_private *mxsfb = drm->dev_private;
-
-	/* Disable and clear VBLANK IRQ */
-	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-}
-
-static int mxsfb_irq_install(struct drm_device *dev, int irq)
-{
-	if (irq == IRQ_NOTCONNECTED)
-		return -ENOTCONN;
-
-	mxsfb_irq_disable(dev);
-
-	return request_irq(irq, mxsfb_irq_handler, 0,  dev->driver->name, dev);
-}
-
-static void mxsfb_irq_uninstall(struct drm_device *dev)
-{
-	struct mxsfb_drm_private *mxsfb = dev->dev_private;
-
-	mxsfb_irq_disable(dev);
-	free_irq(mxsfb->irq, dev);
-}
-
 static int mxsfb_load(struct drm_device *drm,
 		      const struct mxsfb_devdata *devdata)
 {
@@ -259,7 +232,8 @@ static int mxsfb_load(struct drm_device *drm,
 		return ret;
 	mxsfb->irq = ret;
 
-	ret = mxsfb_irq_install(drm, mxsfb->irq);
+	ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0,
+			  drm->driver->name, drm);
 	if (ret < 0) {
 		dev_err(drm->dev, "Failed to install IRQ handler\n");
 		return ret;
@@ -276,16 +250,20 @@ static int mxsfb_load(struct drm_device *drm,
 
 static void mxsfb_unload(struct drm_device *drm)
 {
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
 	pm_runtime_get_sync(drm->dev);
 
+	drm_crtc_vblank_off(&mxsfb->crtc);
+
 	drm_kms_helper_poll_fini(drm);
 	drm_mode_config_cleanup(drm);
 
-	mxsfb_irq_uninstall(drm);
-
 	pm_runtime_put_sync(drm->dev);
 	pm_runtime_disable(drm->dev);
 
+	free_irq(mxsfb->irq, drm->dev);
+
 	drm->dev_private = NULL;
 }
 
-- 
2.34.1

[PATCH v2 3/7] drm: mxsfb: Wrap FIFO reset and comments into mxsfb_reset_block()

[Marek Vasut]

Wrap FIFO reset and comments into mxsfb_reset_block(), this is a clean up.
No functional change.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 36 +++++++++++++++++--------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 657b6afbbf1f9..015b289d93a3c 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -183,6 +183,12 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 {
 	int ret;
 
+	/*
+	 * It seems, you can't re-program the controller if it is still
+	 * running. This may lead to shifted pictures (FIFO issue?), so
+	 * first stop the controller and drain its FIFOs.
+	 */
+
 	ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_SFTRST);
 	if (ret)
 		return ret;
@@ -193,7 +199,20 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 	if (ret)
 		return ret;
 
-	return clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
+	ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
+	if (ret)
+		return ret;
+
+	/* Clear the FIFOs */
+	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
+	readl(mxsfb->base + LCDC_CTRL1);
+	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+	readl(mxsfb->base + LCDC_CTRL1);
+
+	if (mxsfb->devdata->has_overlay)
+		writel(0, mxsfb->base + LCDC_AS_CTRL);
+
+	return 0;
 }
 
 static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
@@ -220,26 +239,11 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
 	int err;
 
-	/*
-	 * It seems, you can't re-program the controller if it is still
-	 * running. This may lead to shifted pictures (FIFO issue?), so
-	 * first stop the controller and drain its FIFOs.
-	 */
-
 	/* Mandatory eLCDIF reset as per the Reference Manual */
 	err = mxsfb_reset_block(mxsfb);
 	if (err)
 		return;
 
-	/* Clear the FIFOs */
-	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
-	readl(mxsfb->base + LCDC_CTRL1);
-	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
-	readl(mxsfb->base + LCDC_CTRL1);
-
-	if (mxsfb->devdata->has_overlay)
-		writel(0, mxsfb->base + LCDC_AS_CTRL);
-
 	mxsfb_set_formats(mxsfb, bus_format);
 
 	if (mxsfb->bridge && mxsfb->bridge->timings)
-- 
2.34.1

[PATCH v2 4/7] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Marek Vasut]

Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
This is a clean up. No functional change.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 015b289d93a3c..7b0abd0472aae 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -43,6 +43,21 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val)
 		mxsfb->devdata->hs_wdth_shift;
 }
 
+static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
+{
+	struct drm_framebuffer *fb = plane->state->fb;
+	struct drm_gem_cma_object *gem;
+
+	if (!fb)
+		return 0;
+
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	if (!gem)
+		return 0;
+
+	return gem->paddr;
+}
+
 /*
  * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
  * outputting them on the bus.
@@ -215,21 +230,6 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
 	return 0;
 }
 
-static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
-{
-	struct drm_framebuffer *fb = plane->state->fb;
-	struct drm_gem_cma_object *gem;
-
-	if (!fb)
-		return 0;
-
-	gem = drm_fb_cma_get_gem_obj(fb, 0);
-	if (!gem)
-		return 0;
-
-	return gem->paddr;
-}
-
 static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 				     const u32 bus_format)
 {
-- 
2.34.1

[PATCH v2 5/7] drm: mxsfb: Factor out mxsfb_set_mode()

[Marek Vasut]

Pull mode registers programming from mxsfb_enable_controller() into
dedicated function mxsfb_set_mode(). This is a clean up. No functional
change.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 96 +++++++++++++++++--------------
 1 file changed, 52 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 7b0abd0472aae..14f5cc590a51b 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -111,6 +111,57 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb,
 	writel(ctrl, mxsfb->base + LCDC_CTRL);
 }
 
+static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
+{
+	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
+	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
+
+	writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
+	       TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
+	       mxsfb->base + mxsfb->devdata->transfer_count);
+
+	vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
+
+	vdctrl0 = VDCTRL0_ENABLE_PRESENT |	/* Always in DOTCLOCK mode */
+		  VDCTRL0_VSYNC_PERIOD_UNIT |
+		  VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |
+		  VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);
+	if (m->flags & DRM_MODE_FLAG_PHSYNC)
+		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
+	if (m->flags & DRM_MODE_FLAG_PVSYNC)
+		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
+	/* Make sure Data Enable is high active by default */
+	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
+		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
+	/*
+	 * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,
+	 * controllers VDCTRL0_DOTCLK is display centric.
+	 * Drive on positive edge       -> display samples on falling edge
+	 * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
+	 */
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
+
+	writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
+
+	/* Frame length in lines. */
+	writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
+
+	/* Line length in units of clocks or pixels. */
+	hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
+	writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
+	       VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),
+	       mxsfb->base + LCDC_VDCTRL2);
+
+	writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
+	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
+	       mxsfb->base + LCDC_VDCTRL3);
+
+	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
+	       mxsfb->base + LCDC_VDCTRL4);
+
+}
+
 static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 {
 	u32 reg;
@@ -236,7 +287,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 	struct drm_device *drm = mxsfb->crtc.dev;
 	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
 	u32 bus_flags = mxsfb->connector->display_info.bus_flags;
-	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
 	int err;
 
 	/* Mandatory eLCDIF reset as per the Reference Manual */
@@ -256,49 +306,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 			     bus_flags);
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
 
-	writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
-	       TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
-	       mxsfb->base + mxsfb->devdata->transfer_count);
-
-	vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
-
-	vdctrl0 = VDCTRL0_ENABLE_PRESENT |	/* Always in DOTCLOCK mode */
-		  VDCTRL0_VSYNC_PERIOD_UNIT |
-		  VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |
-		  VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);
-	if (m->flags & DRM_MODE_FLAG_PHSYNC)
-		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
-	if (m->flags & DRM_MODE_FLAG_PVSYNC)
-		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
-	/* Make sure Data Enable is high active by default */
-	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
-		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
-	/*
-	 * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,
-	 * controllers VDCTRL0_DOTCLK is display centric.
-	 * Drive on positive edge       -> display samples on falling edge
-	 * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
-	 */
-	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
-		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
-
-	writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
-
-	/* Frame length in lines. */
-	writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
-
-	/* Line length in units of clocks or pixels. */
-	hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
-	writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
-	       VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),
-	       mxsfb->base + LCDC_VDCTRL2);
-
-	writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
-	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
-	       mxsfb->base + LCDC_VDCTRL3);
-
-	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
-	       mxsfb->base + LCDC_VDCTRL4);
+	mxsfb_set_mode(mxsfb, bus_flags);
 }
 
 static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
-- 
2.34.1

[PATCH v2 6/7] drm: mxsfb: Reorder mxsfb_crtc_mode_set_nofb()

[Marek Vasut]

Reorder mxsfb_crtc_mode_set_nofb() such that all functions which perform
register IO are called from one single location in this function. This is
a clean up. No functional change.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 14f5cc590a51b..497603964add8 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -289,13 +289,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 	u32 bus_flags = mxsfb->connector->display_info.bus_flags;
 	int err;
 
-	/* Mandatory eLCDIF reset as per the Reference Manual */
-	err = mxsfb_reset_block(mxsfb);
-	if (err)
-		return;
-
-	mxsfb_set_formats(mxsfb, bus_format);
-
 	if (mxsfb->bridge && mxsfb->bridge->timings)
 		bus_flags = mxsfb->bridge->timings->input_bus_flags;
 
@@ -306,6 +299,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
 			     bus_flags);
 	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
 
+	/* Mandatory eLCDIF reset as per the Reference Manual */
+	err = mxsfb_reset_block(mxsfb);
+	if (err)
+		return;
+
+	mxsfb_set_formats(mxsfb, bus_format);
+
 	mxsfb_set_mode(mxsfb, bus_flags);
 }
 
-- 
2.34.1

[PATCH v2 7/7] drm: mxsfb: Factor out mxsfb_update_buffer()

[Marek Vasut]

Pull functionality responsible for programming framebuffer address into
the controller into dedicated function mxsfb_update_buffer(). This is a
clean up. No functional change.

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>
---
V2: No change
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 497603964add8..4baa3db1f3d10 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -58,6 +58,22 @@ static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
 	return gem->paddr;
 }
 
+static void
+mxsfb_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane,
+		    bool both)
+{
+	dma_addr_t paddr;
+
+	paddr = mxsfb_get_fb_paddr(plane);
+	if (!paddr)
+		return;
+
+	if (both)
+		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
+
+	writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+}
+
 /*
  * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
  * outputting them on the bus.
@@ -352,7 +368,6 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	struct drm_bridge_state *bridge_state;
 	struct drm_device *drm = mxsfb->drm;
 	u32 bus_format = 0;
-	dma_addr_t paddr;
 
 	/* If there is a bridge attached to the LCDIF, use its bus format */
 	if (mxsfb->bridge) {
@@ -387,11 +402,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
 
 	/* Write cur_buf as well to avoid an initial corrupt frame */
-	paddr = mxsfb_get_fb_paddr(crtc->primary);
-	if (paddr) {
-		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
-		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
-	}
+	mxsfb_update_buffer(mxsfb, crtc->primary, true);
 
 	mxsfb_enable_controller(mxsfb);
 
@@ -491,11 +502,8 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
 					      struct drm_atomic_state *state)
 {
 	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
-	dma_addr_t paddr;
 
-	paddr = mxsfb_get_fb_paddr(plane);
-	if (paddr)
-		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+	mxsfb_update_buffer(mxsfb, plane, false);
 }
 
 static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
-- 
2.34.1

Re: [PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Lucas Stach]

Hi Marek,

Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> The current clock handling in the LCDIF driver is a convoluted mess.

Here we agree...

> 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.
> 
> This requires slight reordering in mxsfb_crtc_atomic_enable() and
> mxsfb_crtc_atomic_disable(), since all the register accesses must
> happen only with clock enabled and clock frequency configuration
> must happen with clock disabled.
> 
... on that one I don't. Please don't move the pixel clock into the RPM
calls. We have a very well defined point between atomic enable/disable
where the pixel clock is actually needed. All the other configuration
accesses don't need the pixel clock to be active.

> 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>
> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c | 104 +++++++++++++++++-------------
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c |  27 +++-----
>  2 files changed, 67 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 9d71c55a31c07..c790aeff0a6f0 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -73,18 +73,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)
> @@ -173,13 +161,9 @@ static void mxsfb_irq_disable(struct drm_device *drm)
>  {
>  	struct mxsfb_drm_private *mxsfb = drm->dev_private;
>  
> -	mxsfb_enable_axi_clk(mxsfb);
> -
>  	/* Disable and clear VBLANK IRQ */
>  	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
>  	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> -
> -	mxsfb_disable_axi_clk(mxsfb);
>  }
>  
>  static int mxsfb_irq_install(struct drm_device *dev, int irq)
> @@ -225,43 +209,41 @@ 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);
> +
> +	platform_set_drvdata(pdev, drm);
>  
>  	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. */
>  	drm_crtc_vblank_off(&mxsfb->crtc);
>  
>  	ret = mxsfb_attach_bridge(mxsfb);
> -	if (ret) {
> -		dev_err_probe(drm->dev, ret, "Cannot connect bridge\n");
> -		goto err_vblank;
> -	}
> +	if (ret)
> +		return dev_err_probe(drm->dev, ret, "Cannot connect bridge\n");
>  
>  	drm->mode_config.min_width	= MXSFB_MIN_XRES;
>  	drm->mode_config.min_height	= MXSFB_MIN_YRES;
> @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm,
>  
>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
> -		goto err_vblank;
> +		return ret;
>  	mxsfb->irq = ret;
>  
> -	pm_runtime_get_sync(drm->dev);
>  	ret = mxsfb_irq_install(drm, mxsfb->irq);
> -	pm_runtime_put_sync(drm->dev);
> -
Here you do a bunch of stuff resulting in register access without
enabling the clocks. I don't really see how this works, maybe because
the clocks are still on when you run the probe?

Better enable the register access clocks here and then tell RPM about
the fact that the device is running by calling pm_runtime_set_active
before pm_runtime_enable. This way theoretically allows to build a
kernel without CONFIG_PM and things still work, even if the RPM calls
are stubs.

>  	if (ret < 0) {
>  		dev_err(drm->dev, "Failed to install IRQ handler\n");
> -		goto err_vblank;
> +		return ret;
>  	}
>  
>  	drm_kms_helper_poll_init(drm);
>  
> -	platform_set_drvdata(pdev, 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)
>  {
> +	pm_runtime_get_sync(drm->dev);
> +
>  	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);
> +	pm_runtime_disable(drm->dev);
>  
>  	drm->dev_private = NULL;
> -
> -	pm_runtime_disable(drm->dev);
>  }
>  
>  DEFINE_DRM_GEM_CMA_FOPS(fops);
> @@ -388,23 +363,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_RUNTIME_PM_OPS(mxsfb_rpm_suspend, mxsfb_rpm_resume, NULL)

>  	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 4cfb6c0016799..657b6afbbf1f9 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);
>  }
>  
>  /*
> @@ -250,8 +242,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  
>  	mxsfb_set_formats(mxsfb, bus_format);
>  
> -	clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
> -
>  	if (mxsfb->bridge && mxsfb->bridge->timings)
>  		bus_flags = mxsfb->bridge->timings->input_bus_flags;
>  
> @@ -346,16 +336,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
>  				     struct drm_atomic_state *state)
>  {
>  	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
> +	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
>  	struct drm_bridge_state *bridge_state;
>  	struct drm_device *drm = mxsfb->drm;
>  	u32 bus_format = 0;
>  	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) {
>  		bridge_state =
> @@ -382,6 +368,10 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
>  	if (!bus_format)
>  		bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
> +	clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
> +
> +	pm_runtime_get_sync(drm->dev);
> +
>  	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
>  
>  	/* Write cur_buf as well to avoid an initial corrupt frame */
> @@ -392,6 +382,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,
> @@ -401,6 +393,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);
> @@ -411,9 +405,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);
> 
Not the fault of your patch, but why is this a _sync call?

Regards,
Lucas

Re: [PATCH v2 2/7] drm: mxsfb: Simplify LCDIF IRQ handling

[Lucas Stach]

Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation
> from the LCDIF block already and this is called in mxsfb_load() before
> request_irq(), so explicitly disabling IRQ using custom function like
> mxsfb_irq_disable() is not needed, remove it.
> 

Have you checked that the drm_vblank_off in probe actually results in a
call to mxsfb_crtc_disable_vblank? From my reading of the core code,
this should be a no-op without a previous drm_vblank_on, so it would
not result in the desired masking before the IRQ is requested.

Regards,
Lucas

>  The request_irq() call
> would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove
> the unnecessary check. Finally, remove both mxsfb_irq_install() and
> mxsfb_irq_uninstall() as well, since they are no longer useful.
> 
> 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>
> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c | 38 +++++++------------------------
>  1 file changed, 8 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index c790aeff0a6f0..94cafff7f68d5 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -157,33 +157,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void mxsfb_irq_disable(struct drm_device *drm)
> -{
> -	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> -
> -	/* Disable and clear VBLANK IRQ */
> -	writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> -	writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> -}
> -
> -static int mxsfb_irq_install(struct drm_device *dev, int irq)
> -{
> -	if (irq == IRQ_NOTCONNECTED)
> -		return -ENOTCONN;
> -
> -	mxsfb_irq_disable(dev);
> -
> -	return request_irq(irq, mxsfb_irq_handler, 0,  dev->driver->name, dev);
> -}
> -
> -static void mxsfb_irq_uninstall(struct drm_device *dev)
> -{
> -	struct mxsfb_drm_private *mxsfb = dev->dev_private;
> -
> -	mxsfb_irq_disable(dev);
> -	free_irq(mxsfb->irq, dev);
> -}
> -
>  static int mxsfb_load(struct drm_device *drm,
>  		      const struct mxsfb_devdata *devdata)
>  {
> @@ -259,7 +232,8 @@ static int mxsfb_load(struct drm_device *drm,
>  		return ret;
>  	mxsfb->irq = ret;
>  
> -	ret = mxsfb_irq_install(drm, mxsfb->irq);
> +	ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0,
> +			  drm->driver->name, drm);
>  	if (ret < 0) {
>  		dev_err(drm->dev, "Failed to install IRQ handler\n");
>  		return ret;
> @@ -276,16 +250,20 @@ static int mxsfb_load(struct drm_device *drm,
>  
>  static void mxsfb_unload(struct drm_device *drm)
>  {
> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> +
>  	pm_runtime_get_sync(drm->dev);
>  
> +	drm_crtc_vblank_off(&mxsfb->crtc);
> +
>  	drm_kms_helper_poll_fini(drm);
>  	drm_mode_config_cleanup(drm);
>  
> -	mxsfb_irq_uninstall(drm);
> -
>  	pm_runtime_put_sync(drm->dev);
>  	pm_runtime_disable(drm->dev);
>  
> +	free_irq(mxsfb->irq, drm->dev);
> +
>  	drm->dev_private = NULL;
>  }
>  

Re: [PATCH v2 3/7] drm: mxsfb: Wrap FIFO reset and comments into mxsfb_reset_block()

[Lucas Stach]

Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> Wrap FIFO reset and comments into mxsfb_reset_block(), this is a clean up.
> No functional change.
> 
> 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>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 36 +++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index 657b6afbbf1f9..015b289d93a3c 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -183,6 +183,12 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
>  {
>  	int ret;
>  
> +	/*
> +	 * It seems, you can't re-program the controller if it is still
> +	 * running. This may lead to shifted pictures (FIFO issue?), so
> +	 * first stop the controller and drain its FIFOs.
> +	 */
> +
>  	ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_SFTRST);
>  	if (ret)
>  		return ret;
> @@ -193,7 +199,20 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
>  	if (ret)
>  		return ret;
>  
> -	return clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
> +	ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear the FIFOs */
> +	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
> +	readl(mxsfb->base + LCDC_CTRL1);
> +	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> +	readl(mxsfb->base + LCDC_CTRL1);
> +
> +	if (mxsfb->devdata->has_overlay)
> +		writel(0, mxsfb->base + LCDC_AS_CTRL);
> +
> +	return 0;
>  }
>  
>  static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
> @@ -220,26 +239,11 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
>  	int err;
>  
> -	/*
> -	 * It seems, you can't re-program the controller if it is still
> -	 * running. This may lead to shifted pictures (FIFO issue?), so
> -	 * first stop the controller and drain its FIFOs.
> -	 */
> -
>  	/* Mandatory eLCDIF reset as per the Reference Manual */
>  	err = mxsfb_reset_block(mxsfb);
>  	if (err)
>  		return;
>  
> -	/* Clear the FIFOs */
> -	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
> -	readl(mxsfb->base + LCDC_CTRL1);
> -	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> -	readl(mxsfb->base + LCDC_CTRL1);
> -
> -	if (mxsfb->devdata->has_overlay)
> -		writel(0, mxsfb->base + LCDC_AS_CTRL);
> -
>  	mxsfb_set_formats(mxsfb, bus_format);
>  
>  	if (mxsfb->bridge && mxsfb->bridge->timings)

Re: [PATCH v2 4/7] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Lucas Stach]

Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
> This is a clean up. No functional change.
> 
> 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>

Hm, I don't see any real benefit, but I also fail to see why it
shouldn't be done so:
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index 015b289d93a3c..7b0abd0472aae 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -43,6 +43,21 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val)
>  		mxsfb->devdata->hs_wdth_shift;
>  }
>  
> +static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
> +{
> +	struct drm_framebuffer *fb = plane->state->fb;
> +	struct drm_gem_cma_object *gem;
> +
> +	if (!fb)
> +		return 0;
> +
> +	gem = drm_fb_cma_get_gem_obj(fb, 0);
> +	if (!gem)
> +		return 0;
> +
> +	return gem->paddr;
> +}
> +
>  /*
>   * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
>   * outputting them on the bus.
> @@ -215,21 +230,6 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
>  	return 0;
>  }
>  
> -static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
> -{
> -	struct drm_framebuffer *fb = plane->state->fb;
> -	struct drm_gem_cma_object *gem;
> -
> -	if (!fb)
> -		return 0;
> -
> -	gem = drm_fb_cma_get_gem_obj(fb, 0);
> -	if (!gem)
> -		return 0;
> -
> -	return gem->paddr;
> -}
> -
>  static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  				     const u32 bus_format)
>  {

Re: [PATCH v2 5/7] drm: mxsfb: Factor out mxsfb_set_mode()

[Lucas Stach]

Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> Pull mode registers programming from mxsfb_enable_controller() into
> dedicated function mxsfb_set_mode(). This is a clean up. No functional
> change.

This one however looks like over-factorization to me. Why pull out a
mode_set function out of a mode_set function?

> 
> 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>
> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 96 +++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index 7b0abd0472aae..14f5cc590a51b 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -111,6 +111,57 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb,
>  	writel(ctrl, mxsfb->base + LCDC_CTRL);
>  }
>  
> +static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags)
> +{
> +	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
> +	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
> +
> +	writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
> +	       TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
> +	       mxsfb->base + mxsfb->devdata->transfer_count);
> +
> +	vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
> +
> +	vdctrl0 = VDCTRL0_ENABLE_PRESENT |	/* Always in DOTCLOCK mode */
> +		  VDCTRL0_VSYNC_PERIOD_UNIT |
> +		  VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |
> +		  VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);
> +	if (m->flags & DRM_MODE_FLAG_PHSYNC)
> +		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
> +	if (m->flags & DRM_MODE_FLAG_PVSYNC)
> +		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
> +	/* Make sure Data Enable is high active by default */
> +	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
> +		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
> +	/*
> +	 * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,
> +	 * controllers VDCTRL0_DOTCLK is display centric.
> +	 * Drive on positive edge       -> display samples on falling edge
> +	 * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
> +	 */
> +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
> +		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
> +
> +	writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
> +
> +	/* Frame length in lines. */
> +	writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
> +
> +	/* Line length in units of clocks or pixels. */
> +	hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
> +	writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
> +	       VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),
> +	       mxsfb->base + LCDC_VDCTRL2);
> +
> +	writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
> +	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
> +	       mxsfb->base + LCDC_VDCTRL3);
> +
> +	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
> +	       mxsfb->base + LCDC_VDCTRL4);
> +
> +}
> +
>  static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
>  {
>  	u32 reg;
> @@ -236,7 +287,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  	struct drm_device *drm = mxsfb->crtc.dev;
>  	struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
>  	u32 bus_flags = mxsfb->connector->display_info.bus_flags;
> -	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
>  	int err;
>  
>  	/* Mandatory eLCDIF reset as per the Reference Manual */
> @@ -256,49 +306,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  			     bus_flags);
>  	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
>  
> -	writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
> -	       TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
> -	       mxsfb->base + mxsfb->devdata->transfer_count);
> -
> -	vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
> -
> -	vdctrl0 = VDCTRL0_ENABLE_PRESENT |	/* Always in DOTCLOCK mode */
> -		  VDCTRL0_VSYNC_PERIOD_UNIT |
> -		  VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |
> -		  VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);
> -	if (m->flags & DRM_MODE_FLAG_PHSYNC)
> -		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
> -	if (m->flags & DRM_MODE_FLAG_PVSYNC)
> -		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
> -	/* Make sure Data Enable is high active by default */
> -	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
> -		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
> -	/*
> -	 * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,
> -	 * controllers VDCTRL0_DOTCLK is display centric.
> -	 * Drive on positive edge       -> display samples on falling edge
> -	 * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING
> -	 */
> -	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
> -		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;
> -
> -	writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
> -
> -	/* Frame length in lines. */
> -	writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
> -
> -	/* Line length in units of clocks or pixels. */
> -	hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
> -	writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
> -	       VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),
> -	       mxsfb->base + LCDC_VDCTRL2);
> -
> -	writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
> -	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
> -	       mxsfb->base + LCDC_VDCTRL3);
> -
> -	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
> -	       mxsfb->base + LCDC_VDCTRL4);
> +	mxsfb_set_mode(mxsfb, bus_flags);
>  }
>  
>  static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,

Re: [PATCH v2 6/7] drm: mxsfb: Reorder mxsfb_crtc_mode_set_nofb()

[Lucas Stach]

Am Freitag, dem 11.03.2022 um 18:06 +0100 schrieb Marek Vasut:
> Reorder mxsfb_crtc_mode_set_nofb() such that all functions which perform
> register IO are called from one single location in this function. This is
> a clean up. No functional change.
> 
> 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>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index 14f5cc590a51b..497603964add8 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -289,13 +289,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  	u32 bus_flags = mxsfb->connector->display_info.bus_flags;
>  	int err;
>  
> -	/* Mandatory eLCDIF reset as per the Reference Manual */
> -	err = mxsfb_reset_block(mxsfb);
> -	if (err)
> -		return;
> -
> -	mxsfb_set_formats(mxsfb, bus_format);
> -
>  	if (mxsfb->bridge && mxsfb->bridge->timings)
>  		bus_flags = mxsfb->bridge->timings->input_bus_flags;
>  
> @@ -306,6 +299,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>  			     bus_flags);
>  	DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
>  
> +	/* Mandatory eLCDIF reset as per the Reference Manual */
> +	err = mxsfb_reset_block(mxsfb);
> +	if (err)
> +		return;
> +
> +	mxsfb_set_formats(mxsfb, bus_format);
> +
>  	mxsfb_set_mode(mxsfb, bus_flags);
>  }
>  

Re: [PATCH v2 7/7] drm: mxsfb: Factor out mxsfb_update_buffer()

[Lucas Stach]

Am Freitag, dem 11.03.2022 um 18:06 +0100 schrieb Marek Vasut:
> Pull functionality responsible for programming framebuffer address into
> the controller into dedicated function mxsfb_update_buffer(). This is a
> clean up. No functional change.
> 
> 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>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: No change
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index 497603964add8..4baa3db1f3d10 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -58,6 +58,22 @@ static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
>  	return gem->paddr;
>  }
>  
> +static void
> +mxsfb_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane,
> +		    bool both)
> +{
> +	dma_addr_t paddr;
> +
> +	paddr = mxsfb_get_fb_paddr(plane);
> +	if (!paddr)
> +		return;
> +
> +	if (both)
> +		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
> +
> +	writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> +}
> +
>  /*
>   * Setup the MXSFB registers for decoding the pixels out of the framebuffer and
>   * outputting them on the bus.
> @@ -352,7 +368,6 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
>  	struct drm_bridge_state *bridge_state;
>  	struct drm_device *drm = mxsfb->drm;
>  	u32 bus_format = 0;
> -	dma_addr_t paddr;
>  
>  	/* If there is a bridge attached to the LCDIF, use its bus format */
>  	if (mxsfb->bridge) {
> @@ -387,11 +402,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
>  	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
>  
>  	/* Write cur_buf as well to avoid an initial corrupt frame */
> -	paddr = mxsfb_get_fb_paddr(crtc->primary);
> -	if (paddr) {
> -		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
> -		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> -	}
> +	mxsfb_update_buffer(mxsfb, crtc->primary, true);
>  
>  	mxsfb_enable_controller(mxsfb);
>  
> @@ -491,11 +502,8 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
>  					      struct drm_atomic_state *state)
>  {
>  	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
> -	dma_addr_t paddr;
>  
> -	paddr = mxsfb_get_fb_paddr(plane);
> -	if (paddr)
> -		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> +	mxsfb_update_buffer(mxsfb, plane, false);
>  }
>  
>  static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,

Re: [PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Marek Vasut]

On 4/6/22 21:32, Lucas Stach wrote:
> Hi Marek,

Hi,

> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>> The current clock handling in the LCDIF driver is a convoluted mess.
> 
> Here we agree...
> 
>> 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.
>>
>> This requires slight reordering in mxsfb_crtc_atomic_enable() and
>> mxsfb_crtc_atomic_disable(), since all the register accesses must
>> happen only with clock enabled and clock frequency configuration
>> must happen with clock disabled.
>>
> ... on that one I don't. Please don't move the pixel clock into the RPM
> calls. We have a very well defined point between atomic enable/disable
> where the pixel clock is actually needed. All the other configuration
> accesses don't need the pixel clock to be active.

On the other hand, "all the other" configuration happens during probe, 
at which point all the clock are enabled anyway. And then during 
runtime, the pixel clock of this IP are either enabled or this IP is 
completely shut down.

So, where exactly does this patch make the clock handling any worse than 
it currently is ?

[...]

>> @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm,
>>   
>>   	ret = platform_get_irq(pdev, 0);
>>   	if (ret < 0)
>> -		goto err_vblank;
>> +		return ret;
>>   	mxsfb->irq = ret;
>>   
>> -	pm_runtime_get_sync(drm->dev);
>>   	ret = mxsfb_irq_install(drm, mxsfb->irq);
>> -	pm_runtime_put_sync(drm->dev);
>> -
> Here you do a bunch of stuff resulting in register access without
> enabling the clocks. I don't really see how this works, maybe because
> the clocks are still on when you run the probe?

Right

> Better enable the register access clocks here and then tell RPM about
> the fact that the device is running by calling pm_runtime_set_active
> before pm_runtime_enable. This way theoretically allows to build a
> kernel without CONFIG_PM and things still work, even if the RPM calls
> are stubs.

I would much rather move this driver to RPM and have RPM handle the 
clock altogether in one place.

[...]

>> @@ -411,9 +405,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);
>>
> Not the fault of your patch, but why is this a _sync call?

See 4201f4e848450 ("drm/mxsfb: Add pm_runtime calls to 
pipe_enable/disable"), likely the intention was for this call to 
complete before existing the atomic_disable.

Re: [PATCH v2 2/7] drm: mxsfb: Simplify LCDIF IRQ handling

[Marek Vasut]

On 4/6/22 21:41, Lucas Stach wrote:
> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>> The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation
>> from the LCDIF block already and this is called in mxsfb_load() before
>> request_irq(), so explicitly disabling IRQ using custom function like
>> mxsfb_irq_disable() is not needed, remove it.
>>
> 
> Have you checked that the drm_vblank_off in probe actually results in a
> call to mxsfb_crtc_disable_vblank? From my reading of the core code,
> this should be a no-op without a previous drm_vblank_on, so it would
> not result in the desired masking before the IRQ is requested.

I must've missed the vblank->enabled check, but then, I am also not 
getting any interrupts, so presumably they are already disabled after 
the IP is reset.

Re: [PATCH v2 4/7] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Marek Vasut]

On 4/6/22 21:45, Lucas Stach wrote:
> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>> Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
>> This is a clean up. No functional change.
>>
>> 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>
> 
> Hm, I don't see any real benefit, but I also fail to see why it
> shouldn't be done so:

The entire point of this series is to clean up the mxsfb and isolate 
lcdif (the original lcdif) from any of the common code.

Re: [PATCH v2 5/7] drm: mxsfb: Factor out mxsfb_set_mode()

[Marek Vasut]

On 4/6/22 21:47, Lucas Stach wrote:
> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>> Pull mode registers programming from mxsfb_enable_controller() into
>> dedicated function mxsfb_set_mode(). This is a clean up. No functional
>> change.
> 
> This one however looks like over-factorization to me. Why pull out a
> mode_set function out of a mode_set function?

The entire point of this series is to clean up the mxsfb and isolate 
lcdif (the original lcdif) from any of the common code. Then I can just 
replace those functions with lcdif mx8mp variant ones in the other lcdif 
driver, while keeping the common code in sync (until deduplication happens).

Re: [PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Lucas Stach]

Am Mittwoch, dem 06.04.2022 um 23:45 +0200 schrieb Marek Vasut:
> On 4/6/22 21:32, Lucas Stach wrote:
> > Hi Marek,
> 
> Hi,
> 
> > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> > > The current clock handling in the LCDIF driver is a convoluted mess.
> > 
> > Here we agree...
> > 
> > > 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.
> > > 
> > > This requires slight reordering in mxsfb_crtc_atomic_enable() and
> > > mxsfb_crtc_atomic_disable(), since all the register accesses must
> > > happen only with clock enabled and clock frequency configuration
> > > must happen with clock disabled.
> > > 
> > ... on that one I don't. Please don't move the pixel clock into the RPM
> > calls. We have a very well defined point between atomic enable/disable
> > where the pixel clock is actually needed. All the other configuration
> > accesses don't need the pixel clock to be active.
> 
> On the other hand, "all the other" configuration happens during probe, 
> at which point all the clock are enabled anyway. And then during 
> runtime, the pixel clock of this IP are either enabled or this IP is 
> completely shut down.
> 
> So, where exactly does this patch make the clock handling any worse than 
> it currently is ?
> 
There is an even stronger argument: runtime PM does not guarantee that
the runtime_suspend is actually called after you put your last
reference. A simple "echo on > /sys/your-device/power/control" will
prevent the device from ever entering runtime suspend. So if you have a
clock like the pixel clock that _needs_ to be disabled for
configuration purposes you _must_ not handle this clock via RPM, but
via explicit clock handling in the driver.

> [...]
> 
> > > @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm,
> > >   
> > >   	ret = platform_get_irq(pdev, 0);
> > >   	if (ret < 0)
> > > -		goto err_vblank;
> > > +		return ret;
> > >   	mxsfb->irq = ret;
> > >   
> > > -	pm_runtime_get_sync(drm->dev);
> > >   	ret = mxsfb_irq_install(drm, mxsfb->irq);
> > > -	pm_runtime_put_sync(drm->dev);
> > > -
> > Here you do a bunch of stuff resulting in register access without
> > enabling the clocks. I don't really see how this works, maybe because
> > the clocks are still on when you run the probe?
> 
> Right
> 
> > Better enable the register access clocks here and then tell RPM about
> > the fact that the device is running by calling pm_runtime_set_active
> > before pm_runtime_enable. This way theoretically allows to build a
> > kernel without CONFIG_PM and things still work, even if the RPM calls
> > are stubs.
> 
> I would much rather move this driver to RPM and have RPM handle the 
> clock altogether in one place.
> 
See above. Same argument applies. The driver should work without RPM
support.

> [...]
> 
> > > @@ -411,9 +405,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);
> > > 
> > Not the fault of your patch, but why is this a _sync call?
> 
> See 4201f4e848450 ("drm/mxsfb: Add pm_runtime calls to 
> pipe_enable/disable"), likely the intention was for this call to 
> complete before existing the atomic_disable.

Hm, still don't see why this would be necessary. But as this was just a
passing comment, we should revisit this later, not in the context of
this patch.

Regards,
Lucas

Re: [PATCH v2 2/7] drm: mxsfb: Simplify LCDIF IRQ handling

[Lucas Stach]

Am Donnerstag, dem 07.04.2022 um 00:03 +0200 schrieb Marek Vasut:
> On 4/6/22 21:41, Lucas Stach wrote:
> > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> > > The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation
> > > from the LCDIF block already and this is called in mxsfb_load() before
> > > request_irq(), so explicitly disabling IRQ using custom function like
> > > mxsfb_irq_disable() is not needed, remove it.
> > > 
> > 
> > Have you checked that the drm_vblank_off in probe actually results in a
> > call to mxsfb_crtc_disable_vblank? From my reading of the core code,
> > this should be a no-op without a previous drm_vblank_on, so it would
> > not result in the desired masking before the IRQ is requested.
> 
> I must've missed the vblank->enabled check, but then, I am also not 
> getting any interrupts, so presumably they are already disabled after 
> the IP is reset.

Yep, it should be the default for every peripheral to not send any
unsolicited interrupts. But then I don't see explicit reset of the IP
in the driver probe. So maybe this is a workaround for situation where
something running before Linux has already enabled the display
controller and for whatever reason also enabled the interrupt
requests? 

Either we should have a proper reset of the block in the probe path, or
this interrupt masking must be kept in one form or the other. My vote
would be on just masking the IRQs and dropping the useless
drm_vblank_off.

Regards,
Lucas

Re: [PATCH v2 4/7] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Lucas Stach]

Am Donnerstag, dem 07.04.2022 um 00:05 +0200 schrieb Marek Vasut:
> On 4/6/22 21:45, Lucas Stach wrote:
> > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> > > Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
> > > This is a clean up. No functional change.
> > > 
> > > 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>
> > 
> > Hm, I don't see any real benefit, but I also fail to see why it
> > shouldn't be done so:
> 
> The entire point of this series is to clean up the mxsfb and isolate 
> lcdif (the original lcdif) from any of the common code.

Actually, just use drm_fb_cma_get_gem_addr() instead?

Regards,
Lucas

Re: [PATCH v2 4/7] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Marek Vasut]

On 4/7/22 11:47, Lucas Stach wrote:
> Am Donnerstag, dem 07.04.2022 um 00:05 +0200 schrieb Marek Vasut:
>> On 4/6/22 21:45, Lucas Stach wrote:
>>> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>>>> Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
>>>> This is a clean up. No functional change.
>>>>
>>>> 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>
>>>
>>> Hm, I don't see any real benefit, but I also fail to see why it
>>> shouldn't be done so:
>>
>> The entire point of this series is to clean up the mxsfb and isolate
>> lcdif (the original lcdif) from any of the common code.
> 
> Actually, just use drm_fb_cma_get_gem_addr() instead?

That function seems to add only extra code that is executed, but does 
not do away with the !fb check anyway. So, why ? (Also, seems unrelated 
to this patch)

Re: [PATCH v2 4/7] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions

[Lucas Stach]

Am Montag, dem 11.04.2022 um 00:17 +0200 schrieb Marek Vasut:
> On 4/7/22 11:47, Lucas Stach wrote:
> > Am Donnerstag, dem 07.04.2022 um 00:05 +0200 schrieb Marek Vasut:
> > > On 4/6/22 21:45, Lucas Stach wrote:
> > > > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
> > > > > Move mxsfb_get_fb_paddr() out of the way, away from register IO functions.
> > > > > This is a clean up. No functional change.
> > > > > 
> > > > > 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>
> > > > 
> > > > Hm, I don't see any real benefit, but I also fail to see why it
> > > > shouldn't be done so:
> > > 
> > > The entire point of this series is to clean up the mxsfb and isolate
> > > lcdif (the original lcdif) from any of the common code.
> > 
> > Actually, just use drm_fb_cma_get_gem_addr() instead?
> 
> That function seems to add only extra code that is executed, 
> 
Yep, and thus it is the correct way to do it, as it actually takes into
account the FB offset parameter. Currently mxsfb seems to just do the
wrong thing when the FB is not at offset 0 in the GEM object.

> but does not do away with the !fb check anyway. 
> 
And that one seems bogus. If you have no FB there is no way you can
reasonably start scanout or flip to the next buffer. What would you
scan out in that case? Random memory? FB should never be NULL in those
code paths.

> So, why ? (Also, seems unrelated to this patch)

Because you aim to clean up the driver and make the code reusable, so
why not use the reusable and correct DRM helper?

Regards,
Lucas

Re: [PATCH v2 2/7] drm: mxsfb: Simplify LCDIF IRQ handling

[Marek Vasut]

On 4/7/22 10:01, Lucas Stach wrote:
> Am Donnerstag, dem 07.04.2022 um 00:03 +0200 schrieb Marek Vasut:
>> On 4/6/22 21:41, Lucas Stach wrote:
>>> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>>>> The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation
>>>> from the LCDIF block already and this is called in mxsfb_load() before
>>>> request_irq(), so explicitly disabling IRQ using custom function like
>>>> mxsfb_irq_disable() is not needed, remove it.
>>>>
>>>
>>> Have you checked that the drm_vblank_off in probe actually results in a
>>> call to mxsfb_crtc_disable_vblank? From my reading of the core code,
>>> this should be a no-op without a previous drm_vblank_on, so it would
>>> not result in the desired masking before the IRQ is requested.
>>
>> I must've missed the vblank->enabled check, but then, I am also not
>> getting any interrupts, so presumably they are already disabled after
>> the IP is reset.
> 
> Yep, it should be the default for every peripheral to not send any
> unsolicited interrupts. But then I don't see explicit reset of the IP
> in the driver probe. So maybe this is a workaround for situation where
> something running before Linux has already enabled the display
> controller and for whatever reason also enabled the interrupt
> requests?
> 
> Either we should have a proper reset of the block in the probe path, or
> this interrupt masking must be kept in one form or the other. My vote
> would be on just masking the IRQs and dropping the useless
> drm_vblank_off.

I can just discard this patch, OK.

Re: [PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Marek Vasut]

On 4/7/22 09:57, Lucas Stach wrote:
> Am Mittwoch, dem 06.04.2022 um 23:45 +0200 schrieb Marek Vasut:
>> On 4/6/22 21:32, Lucas Stach wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>>>> The current clock handling in the LCDIF driver is a convoluted mess.
>>>
>>> Here we agree...
>>>
>>>> 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.
>>>>
>>>> This requires slight reordering in mxsfb_crtc_atomic_enable() and
>>>> mxsfb_crtc_atomic_disable(), since all the register accesses must
>>>> happen only with clock enabled and clock frequency configuration
>>>> must happen with clock disabled.
>>>>
>>> ... on that one I don't. Please don't move the pixel clock into the RPM
>>> calls. We have a very well defined point between atomic enable/disable
>>> where the pixel clock is actually needed. All the other configuration
>>> accesses don't need the pixel clock to be active.
>>
>> On the other hand, "all the other" configuration happens during probe,
>> at which point all the clock are enabled anyway. And then during
>> runtime, the pixel clock of this IP are either enabled or this IP is
>> completely shut down.
>>
>> So, where exactly does this patch make the clock handling any worse than
>> it currently is ?
>>
> There is an even stronger argument: runtime PM does not guarantee that
> the runtime_suspend is actually called after you put your last
> reference. A simple "echo on > /sys/your-device/power/control" will
> prevent the device from ever entering runtime suspend. So if you have a
> clock like the pixel clock that _needs_ to be disabled for
> configuration purposes you _must_ not handle this clock via RPM, but
> via explicit clock handling in the driver.

OK, patch discarded.