[PATCH v2 0/2] drm/imx/lcdc: Implement DRM driver for imx21

[PATCH v2 0/2] drm/imx/lcdc: Implement DRM driver for imx21

[Uwe Kleine-König]

Hello,

Compared to (implicit) v1[1] the device tree binding should be good
enough now to pass the dts linters.

As before, Marian Cichy is the declared author of the driver, but he
left Pengutronix, so the email address doesn't work any more.

This is based on top of v6.1 + 93266da2409b ("dt-bindings: display:
Convert fsl,imx-fb.txt to dt-schema") which is currently in next via
branch 'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git .

Best regards
Uwe

[1] https://lore.kernel.org/dri-devel/20220128105849.368438-1-u.kleine-koenig@pengutronix.de

Marian Cichy (1):
  drm/imx/lcdc: Implement DRM driver for imx21

Uwe Kleine-König (1):
  dt-bindings: display: imx: Describe drm binding for fsl,imx-lcdc

 .../bindings/display/imx/fsl,imx-lcdc.yaml    |  45 +-
 drivers/gpu/drm/imx/Kconfig                   |   7 +
 drivers/gpu/drm/imx/Makefile                  |   2 +
 drivers/gpu/drm/imx/imx-lcdc.c                | 610 ++++++++++++++++++
 4 files changed, 663 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/imx/imx-lcdc.c


base-commit: 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
prerequisite-patch-id: c3ef3de02516b5c159e76b40d2b4348a5ce0fe51
-- 
2.38.1

[PATCH v2 1/2] dt-bindings: display: imx: Describe drm binding for fsl,imx-lcdc

[Uwe Kleine-König]

Modify the existing (fb-like) binding to support the drm-like binding in
parallel.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../bindings/display/imx/fsl,imx-lcdc.yaml    | 45 ++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml
index 35a8fff036ca..2a8225b10890 100644
--- a/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml
+++ b/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml
@@ -21,6 +21,9 @@ properties:
               - fsl,imx25-fb
               - fsl,imx27-fb
           - const: fsl,imx21-fb
+      - items:
+          - const: fsl,imx25-lcdc
+          - const: fsl,imx21-lcdc
 
   clocks:
     maxItems: 3
@@ -31,6 +34,9 @@ properties:
       - const: ahb
       - const: per
 
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+
   display:
     $ref: /schemas/types.yaml#/definitions/phandle
 
@@ -59,17 +65,54 @@ properties:
     description:
       LCDC Sharp Configuration Register value.
 
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - fsl,imx1-lcdc
+          - fsl,imx21-lcdc
+then:
+  properties:
+    display: false
+    fsl,dmacr: false
+    fsl,lpccr: false
+    fsl,lscr1: false
+
+  required:
+    - port
+
+else:
+  properties:
+    port: false
+
+  required:
+    - display
+
 required:
   - compatible
   - clocks
   - clock-names
-  - display
   - interrupts
   - reg
 
 additionalProperties: false
 
 examples:
+  - |
+    lcdc@53fbc000 {
+        compatible = "fsl,imx25-lcdc", "fsl,imx21-lcdc";
+        reg = <0x53fbc000 0x4000>;
+        interrupts = <39>;
+        clocks = <&clks 103>, <&clks 66>, <&clks 49>;
+        clock-names = "ipg", "ahb", "per";
+
+        port {
+            parallel_out: endpoint {
+              remote-endpoint = <&panel_in>;
+            };
+        };
+    };
   - |
     imxfb: fb@10021000 {
         compatible = "fsl,imx21-fb";
-- 
2.38.1

[PATCH v2 2/2] drm/imx/lcdc: Implement DRM driver for imx21

[Uwe Kleine-König]

From: Marian Cichy <m.cichy@pengutronix.de>

Add support for the LCD Controller found on i.MX21 and i.MX25.

It targets to be a drop in replacement for the imx-fb driver.

Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
[ukl: Rebase to v6.1, various smaller fixes]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpu/drm/imx/Kconfig    |   7 +
 drivers/gpu/drm/imx/Makefile   |   2 +
 drivers/gpu/drm/imx/imx-lcdc.c | 610 +++++++++++++++++++++++++++++++++
 3 files changed, 619 insertions(+)
 create mode 100644 drivers/gpu/drm/imx/imx-lcdc.c

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index fd5b2471fdf0..af5c6cb8c445 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -41,3 +41,10 @@ config DRM_IMX_HDMI
 	  Choose this if you want to use HDMI on i.MX6.
 
 source "drivers/gpu/drm/imx/dcss/Kconfig"
+
+config DRM_IMX_LCDC
+	tristate "Freescale i.MX LCDC displays"
+	depends on DRM && (ARCH_MXC || COMPILE_TEST)
+	select DRM_KMS_CMA_HELPER
+	help
+	  Found on i.MX1, i.MX21, i.MX25 and i.MX27.
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index b644deffe948..1f96de7f15b4 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
 
 obj-$(CONFIG_DRM_IMX_HDMI) += dw_hdmi-imx.o
 obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
+
+obj-$(CONFIG_DRM_IMX_LCDC) += imx-lcdc.o
diff --git a/drivers/gpu/drm/imx/imx-lcdc.c b/drivers/gpu/drm/imx/imx-lcdc.c
new file mode 100644
index 000000000000..14d4962cecfd
--- /dev/null
+++ b/drivers/gpu/drm/imx/imx-lcdc.c
@@ -0,0 +1,610 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
+
+#include "drm/drm_fourcc.h"
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_dma_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_vblank.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define IMX21LCDC_LSSAR         0x0000 /* LCDC Screen Start Address Register */
+#define IMX21LCDC_LSR           0x0004 /* LCDC Size Register */
+#define IMX21LCDC_LVPWR         0x0008 /* LCDC Virtual Page Width Register */
+#define IMX21LCDC_LCPR          0x000C /* LCDC Cursor Position Register */
+#define IMX21LCDC_LCWHB         0x0010 /* LCDC Cursor Width Height and Blink Register*/
+#define IMX21LCDC_LCCMR         0x0014 /* LCDC Color Cursor Mapping Register */
+#define IMX21LCDC_LPCR          0x0018 /* LCDC Panel Configuration Register */
+#define IMX21LCDC_LHCR          0x001C /* LCDC Horizontal Configuration Register */
+#define IMX21LCDC_LVCR          0x0020 /* LCDC Vertical Configuration Register */
+#define IMX21LCDC_LPOR          0x0024 /* LCDC Panning Offset Register */
+#define IMX21LCDC_LSCR          0x0028 /* LCDC Sharp Configuration Register */
+#define IMX21LCDC_LPCCR         0x002C /* LCDC PWM Contrast Control Register */
+#define IMX21LCDC_LDCR          0x0030 /* LCDC DMA Control Register */
+#define IMX21LCDC_LRMCR         0x0034 /* LCDC Refresh Mode Control Register */
+#define IMX21LCDC_LICR          0x0038 /* LCDC Interrupt Configuration Register */
+#define IMX21LCDC_LIER          0x003C /* LCDC Interrupt Enable Register */
+#define IMX21LCDC_LISR          0x0040 /* LCDC Interrupt Status Register */
+#define IMX21LCDC_LGWSAR        0x0050 /* LCDC Graphic Window Start Address Register */
+#define IMX21LCDC_LGWSR         0x0054 /* LCDC Graph Window Size Register */
+#define IMX21LCDC_LGWVPWR       0x0058 /* LCDC Graphic Window Virtual Page Width Register */
+#define IMX21LCDC_LGWPOR        0x005C /* LCDC Graphic Window Panning Offset Register */
+#define IMX21LCDC_LGWPR         0x0060 /* LCDC Graphic Window Position Register */
+#define IMX21LCDC_LGWCR         0x0064 /* LCDC Graphic Window Control Register */
+#define IMX21LCDC_LGWDCR        0x0068 /* LCDC Graphic Window DMA Control Register */
+#define IMX21LCDC_LAUSCR        0x0080 /* LCDC AUS Mode Control Register */
+#define IMX21LCDC_LAUSCCR       0x0084 /* LCDC AUS Mode Cursor Control Register */
+#define IMX21LCDC_BGLUT         0x0800 /* Background Lookup Table */
+#define IMX21LCDC_GWLUT         0x0C00 /* Graphic Window Lookup Table */
+
+#define IMX21LCDC_LCPR_CC0 BIT(30) /* Cursor Control Bit 0 */
+#define IMX21LCDC_LCPR_CC1 BIT(31) /* Cursor Control Bit 1 */
+
+/* Values HSYNC, VSYNC and Framesize Register */
+#define IMX21LCDC_LHCR_H_WIDTH(val)	(FIELD_PREP(GENMASK(31, 26), (val)))
+#define IMX21LCDC_LHCR_H_BPORCH(val)	(FIELD_PREP(GENMASK(7, 0), (val)))
+#define IMX21LCDC_LHCR_H_FPORCH(val)	(FIELD_PREP(GENMASK(15, 8), (val)))
+
+#define IMX21LCDC_LVCR_V_WIDTH(val)	(FIELD_PREP(GENMASK(31, 26), (val)))
+#define IMX21LCDC_LVCR_V_BPORCH(val)	(FIELD_PREP(GENMASK(7, 0), (val)))
+#define IMX21LCDC_LVCR_V_FPORCH(val)	(FIELD_PREP(GENMASK(15, 8), (val)))
+
+#define IMX21LCDC_FRAME_WIDTH(val)	(((val) / 16) << 20)
+#define IMX21LCDC_FRAME_HEIGHT(val)	(val)
+
+/* Values for LPCR Register */
+#define IMX21LCDC_PCD(val)		(FIELD_PREP(GENMASK(5, 0), --(val)))
+#define IMX21LCDC_SHARP(val)		(FIELD_PREP(GENMASK(6, 6), (val)))
+#define IMX21LCDC_SCLKSEL(val)		(FIELD_PREP(GENMASK(7, 7), (val)))
+#define IMX21LCDC_ACD(val)		(FIELD_PREP(GENMASK(14, 8), (val)))
+#define IMX21LCDC_ACDSEL(val)		(FIELD_PREP(GENMASK(15, 15), (val)))
+#define IMX21LCDC_REV_VS(val)		(FIELD_PREP(GENMASK(16, 16), (val)))
+#define IMX21LCDC_SWAP_SEL(val)	(FIELD_PREP(GENMASK(17, 17), (val)))
+#define IMX21LCDC_END_SEL(val)		(FIELD_PREP(GENMASK(18, 18), (val)))
+#define IMX21LCDC_SCLKIDLE(val)	(FIELD_PREP(GENMASK(19, 19), (val)))
+#define IMX21LCDC_OEPOL(val)		(FIELD_PREP(GENMASK(20, 20), (val)))
+#define IMX21LCDC_CLKPOL(val)		(FIELD_PREP(GENMASK(21, 21), (val)))
+#define IMX21LCDC_LPPOL(val)		(FIELD_PREP(GENMASK(22, 22), (val)))
+#define IMX21LCDC_FLMPOL(val)		(FIELD_PREP(GENMASK(23, 23), (val)))
+#define IMX21LCDC_PIXPOL(val)		(FIELD_PREP(GENMASK(24, 24), (val)))
+#define IMX21LCDC_BPIX(val)		(FIELD_PREP(GENMASK(27, 25), (val)))
+#define IMX21LCDC_PBSIZ(val)		(FIELD_PREP(GENMASK(29, 28), (val)))
+#define IMX21LCDC_COLOR(val)		(FIELD_PREP(GENMASK(30, 30), (val)))
+#define IMX21LCDC_TFT(val)		(FIELD_PREP(GENMASK(31, 31), (val)))
+
+#define INTR_EOF BIT(1) /* VBLANK Interrupt Bit */
+
+#define BPP_RGB565 0x05
+
+#define LCDC_MIN_XRES 64
+#define LCDC_MIN_YRES 64
+
+#define LCDC_MAX_XRES 1024
+#define LCDC_MAX_YRES 1024
+
+struct imx_lcdc {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	const struct drm_display_mode *mode;
+	struct drm_connector connector;
+	struct drm_panel *panel;
+	struct drm_bridge *bridge;
+	void __iomem *base;
+
+	struct clk *clk_ipg;
+	struct clk *clk_ahb;
+	struct clk *clk_per;
+};
+
+static const u32 imx_lcdc_formats[] = {
+	DRM_FORMAT_RGB565,
+};
+
+static inline struct imx_lcdc *drm_to_lcdc(struct drm_device *drm)
+{
+	return container_of(drm, struct imx_lcdc, drm);
+}
+
+static unsigned int imx_lcdc_get_format(unsigned int drm_format)
+{
+	unsigned int bpp;
+
+	switch (drm_format) {
+	default:
+		DRM_WARN("Format not supported - fallback to RGB565\n");
+		fallthrough;
+	case DRM_FORMAT_RGB565:
+		bpp = BPP_RGB565;
+		break;
+	}
+
+	return bpp;
+}
+
+static int imx_lcdc_connector_get_modes(struct drm_connector *connector)
+{
+	struct imx_lcdc *lcdc = drm_to_lcdc(connector->dev);
+
+	if (lcdc->panel)
+		return drm_panel_get_modes(lcdc->panel, connector);
+
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs imx_lcdc_connector_hfuncs = {
+	.get_modes = imx_lcdc_connector_get_modes,
+};
+
+static const struct drm_connector_funcs imx_lcdc_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
+					 struct drm_plane_state *old_state,
+					 bool mode_set)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_framebuffer *fb = new_state->fb;
+	struct imx_lcdc *lcdc = drm_to_lcdc(pipe->crtc.dev);
+	unsigned int bpp;
+	unsigned int lvcr; /* LVCR-Register value */
+	unsigned int lhcr; /* LHCR-Register value */
+	unsigned int framesize;
+	dma_addr_t addr;
+
+	addr = drm_fb_dma_get_gem_addr(fb, new_state, 0);
+	/* The LSSAR register specifies the LCD screen start address (SSA). */
+	writel(addr, lcdc->base + IMX21LCDC_LSSAR);
+
+	if (!mode_set)
+		return;
+
+	/* Disable PER clock to make register write possible */
+	if (old_state && old_state->crtc && old_state->crtc->enabled)
+		clk_disable_unprepare(lcdc->clk_per);
+
+	/* Framesize */
+	framesize = IMX21LCDC_FRAME_WIDTH(crtc->mode.hdisplay);
+	framesize |= IMX21LCDC_FRAME_HEIGHT(crtc->mode.vdisplay);
+	writel(framesize, lcdc->base + IMX21LCDC_LSR);
+
+	/* HSYNC */
+	lhcr = IMX21LCDC_LHCR_H_FPORCH(crtc->mode.hsync_start - crtc->mode.hdisplay - 1);
+	lhcr |= IMX21LCDC_LHCR_H_WIDTH(crtc->mode.hsync_end - crtc->mode.hsync_start - 1);
+	lhcr |= IMX21LCDC_LHCR_H_BPORCH(crtc->mode.htotal - crtc->mode.hsync_end - 3);
+	writel(lhcr, lcdc->base + IMX21LCDC_LHCR);
+
+	/* VSYNC */
+	lvcr = IMX21LCDC_LVCR_V_FPORCH(crtc->mode.vsync_start - crtc->mode.vdisplay);
+	lvcr |= IMX21LCDC_LVCR_V_WIDTH(crtc->mode.vsync_end - crtc->mode.vsync_start);
+	lvcr |= IMX21LCDC_LVCR_V_BPORCH(crtc->mode.vtotal - crtc->mode.vsync_end);
+	writel(lvcr, lcdc->base + IMX21LCDC_LVCR);
+
+	bpp = imx_lcdc_get_format(fb->format->format);
+	writel(readl(lcdc->base + IMX21LCDC_LPCR) | IMX21LCDC_BPIX(bpp),
+	       lcdc->base + IMX21LCDC_LPCR);
+
+	/* Virtual Page Width */
+	writel(new_state->fb->pitches[0] / 4, lcdc->base + IMX21LCDC_LVPWR);
+
+	/* Enable PER clock */
+	if (new_state->crtc->enabled)
+		clk_prepare_enable(lcdc->clk_per);
+}
+
+static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
+				 struct drm_crtc_state *crtc_state,
+				 struct drm_plane_state *plane_state)
+{
+	int ret;
+	int clk_div;
+	int bpp;
+	struct imx_lcdc *lcdc = drm_to_lcdc(pipe->crtc.dev);
+	struct drm_display_mode *mode = &pipe->crtc.mode;
+	struct drm_display_info *disp_info = &pipe->connector->display_info;
+	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
+	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
+	const int data_enable_pol =
+		(disp_info->bus_flags & DRM_BUS_FLAG_DE_HIGH) ? 0 : 1;
+	const int clk_pol =
+		(disp_info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) ? 0 : 1;
+
+	drm_panel_prepare(lcdc->panel);
+
+	clk_div = DIV_ROUND_CLOSEST_ULL(clk_get_rate(lcdc->clk_per),
+					mode->clock * 1000);
+	bpp = imx_lcdc_get_format(plane_state->fb->format->format);
+
+	writel(IMX21LCDC_PCD(clk_div) | IMX21LCDC_LPPOL(hsync_pol) | IMX21LCDC_FLMPOL(vsync_pol) |
+	       IMX21LCDC_OEPOL(data_enable_pol) | IMX21LCDC_TFT(1) | IMX21LCDC_COLOR(1) |
+	       IMX21LCDC_PBSIZ(3) | IMX21LCDC_BPIX(bpp) | IMX21LCDC_SCLKSEL(1) |
+	       IMX21LCDC_PIXPOL(0) | IMX21LCDC_CLKPOL(clk_pol),
+	       lcdc->base + IMX21LCDC_LPCR);
+
+	/* 0px panning offset */
+	writel(0x00000000, lcdc->base + IMX21LCDC_LPOR);
+
+	/* disable hardware cursor */
+	writel(readl(lcdc->base + IMX21LCDC_LCPR) & ~(IMX21LCDC_LCPR_CC0 | IMX21LCDC_LCPR_CC1),
+	       lcdc->base + IMX21LCDC_LCPR);
+
+	ret = clk_prepare_enable(lcdc->clk_ipg);
+	if (ret) {
+		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
+		return;
+	}
+	ret = clk_prepare_enable(lcdc->clk_ahb);
+	if (ret) {
+		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
+		clk_disable_unprepare(lcdc->clk_ipg);
+		return;
+	}
+
+	imx_lcdc_update_hw_registers(pipe, NULL, true);
+	drm_panel_enable(lcdc->panel);
+
+	/* Enable VBLANK Interrupt */
+	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
+}
+
+static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct imx_lcdc *lcdc = drm_to_lcdc(pipe->crtc.dev);
+	struct drm_panel *panel = lcdc->panel;
+	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	struct drm_pending_vblank_event *event;
+
+	drm_panel_disable(panel);
+
+	clk_disable_unprepare(lcdc->clk_ahb);
+	clk_disable_unprepare(lcdc->clk_ipg);
+
+	if (pipe->crtc.enabled)
+		clk_disable_unprepare(lcdc->clk_per);
+
+	drm_panel_unprepare(panel);
+
+	spin_lock_irq(&lcdc->drm.event_lock);
+	event = crtc->state->event;
+	if (event) {
+		crtc->state->event = NULL;
+		drm_crtc_send_vblank_event(crtc, event);
+	}
+	spin_unlock_irq(&lcdc->drm.event_lock);
+
+	/* Disable VBLANK Interrupt */
+	writel(0, lcdc->base + IMX21LCDC_LIER);
+}
+
+static int imx_lcdc_check_mode_change(struct drm_display_mode *new_mode,
+				      struct drm_display_mode *old_mode)
+{
+	if (old_mode->hdisplay != new_mode->hdisplay ||
+	    old_mode->vdisplay != new_mode->vdisplay)
+		return true;
+	return false;
+}
+
+static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
+			       struct drm_plane_state *plane_state,
+			       struct drm_crtc_state *crtc_state)
+{
+	const struct drm_display_mode *mode = &crtc_state->mode;
+
+	if ((mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES) ||
+	    (mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES) ||
+	    (mode->hdisplay & 0x10)) { /* must be multiple of 16 */
+		DRM_ERROR("unsupported display mode (%u x %u)\n",
+			  mode->hdisplay, mode->vdisplay);
+		return -EINVAL;
+	}
+
+	crtc_state->mode_changed = imx_lcdc_check_mode_change(&crtc_state->mode,
+							      &pipe->crtc.state->mode);
+
+	return 0;
+}
+
+static void imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *old_state)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_framebuffer *fb = new_state->fb;
+	struct drm_framebuffer *old_fb = old_state->fb;
+	struct drm_crtc *old_crtc = old_state->crtc;
+	bool mode_changed = false;
+
+	if (old_fb && old_fb->format != fb->format)
+		mode_changed = true;
+	else if (old_crtc != crtc)
+		mode_changed = true;
+
+	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
+
+	if (event) {
+		crtc->state->event = NULL;
+
+		spin_lock_irq(&crtc->dev->event_lock);
+
+		if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
+			drm_crtc_arm_vblank_event(crtc, event);
+		else
+			drm_crtc_send_vblank_event(crtc, event);
+
+		spin_unlock_irq(&crtc->dev->event_lock);
+	}
+}
+
+static const struct drm_simple_display_pipe_funcs imx_lcdc_pipe_funcs = {
+	.enable = imx_lcdc_pipe_enable,
+	.disable = imx_lcdc_pipe_disable,
+	.check = imx_lcdc_pipe_check,
+	.update = imx_lcdc_pipe_update,
+	.prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
+};
+
+static const struct drm_mode_config_funcs imx_lcdc_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_mode_config_helper_funcs imx_lcdc_mode_config_helpers = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static void imx_lcdc_release(struct drm_device *drm)
+{
+	struct imx_lcdc *lcdc = drm_to_lcdc(drm);
+
+	drm_kms_helper_poll_fini(drm);
+	kfree(lcdc);
+}
+
+DEFINE_DRM_GEM_DMA_FOPS(imx_lcdc_drm_fops);
+
+static struct drm_driver imx_lcdc_drm_driver = {
+	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops = &imx_lcdc_drm_fops,
+	DRM_GEM_DMA_DRIVER_OPS_VMAP,
+	.release = imx_lcdc_release,
+	.name = "imx-lcdc",
+	.desc = "i.MX LCDC driver",
+	.date = "20200716",
+};
+
+static const struct of_device_id imx_lcdc_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx21-lcdc",
+	},
+	{
+		.compatible = "fsl,imx25-lcdc",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_lcdc_of_dev_id);
+
+static irqreturn_t irq_handler(int irq, void *arg)
+{
+	struct imx_lcdc *lcdc = (struct imx_lcdc *)arg;
+	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	unsigned int status;
+
+	status = readl(lcdc->base + IMX21LCDC_LISR);
+
+	if (status & INTR_EOF) {
+		drm_crtc_handle_vblank(crtc);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int imx_lcdc_probe(struct platform_device *pdev)
+{
+	struct imx_lcdc *lcdc;
+	struct drm_device *drm;
+	int irq;
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	lcdc = devm_drm_dev_alloc(&pdev->dev, &imx_lcdc_drm_driver,
+				  struct imx_lcdc, drm);
+	if (!lcdc)
+		return -ENOMEM;
+
+	drm = &lcdc->drm;
+
+	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lcdc->base)) {
+		dev_err(dev, "Cannot get IO memory\n");
+		return PTR_ERR(lcdc->base);
+	}
+
+	/* Panel */
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &lcdc->panel, &lcdc->bridge);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to find panel or bridge: %pe", ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Get Clocks */
+	lcdc->clk_ipg = devm_clk_get(dev, "ipg");
+	if (IS_ERR(lcdc->clk_ipg)) {
+		dev_err(dev, "Failed to get %s clk: %pe\n", "ipg",
+			lcdc->clk_ipg);
+		return PTR_ERR(lcdc->clk_ipg);
+	}
+
+	lcdc->clk_ahb = devm_clk_get(dev, "ahb");
+	if (IS_ERR(lcdc->clk_ahb)) {
+		dev_err(dev, "Failed to get %s clk: %pe\n", "ahb",
+			lcdc->clk_ahb);
+		return PTR_ERR(lcdc->clk_ahb);
+	}
+
+	lcdc->clk_per = devm_clk_get(dev, "per");
+	if (IS_ERR(lcdc->clk_per)) {
+		dev_err(dev, "Failed to get %s clk: %pe\n", "per",
+			lcdc->clk_per);
+		return PTR_ERR(lcdc->clk_per);
+	}
+
+	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(drm->dev, "Cannot set DMA Mask\n");
+		return ret;
+	}
+
+	/* Modeset init */
+	drm_mode_config_init(drm);
+
+	/* CRTC, Plane, Encoder */
+	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe, &imx_lcdc_pipe_funcs, imx_lcdc_formats,
+					   ARRAY_SIZE(imx_lcdc_formats), NULL, &lcdc->connector);
+	if (ret < 0) {
+		dev_err(drm->dev, "Cannot setup simple display pipe\n");
+		return ret;
+	}
+
+	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
+	if (ret < 0) {
+		dev_err(drm->dev, "Failed to initialize vblank\n");
+		return ret;
+	}
+
+	if (lcdc->bridge) {
+		ret = drm_simple_display_pipe_attach_bridge(&lcdc->pipe,
+							    lcdc->bridge);
+		if (ret) {
+			dev_err(drm->dev, "Cannot connect bridge: %pe\n",
+				ERR_PTR(ret));
+			return ret;
+		}
+	}
+
+	/* Connector */
+	drm_connector_helper_add(&lcdc->connector, &imx_lcdc_connector_hfuncs);
+	drm_connector_init(drm, &lcdc->connector, &imx_lcdc_connector_funcs,
+			   DRM_MODE_CONNECTOR_DPI);
+
+	/*
+	 * The LCDC controller does not have an enable bit. The
+	 * controller starts directly when the clocks are enabled.
+	 * If the clocks are enabled when the controller is not yet
+	 * programmed with proper register values (enabled at the
+	 * bootloader, for example) then it just goes into some undefined
+	 * state.
+	 * To avoid this issue, let's enable and disable LCDC IPG,
+	 * PER and AHB clock so that we force some kind of 'reset'
+	 * to the LCDC block.
+	 */
+
+	ret = clk_prepare_enable(lcdc->clk_ipg);
+	if (ret) {
+		dev_err(dev, "Cannot enable ipg clock\n");
+		return ret;
+	}
+	clk_disable_unprepare(lcdc->clk_ipg);
+
+	ret = clk_prepare_enable(lcdc->clk_per);
+	if (ret) {
+		dev_err(dev, "Cannot enable per clock\n");
+		return ret;
+	}
+	clk_disable_unprepare(lcdc->clk_per);
+
+	ret = clk_prepare_enable(lcdc->clk_ahb);
+	if (ret) {
+		dev_err(dev, "Cannot enable ahb clock\n");
+		return ret;
+	}
+	clk_disable_unprepare(lcdc->clk_ahb);
+
+	drm->mode_config.min_width = LCDC_MIN_XRES;
+	drm->mode_config.max_width = LCDC_MAX_XRES;
+	drm->mode_config.min_height = LCDC_MIN_YRES;
+	drm->mode_config.max_height = LCDC_MAX_YRES;
+	drm->mode_config.preferred_depth = 16;
+	drm->mode_config.funcs = &imx_lcdc_mode_config_funcs;
+	drm->mode_config.helper_private = &imx_lcdc_mode_config_helpers;
+
+	drm_mode_config_reset(drm);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, irq, irq_handler, 0, "imx-lcdc", lcdc);
+	if (ret < 0) {
+		dev_err(drm->dev, "Failed to install IRQ handler\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, drm);
+
+	ret = drm_dev_register(&lcdc->drm, 0);
+	if (ret) {
+		dev_err(dev, "Cannot register device\n");
+		return ret;
+	}
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int imx_lcdc_remove(struct platform_device *pdev)
+{
+	struct imx_lcdc *lcdc = drm_to_lcdc(platform_get_drvdata(pdev));
+
+	drm_dev_unregister(&lcdc->drm);
+	drm_atomic_helper_shutdown(&lcdc->drm);
+
+	return 0;
+}
+
+static void imx_lcdc_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
+static struct platform_driver imx_lcdc_driver = {
+	.driver = {
+		.name = "imx-lcdc",
+		.of_match_table = imx_lcdc_of_dev_id,
+	},
+	.probe = imx_lcdc_probe,
+	.remove = imx_lcdc_remove,
+	.shutdown = imx_lcdc_shutdown,
+};
+module_platform_driver(imx_lcdc_driver);
+
+MODULE_AUTHOR("Marian Cichy <M.Cichy@pengutronix.de>");
+MODULE_DESCRIPTION("Freescale i.MX LCDC driver");
+MODULE_LICENSE("GPL");
-- 
2.38.1

Re: [PATCH v2 2/2] drm/imx/lcdc: Implement DRM driver for imx21

[Philipp Zabel]

On Wed, Dec 14, 2022 at 12:59:21PM +0100, Uwe Kleine-König wrote:
> From: Marian Cichy <m.cichy@pengutronix.de>
> 
> Add support for the LCD Controller found on i.MX21 and i.MX25.
> 
> It targets to be a drop in replacement for the imx-fb driver.
> 
> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
> [ukl: Rebase to v6.1, various smaller fixes]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/Kconfig    |   7 +
>  drivers/gpu/drm/imx/Makefile   |   2 +
>  drivers/gpu/drm/imx/imx-lcdc.c | 610 +++++++++++++++++++++++++++++++++

We are in the process of placing imx drivers in subdirectories,
could you move this into drivers/gpu/drm/imx/lcdc/ ?

>  3 files changed, 619 insertions(+)
>  create mode 100644 drivers/gpu/drm/imx/imx-lcdc.c
> 
> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> index fd5b2471fdf0..af5c6cb8c445 100644
> --- a/drivers/gpu/drm/imx/Kconfig
> +++ b/drivers/gpu/drm/imx/Kconfig
> @@ -41,3 +41,10 @@ config DRM_IMX_HDMI
>  	  Choose this if you want to use HDMI on i.MX6.
>  
>  source "drivers/gpu/drm/imx/dcss/Kconfig"
> +
> +config DRM_IMX_LCDC
> +	tristate "Freescale i.MX LCDC displays"
> +	depends on DRM && (ARCH_MXC || COMPILE_TEST)

Select DRM_GEM_DMA_HELPER for DEFINE_DRM_GEM_DMA_FOPS().

> +	select DRM_KMS_CMA_HELPER

Select DRM_KMS_HELPER instead, see commit 09717af7d13d ("drm: Remove
CONFIG_DRM_KMS_CMA_HELPER option").

> +	help
> +	  Found on i.MX1, i.MX21, i.MX25 and i.MX27.
> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
> index b644deffe948..1f96de7f15b4 100644
> --- a/drivers/gpu/drm/imx/Makefile
> +++ b/drivers/gpu/drm/imx/Makefile
> @@ -10,3 +10,5 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
>  
>  obj-$(CONFIG_DRM_IMX_HDMI) += dw_hdmi-imx.o
>  obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
> +
> +obj-$(CONFIG_DRM_IMX_LCDC) += imx-lcdc.o
> diff --git a/drivers/gpu/drm/imx/imx-lcdc.c b/drivers/gpu/drm/imx/imx-lcdc.c
> new file mode 100644
> index 000000000000..14d4962cecfd
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/imx-lcdc.c
> @@ -0,0 +1,610 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
> +
> +#include "drm/drm_fourcc.h"

#include <drm/drm_fourcc.h>

and sort alphabetically?

> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_dma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_vblank.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#define IMX21LCDC_LSSAR         0x0000 /* LCDC Screen Start Address Register */
> +#define IMX21LCDC_LSR           0x0004 /* LCDC Size Register */
> +#define IMX21LCDC_LVPWR         0x0008 /* LCDC Virtual Page Width Register */
> +#define IMX21LCDC_LCPR          0x000C /* LCDC Cursor Position Register */
> +#define IMX21LCDC_LCWHB         0x0010 /* LCDC Cursor Width Height and Blink Register*/
> +#define IMX21LCDC_LCCMR         0x0014 /* LCDC Color Cursor Mapping Register */
> +#define IMX21LCDC_LPCR          0x0018 /* LCDC Panel Configuration Register */
> +#define IMX21LCDC_LHCR          0x001C /* LCDC Horizontal Configuration Register */
> +#define IMX21LCDC_LVCR          0x0020 /* LCDC Vertical Configuration Register */
> +#define IMX21LCDC_LPOR          0x0024 /* LCDC Panning Offset Register */
> +#define IMX21LCDC_LSCR          0x0028 /* LCDC Sharp Configuration Register */
> +#define IMX21LCDC_LPCCR         0x002C /* LCDC PWM Contrast Control Register */
> +#define IMX21LCDC_LDCR          0x0030 /* LCDC DMA Control Register */
> +#define IMX21LCDC_LRMCR         0x0034 /* LCDC Refresh Mode Control Register */
> +#define IMX21LCDC_LICR          0x0038 /* LCDC Interrupt Configuration Register */
> +#define IMX21LCDC_LIER          0x003C /* LCDC Interrupt Enable Register */
> +#define IMX21LCDC_LISR          0x0040 /* LCDC Interrupt Status Register */
> +#define IMX21LCDC_LGWSAR        0x0050 /* LCDC Graphic Window Start Address Register */
> +#define IMX21LCDC_LGWSR         0x0054 /* LCDC Graph Window Size Register */
> +#define IMX21LCDC_LGWVPWR       0x0058 /* LCDC Graphic Window Virtual Page Width Register */
> +#define IMX21LCDC_LGWPOR        0x005C /* LCDC Graphic Window Panning Offset Register */
> +#define IMX21LCDC_LGWPR         0x0060 /* LCDC Graphic Window Position Register */
> +#define IMX21LCDC_LGWCR         0x0064 /* LCDC Graphic Window Control Register */
> +#define IMX21LCDC_LGWDCR        0x0068 /* LCDC Graphic Window DMA Control Register */
> +#define IMX21LCDC_LAUSCR        0x0080 /* LCDC AUS Mode Control Register */
> +#define IMX21LCDC_LAUSCCR       0x0084 /* LCDC AUS Mode Cursor Control Register */
> +#define IMX21LCDC_BGLUT         0x0800 /* Background Lookup Table */
> +#define IMX21LCDC_GWLUT         0x0C00 /* Graphic Window Lookup Table */
> +
> +#define IMX21LCDC_LCPR_CC0 BIT(30) /* Cursor Control Bit 0 */
> +#define IMX21LCDC_LCPR_CC1 BIT(31) /* Cursor Control Bit 1 */
> +
> +/* Values HSYNC, VSYNC and Framesize Register */
> +#define IMX21LCDC_LHCR_H_WIDTH(val)	(FIELD_PREP(GENMASK(31, 26), (val)))
> +#define IMX21LCDC_LHCR_H_BPORCH(val)	(FIELD_PREP(GENMASK(7, 0), (val)))
> +#define IMX21LCDC_LHCR_H_FPORCH(val)	(FIELD_PREP(GENMASK(15, 8), (val)))
> +
> +#define IMX21LCDC_LVCR_V_WIDTH(val)	(FIELD_PREP(GENMASK(31, 26), (val)))
> +#define IMX21LCDC_LVCR_V_BPORCH(val)	(FIELD_PREP(GENMASK(7, 0), (val)))
> +#define IMX21LCDC_LVCR_V_FPORCH(val)	(FIELD_PREP(GENMASK(15, 8), (val)))
> +
> +#define IMX21LCDC_FRAME_WIDTH(val)	(((val) / 16) << 20)
> +#define IMX21LCDC_FRAME_HEIGHT(val)	(val)
> +
> +/* Values for LPCR Register */
> +#define IMX21LCDC_PCD(val)		(FIELD_PREP(GENMASK(5, 0), --(val)))
> +#define IMX21LCDC_SHARP(val)		(FIELD_PREP(GENMASK(6, 6), (val)))
> +#define IMX21LCDC_SCLKSEL(val)		(FIELD_PREP(GENMASK(7, 7), (val)))
> +#define IMX21LCDC_ACD(val)		(FIELD_PREP(GENMASK(14, 8), (val)))
> +#define IMX21LCDC_ACDSEL(val)		(FIELD_PREP(GENMASK(15, 15), (val)))
> +#define IMX21LCDC_REV_VS(val)		(FIELD_PREP(GENMASK(16, 16), (val)))
> +#define IMX21LCDC_SWAP_SEL(val)	(FIELD_PREP(GENMASK(17, 17), (val)))
> +#define IMX21LCDC_END_SEL(val)		(FIELD_PREP(GENMASK(18, 18), (val)))
> +#define IMX21LCDC_SCLKIDLE(val)	(FIELD_PREP(GENMASK(19, 19), (val)))
> +#define IMX21LCDC_OEPOL(val)		(FIELD_PREP(GENMASK(20, 20), (val)))
> +#define IMX21LCDC_CLKPOL(val)		(FIELD_PREP(GENMASK(21, 21), (val)))
> +#define IMX21LCDC_LPPOL(val)		(FIELD_PREP(GENMASK(22, 22), (val)))
> +#define IMX21LCDC_FLMPOL(val)		(FIELD_PREP(GENMASK(23, 23), (val)))
> +#define IMX21LCDC_PIXPOL(val)		(FIELD_PREP(GENMASK(24, 24), (val)))
> +#define IMX21LCDC_BPIX(val)		(FIELD_PREP(GENMASK(27, 25), (val)))
> +#define IMX21LCDC_PBSIZ(val)		(FIELD_PREP(GENMASK(29, 28), (val)))
> +#define IMX21LCDC_COLOR(val)		(FIELD_PREP(GENMASK(30, 30), (val)))
> +#define IMX21LCDC_TFT(val)		(FIELD_PREP(GENMASK(31, 31), (val)))
> +
> +#define INTR_EOF BIT(1) /* VBLANK Interrupt Bit */
> +
> +#define BPP_RGB565 0x05
> +
> +#define LCDC_MIN_XRES 64
> +#define LCDC_MIN_YRES 64
> +
> +#define LCDC_MAX_XRES 1024
> +#define LCDC_MAX_YRES 1024
> +
> +struct imx_lcdc {
> +	struct drm_device drm;
> +	struct drm_simple_display_pipe pipe;
> +	const struct drm_display_mode *mode;
> +	struct drm_connector connector;
> +	struct drm_panel *panel;
> +	struct drm_bridge *bridge;
> +	void __iomem *base;
> +
> +	struct clk *clk_ipg;
> +	struct clk *clk_ahb;
> +	struct clk *clk_per;
> +};
> +
> +static const u32 imx_lcdc_formats[] = {
> +	DRM_FORMAT_RGB565,
> +};
> +
> +static inline struct imx_lcdc *drm_to_lcdc(struct drm_device *drm)
> +{
> +	return container_of(drm, struct imx_lcdc, drm);
> +}
> +
> +static unsigned int imx_lcdc_get_format(unsigned int drm_format)
> +{
> +	unsigned int bpp;
> +
> +	switch (drm_format) {
> +	default:
> +		DRM_WARN("Format not supported - fallback to RGB565\n");
> +		fallthrough;
> +	case DRM_FORMAT_RGB565:
> +		bpp = BPP_RGB565;
> +		break;
> +	}
> +
> +	return bpp;
> +}
> +
> +static int imx_lcdc_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct imx_lcdc *lcdc = drm_to_lcdc(connector->dev);
> +
> +	if (lcdc->panel)
> +		return drm_panel_get_modes(lcdc->panel, connector);
> +
> +	return 0;
> +}
> +
> +static const struct drm_connector_helper_funcs imx_lcdc_connector_hfuncs = {
> +	.get_modes = imx_lcdc_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs imx_lcdc_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
> +					 struct drm_plane_state *old_state,
> +					 bool mode_set)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_plane_state *new_state = pipe->plane.state;
> +	struct drm_framebuffer *fb = new_state->fb;
> +	struct imx_lcdc *lcdc = drm_to_lcdc(pipe->crtc.dev);
> +	unsigned int bpp;
> +	unsigned int lvcr; /* LVCR-Register value */
> +	unsigned int lhcr; /* LHCR-Register value */
> +	unsigned int framesize;
> +	dma_addr_t addr;
> +
> +	addr = drm_fb_dma_get_gem_addr(fb, new_state, 0);
> +	/* The LSSAR register specifies the LCD screen start address (SSA). */
> +	writel(addr, lcdc->base + IMX21LCDC_LSSAR);
> +
> +	if (!mode_set)
> +		return;
> +
> +	/* Disable PER clock to make register write possible */
> +	if (old_state && old_state->crtc && old_state->crtc->enabled)
> +		clk_disable_unprepare(lcdc->clk_per);
> +
> +	/* Framesize */
> +	framesize = IMX21LCDC_FRAME_WIDTH(crtc->mode.hdisplay);
> +	framesize |= IMX21LCDC_FRAME_HEIGHT(crtc->mode.vdisplay);
> +	writel(framesize, lcdc->base + IMX21LCDC_LSR);
> +
> +	/* HSYNC */
> +	lhcr = IMX21LCDC_LHCR_H_FPORCH(crtc->mode.hsync_start - crtc->mode.hdisplay - 1);
> +	lhcr |= IMX21LCDC_LHCR_H_WIDTH(crtc->mode.hsync_end - crtc->mode.hsync_start - 1);
> +	lhcr |= IMX21LCDC_LHCR_H_BPORCH(crtc->mode.htotal - crtc->mode.hsync_end - 3);
> +	writel(lhcr, lcdc->base + IMX21LCDC_LHCR);
> +
> +	/* VSYNC */
> +	lvcr = IMX21LCDC_LVCR_V_FPORCH(crtc->mode.vsync_start - crtc->mode.vdisplay);
> +	lvcr |= IMX21LCDC_LVCR_V_WIDTH(crtc->mode.vsync_end - crtc->mode.vsync_start);
> +	lvcr |= IMX21LCDC_LVCR_V_BPORCH(crtc->mode.vtotal - crtc->mode.vsync_end);
> +	writel(lvcr, lcdc->base + IMX21LCDC_LVCR);
> +
> +	bpp = imx_lcdc_get_format(fb->format->format);
> +	writel(readl(lcdc->base + IMX21LCDC_LPCR) | IMX21LCDC_BPIX(bpp),
> +	       lcdc->base + IMX21LCDC_LPCR);
> +
> +	/* Virtual Page Width */
> +	writel(new_state->fb->pitches[0] / 4, lcdc->base + IMX21LCDC_LVPWR);
> +
> +	/* Enable PER clock */
> +	if (new_state->crtc->enabled)
> +		clk_prepare_enable(lcdc->clk_per);
> +}
> +
> +static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
> +				 struct drm_crtc_state *crtc_state,
> +				 struct drm_plane_state *plane_state)
> +{
> +	int ret;
> +	int clk_div;
> +	int bpp;
> +	struct imx_lcdc *lcdc = drm_to_lcdc(pipe->crtc.dev);
> +	struct drm_display_mode *mode = &pipe->crtc.mode;
> +	struct drm_display_info *disp_info = &pipe->connector->display_info;
> +	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
> +	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
> +	const int data_enable_pol =
> +		(disp_info->bus_flags & DRM_BUS_FLAG_DE_HIGH) ? 0 : 1;
> +	const int clk_pol =
> +		(disp_info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) ? 0 : 1;
> +
> +	drm_panel_prepare(lcdc->panel);

No error handling here ...

> +	clk_div = DIV_ROUND_CLOSEST_ULL(clk_get_rate(lcdc->clk_per),
> +					mode->clock * 1000);
> +	bpp = imx_lcdc_get_format(plane_state->fb->format->format);
> +
> +	writel(IMX21LCDC_PCD(clk_div) | IMX21LCDC_LPPOL(hsync_pol) | IMX21LCDC_FLMPOL(vsync_pol) |
> +	       IMX21LCDC_OEPOL(data_enable_pol) | IMX21LCDC_TFT(1) | IMX21LCDC_COLOR(1) |
> +	       IMX21LCDC_PBSIZ(3) | IMX21LCDC_BPIX(bpp) | IMX21LCDC_SCLKSEL(1) |
> +	       IMX21LCDC_PIXPOL(0) | IMX21LCDC_CLKPOL(clk_pol),
> +	       lcdc->base + IMX21LCDC_LPCR);
> +
> +	/* 0px panning offset */
> +	writel(0x00000000, lcdc->base + IMX21LCDC_LPOR);
> +
> +	/* disable hardware cursor */
> +	writel(readl(lcdc->base + IMX21LCDC_LCPR) & ~(IMX21LCDC_LCPR_CC0 | IMX21LCDC_LCPR_CC1),
> +	       lcdc->base + IMX21LCDC_LCPR);
> +
> +	ret = clk_prepare_enable(lcdc->clk_ipg);
> +	if (ret) {
> +		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
> +		return;

... but here seems inconsistent.

> +	}
> +	ret = clk_prepare_enable(lcdc->clk_ahb);
> +	if (ret) {
> +		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
> +		clk_disable_unprepare(lcdc->clk_ipg);
> +		return;
> +	}
> +
> +	imx_lcdc_update_hw_registers(pipe, NULL, true);
> +	drm_panel_enable(lcdc->panel);
> +
> +	/* Enable VBLANK Interrupt */
> +	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
> +}
> +
> +static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	struct imx_lcdc *lcdc = drm_to_lcdc(pipe->crtc.dev);
> +	struct drm_panel *panel = lcdc->panel;
> +	struct drm_crtc *crtc = &lcdc->pipe.crtc;
> +	struct drm_pending_vblank_event *event;
> +
> +	drm_panel_disable(panel);
> +
> +	clk_disable_unprepare(lcdc->clk_ahb);
> +	clk_disable_unprepare(lcdc->clk_ipg);
> +
> +	if (pipe->crtc.enabled)
> +		clk_disable_unprepare(lcdc->clk_per);
> +
> +	drm_panel_unprepare(panel);
> +
> +	spin_lock_irq(&lcdc->drm.event_lock);
> +	event = crtc->state->event;
> +	if (event) {
> +		crtc->state->event = NULL;
> +		drm_crtc_send_vblank_event(crtc, event);
> +	}
> +	spin_unlock_irq(&lcdc->drm.event_lock);
> +
> +	/* Disable VBLANK Interrupt */
> +	writel(0, lcdc->base + IMX21LCDC_LIER);
> +}
> +
> +static int imx_lcdc_check_mode_change(struct drm_display_mode *new_mode,
> +				      struct drm_display_mode *old_mode)
> +{
> +	if (old_mode->hdisplay != new_mode->hdisplay ||
> +	    old_mode->vdisplay != new_mode->vdisplay)
> +		return true;
> +	return false;

Could just

        return (old_mode->hdisplay != new_mode->hdisplay ||
                old_mode->vdisplay != new_mode->vdisplay);

or fold this into the single use below.

> +
> +static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
> +			       struct drm_plane_state *plane_state,
> +			       struct drm_crtc_state *crtc_state)
> +{
> +	const struct drm_display_mode *mode = &crtc_state->mode;
> +
> +	if ((mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES) ||
> +	    (mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES) ||
> +	    (mode->hdisplay & 0x10)) { /* must be multiple of 16 */

More parantheses than needed.

> +		DRM_ERROR("unsupported display mode (%u x %u)\n",
> +			  mode->hdisplay, mode->vdisplay);

Could use drm_err() instead.

> +		return -EINVAL;
> +	}
> +
> +	crtc_state->mode_changed = imx_lcdc_check_mode_change(&crtc_state->mode,
> +							      &pipe->crtc.state->mode);
> +
> +	return 0;
> +}
> +
> +static void imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
> +				 struct drm_plane_state *old_state)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_pending_vblank_event *event = crtc->state->event;
> +	struct drm_plane_state *new_state = pipe->plane.state;
> +	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_framebuffer *old_fb = old_state->fb;
> +	struct drm_crtc *old_crtc = old_state->crtc;
> +	bool mode_changed = false;
> +
> +	if (old_fb && old_fb->format != fb->format)
> +		mode_changed = true;
> +	else if (old_crtc != crtc)
> +		mode_changed = true;
> +
> +	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
> +
> +	if (event) {
> +		crtc->state->event = NULL;
> +
> +		spin_lock_irq(&crtc->dev->event_lock);
> +
> +		if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
> +			drm_crtc_arm_vblank_event(crtc, event);
> +		else
> +			drm_crtc_send_vblank_event(crtc, event);
> +
> +		spin_unlock_irq(&crtc->dev->event_lock);
> +	}
> +}
> +
> +static const struct drm_simple_display_pipe_funcs imx_lcdc_pipe_funcs = {
> +	.enable = imx_lcdc_pipe_enable,
> +	.disable = imx_lcdc_pipe_disable,
> +	.check = imx_lcdc_pipe_check,
> +	.update = imx_lcdc_pipe_update,
> +	.prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_mode_config_funcs imx_lcdc_mode_config_funcs = {
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static const struct drm_mode_config_helper_funcs imx_lcdc_mode_config_helpers = {
> +	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};
> +
> +static void imx_lcdc_release(struct drm_device *drm)
> +{
> +	struct imx_lcdc *lcdc = drm_to_lcdc(drm);
> +
> +	drm_kms_helper_poll_fini(drm);
> +	kfree(lcdc);
> +}
> +
> +DEFINE_DRM_GEM_DMA_FOPS(imx_lcdc_drm_fops);
> +
> +static struct drm_driver imx_lcdc_drm_driver = {
> +	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops = &imx_lcdc_drm_fops,
> +	DRM_GEM_DMA_DRIVER_OPS_VMAP,
> +	.release = imx_lcdc_release,
> +	.name = "imx-lcdc",
> +	.desc = "i.MX LCDC driver",
> +	.date = "20200716",
> +};
> +
> +static const struct of_device_id imx_lcdc_of_dev_id[] = {
> +	{
> +		.compatible = "fsl,imx21-lcdc",
> +	},
> +	{
> +		.compatible = "fsl,imx25-lcdc",
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_lcdc_of_dev_id);
> +
> +static irqreturn_t irq_handler(int irq, void *arg)
> +{
> +	struct imx_lcdc *lcdc = (struct imx_lcdc *)arg;

Unnecessary cast.

> +	struct drm_crtc *crtc = &lcdc->pipe.crtc;
> +	unsigned int status;
> +
> +	status = readl(lcdc->base + IMX21LCDC_LISR);
> +
> +	if (status & INTR_EOF) {
> +		drm_crtc_handle_vblank(crtc);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static int imx_lcdc_probe(struct platform_device *pdev)
> +{
> +	struct imx_lcdc *lcdc;
> +	struct drm_device *drm;
> +	int irq;
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	lcdc = devm_drm_dev_alloc(&pdev->dev, &imx_lcdc_drm_driver,

Could use the local dev variable.

> +				  struct imx_lcdc, drm);
> +	if (!lcdc)
> +		return -ENOMEM;
> +
> +	drm = &lcdc->drm;
> +
> +	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lcdc->base)) {
> +		dev_err(dev, "Cannot get IO memory\n");
> +		return PTR_ERR(lcdc->base);
> +	}
> +
> +	/* Panel */
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &lcdc->panel, &lcdc->bridge);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to find panel or bridge: %pe", ERR_PTR(ret));

Could be simplified with dev_err_probe().

> +		return ret;
> +	}
> +
> +	/* Get Clocks */
> +	lcdc->clk_ipg = devm_clk_get(dev, "ipg");
> +	if (IS_ERR(lcdc->clk_ipg)) {
> +		dev_err(dev, "Failed to get %s clk: %pe\n", "ipg",
> +			lcdc->clk_ipg);
> +		return PTR_ERR(lcdc->clk_ipg);

Maybe also use dev_err_probe() for the clocks.

> +	}
> +
> +	lcdc->clk_ahb = devm_clk_get(dev, "ahb");
> +	if (IS_ERR(lcdc->clk_ahb)) {
> +		dev_err(dev, "Failed to get %s clk: %pe\n", "ahb",
> +			lcdc->clk_ahb);
> +		return PTR_ERR(lcdc->clk_ahb);
> +	}
> +
> +	lcdc->clk_per = devm_clk_get(dev, "per");
> +	if (IS_ERR(lcdc->clk_per)) {
> +		dev_err(dev, "Failed to get %s clk: %pe\n", "per",
> +			lcdc->clk_per);
> +		return PTR_ERR(lcdc->clk_per);
> +	}
> +
> +	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(drm->dev, "Cannot set DMA Mask\n");
> +		return ret;
> +	}
> +
> +	/* Modeset init */
> +	drm_mode_config_init(drm);

Use drmm_mode_config_init() directly and handle the return value.

> +
> +	/* CRTC, Plane, Encoder */
> +	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe, &imx_lcdc_pipe_funcs, imx_lcdc_formats,
> +					   ARRAY_SIZE(imx_lcdc_formats), NULL, &lcdc->connector);
> +	if (ret < 0) {
> +		dev_err(drm->dev, "Cannot setup simple display pipe\n");
> +		return ret;
> +	}
> +
> +	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> +	if (ret < 0) {
> +		dev_err(drm->dev, "Failed to initialize vblank\n");
> +		return ret;
> +	}
> +
> +	if (lcdc->bridge) {
> +		ret = drm_simple_display_pipe_attach_bridge(&lcdc->pipe,
> +							    lcdc->bridge);
> +		if (ret) {
> +			dev_err(drm->dev, "Cannot connect bridge: %pe\n",
> +				ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +
> +	/* Connector */
> +	drm_connector_helper_add(&lcdc->connector, &imx_lcdc_connector_hfuncs);
> +	drm_connector_init(drm, &lcdc->connector, &imx_lcdc_connector_funcs,
> +			   DRM_MODE_CONNECTOR_DPI);

Missing error handling.

regards
Philipp

Re: [PATCH v2 1/2] dt-bindings: display: imx: Describe drm binding for fsl,imx-lcdc

[Krzysztof Kozlowski]

On 14/12/2022 12:59, Uwe Kleine-König wrote:
> Modify the existing (fb-like) binding to support the drm-like binding in
> parallel.

Aren't you now adding two compatibles to the same hardware, just for two
Linux drivers? One hardware should have one compatible, regardless of
Linux display implementation.

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  .../bindings/display/imx/fsl,imx-lcdc.yaml    | 45 ++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml
> index 35a8fff036ca..2a8225b10890 100644
> --- a/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml
> +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx-lcdc.yaml
> @@ -21,6 +21,9 @@ properties:
>                - fsl,imx25-fb
>                - fsl,imx27-fb
>            - const: fsl,imx21-fb
> +      - items:
> +          - const: fsl,imx25-lcdc
> +          - const: fsl,imx21-lcdc
>  
>    clocks:
>      maxItems: 3
> @@ -31,6 +34,9 @@ properties:
>        - const: ahb
>        - const: per
>  
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +
>    display:
>      $ref: /schemas/types.yaml#/definitions/phandle
>  
> @@ -59,17 +65,54 @@ properties:
>      description:
>        LCDC Sharp Configuration Register value.
>  
> +if:

Put it under allOf. It grows pretty often so this would avoid future
re-indents.

> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - fsl,imx1-lcdc
> +          - fsl,imx21-lcdc
> +then:
> +  properties:
> +    display: false
> +    fsl,dmacr: false
> +    fsl,lpccr: false
> +    fsl,lscr1: false
> +
> +  required:
> +    - port
> +
> +else:
> +  properties:
> +    port: false
> +
> +  required:
> +    - display
> +
>  required:
>    - compatible
>    - clocks
>    - clock-names
> -  - display
>    - interrupts
>    - reg
>  
>  additionalProperties: false
>  
>  examples:
> +  - |
> +    lcdc@53fbc000 {
> +        compatible = "fsl,imx25-lcdc", "fsl,imx21-lcdc";
> +        reg = <0x53fbc000 0x4000>;
> +        interrupts = <39>;
> +        clocks = <&clks 103>, <&clks 66>, <&clks 49>;
> +        clock-names = "ipg", "ahb", "per";
> +
> +        port {
> +            parallel_out: endpoint {
> +              remote-endpoint = <&panel_in>;
> +            };
> +        };
> +    };
>    - |
>      imxfb: fb@10021000 {
>          compatible = "fsl,imx21-fb";

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: display: imx: Describe drm binding for fsl,imx-lcdc

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Fri, Dec 16, 2022 at 11:41:30AM +0100, Krzysztof Kozlowski wrote:
> On 14/12/2022 12:59, Uwe Kleine-König wrote:
> > Modify the existing (fb-like) binding to support the drm-like binding in
> > parallel.
> 
> Aren't you now adding two compatibles to the same hardware, just for two
> Linux drivers? One hardware should have one compatible, regardless of
> Linux display implementation.

The (up to now unopposed) idea was to use the opportunity to pick a
better name for the compatible. The hardware component is called LCDC
and I guess fsl,imx21-fb was only picked because the linux driver is
called imxfb. Unless I understood Rob wrong, he insisted to describe
both variants in a single binding document only.

> > +if:
> 
> Put it under allOf. It grows pretty often so this would avoid future
> re-indents.

ok.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] dt-bindings: display: imx: Describe drm binding for fsl,imx-lcdc

[Krzysztof Kozlowski]

On 16/12/2022 12:38, Uwe Kleine-König wrote:
> On Fri, Dec 16, 2022 at 11:41:30AM +0100, Krzysztof Kozlowski wrote:
>> On 14/12/2022 12:59, Uwe Kleine-König wrote:
>>> Modify the existing (fb-like) binding to support the drm-like binding in
>>> parallel.
>>
>> Aren't you now adding two compatibles to the same hardware, just for two
>> Linux drivers? One hardware should have one compatible, regardless of
>> Linux display implementation.
> 
> The (up to now unopposed) idea was to use the opportunity to pick a
> better name for the compatible. The hardware component is called LCDC
> and I guess fsl,imx21-fb was only picked because the linux driver is
> called imxfb. Unless I understood Rob wrong, he insisted to describe
> both variants in a single binding document only.

OK, I'll leave it then to Rob.

Best regards,
Krzysztof