[PATCH v2 0/6] Add drivers for Exynos5433 display

[PATCH v2 0/6] Add drivers for Exynos5433 display

[Hyungwon Hwang]

This patchset is based on the git(branch name: exynos-drm-next) which is
maintained by Inki Dae.
https://kernel.googlesource.com/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

This patchset adds 2 new device drivers, decon and mic, and adds support for
Exynos5433 mipi dsi. To enable display in a Exynos5433 board, decon(display
controller), MIC(Mobile image compressor), mipi dsi, and panel have to be turned
on. This patchset contains support for 3 drivers for SoC level devices.

Changes for v2:
- change config, file, and variable names of decon to represnt exynos5433
instead of exynos to distinguish them from exynos7 decon
- change the initialization order of decon to make it initialized in order like
FIMD or exynos7 decon
- make mic driver to be registered by exynos drm driver instead as a module
driver
- change the description of mic driver in documentation
- add module author at the top of the source file removing MODULE_OWNER,
MODULE_DESCRIPTION, MODULE_LICENSE
- change the author of "drm/exynos: dsi: add support for Exynos5433 SoC" to
Hyungwon Hwang by the previous author's will

Hyungwon Hwang (5):
  of: add helper for getting endpoint node of specific identifiers
  drm/exynos: mic: add MIC driver
  drm/exynos: dsi: add support for Exynos5433 SoC
  drm/exynos: dsi: add support for MIC driver as a bridge
  drm/exynos: dsi: do not set TE GPIO direction by input

Joonyoung Shim (1):
  drm/exynos: add Exynos5433 decon driver

 .../devicetree/bindings/video/exynos-mic.txt       |  49 ++
 .../devicetree/bindings/video/exynos5433-decon.txt |  65 +++
 .../devicetree/bindings/video/exynos_dsim.txt      |  24 +-
 drivers/gpu/drm/exynos/Kconfig                     |  14 +-
 drivers/gpu/drm/exynos/Makefile                    |   2 +
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c      | 543 +++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_drv.c            |   6 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h            |   2 +
 drivers/gpu/drm/exynos/exynos_drm_dsi.c            | 459 +++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_mic.c            | 481 ++++++++++++++++++
 drivers/gpu/drm/exynos/regs-exynos5433-decon.h     | 163 +++++++
 drivers/of/base.c                                  |  33 ++
 include/linux/of_graph.h                           |   8 +
 13 files changed, 1703 insertions(+), 146 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/exynos-mic.txt
 create mode 100644 Documentation/devicetree/bindings/video/exynos5433-decon.txt
 create mode 100644 drivers/gpu/drm/exynos/exynos5433_drm_decon.c
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm_mic.c
 create mode 100644 drivers/gpu/drm/exynos/regs-exynos5433-decon.h

--
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/exynos: add Exynos5433 decon driver

[Hyungwon Hwang]

From: Joonyoung Shim <jy0922.shim@samsung.com>

DECON(Display and Enhancement Controller) is new IP replacing FIMD in
Exynos5433. This patch adds Exynos5433 decon driver.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
Changes for v2:
change file names and variable names of decon to represnt exynos5433 instead of
exynos to distinguish them from exynos7 decon
 .../devicetree/bindings/video/exynos5433-decon.txt |  65 +++
 drivers/gpu/drm/exynos/Kconfig                     |   6 +
 drivers/gpu/drm/exynos/Makefile                    |   1 +
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c      | 543 +++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_drv.c            |   3 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h            |   1 +
 drivers/gpu/drm/exynos/regs-exynos5433-decon.h     | 163 +++++++
 7 files changed, 782 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/exynos5433-decon.txt
 create mode 100644 drivers/gpu/drm/exynos/exynos5433_drm_decon.c
 create mode 100644 drivers/gpu/drm/exynos/regs-exynos5433-decon.h

diff --git a/Documentation/devicetree/bindings/video/exynos5433-decon.txt b/Documentation/devicetree/bindings/video/exynos5433-decon.txt
new file mode 100644
index 0000000..377afbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/exynos5433-decon.txt
@@ -0,0 +1,65 @@
+Device-Tree bindings for Samsung Exynos SoC display controller (DECON)
+
+DECON (Display and Enhancement Controller) is the Display Controller for the
+Exynos series of SoCs which transfers the image data from a video memory
+buffer to an external LCD interface.
+
+Required properties:
+- compatible: value should be "samsung,exynos5433-decon";
+- reg: physical base address and length of the DECON registers set.
+- interrupts: should contain a list of all DECON IP block interrupts in the
+	      order: VSYNC, LCD_SYSTEM. The interrupt specifier format
+	      depends on the interrupt controller used.
+- interrupt-names: should contain the interrupt names: "vsync", "lcd_sys"
+		   in the same order as they were listed in the interrupts
+		   property.
+- clocks: must include clock specifiers corresponding to entries in the
+	  clock-names property.
+- clock-names: list of clock names sorted in the same order as the clocks
+	       property. Must contain "aclk_decon", "aclk_smmu_decon0x",
+	       "aclk_xiu_decon0x", "pclk_smmu_decon0x", clk_decon_vclk",
+	       "sclk_decon_eclk"
+- ports: contains a port which is connected to mic node. address-cells and
+	 size-cells must 1 and 0, respectively.
+- port: contains an endpoint node which is connected to the endpoint in the mic
+	node. The reg value muset be 0.
+- i80-if-timings: specify whether the panel which is connected to decon uses
+		  i80 lcd interface or mipi video interface. This node contains
+		  no timing information as that of fimd does. Because there is
+		  no register in decon to specify i80 interface timing value,
+		  it is not needed, but make it remain to use same kind of node
+		  in fimd and exynos7 decon.
+
+Example:
+SoC specific DT entry:
+decon: decon@13800000 {
+	compatible = "samsung,exynos5433-decon";
+	reg = <0x13800000 0x2104>;
+	clocks = <&cmu_disp CLK_ACLK_DECON>, <&cmu_disp CLK_ACLK_SMMU_DECON0X>,
+		<&cmu_disp CLK_ACLK_XIU_DECON0X>,
+		<&cmu_disp CLK_PCLK_SMMU_DECON0X>,
+		<&cmu_disp CLK_SCLK_DECON_VCLK>,
+		<&cmu_disp CLK_SCLK_DECON_ECLK>;
+	clock-names = "aclk_decon", "aclk_smmu_decon0x", "aclk_xiu_decon0x",
+		"pclk_smmu_decon0x", "sclk_decon_vclk", "sclk_decon_eclk";
+	interrupt-names = "vsync", "lcd_sys";
+	interrupts = <0 202 0>, <0 203 0>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			decon_to_mic: endpoint {
+				remote-endpoint = <&mic_to_decon>;
+			};
+		};
+	};
+};
+
+Board specific DT entry:
+&decon {
+	i80-if-timings {
+	};
+};
diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index a5e7461..e15cc2e 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -24,6 +24,12 @@ config DRM_EXYNOS_FIMD
 	help
 	  Choose this option if you want to use Exynos FIMD for DRM.

+config DRM_EXYNOS5433_DECON
+	bool "Exynos5433 DRM DECON"
+	depends on DRM_EXYNOS
+	help
+	  Choose this option if you want to use Exynos5433 DECON for DRM.
+
 config DRM_EXYNOS7_DECON
 	bool "Exynos DRM DECON"
 	depends on DRM_EXYNOS
diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
index cc90679..fbd084d 100644
--- a/drivers/gpu/drm/exynos/Makefile
+++ b/drivers/gpu/drm/exynos/Makefile
@@ -10,6 +10,7 @@ exynosdrm-y := exynos_drm_drv.o exynos_drm_encoder.o \

 exynosdrm-$(CONFIG_DRM_EXYNOS_IOMMU) += exynos_drm_iommu.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_FIMD)	+= exynos_drm_fimd.o
+exynosdrm-$(CONFIG_DRM_EXYNOS5433_DECON)	+= exynos5433_drm_decon.o
 exynosdrm-$(CONFIG_DRM_EXYNOS7_DECON)	+= exynos7_drm_decon.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_DPI)	+= exynos_drm_dpi.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_DSI)	+= exynos_drm_dsi.o
diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
new file mode 100644
index 0000000..c7e5def
--- /dev/null
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -0,0 +1,543 @@
+/* drivers/gpu/drm/exynos5433_drm_decon.c
+ *
+ * Copyright (C) 2015 Samsung Electronics Co.Ltd
+ * Authors:
+ *	Joonyoung Shim <jy0922.shim@samsung.com>
+ *	Hyungwon Hwang <human.hwang@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundationr
+ */
+
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/of_gpio.h>
+#include "exynos_drm_drv.h"
+#include "exynos_drm_crtc.h"
+#include "regs-exynos5433-decon.h"
+
+struct decon_reg_data {
+	u32		wincon;
+	u32		vidosd_a;	/* left top */
+	u32		vidosd_b;	/* right bottom */
+	u32		vidosd_c;	/* alpha */
+	u32		vidosd_d;	/* alpha */
+	dma_addr_t	vidw_add0;	/* start address */
+	dma_addr_t	vidw_add1;	/* end address */
+	u32		vidw_add2;	/* offsize & pagewidth */
+};
+
+struct exynos_decon {
+	struct device			*dev;
+	struct drm_device		*drm_dev;
+	struct exynos_drm_crtc          *crtc;
+	void __iomem			*addr;
+	struct clk			*clks[6];
+	struct decon_reg_data		reg_data[5];
+	int				pipe;
+
+#define BIT_CLKS_ENABLED		0
+#define BIT_IRQS_ENABLED		1
+	unsigned long			enabled;
+	bool				i80_if;
+	atomic_t			win_updated;
+};
+
+static const char * const decon_clks_name[] = {
+	"aclk_decon",
+	"aclk_smmu_decon0x",
+	"aclk_xiu_decon0x",
+	"pclk_smmu_decon0x",
+	"sclk_decon_vclk",
+	"sclk_decon_eclk",
+};
+
+static int decon_enable_vblank(struct exynos_drm_crtc *crtc)
+{
+	struct exynos_decon *decon = crtc->ctx;
+	u32 val;
+
+	if (!test_and_set_bit(BIT_IRQS_ENABLED, &decon->enabled)) {
+		val = VIDINTCON0_FRAMEDONE | VIDINTCON0_INTFRMEN |
+			VIDINTCON0_INTEN;
+		writel(val, decon->addr + DECON_VIDINTCON0);
+	}
+
+	return 0;
+}
+
+static void decon_disable_vblank(struct exynos_drm_crtc *crtc)
+{
+	struct exynos_decon *decon = crtc->ctx;
+
+	if (test_and_clear_bit(BIT_IRQS_ENABLED, &decon->enabled))
+		writel(0, decon->addr + DECON_VIDINTCON0);
+}
+
+static void decon_dpms_on(struct exynos_decon *decon)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
+		ret = clk_prepare_enable(decon->clks[i]);
+		if (ret < 0)
+			goto err;
+	}
+
+	set_bit(BIT_CLKS_ENABLED, &decon->enabled);
+
+	return;
+
+err:
+	while (--i >= 0)
+		clk_disable_unprepare(decon->clks[i]);
+}
+
+static void decon_dpms_off(struct exynos_decon *decon)
+{
+	int i;
+
+	clear_bit(BIT_CLKS_ENABLED, &decon->enabled);
+
+	for (i = ARRAY_SIZE(decon_clks_name) - 1; i >= 0; i--)
+		clk_disable_unprepare(decon->clks[i]);
+}
+
+static void decon_dpms(struct exynos_drm_crtc *crtc, int mode)
+{
+	struct exynos_decon *decon = crtc->ctx;
+
+	switch (mode) {
+	case DRM_MODE_DPMS_ON:
+		decon_dpms_on(decon);
+		break;
+	case DRM_MODE_DPMS_STANDBY:
+	case DRM_MODE_DPMS_SUSPEND:
+	case DRM_MODE_DPMS_OFF:
+		decon_dpms_off(decon);
+		break;
+	default:
+		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
+		break;
+	}
+}
+
+static void decon_commit(struct exynos_drm_crtc *crtc)
+{
+	struct exynos_decon *decon = crtc->ctx;
+	struct drm_display_mode *mode = &crtc->base.mode;
+	u32 val;
+
+	/* enable clock gate */
+	val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
+	writel(val, decon->addr + DECON_CMU);
+
+	/* lcd on and use command if */
+	val = VIDOUT_LCD_ON;
+	if (decon->i80_if)
+		val |= VIDOUT_COMMAND_IF;
+	else
+		val |= VIDOUT_RGB_IF;
+	writel(val, decon->addr + DECON_VIDOUTCON0);
+
+	val = VIDTCON2_LINEVAL(mode->vdisplay - 1) |
+		VIDTCON2_HOZVAL(mode->hdisplay - 1);
+	writel(val, decon->addr + DECON_VIDTCON2);
+
+	if (!decon->i80_if) {
+		val = VIDTCON00_VBPD_F(
+				mode->crtc_vtotal - mode->crtc_vsync_end) |
+			VIDTCON00_VFPD_F(
+				mode->crtc_vsync_start - mode->crtc_vdisplay);
+		writel(val, decon->addr + DECON_VIDTCON00);
+
+		val = VIDTCON01_VSPW_F(
+				mode->crtc_vsync_end - mode->crtc_vsync_start);
+		writel(val, decon->addr + DECON_VIDTCON01);
+
+		val = VIDTCON10_HBPD_F(
+				mode->crtc_htotal - mode->crtc_hsync_end) |
+			VIDTCON10_HFPD_F(
+				mode->crtc_hsync_start - mode->crtc_hdisplay);
+		writel(val, decon->addr + DECON_VIDTCON10);
+
+		val = VIDTCON11_HSPW_F(
+				mode->crtc_hsync_end - mode->crtc_hsync_start);
+		writel(val, decon->addr + DECON_VIDTCON11);
+	}
+
+	/* sw trigger enable */
+	val = TRIGCON_TRIGEN_PER_F | TRIGCON_TRIGEN_F | TRIGCON_TE_AUTO_MASK |
+		TRIGCON_SWTRIGEN;
+	writel(val, decon->addr + DECON_TRIGCON);
+
+	/* enable output and display signal */
+	val = VIDCON0_ENVID | VIDCON0_ENVID_F;
+	writel(val, decon->addr + DECON_VIDCON0);
+}
+
+#define COORDINATE_X(x)		(((x) & 0xfff) << 12)
+#define COORDINATE_Y(x)		((x) & 0xfff)
+#define OFFSIZE(x)		(((x) & 0x3fff) << 14)
+#define PAGEWIDTH(x)		((x) & 0x3fff)
+
+static void decon_win_mode_set(struct exynos_drm_crtc *crtc,
+			       struct exynos_drm_plane *plane)
+{
+	struct exynos_decon *decon = crtc->ctx;
+	struct decon_reg_data *reg_data;
+	unsigned int bytes_per_pixel = plane->bpp >> 3;
+	unsigned int val_h;
+	unsigned int val_l;
+	unsigned int win;
+	dma_addr_t addr;
+	u32 val = 0;
+
+	if (!plane) {
+		DRM_ERROR("plane is NULL\n");
+		return;
+	}
+
+	win = plane->zpos;
+	if (win == DEFAULT_ZPOS)
+		win = 0;
+
+	if (win < 0 || win >= 5)
+		return;
+
+	reg_data = &decon->reg_data[win];
+
+	switch (plane->pixel_format) {
+	case DRM_FORMAT_XRGB1555:
+		val |= WINCONx_BPPMODE_16BPP_I1555;
+		val |= WINCONx_HAWSWP_F;
+		val |= WINCONx_BURSTLEN_16WORD;
+		break;
+	case DRM_FORMAT_RGB565:
+		val |= WINCONx_BPPMODE_16BPP_565;
+		val |= WINCONx_HAWSWP_F;
+		val |= WINCONx_BURSTLEN_16WORD;
+		break;
+	case DRM_FORMAT_XRGB8888:
+		val |= WINCONx_BPPMODE_24BPP_888;
+		val |= WINCONx_WSWP_F;
+		val |= WINCONx_BURSTLEN_16WORD;
+		break;
+	case DRM_FORMAT_ARGB8888:
+		val |= WINCONx_BPPMODE_32BPP_A8888;
+		val |= WINCONx_WSWP_F | WINCONx_BLD_PIX_F | WINCONx_ALPHA_SEL_F;
+		val |= WINCONx_BURSTLEN_16WORD;
+		break;
+	default:
+		val |= WINCONx_BPPMODE_24BPP_888;
+		val |= WINCONx_WSWP_F;
+		val |= WINCONx_BURSTLEN_16WORD;
+		break;
+	}
+
+	reg_data->wincon = val;
+	reg_data->vidosd_a = COORDINATE_X(plane->crtc_x) |
+				COORDINATE_Y(plane->crtc_y);
+	reg_data->vidosd_b =
+		COORDINATE_X(plane->crtc_x + plane->crtc_width - 1) |
+		COORDINATE_Y(plane->crtc_y + plane->crtc_height - 1);
+	reg_data->vidosd_c = VIDOSD_Wx_ALPHA_R_F(0x0) |
+				VIDOSD_Wx_ALPHA_G_F(0x0) |
+				VIDOSD_Wx_ALPHA_B_F(0x0);
+	reg_data->vidosd_d = VIDOSD_Wx_ALPHA_R_F(0x0) |
+				VIDOSD_Wx_ALPHA_G_F(0x0) |
+				VIDOSD_Wx_ALPHA_B_F(0x0);
+
+	addr = plane->dma_addr[0];
+	addr += plane->fb_width * plane->fb_y * bytes_per_pixel;
+	addr += plane->fb_x * bytes_per_pixel;
+
+	reg_data->vidw_add0 = addr;
+
+	addr += plane->fb_width * plane->crtc_height * bytes_per_pixel;
+
+	reg_data->vidw_add1 = addr;
+
+	val_h = (plane->fb_width - plane->crtc_width) * bytes_per_pixel;
+	val_l = plane->crtc_width * bytes_per_pixel;
+
+	reg_data->vidw_add2 = OFFSIZE(val_h) | PAGEWIDTH(val_l);
+}
+
+static void decon_win_commit(struct exynos_drm_crtc *crtc, int zpos)
+{
+	struct exynos_decon *decon = crtc->ctx;
+	struct decon_reg_data *reg_data;
+	unsigned int win = zpos;
+	u32 val;
+
+	if (win == DEFAULT_ZPOS)
+		win = 0;
+
+	if (win < 0 || win >= 5)
+		return;
+
+	reg_data = &decon->reg_data[win];
+
+	/* shadow update disable */
+	val = readl(decon->addr + DECON_SHADOWCON);
+	val |= SHADOWCON_Wx_PROTECT(win);
+	writel(val, decon->addr + DECON_SHADOWCON);
+
+	writel(reg_data->vidosd_a, decon->addr + DECON_VIDOSDxA(win));
+	writel(reg_data->vidosd_b, decon->addr + DECON_VIDOSDxB(win));
+	writel(reg_data->vidosd_c, decon->addr + DECON_VIDOSDxC(win));
+	writel(reg_data->vidosd_d, decon->addr + DECON_VIDOSDxD(win));
+	writel(reg_data->vidw_add0, decon->addr + DECON_VIDW0xADD0B0(win));
+	writel(reg_data->vidw_add1, decon->addr + DECON_VIDW0xADD1B0(win));
+	writel(reg_data->vidw_add2, decon->addr + DECON_VIDW0xADD2(win));
+
+	/* window enable */
+	val = reg_data->wincon;
+	val |= WINCONx_ENWIN_F;
+	writel(val, decon->addr + DECON_WINCONx(win));
+
+	/* shadow update enable */
+	val = readl(decon->addr + DECON_SHADOWCON);
+	val &= ~SHADOWCON_Wx_PROTECT(win);
+	writel(val, decon->addr + DECON_SHADOWCON);
+
+	/* standalone update */
+	val = readl(decon->addr + DECON_UPDATE);
+	val |= STANDALONE_UPDATE_F;
+	writel(val, decon->addr + DECON_UPDATE);
+
+	if (decon->i80_if)
+		atomic_set(&decon->win_updated, 1);
+}
+
+static void decon_win_disable(struct exynos_drm_crtc *crtc, int zpos)
+{
+	struct exynos_decon *decon = crtc->ctx;
+	struct decon_reg_data *reg_data;
+	unsigned int win = zpos;
+	u32 val;
+
+	if (win == DEFAULT_ZPOS)
+		win = 0;
+
+	if (win < 0 || win >= 5)
+		return;
+
+	reg_data = &decon->reg_data[win];
+
+	/* shadow update disable */
+	val = readl(decon->addr + DECON_SHADOWCON);
+	val |= SHADOWCON_Wx_PROTECT(win);
+	writel(val, decon->addr + DECON_SHADOWCON);
+
+	/* window disable */
+	val = reg_data->wincon;
+	val &= ~WINCONx_ENWIN_F;
+	writel(val, decon->addr + DECON_WINCONx(win));
+
+	/* shadow update enable */
+	val = readl(decon->addr + DECON_SHADOWCON);
+	val &= ~SHADOWCON_Wx_PROTECT(win);
+	writel(val, decon->addr + DECON_SHADOWCON);
+
+	/* standalone update */
+	val = readl(decon->addr + DECON_UPDATE);
+	val |= STANDALONE_UPDATE_F;
+	writel(val, decon->addr + DECON_UPDATE);
+}
+
+void decon_te_irq_handler(struct exynos_drm_crtc *crtc)
+{
+	struct exynos_decon *decon = crtc->ctx;
+	u32 val;
+
+	if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
+		return;
+
+	if (atomic_add_unless(&decon->win_updated, -1, 0)) {
+		/* trigger */
+		val = readl(decon->addr + DECON_TRIGCON);
+		val |= TRIGCON_SWTRIGCMD;
+		writel(val, decon->addr + DECON_TRIGCON);
+	}
+}
+
+static struct exynos_drm_crtc_ops decon_crtc_ops = {
+	.dpms			= decon_dpms,
+	.enable_vblank		= decon_enable_vblank,
+	.disable_vblank		= decon_disable_vblank,
+	.commit			= decon_commit,
+	.win_mode_set		= decon_win_mode_set,
+	.win_commit		= decon_win_commit,
+	.win_disable		= decon_win_disable,
+	.te_handler		= decon_te_irq_handler,
+};
+
+static int decon_bind(struct device *dev, struct device *master, void *data)
+{
+	struct exynos_decon *decon = dev_get_drvdata(dev);
+	struct drm_device *drm_dev = data;
+	struct exynos_drm_private *priv = drm_dev->dev_private;
+
+	decon->drm_dev = drm_dev;
+	decon->pipe = priv->pipe++;
+
+	decon->crtc = exynos_drm_crtc_create(drm_dev, decon->pipe,
+			EXYNOS_DISPLAY_TYPE_LCD, &decon_crtc_ops, decon);
+	if (IS_ERR(decon->crtc))
+		return PTR_ERR(decon->crtc);
+
+	return 0;
+}
+
+static void decon_unbind(struct device *dev, struct device *master, void *data)
+{
+	struct exynos_decon *decon = dev_get_drvdata(dev);
+
+	decon_dpms(decon->crtc, DRM_MODE_DPMS_OFF);
+}
+
+static const struct component_ops decon_component_ops = {
+	.bind	= decon_bind,
+	.unbind = decon_unbind,
+};
+
+static irqreturn_t decon_vsync_irq_handler(int irq, void *dev_id)
+{
+	struct exynos_decon *decon = dev_id;
+	u32 val;
+
+	if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
+		goto out;
+
+	val = readl(decon->addr + DECON_VIDINTCON1);
+	if (val & VIDINTCON1_INTFRMPEND) {
+		if (decon->drm_dev)
+			drm_handle_vblank(decon->drm_dev, decon->pipe);
+
+		/* clear */
+		writel(VIDINTCON1_INTFRMPEND, decon->addr + DECON_VIDINTCON1);
+	}
+
+out:
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t decon_lcd_sys_irq_handler(int irq, void *dev_id)
+{
+	struct exynos_decon *decon = dev_id;
+	u32 val;
+
+	if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
+		goto out;
+
+	val = readl(decon->addr + DECON_VIDINTCON1);
+	if (val & VIDINTCON1_INTFRMDONEPEND) {
+		if (decon->drm_dev)
+			exynos_drm_crtc_finish_pageflip(decon->drm_dev,
+							decon->pipe);
+		/* clear */
+		writel(VIDINTCON1_INTFRMDONEPEND,
+				decon->addr + DECON_VIDINTCON1);
+	}
+
+out:
+	return IRQ_HANDLED;
+}
+
+static int exynos5433_decon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct exynos_decon *decon;
+	struct resource *res;
+	int ret;
+	int i;
+
+	decon = devm_kzalloc(dev, sizeof(*decon), GFP_KERNEL);
+	if (!decon)
+		return -ENOMEM;
+
+	decon->dev = dev;
+	if (of_get_child_by_name(dev->of_node, "i80-if-timings"))
+		decon->i80_if = true;
+
+	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
+		struct clk *clk;
+
+		clk = devm_clk_get(decon->dev, decon_clks_name[i]);
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		decon->clks[i] = clk;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "cannot find IO resource\n");
+		return -ENXIO;
+	}
+
+	decon->addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(decon->addr)) {
+		dev_err(dev, "ioremap failed\n");
+		return PTR_ERR(decon->addr);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
+			decon->i80_if ? "lcd_sys" : "vsync");
+	if (!res) {
+		dev_err(dev, "cannot find IRQ resource\n");
+		return -ENXIO;
+	}
+
+	ret = devm_request_irq(dev, res->start, decon->i80_if ?
+			decon_lcd_sys_irq_handler : decon_vsync_irq_handler, 0,
+			"drm_decon", decon);
+	if (ret < 0) {
+		dev_err(dev, "lcd_sys irq request failed\n");
+		return ret;
+	}
+
+	ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,
+				       EXYNOS_DISPLAY_TYPE_LCD);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, decon);
+
+	ret = component_add(dev, &decon_component_ops);
+	if (ret < 0) {
+		exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CRTC);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int exynos5433_decon_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &decon_component_ops);
+	exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CRTC);
+
+	return 0;
+}
+
+static const struct of_device_id exynos5433_decon_driver_dt_match[] = {
+	{ .compatible = "samsung,exynos5433-decon" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, exynos5433_decon_driver_dt_match);
+
+struct platform_driver exynos5433_decon_driver = {
+	.probe		= exynos5433_decon_probe,
+	.remove		= exynos5433_decon_remove,
+	.driver		= {
+		.name	= "exynos5433-decon",
+		.owner	= THIS_MODULE,
+		.of_match_table = exynos5433_decon_driver_dt_match,
+	},
+};
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 98a239a..1fa0dd0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -556,6 +556,9 @@ static struct platform_driver *const exynos_drm_kms_drivers[] = {
 #ifdef CONFIG_DRM_EXYNOS_FIMD
 	&fimd_driver,
 #endif
+#ifdef CONFIG_DRM_EXYNOS5433_DECON
+	&exynos5433_decon_driver,
+#endif
 #ifdef CONFIG_DRM_EXYNOS7_DECON
 	&decon_driver,
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 9afd390..40996d8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -344,6 +344,7 @@ void exynos_drm_component_del(struct device *dev,
 				enum exynos_drm_device_type dev_type);

 extern struct platform_driver fimd_driver;
+extern struct platform_driver exynos5433_decon_driver;
 extern struct platform_driver decon_driver;
 extern struct platform_driver dp_driver;
 extern struct platform_driver dsi_driver;
diff --git a/drivers/gpu/drm/exynos/regs-exynos5433-decon.h b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h
new file mode 100644
index 0000000..9e7f851
--- /dev/null
+++ b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h
@@ -0,0 +1,163 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co.Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundationr
+ */
+
+#ifndef EXYNOS_REGS_DECON_H
+#define EXYNOS_REGS_DECON_H
+
+/* Exynos543X DECON */
+#define DECON_VIDCON0			0x0000
+#define DECON_VIDOUTCON0		0x0010
+#define DECON_WINCONx(n)		(0x0020 + ((n) * 4))
+#define DECON_VIDOSDxH(n)		(0x0080 + ((n) * 4))
+#define DECON_SHADOWCON			0x00A0
+#define DECON_VIDOSDxA(n)		(0x00B0 + ((n) * 0x20))
+#define DECON_VIDOSDxB(n)		(0x00B4 + ((n) * 0x20))
+#define DECON_VIDOSDxC(n)		(0x00B8 + ((n) * 0x20))
+#define DECON_VIDOSDxD(n)		(0x00BC + ((n) * 0x20))
+#define DECON_VIDOSDxE(n)		(0x00C0 + ((n) * 0x20))
+#define DECON_VIDW0xADD0B0(n)		(0x0150 + ((n) * 0x10))
+#define DECON_VIDW0xADD0B1(n)		(0x0154 + ((n) * 0x10))
+#define DECON_VIDW0xADD0B2(n)		(0x0158 + ((n) * 0x10))
+#define DECON_VIDW0xADD1B0(n)		(0x01A0 + ((n) * 0x10))
+#define DECON_VIDW0xADD1B1(n)		(0x01A4 + ((n) * 0x10))
+#define DECON_VIDW0xADD1B2(n)		(0x01A8 + ((n) * 0x10))
+#define DECON_VIDW0xADD2(n)		(0x0200 + ((n) * 4))
+#define DECON_LOCALxSIZE(n)		(0x0214 + ((n) * 4))
+#define DECON_VIDINTCON0		0x0220
+#define DECON_VIDINTCON1		0x0224
+#define DECON_WxKEYCON0(n)		(0x0230 + ((n - 1) * 8))
+#define DECON_WxKEYCON1(n)		(0x0234 + ((n - 1) * 8))
+#define DECON_WxKEYALPHA(n)		(0x0250 + ((n - 1) * 4))
+#define DECON_WINxMAP(n)		(0x0270 + ((n) * 4))
+#define DECON_QOSLUT07_00		0x02C0
+#define DECON_QOSLUT15_08		0x02C4
+#define DECON_QOSCTRL			0x02C8
+#define DECON_BLENDERQx(n)		(0x0300 + ((n - 1) * 4))
+#define DECON_BLENDCON			0x0310
+#define DECON_OPE_VIDW0xADD0(n)		(0x0400 + ((n) * 4))
+#define DECON_OPE_VIDW0xADD1(n)		(0x0414 + ((n) * 4))
+#define DECON_FRAMEFIFO_REG7		0x051C
+#define DECON_FRAMEFIFO_REG8		0x0520
+#define DECON_FRAMEFIFO_STATUS		0x0524
+#define DECON_CMU			0x1404
+#define DECON_UPDATE			0x1410
+#define DECON_UPDATE_SCHEME		0x1438
+#define DECON_VIDCON1			0x2000
+#define DECON_VIDCON2			0x2004
+#define DECON_VIDCON3			0x2008
+#define DECON_VIDCON4			0x200C
+#define DECON_VIDTCON2			0x2028
+#define DECON_FRAME_SIZE		0x2038
+#define DECON_LINECNT_OP_THRESHOLD	0x203C
+#define DECON_TRIGCON			0x2040
+#define DECON_TRIGSKIP			0x2050
+#define DECON_CRCRDATA			0x20B0
+#define DECON_CRCCTRL			0x20B4
+
+/* Exynos5430 DECON */
+#define DECON_VIDTCON0			0x2020
+#define DECON_VIDTCON1			0x2024
+
+/* Exynos5433 DECON */
+#define DECON_VIDTCON00			0x2010
+#define DECON_VIDTCON01			0x2014
+#define DECON_VIDTCON10			0x2018
+#define DECON_VIDTCON11			0x201C
+
+/* Exynos543X DECON Internal */
+#define DECON_W013DSTREOCON		0x0320
+#define DECON_W233DSTREOCON		0x0324
+#define DECON_FRAMEFIFO_REG0		0x0500
+#define DECON_ENHANCER_CTRL		0x2100
+
+/* Exynos543X DECON TV */
+#define DECON_VCLKCON0			0x0014
+#define DECON_VIDINTCON2		0x0228
+#define DECON_VIDINTCON3		0x022C
+
+/* VIDCON0 */
+#define VIDCON0_ENVID			(1 << 1)
+#define VIDCON0_ENVID_F			(1 << 0)
+
+/* VIDOUTCON0 */
+#define VIDOUT_LCD_ON			(1 << 24)
+#define VIDOUT_IF_F_MASK		(0x3 << 20)
+#define VIDOUT_RGB_IF			(0x0 << 20)
+#define VIDOUT_COMMAND_IF		(0x2 << 20)
+
+/* WINCONx */
+#define WINCONx_HAWSWP_F		(1 << 16)
+#define WINCONx_WSWP_F			(1 << 15)
+#define WINCONx_BURSTLEN_MASK		(0x3 << 10)
+#define WINCONx_BURSTLEN_16WORD		(0x0 << 10)
+#define WINCONx_BURSTLEN_8WORD		(0x1 << 10)
+#define WINCONx_BURSTLEN_4WORD		(0x2 << 10)
+#define WINCONx_BLD_PIX_F		(1 << 6)
+#define WINCONx_BPPMODE_MASK		(0xf << 2)
+#define WINCONx_BPPMODE_16BPP_565	(0x5 << 2)
+#define WINCONx_BPPMODE_16BPP_A1555	(0x6 << 2)
+#define WINCONx_BPPMODE_16BPP_I1555	(0x7 << 2)
+#define WINCONx_BPPMODE_24BPP_888	(0xb << 2)
+#define WINCONx_BPPMODE_24BPP_A1887	(0xc << 2)
+#define WINCONx_BPPMODE_25BPP_A1888	(0xd << 2)
+#define WINCONx_BPPMODE_32BPP_A8888	(0xd << 2)
+#define WINCONx_BPPMODE_16BPP_A4444	(0xe << 2)
+#define WINCONx_ALPHA_SEL_F		(1 << 1)
+#define WINCONx_ENWIN_F			(1 << 0)
+
+/* SHADOWCON */
+#define SHADOWCON_Wx_PROTECT(n)		(1 << (10 + (n)))
+
+/* VIDOSDxD */
+#define VIDOSD_Wx_ALPHA_R_F(n)		(((n) & 0xff) << 16)
+#define VIDOSD_Wx_ALPHA_G_F(n)		(((n) & 0xff) << 8)
+#define VIDOSD_Wx_ALPHA_B_F(n)		(((n) & 0xff) << 0)
+
+/* VIDINTCON0 */
+#define VIDINTCON0_FRAMEDONE		(1 << 17)
+#define VIDINTCON0_INTFRMEN		(1 << 12)
+#define VIDINTCON0_INTEN		(1 << 0)
+
+/* VIDINTCON1 */
+#define VIDINTCON1_INTFRMDONEPEND	(1 << 2)
+#define VIDINTCON1_INTFRMPEND		(1 << 1)
+#define VIDINTCON1_INTFIFOPEND		(1 << 0)
+
+/* DECON_CMU */
+#define CMU_CLKGAGE_MODE_SFR_F		(1 << 1)
+#define CMU_CLKGAGE_MODE_MEM_F		(1 << 0)
+
+/* DECON_UPDATE */
+#define STANDALONE_UPDATE_F		(1 << 0)
+
+/* DECON_VIDTCON00 */
+#define VIDTCON00_VBPD_F(x)		(((x) & 0xfff) << 16)
+#define VIDTCON00_VFPD_F(x)		((x) & 0xfff)
+
+/* DECON_VIDTCON01 */
+#define VIDTCON01_VSPW_F(x)		(((x) & 0xfff) << 16)
+
+/* DECON_VIDTCON10 */
+#define VIDTCON10_HBPD_F(x)		(((x) & 0xfff) << 16)
+#define VIDTCON10_HFPD_F(x)		((x) & 0xfff)
+
+/* DECON_VIDTCON11 */
+#define VIDTCON11_HSPW_F(x)		(((x) & 0xfff) << 16)
+
+/* DECON_VIDTCON2 */
+#define VIDTCON2_LINEVAL(x)		(((x) & 0xfff) << 16)
+#define VIDTCON2_HOZVAL(x)		((x) & 0xfff)
+
+/* TRIGCON */
+#define TRIGCON_TRIGEN_PER_F		(1 << 31)
+#define TRIGCON_TRIGEN_F		(1 << 30)
+#define TRIGCON_TE_AUTO_MASK		(1 << 29)
+#define TRIGCON_SWTRIGCMD		(1 << 1)
+#define TRIGCON_SWTRIGEN		(1 << 0)
+
+#endif /* EXYNOS_REGS_DECON_H */
--
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/6] drm/exynos: add Exynos5433 decon driver

[Daniel Stone]

Hi,
Some feedback comments - most of these are not unique to your 5433
DECON driver but endemic throughout Exynos, so I don't blame you for
them - but they should be fixed anyway.

On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@samsung.com> wrote:
> +static void decon_dpms_on(struct exynos_decon *decon)
> +{
> +       int ret;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
> +               ret = clk_prepare_enable(decon->clks[i]);
> +               if (ret < 0)
> +                       goto err;
> +       }
> +
> +       set_bit(BIT_CLKS_ENABLED, &decon->enabled);

Do you really not even need to set a control register?

> +static void decon_commit(struct exynos_drm_crtc *crtc)
> +{
> +       struct exynos_decon *decon = crtc->ctx;
> +       struct drm_display_mode *mode = &crtc->base.mode;
> +       u32 val;
> +
> +       /* enable clock gate */
> +       val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
> +       writel(val, decon->addr + DECON_CMU);
> +
> +       /* lcd on and use command if */
> +       val = VIDOUT_LCD_ON;
> +       if (decon->i80_if)
> +               val |= VIDOUT_COMMAND_IF;
> +       else
> +               val |= VIDOUT_RGB_IF;
> +       writel(val, decon->addr + DECON_VIDOUTCON0);

This seems much more likely to be DPMS, no?

> +       [...]
> +       /* enable output and display signal */
> +       val = VIDCON0_ENVID | VIDCON0_ENVID_F;
> +       writel(val, decon->addr + DECON_VIDCON0);

As does this.

Have you tested DPMS on/off, without enabling/disabling the CRTC
first? Does it work?

> +static void decon_win_mode_set(struct exynos_drm_crtc *crtc,
> +                              struct exynos_drm_plane *plane)
> +{
> +       struct exynos_decon *decon = crtc->ctx;
> +       struct decon_reg_data *reg_data;
> +       unsigned int bytes_per_pixel = plane->bpp >> 3;
> +       unsigned int val_h;
> +       unsigned int val_l;
> +       unsigned int win;
> +       dma_addr_t addr;
> +       u32 val = 0;
> +
> +       if (!plane) {
> +               DRM_ERROR("plane is NULL\n");
> +               return;
> +       }
> +
> +       win = plane->zpos;
> +       if (win == DEFAULT_ZPOS)
> +               win = 0;
> +
> +       if (win < 0 || win >= 5)
> +               return;

It would be nice to have a #define for the largest-supported window number.

> +       reg_data = &decon->reg_data[win];
> +
> +       switch (plane->pixel_format) {
> +       case DRM_FORMAT_XRGB1555:
> +               val |= WINCONx_BPPMODE_16BPP_I1555;
> +               val |= WINCONx_HAWSWP_F;
> +               val |= WINCONx_BURSTLEN_16WORD;
> +               break;
> +       case DRM_FORMAT_RGB565:
> +               val |= WINCONx_BPPMODE_16BPP_565;
> +               val |= WINCONx_HAWSWP_F;
> +               val |= WINCONx_BURSTLEN_16WORD;
> +               break;
> +       case DRM_FORMAT_XRGB8888:
> +               val |= WINCONx_BPPMODE_24BPP_888;
> +               val |= WINCONx_WSWP_F;
> +               val |= WINCONx_BURSTLEN_16WORD;
> +               break;
> +       case DRM_FORMAT_ARGB8888:
> +               val |= WINCONx_BPPMODE_32BPP_A8888;
> +               val |= WINCONx_WSWP_F | WINCONx_BLD_PIX_F | WINCONx_ALPHA_SEL_F;
> +               val |= WINCONx_BURSTLEN_16WORD;
> +               break;
> +       default:

Please remove the 'default' case. If you get here with a format you
don't know how to configure, then it is a bug and should be fixed: the
plane should never advertise a format that it cannot support.

> +               val |= WINCONx_BPPMODE_24BPP_888;
> +               val |= WINCONx_WSWP_F;
> +               val |= WINCONx_BURSTLEN_16WORD;
> +               break;
> +       }
> +
> +       reg_data->wincon = val;
> +       reg_data->vidosd_a = COORDINATE_X(plane->crtc_x) |
> +                               COORDINATE_Y(plane->crtc_y);
> +       reg_data->vidosd_b =
> +               COORDINATE_X(plane->crtc_x + plane->crtc_width - 1) |
> +               COORDINATE_Y(plane->crtc_y + plane->crtc_height - 1);
> +       reg_data->vidosd_c = VIDOSD_Wx_ALPHA_R_F(0x0) |
> +                               VIDOSD_Wx_ALPHA_G_F(0x0) |
> +                               VIDOSD_Wx_ALPHA_B_F(0x0);
> +       reg_data->vidosd_d = VIDOSD_Wx_ALPHA_R_F(0x0) |
> +                               VIDOSD_Wx_ALPHA_G_F(0x0) |
> +                               VIDOSD_Wx_ALPHA_B_F(0x0);
> +
> +       addr = plane->dma_addr[0];
> +       addr += plane->fb_width * plane->fb_y * bytes_per_pixel;

Replace plane->fb_width * bytes_per_pixel by plane->fb_pitch please,
and set plane->fb_pitch from exynos_drm_plane->pitch. See this patch:
https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg42861.html

You should be able to test this case, either by making a specialised
userspace program which has a larger pitch with garbage values in the
padding (see https://msdn.microsoft.com/en-us/library/windows/desktop/aa473780(v=vs.85).aspx),
or by testing a resolution where width*bytespp is not a multiple of 4,
e.g. 1366x768.

> +       addr += plane->fb_x * bytes_per_pixel;
> +
> +       reg_data->vidw_add0 = addr;
> +
> +       addr += plane->fb_width * plane->crtc_height * bytes_per_pixel;

Again, replace fb_width*bytes_per_pixel with fb_pitch.

> +       reg_data->vidw_add1 = addr;
> +
> +       val_h = (plane->fb_width - plane->crtc_width) * bytes_per_pixel;

val_h = plane->fb_pitch - (plane->crtc_width * bytes_per_pixel);

> +static void decon_win_commit(struct exynos_drm_crtc *crtc, int zpos)
> +{
> +       struct exynos_decon *decon = crtc->ctx;
> +       struct decon_reg_data *reg_data;
> +       unsigned int win = zpos;
> +       u32 val;
> +
> +       if (win == DEFAULT_ZPOS)
> +               win = 0;
> +
> +       if (win < 0 || win >= 5)
> +               return;
> +
> +       reg_data = &decon->reg_data[win];
> +
> +       /* shadow update disable */
> +       val = readl(decon->addr + DECON_SHADOWCON);
> +       val |= SHADOWCON_Wx_PROTECT(win);
> +       writel(val, decon->addr + DECON_SHADOWCON);
> +
> +       writel(reg_data->vidosd_a, decon->addr + DECON_VIDOSDxA(win));
> +       writel(reg_data->vidosd_b, decon->addr + DECON_VIDOSDxB(win));
> +       writel(reg_data->vidosd_c, decon->addr + DECON_VIDOSDxC(win));
> +       writel(reg_data->vidosd_d, decon->addr + DECON_VIDOSDxD(win));
> +       writel(reg_data->vidw_add0, decon->addr + DECON_VIDW0xADD0B0(win));
> +       writel(reg_data->vidw_add1, decon->addr + DECON_VIDW0xADD1B0(win));
> +       writel(reg_data->vidw_add2, decon->addr + DECON_VIDW0xADD2(win));
> +
> +       /* window enable */
> +       val = reg_data->wincon;
> +       val |= WINCONx_ENWIN_F;
> +       writel(val, decon->addr + DECON_WINCONx(win));
> +
> +       /* shadow update enable */
> +       val = readl(decon->addr + DECON_SHADOWCON);
> +       val &= ~SHADOWCON_Wx_PROTECT(win);
> +       writel(val, decon->addr + DECON_SHADOWCON);
> +
> +       /* standalone update */
> +       val = readl(decon->addr + DECON_UPDATE);
> +       val |= STANDALONE_UPDATE_F;
> +       writel(val, decon->addr + DECON_UPDATE);
> +
> +       if (decon->i80_if)
> +               atomic_set(&decon->win_updated, 1);
> +}
> +
> +static void decon_win_disable(struct exynos_drm_crtc *crtc, int zpos)
> +{
> +       struct exynos_decon *decon = crtc->ctx;
> +       struct decon_reg_data *reg_data;
> +       unsigned int win = zpos;
> +       u32 val;
> +
> +       if (win == DEFAULT_ZPOS)
> +               win = 0;
> +
> +       if (win < 0 || win >= 5)
> +               return;
> +
> +       reg_data = &decon->reg_data[win];
> +
> +       /* shadow update disable */
> +       val = readl(decon->addr + DECON_SHADOWCON);
> +       val |= SHADOWCON_Wx_PROTECT(win);
> +       writel(val, decon->addr + DECON_SHADOWCON);
> +
> +       /* window disable */
> +       val = reg_data->wincon;
> +       val &= ~WINCONx_ENWIN_F;
> +       writel(val, decon->addr + DECON_WINCONx(win));
> +
> +       /* shadow update enable */
> +       val = readl(decon->addr + DECON_SHADOWCON);
> +       val &= ~SHADOWCON_Wx_PROTECT(win);
> +       writel(val, decon->addr + DECON_SHADOWCON);
> +
> +       /* standalone update */
> +       val = readl(decon->addr + DECON_UPDATE);
> +       val |= STANDALONE_UPDATE_F;
> +       writel(val, decon->addr + DECON_UPDATE);
> +}
> +
> +void decon_te_irq_handler(struct exynos_drm_crtc *crtc)
> +{
> +       struct exynos_decon *decon = crtc->ctx;
> +       u32 val;
> +
> +       if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
> +               return;
> +
> +       if (atomic_add_unless(&decon->win_updated, -1, 0)) {
> +               /* trigger */
> +               val = readl(decon->addr + DECON_TRIGCON);
> +               val |= TRIGCON_SWTRIGCMD;
> +               writel(val, decon->addr + DECON_TRIGCON);
> +       }
> +}
> +
> +static struct exynos_drm_crtc_ops decon_crtc_ops = {
> +       .dpms                   = decon_dpms,
> +       .enable_vblank          = decon_enable_vblank,
> +       .disable_vblank         = decon_disable_vblank,
> +       .commit                 = decon_commit,
> +       .win_mode_set           = decon_win_mode_set,
> +       .win_commit             = decon_win_commit,
> +       .win_disable            = decon_win_disable,
> +       .te_handler             = decon_te_irq_handler,
> +};
> +
> +static int decon_bind(struct device *dev, struct device *master, void *data)
> +{
> +       struct exynos_decon *decon = dev_get_drvdata(dev);
> +       struct drm_device *drm_dev = data;
> +       struct exynos_drm_private *priv = drm_dev->dev_private;
> +
> +       decon->drm_dev = drm_dev;
> +       decon->pipe = priv->pipe++;
> +
> +       decon->crtc = exynos_drm_crtc_create(drm_dev, decon->pipe,
> +                       EXYNOS_DISPLAY_TYPE_LCD, &decon_crtc_ops, decon);
> +       if (IS_ERR(decon->crtc))
> +               return PTR_ERR(decon->crtc);

Should failing to register the CRTC also decrement priv->pipe?

> +
> +       return 0;
> +}
> +
> +static void decon_unbind(struct device *dev, struct device *master, void *data)
> +{
> +       struct exynos_decon *decon = dev_get_drvdata(dev);
> +
> +       decon_dpms(decon->crtc, DRM_MODE_DPMS_OFF);
> +}

As above, it seems like DPMS will not do enough here.

> +static const struct component_ops decon_component_ops = {
> +       .bind   = decon_bind,
> +       .unbind = decon_unbind,
> +};
> +
> +static irqreturn_t decon_vsync_irq_handler(int irq, void *dev_id)
> +{
> +       struct exynos_decon *decon = dev_id;
> +       u32 val;
> +
> +       if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
> +               goto out;
> +
> +       val = readl(decon->addr + DECON_VIDINTCON1);
> +       if (val & VIDINTCON1_INTFRMPEND) {
> +               if (decon->drm_dev)
> +                       drm_handle_vblank(decon->drm_dev, decon->pipe);

If decon->drm_dev is ever NULL, it sounds like something has gone very
seriously wrong.

> +               /* clear */
> +               writel(VIDINTCON1_INTFRMPEND, decon->addr + DECON_VIDINTCON1);
> +       }
> +
> +out:
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t decon_lcd_sys_irq_handler(int irq, void *dev_id)
> +{
> +       struct exynos_decon *decon = dev_id;
> +       u32 val;
> +
> +       if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
> +               goto out;
> +
> +       val = readl(decon->addr + DECON_VIDINTCON1);
> +       if (val & VIDINTCON1_INTFRMDONEPEND) {
> +               if (decon->drm_dev)
> +                       exynos_drm_crtc_finish_pageflip(decon->drm_dev,
> +                                                       decon->pipe);

Ditto - when would drm_dev be NULL?

> +               /* clear */
> +               writel(VIDINTCON1_INTFRMDONEPEND,
> +                               decon->addr + DECON_VIDINTCON1);
> +       }
> +
> +out:
> +       return IRQ_HANDLED;
> +}
> +
> +static int exynos5433_decon_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct exynos_decon *decon;
> +       struct resource *res;
> +       int ret;
> +       int i;
> +
> +       decon = devm_kzalloc(dev, sizeof(*decon), GFP_KERNEL);
> +       if (!decon)
> +               return -ENOMEM;
> +
> +       decon->dev = dev;
> +       if (of_get_child_by_name(dev->of_node, "i80-if-timings"))
> +               decon->i80_if = true;
> +
> +       for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
> +               struct clk *clk;
> +
> +               clk = devm_clk_get(decon->dev, decon_clks_name[i]);
> +               if (IS_ERR(clk))
> +                       return PTR_ERR(clk);
> +
> +               decon->clks[i] = clk;
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_err(dev, "cannot find IO resource\n");
> +               return -ENXIO;
> +       }
> +
> +       decon->addr = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(decon->addr)) {
> +               dev_err(dev, "ioremap failed\n");
> +               return PTR_ERR(decon->addr);
> +       }
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> +                       decon->i80_if ? "lcd_sys" : "vsync");
> +       if (!res) {
> +               dev_err(dev, "cannot find IRQ resource\n");
> +               return -ENXIO;
> +       }
> +
> +       ret = devm_request_irq(dev, res->start, decon->i80_if ?
> +                       decon_lcd_sys_irq_handler : decon_vsync_irq_handler, 0,
> +                       "drm_decon", decon);
> +       if (ret < 0) {
> +               dev_err(dev, "lcd_sys irq request failed\n");
> +               return ret;
> +       }
> +
> +       ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,
> +                                      EXYNOS_DISPLAY_TYPE_LCD);
> +       if (ret < 0)
> +               return ret;
> +
> +       platform_set_drvdata(pdev, decon);
> +
> +       ret = component_add(dev, &decon_component_ops);
> +       if (ret < 0) {
> +               exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CRTC);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int exynos5433_decon_remove(struct platform_device *pdev)
> +{
> +       component_del(&pdev->dev, &decon_component_ops);
> +       exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CRTC);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id exynos5433_decon_driver_dt_match[] = {
> +       { .compatible = "samsung,exynos5433-decon" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos5433_decon_driver_dt_match);
> +
> +struct platform_driver exynos5433_decon_driver = {
> +       .probe          = exynos5433_decon_probe,
> +       .remove         = exynos5433_decon_remove,
> +       .driver         = {
> +               .name   = "exynos5433-decon",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = exynos5433_decon_driver_dt_match,
> +       },
> +};
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 98a239a..1fa0dd0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -556,6 +556,9 @@ static struct platform_driver *const exynos_drm_kms_drivers[] = {
>  #ifdef CONFIG_DRM_EXYNOS_FIMD
>         &fimd_driver,
>  #endif
> +#ifdef CONFIG_DRM_EXYNOS5433_DECON
> +       &exynos5433_decon_driver,
> +#endif
>  #ifdef CONFIG_DRM_EXYNOS7_DECON
>         &decon_driver,
>  #endif
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 9afd390..40996d8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -344,6 +344,7 @@ void exynos_drm_component_del(struct device *dev,
>                                 enum exynos_drm_device_type dev_type);
>
>  extern struct platform_driver fimd_driver;
> +extern struct platform_driver exynos5433_decon_driver;
>  extern struct platform_driver decon_driver;
>  extern struct platform_driver dp_driver;
>  extern struct platform_driver dsi_driver;
> diff --git a/drivers/gpu/drm/exynos/regs-exynos5433-decon.h b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h
> new file mode 100644
> index 0000000..9e7f851
> --- /dev/null
> +++ b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h
> @@ -0,0 +1,163 @@
> +/*
> + * Copyright (C) 2014 Samsung Electronics Co.Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundationr
> + */
> +
> +#ifndef EXYNOS_REGS_DECON_H
> +#define EXYNOS_REGS_DECON_H
> +
> +/* Exynos543X DECON */
> +#define DECON_VIDCON0                  0x0000
> +#define DECON_VIDOUTCON0               0x0010
> +#define DECON_WINCONx(n)               (0x0020 + ((n) * 4))
> +#define DECON_VIDOSDxH(n)              (0x0080 + ((n) * 4))
> +#define DECON_SHADOWCON                        0x00A0
> +#define DECON_VIDOSDxA(n)              (0x00B0 + ((n) * 0x20))
> +#define DECON_VIDOSDxB(n)              (0x00B4 + ((n) * 0x20))
> +#define DECON_VIDOSDxC(n)              (0x00B8 + ((n) * 0x20))
> +#define DECON_VIDOSDxD(n)              (0x00BC + ((n) * 0x20))
> +#define DECON_VIDOSDxE(n)              (0x00C0 + ((n) * 0x20))
> +#define DECON_VIDW0xADD0B0(n)          (0x0150 + ((n) * 0x10))
> +#define DECON_VIDW0xADD0B1(n)          (0x0154 + ((n) * 0x10))
> +#define DECON_VIDW0xADD0B2(n)          (0x0158 + ((n) * 0x10))
> +#define DECON_VIDW0xADD1B0(n)          (0x01A0 + ((n) * 0x10))
> +#define DECON_VIDW0xADD1B1(n)          (0x01A4 + ((n) * 0x10))
> +#define DECON_VIDW0xADD1B2(n)          (0x01A8 + ((n) * 0x10))
> +#define DECON_VIDW0xADD2(n)            (0x0200 + ((n) * 4))
> +#define DECON_LOCALxSIZE(n)            (0x0214 + ((n) * 4))
> +#define DECON_VIDINTCON0               0x0220
> +#define DECON_VIDINTCON1               0x0224
> +#define DECON_WxKEYCON0(n)             (0x0230 + ((n - 1) * 8))
> +#define DECON_WxKEYCON1(n)             (0x0234 + ((n - 1) * 8))
> +#define DECON_WxKEYALPHA(n)            (0x0250 + ((n - 1) * 4))
> +#define DECON_WINxMAP(n)               (0x0270 + ((n) * 4))
> +#define DECON_QOSLUT07_00              0x02C0
> +#define DECON_QOSLUT15_08              0x02C4
> +#define DECON_QOSCTRL                  0x02C8
> +#define DECON_BLENDERQx(n)             (0x0300 + ((n - 1) * 4))
> +#define DECON_BLENDCON                 0x0310
> +#define DECON_OPE_VIDW0xADD0(n)                (0x0400 + ((n) * 4))
> +#define DECON_OPE_VIDW0xADD1(n)                (0x0414 + ((n) * 4))
> +#define DECON_FRAMEFIFO_REG7           0x051C
> +#define DECON_FRAMEFIFO_REG8           0x0520
> +#define DECON_FRAMEFIFO_STATUS         0x0524
> +#define DECON_CMU                      0x1404
> +#define DECON_UPDATE                   0x1410
> +#define DECON_UPDATE_SCHEME            0x1438
> +#define DECON_VIDCON1                  0x2000
> +#define DECON_VIDCON2                  0x2004
> +#define DECON_VIDCON3                  0x2008
> +#define DECON_VIDCON4                  0x200C
> +#define DECON_VIDTCON2                 0x2028
> +#define DECON_FRAME_SIZE               0x2038
> +#define DECON_LINECNT_OP_THRESHOLD     0x203C
> +#define DECON_TRIGCON                  0x2040
> +#define DECON_TRIGSKIP                 0x2050
> +#define DECON_CRCRDATA                 0x20B0
> +#define DECON_CRCCTRL                  0x20B4
> +
> +/* Exynos5430 DECON */
> +#define DECON_VIDTCON0                 0x2020
> +#define DECON_VIDTCON1                 0x2024
> +
> +/* Exynos5433 DECON */
> +#define DECON_VIDTCON00                        0x2010
> +#define DECON_VIDTCON01                        0x2014
> +#define DECON_VIDTCON10                        0x2018
> +#define DECON_VIDTCON11                        0x201C
> +
> +/* Exynos543X DECON Internal */
> +#define DECON_W013DSTREOCON            0x0320
> +#define DECON_W233DSTREOCON            0x0324
> +#define DECON_FRAMEFIFO_REG0           0x0500
> +#define DECON_ENHANCER_CTRL            0x2100
> +
> +/* Exynos543X DECON TV */
> +#define DECON_VCLKCON0                 0x0014
> +#define DECON_VIDINTCON2               0x0228
> +#define DECON_VIDINTCON3               0x022C

Yes, my impression was that DECON supported both internal and
external, i.e. would replace both FIMD/Mixer. This patch only supports
the internal interface: are you planning to add support for the
external/TV/HDMI interface as well? If so, how does this work with the
DT format you have defined? Should there maybe separate decon-int and
decon-tv subnodes, each of which create one CRTC? The Android tree
seems to rely on static configuration, but presumably we would need a
much more nuanced approach.

If so, it seems like the first start would be to add a basic DECON
binding which covered the entire controller, and then as a next step
add support for decon-int as a subdevice of this.

Cheers,
Daniel

> +/* VIDCON0 */
> +#define VIDCON0_ENVID                  (1 << 1)
> +#define VIDCON0_ENVID_F                        (1 << 0)
> +
> +/* VIDOUTCON0 */
> +#define VIDOUT_LCD_ON                  (1 << 24)
> +#define VIDOUT_IF_F_MASK               (0x3 << 20)
> +#define VIDOUT_RGB_IF                  (0x0 << 20)
> +#define VIDOUT_COMMAND_IF              (0x2 << 20)
> +
> +/* WINCONx */
> +#define WINCONx_HAWSWP_F               (1 << 16)
> +#define WINCONx_WSWP_F                 (1 << 15)
> +#define WINCONx_BURSTLEN_MASK          (0x3 << 10)
> +#define WINCONx_BURSTLEN_16WORD                (0x0 << 10)
> +#define WINCONx_BURSTLEN_8WORD         (0x1 << 10)
> +#define WINCONx_BURSTLEN_4WORD         (0x2 << 10)
> +#define WINCONx_BLD_PIX_F              (1 << 6)
> +#define WINCONx_BPPMODE_MASK           (0xf << 2)
> +#define WINCONx_BPPMODE_16BPP_565      (0x5 << 2)
> +#define WINCONx_BPPMODE_16BPP_A1555    (0x6 << 2)
> +#define WINCONx_BPPMODE_16BPP_I1555    (0x7 << 2)
> +#define WINCONx_BPPMODE_24BPP_888      (0xb << 2)
> +#define WINCONx_BPPMODE_24BPP_A1887    (0xc << 2)
> +#define WINCONx_BPPMODE_25BPP_A1888    (0xd << 2)
> +#define WINCONx_BPPMODE_32BPP_A8888    (0xd << 2)
> +#define WINCONx_BPPMODE_16BPP_A4444    (0xe << 2)
> +#define WINCONx_ALPHA_SEL_F            (1 << 1)
> +#define WINCONx_ENWIN_F                        (1 << 0)
> +
> +/* SHADOWCON */
> +#define SHADOWCON_Wx_PROTECT(n)                (1 << (10 + (n)))
> +
> +/* VIDOSDxD */
> +#define VIDOSD_Wx_ALPHA_R_F(n)         (((n) & 0xff) << 16)
> +#define VIDOSD_Wx_ALPHA_G_F(n)         (((n) & 0xff) << 8)
> +#define VIDOSD_Wx_ALPHA_B_F(n)         (((n) & 0xff) << 0)
> +
> +/* VIDINTCON0 */
> +#define VIDINTCON0_FRAMEDONE           (1 << 17)
> +#define VIDINTCON0_INTFRMEN            (1 << 12)
> +#define VIDINTCON0_INTEN               (1 << 0)
> +
> +/* VIDINTCON1 */
> +#define VIDINTCON1_INTFRMDONEPEND      (1 << 2)
> +#define VIDINTCON1_INTFRMPEND          (1 << 1)
> +#define VIDINTCON1_INTFIFOPEND         (1 << 0)
> +
> +/* DECON_CMU */
> +#define CMU_CLKGAGE_MODE_SFR_F         (1 << 1)
> +#define CMU_CLKGAGE_MODE_MEM_F         (1 << 0)
> +
> +/* DECON_UPDATE */
> +#define STANDALONE_UPDATE_F            (1 << 0)
> +
> +/* DECON_VIDTCON00 */
> +#define VIDTCON00_VBPD_F(x)            (((x) & 0xfff) << 16)
> +#define VIDTCON00_VFPD_F(x)            ((x) & 0xfff)
> +
> +/* DECON_VIDTCON01 */
> +#define VIDTCON01_VSPW_F(x)            (((x) & 0xfff) << 16)
> +
> +/* DECON_VIDTCON10 */
> +#define VIDTCON10_HBPD_F(x)            (((x) & 0xfff) << 16)
> +#define VIDTCON10_HFPD_F(x)            ((x) & 0xfff)
> +
> +/* DECON_VIDTCON11 */
> +#define VIDTCON11_HSPW_F(x)            (((x) & 0xfff) << 16)
> +
> +/* DECON_VIDTCON2 */
> +#define VIDTCON2_LINEVAL(x)            (((x) & 0xfff) << 16)
> +#define VIDTCON2_HOZVAL(x)             ((x) & 0xfff)
> +
> +/* TRIGCON */
> +#define TRIGCON_TRIGEN_PER_F           (1 << 31)
> +#define TRIGCON_TRIGEN_F               (1 << 30)
> +#define TRIGCON_TE_AUTO_MASK           (1 << 29)
> +#define TRIGCON_SWTRIGCMD              (1 << 1)
> +#define TRIGCON_SWTRIGEN               (1 << 0)
> +
> +#endif /* EXYNOS_REGS_DECON_H */
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/6] drm/exynos: add Exynos5433 decon driver

[Hyungwon Hwang]

Dear Daniel,

On Wed, 18 Mar 2015 12:24:40 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> Some feedback comments - most of these are not unique to your 5433
> DECON driver but endemic throughout Exynos, so I don't blame you for
> them - but they should be fixed anyway.
> 
> On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@samsung.com>
> wrote:
> > +static void decon_dpms_on(struct exynos_decon *decon)
> > +{
> > +       int ret;
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
> > +               ret = clk_prepare_enable(decon->clks[i]);
> > +               if (ret < 0)
> > +                       goto err;
> > +       }
> > +
> > +       set_bit(BIT_CLKS_ENABLED, &decon->enabled);
> 
> Do you really not even need to set a control register?
> 
> > +static void decon_commit(struct exynos_drm_crtc *crtc)
> > +{
> > +       struct exynos_decon *decon = crtc->ctx;
> > +       struct drm_display_mode *mode = &crtc->base.mode;
> > +       u32 val;
> > +
> > +       /* enable clock gate */
> > +       val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;
> > +       writel(val, decon->addr + DECON_CMU);
> > +
> > +       /* lcd on and use command if */
> > +       val = VIDOUT_LCD_ON;
> > +       if (decon->i80_if)
> > +               val |= VIDOUT_COMMAND_IF;
> > +       else
> > +               val |= VIDOUT_RGB_IF;
> > +       writel(val, decon->addr + DECON_VIDOUTCON0);
> 
> This seems much more likely to be DPMS, no?
> 
> > +       [...]
> > +       /* enable output and display signal */
> > +       val = VIDCON0_ENVID | VIDCON0_ENVID_F;
> > +       writel(val, decon->addr + DECON_VIDCON0);
> 
> As does this.
> 
> Have you tested DPMS on/off, without enabling/disabling the CRTC
> first? Does it work?

Yes. I misunderstanded the behavior of the driver. I will send the
fixed version for working DPMS. Thanks.

> 
> > +static void decon_win_mode_set(struct exynos_drm_crtc *crtc,
> > +                              struct exynos_drm_plane *plane)
> > +{
> > +       struct exynos_decon *decon = crtc->ctx;
> > +       struct decon_reg_data *reg_data;
> > +       unsigned int bytes_per_pixel = plane->bpp >> 3;
> > +       unsigned int val_h;
> > +       unsigned int val_l;
> > +       unsigned int win;
> > +       dma_addr_t addr;
> > +       u32 val = 0;
> > +
> > +       if (!plane) {
> > +               DRM_ERROR("plane is NULL\n");
> > +               return;
> > +       }
> > +
> > +       win = plane->zpos;
> > +       if (win == DEFAULT_ZPOS)
> > +               win = 0;
> > +
> > +       if (win < 0 || win >= 5)
> > +               return;
> 
> It would be nice to have a #define for the largest-supported window
> number.

Yes. That would be better.

> 
> > +       reg_data = &decon->reg_data[win];
> > +
> > +       switch (plane->pixel_format) {
> > +       case DRM_FORMAT_XRGB1555:
> > +               val |= WINCONx_BPPMODE_16BPP_I1555;
> > +               val |= WINCONx_HAWSWP_F;
> > +               val |= WINCONx_BURSTLEN_16WORD;
> > +               break;
> > +       case DRM_FORMAT_RGB565:
> > +               val |= WINCONx_BPPMODE_16BPP_565;
> > +               val |= WINCONx_HAWSWP_F;
> > +               val |= WINCONx_BURSTLEN_16WORD;
> > +               break;
> > +       case DRM_FORMAT_XRGB8888:
> > +               val |= WINCONx_BPPMODE_24BPP_888;
> > +               val |= WINCONx_WSWP_F;
> > +               val |= WINCONx_BURSTLEN_16WORD;
> > +               break;
> > +       case DRM_FORMAT_ARGB8888:
> > +               val |= WINCONx_BPPMODE_32BPP_A8888;
> > +               val |= WINCONx_WSWP_F | WINCONx_BLD_PIX_F |
> > WINCONx_ALPHA_SEL_F;
> > +               val |= WINCONx_BURSTLEN_16WORD;
> > +               break;
> > +       default:
> 
> Please remove the 'default' case. If you get here with a format you
> don't know how to configure, then it is a bug and should be fixed: the
> plane should never advertise a format that it cannot support.
> 

Yes. I agree.

> > +               val |= WINCONx_BPPMODE_24BPP_888;
> > +               val |= WINCONx_WSWP_F;
> > +               val |= WINCONx_BURSTLEN_16WORD;
> > +               break;
> > +       }
> > +
> > +       reg_data->wincon = val;
> > +       reg_data->vidosd_a = COORDINATE_X(plane->crtc_x) |
> > +                               COORDINATE_Y(plane->crtc_y);
> > +       reg_data->vidosd_b =
> > +               COORDINATE_X(plane->crtc_x + plane->crtc_width - 1)
> > |
> > +               COORDINATE_Y(plane->crtc_y + plane->crtc_height -
> > 1);
> > +       reg_data->vidosd_c = VIDOSD_Wx_ALPHA_R_F(0x0) |
> > +                               VIDOSD_Wx_ALPHA_G_F(0x0) |
> > +                               VIDOSD_Wx_ALPHA_B_F(0x0);
> > +       reg_data->vidosd_d = VIDOSD_Wx_ALPHA_R_F(0x0) |
> > +                               VIDOSD_Wx_ALPHA_G_F(0x0) |
> > +                               VIDOSD_Wx_ALPHA_B_F(0x0);
> > +
> > +       addr = plane->dma_addr[0];
> > +       addr += plane->fb_width * plane->fb_y * bytes_per_pixel;
> 
> Replace plane->fb_width * bytes_per_pixel by plane->fb_pitch please,
> and set plane->fb_pitch from exynos_drm_plane->pitch. See this patch:
> https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg42861.html
> 
> You should be able to test this case, either by making a specialised
> userspace program which has a larger pitch with garbage values in the
> padding (see
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa473780(v=vs.85).aspx),
> or by testing a resolution where width*bytespp is not a multiple of
> 4, e.g. 1366x768.
> 

Yes. I agree.

> > +       addr += plane->fb_x * bytes_per_pixel;
> > +
> > +       reg_data->vidw_add0 = addr;
> > +
> > +       addr += plane->fb_width * plane->crtc_height *
> > bytes_per_pixel;
> 
> Again, replace fb_width*bytes_per_pixel with fb_pitch.
> 
> > +       reg_data->vidw_add1 = addr;
> > +
> > +       val_h = (plane->fb_width - plane->crtc_width) *
> > bytes_per_pixel;
> 
> val_h = plane->fb_pitch - (plane->crtc_width * bytes_per_pixel);
> 
> > +static void decon_win_commit(struct exynos_drm_crtc *crtc, int
> > zpos) +{
> > +       struct exynos_decon *decon = crtc->ctx;
> > +       struct decon_reg_data *reg_data;
> > +       unsigned int win = zpos;
> > +       u32 val;
> > +
> > +       if (win == DEFAULT_ZPOS)
> > +               win = 0;
> > +
> > +       if (win < 0 || win >= 5)
> > +               return;
> > +
> > +       reg_data = &decon->reg_data[win];
> > +
> > +       /* shadow update disable */
> > +       val = readl(decon->addr + DECON_SHADOWCON);
> > +       val |= SHADOWCON_Wx_PROTECT(win);
> > +       writel(val, decon->addr + DECON_SHADOWCON);
> > +
> > +       writel(reg_data->vidosd_a, decon->addr +
> > DECON_VIDOSDxA(win));
> > +       writel(reg_data->vidosd_b, decon->addr +
> > DECON_VIDOSDxB(win));
> > +       writel(reg_data->vidosd_c, decon->addr +
> > DECON_VIDOSDxC(win));
> > +       writel(reg_data->vidosd_d, decon->addr +
> > DECON_VIDOSDxD(win));
> > +       writel(reg_data->vidw_add0, decon->addr +
> > DECON_VIDW0xADD0B0(win));
> > +       writel(reg_data->vidw_add1, decon->addr +
> > DECON_VIDW0xADD1B0(win));
> > +       writel(reg_data->vidw_add2, decon->addr +
> > DECON_VIDW0xADD2(win)); +
> > +       /* window enable */
> > +       val = reg_data->wincon;
> > +       val |= WINCONx_ENWIN_F;
> > +       writel(val, decon->addr + DECON_WINCONx(win));
> > +
> > +       /* shadow update enable */
> > +       val = readl(decon->addr + DECON_SHADOWCON);
> > +       val &= ~SHADOWCON_Wx_PROTECT(win);
> > +       writel(val, decon->addr + DECON_SHADOWCON);
> > +
> > +       /* standalone update */
> > +       val = readl(decon->addr + DECON_UPDATE);
> > +       val |= STANDALONE_UPDATE_F;
> > +       writel(val, decon->addr + DECON_UPDATE);
> > +
> > +       if (decon->i80_if)
> > +               atomic_set(&decon->win_updated, 1);
> > +}
> > +
> > +static void decon_win_disable(struct exynos_drm_crtc *crtc, int
> > zpos) +{
> > +       struct exynos_decon *decon = crtc->ctx;
> > +       struct decon_reg_data *reg_data;
> > +       unsigned int win = zpos;
> > +       u32 val;
> > +
> > +       if (win == DEFAULT_ZPOS)
> > +               win = 0;
> > +
> > +       if (win < 0 || win >= 5)
> > +               return;
> > +
> > +       reg_data = &decon->reg_data[win];
> > +
> > +       /* shadow update disable */
> > +       val = readl(decon->addr + DECON_SHADOWCON);
> > +       val |= SHADOWCON_Wx_PROTECT(win);
> > +       writel(val, decon->addr + DECON_SHADOWCON);
> > +
> > +       /* window disable */
> > +       val = reg_data->wincon;
> > +       val &= ~WINCONx_ENWIN_F;
> > +       writel(val, decon->addr + DECON_WINCONx(win));
> > +
> > +       /* shadow update enable */
> > +       val = readl(decon->addr + DECON_SHADOWCON);
> > +       val &= ~SHADOWCON_Wx_PROTECT(win);
> > +       writel(val, decon->addr + DECON_SHADOWCON);
> > +
> > +       /* standalone update */
> > +       val = readl(decon->addr + DECON_UPDATE);
> > +       val |= STANDALONE_UPDATE_F;
> > +       writel(val, decon->addr + DECON_UPDATE);
> > +}
> > +
> > +void decon_te_irq_handler(struct exynos_drm_crtc *crtc)
> > +{
> > +       struct exynos_decon *decon = crtc->ctx;
> > +       u32 val;
> > +
> > +       if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
> > +               return;
> > +
> > +       if (atomic_add_unless(&decon->win_updated, -1, 0)) {
> > +               /* trigger */
> > +               val = readl(decon->addr + DECON_TRIGCON);
> > +               val |= TRIGCON_SWTRIGCMD;
> > +               writel(val, decon->addr + DECON_TRIGCON);
> > +       }
> > +}
> > +
> > +static struct exynos_drm_crtc_ops decon_crtc_ops = {
> > +       .dpms                   = decon_dpms,
> > +       .enable_vblank          = decon_enable_vblank,
> > +       .disable_vblank         = decon_disable_vblank,
> > +       .commit                 = decon_commit,
> > +       .win_mode_set           = decon_win_mode_set,
> > +       .win_commit             = decon_win_commit,
> > +       .win_disable            = decon_win_disable,
> > +       .te_handler             = decon_te_irq_handler,
> > +};
> > +
> > +static int decon_bind(struct device *dev, struct device *master,
> > void *data) +{
> > +       struct exynos_decon *decon = dev_get_drvdata(dev);
> > +       struct drm_device *drm_dev = data;
> > +       struct exynos_drm_private *priv = drm_dev->dev_private;
> > +
> > +       decon->drm_dev = drm_dev;
> > +       decon->pipe = priv->pipe++;
> > +
> > +       decon->crtc = exynos_drm_crtc_create(drm_dev, decon->pipe,
> > +                       EXYNOS_DISPLAY_TYPE_LCD, &decon_crtc_ops,
> > decon);
> > +       if (IS_ERR(decon->crtc))
> > +               return PTR_ERR(decon->crtc);
> 
> Should failing to register the CRTC also decrement priv->pipe?
> 

Yes. That's right.

> > +
> > +       return 0;
> > +}
> > +
> > +static void decon_unbind(struct device *dev, struct device
> > *master, void *data) +{
> > +       struct exynos_decon *decon = dev_get_drvdata(dev);
> > +
> > +       decon_dpms(decon->crtc, DRM_MODE_DPMS_OFF);
> > +}
> 
> As above, it seems like DPMS will not do enough here.
> 
> > +static const struct component_ops decon_component_ops = {
> > +       .bind   = decon_bind,
> > +       .unbind = decon_unbind,
> > +};
> > +
> > +static irqreturn_t decon_vsync_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct exynos_decon *decon = dev_id;
> > +       u32 val;
> > +
> > +       if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
> > +               goto out;
> > +
> > +       val = readl(decon->addr + DECON_VIDINTCON1);
> > +       if (val & VIDINTCON1_INTFRMPEND) {
> > +               if (decon->drm_dev)
> > +                       drm_handle_vblank(decon->drm_dev,
> > decon->pipe);
> 
> If decon->drm_dev is ever NULL, it sounds like something has gone very
> seriously wrong.

decon->drm_dev will not be NULL at any time. So I will remove the if
statement.

> 
> > +               /* clear */
> > +               writel(VIDINTCON1_INTFRMPEND, decon->addr +
> > DECON_VIDINTCON1);
> > +       }
> > +
> > +out:
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t decon_lcd_sys_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct exynos_decon *decon = dev_id;
> > +       u32 val;
> > +
> > +       if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))
> > +               goto out;
> > +
> > +       val = readl(decon->addr + DECON_VIDINTCON1);
> > +       if (val & VIDINTCON1_INTFRMDONEPEND) {
> > +               if (decon->drm_dev)
> > +
> > exynos_drm_crtc_finish_pageflip(decon->drm_dev,
> > +
> > decon->pipe);
> 
> Ditto - when would drm_dev be NULL?

Ditto.

> 
> > +               /* clear */
> > +               writel(VIDINTCON1_INTFRMDONEPEND,
> > +                               decon->addr + DECON_VIDINTCON1);
> > +       }
> > +
> > +out:
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int exynos5433_decon_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct exynos_decon *decon;
> > +       struct resource *res;
> > +       int ret;
> > +       int i;
> > +
> > +       decon = devm_kzalloc(dev, sizeof(*decon), GFP_KERNEL);
> > +       if (!decon)
> > +               return -ENOMEM;
> > +
> > +       decon->dev = dev;
> > +       if (of_get_child_by_name(dev->of_node, "i80-if-timings"))
> > +               decon->i80_if = true;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
> > +               struct clk *clk;
> > +
> > +               clk = devm_clk_get(decon->dev, decon_clks_name[i]);
> > +               if (IS_ERR(clk))
> > +                       return PTR_ERR(clk);
> > +
> > +               decon->clks[i] = clk;
> > +       }
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res) {
> > +               dev_err(dev, "cannot find IO resource\n");
> > +               return -ENXIO;
> > +       }
> > +
> > +       decon->addr = devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(decon->addr)) {
> > +               dev_err(dev, "ioremap failed\n");
> > +               return PTR_ERR(decon->addr);
> > +       }
> > +
> > +       res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> > +                       decon->i80_if ? "lcd_sys" : "vsync");
> > +       if (!res) {
> > +               dev_err(dev, "cannot find IRQ resource\n");
> > +               return -ENXIO;
> > +       }
> > +
> > +       ret = devm_request_irq(dev, res->start, decon->i80_if ?
> > +                       decon_lcd_sys_irq_handler :
> > decon_vsync_irq_handler, 0,
> > +                       "drm_decon", decon);
> > +       if (ret < 0) {
> > +               dev_err(dev, "lcd_sys irq request failed\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,
> > +                                      EXYNOS_DISPLAY_TYPE_LCD);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       platform_set_drvdata(pdev, decon);
> > +
> > +       ret = component_add(dev, &decon_component_ops);
> > +       if (ret < 0) {
> > +               exynos_drm_component_del(dev,
> > EXYNOS_DEVICE_TYPE_CRTC);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int exynos5433_decon_remove(struct platform_device *pdev)
> > +{
> > +       component_del(&pdev->dev, &decon_component_ops);
> > +       exynos_drm_component_del(&pdev->dev,
> > EXYNOS_DEVICE_TYPE_CRTC); +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id
> > exynos5433_decon_driver_dt_match[] = {
> > +       { .compatible = "samsung,exynos5433-decon" },
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos5433_decon_driver_dt_match);
> > +
> > +struct platform_driver exynos5433_decon_driver = {
> > +       .probe          = exynos5433_decon_probe,
> > +       .remove         = exynos5433_decon_remove,
> > +       .driver         = {
> > +               .name   = "exynos5433-decon",
> > +               .owner  = THIS_MODULE,
> > +               .of_match_table = exynos5433_decon_driver_dt_match,
> > +       },
> > +};
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 98a239a..1fa0dd0
> > 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -556,6 +556,9 @@ static struct platform_driver *const
> > exynos_drm_kms_drivers[] = { #ifdef CONFIG_DRM_EXYNOS_FIMD
> >         &fimd_driver,
> >  #endif
> > +#ifdef CONFIG_DRM_EXYNOS5433_DECON
> > +       &exynos5433_decon_driver,
> > +#endif
> >  #ifdef CONFIG_DRM_EXYNOS7_DECON
> >         &decon_driver,
> >  #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 9afd390..40996d8
> > 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > @@ -344,6 +344,7 @@ void exynos_drm_component_del(struct device
> > *dev, enum exynos_drm_device_type dev_type);
> >
> >  extern struct platform_driver fimd_driver;
> > +extern struct platform_driver exynos5433_decon_driver;
> >  extern struct platform_driver decon_driver;
> >  extern struct platform_driver dp_driver;
> >  extern struct platform_driver dsi_driver;
> > diff --git a/drivers/gpu/drm/exynos/regs-exynos5433-decon.h
> > b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h new file mode
> > 100644 index 0000000..9e7f851
> > --- /dev/null
> > +++ b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h
> > @@ -0,0 +1,163 @@
> > +/*
> > + * Copyright (C) 2014 Samsung Electronics Co.Ltd
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundationr
> > + */
> > +
> > +#ifndef EXYNOS_REGS_DECON_H
> > +#define EXYNOS_REGS_DECON_H
> > +
> > +/* Exynos543X DECON */
> > +#define DECON_VIDCON0                  0x0000
> > +#define DECON_VIDOUTCON0               0x0010
> > +#define DECON_WINCONx(n)               (0x0020 + ((n) * 4))
> > +#define DECON_VIDOSDxH(n)              (0x0080 + ((n) * 4))
> > +#define DECON_SHADOWCON                        0x00A0
> > +#define DECON_VIDOSDxA(n)              (0x00B0 + ((n) * 0x20))
> > +#define DECON_VIDOSDxB(n)              (0x00B4 + ((n) * 0x20))
> > +#define DECON_VIDOSDxC(n)              (0x00B8 + ((n) * 0x20))
> > +#define DECON_VIDOSDxD(n)              (0x00BC + ((n) * 0x20))
> > +#define DECON_VIDOSDxE(n)              (0x00C0 + ((n) * 0x20))
> > +#define DECON_VIDW0xADD0B0(n)          (0x0150 + ((n) * 0x10))
> > +#define DECON_VIDW0xADD0B1(n)          (0x0154 + ((n) * 0x10))
> > +#define DECON_VIDW0xADD0B2(n)          (0x0158 + ((n) * 0x10))
> > +#define DECON_VIDW0xADD1B0(n)          (0x01A0 + ((n) * 0x10))
> > +#define DECON_VIDW0xADD1B1(n)          (0x01A4 + ((n) * 0x10))
> > +#define DECON_VIDW0xADD1B2(n)          (0x01A8 + ((n) * 0x10))
> > +#define DECON_VIDW0xADD2(n)            (0x0200 + ((n) * 4))
> > +#define DECON_LOCALxSIZE(n)            (0x0214 + ((n) *
> > 4))change-booting-mode.sh --update +#define
> > DECON_VIDINTCON0               0x0220 +#define
> > DECON_VIDINTCON1               0x0224 +#define
> > DECON_WxKEYCON0(n)             (0x0230 + ((n - 1) * 8)) +#define
> > DECON_WxKEYCON1(n)             (0x0234 + ((n - 1) * 8)) +#define
> > DECON_WxKEYALPHA(n)            (0x0250 + ((n - 1) * 4)) +#define
> > DECON_WINxMAP(n)               (0x0270 + ((n) * 4)) +#define
> > DECON_QOSLUT07_00              0x02C0 +#define
> > DECON_QOSLUT15_08              0x02C4 +#define
> > DECON_QOSCTRL                  0x02C8 +#define
> > DECON_BLENDERQx(n)             (0x0300 + ((n - 1) * 4)) +#define
> > DECON_BLENDCON                 0x0310 +#define
> > DECON_OPE_VIDW0xADD0(n)                (0x0400 + ((n) * 4))
> > +#define DECON_OPE_VIDW0xADD1(n)                (0x0414 + ((n) *
> > 4)) +#define DECON_FRAMEFIFO_REG7           0x051C +#define
> > DECON_FRAMEFIFO_REG8           0x0520 +#define
> > DECON_FRAMEFIFO_STATUS         0x0524 +#define
> > DECON_CMU                      0x1404 +#define
> > DECON_UPDATE                   0x1410 +#define
> > DECON_UPDATE_SCHEME            0x1438 +#define
> > DECON_VIDCON1                  0x2000 +#define
> > DECON_VIDCON2                  0x2004 +#define
> > DECON_VIDCON3                  0x2008 +#define
> > DECON_VIDCON4                  0x200C +#define
> > DECON_VIDTCON2                 0x2028 +#define
> > DECON_FRAME_SIZE               0x2038 +#define
> > DECON_LINECNT_OP_THRESHOLD     0x203C +#define
> > DECON_TRIGCON                  0x2040 +#define
> > DECON_TRIGSKIP                 0x2050 +#define
> > DECON_CRCRDATA                 0x20B0 +#define
> > DECON_CRCCTRL                  0x20B4 +
> > +/* Exynos5430 DECON */
> > +#define DECON_VIDTCON0                 0x2020
> > +#define DECON_VIDTCON1                 0x2024
> > +
> > +/* Exynos5433 DECON */
> > +#define DECON_VIDTCON00                        0x2010
> > +#define DECON_VIDTCON01                        0x2014
> > +#define DECON_VIDTCON10                        0x2018
> > +#define DECON_VIDTCON11                        0x201C
> > +
> > +/* Exynos543X DECON Internal */
> > +#define DECON_W013DSTREOCON            0x0320
> > +#define DECON_W233DSTREOCON            0x0324
> > +#define DECON_FRAMEFIFO_REG0           0x0500
> > +#define DECON_ENHANCER_CTRL            0x2100
> > +
> > +/* Exynos543X DECON TV */
> > +#define DECON_VCLKCON0                 0x0014
> > +#define DECON_VIDINTCON2               0x0228
> > +#define DECON_VIDINTCON3               0x022C
> 
> Yes, my impression was that DECON supported both internal and
> external, i.e. would replace both FIMD/Mixer. This patch only supports
> the internal interface: are you planning to add support for the
> external/TV/HDMI interface as well? If so, how does this work with the
> DT format you have defined? Should there maybe separate decon-int and
> decon-tv subnodes, each of which create one CRTC? The Android tree
> seems to rely on static configuration, but presumably we would need a
> much more nuanced approach.
> 
> If so, it seems like the first start would be to add a basic DECON
> binding which covered the entire controller, and then as a next step
> add support for decon-int as a subdevice of this.
> 

As I know, they are physically different devices even though they have
similar names and similar operations. If it is right, I am not sure
that congregating them in one DT node is right.

I really appreciate your review. Thanks.

> Cheers,
> Daniel
> 

Best regards,
Hyungwon Hwang
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/6] of: add helper for getting endpoint node of specific identifiers

[Hyungwon Hwang]

When there are multiple ports or multiple endpoints in a port, they have to be
distinguished by the value of reg property. It is common. The drivers can get
the specific endpoint in the specific port via this function. Now the drivers
have to implement this code in themselves or have to force the order of dt nodes
to get the right node.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
Acked-by: Rob Herring <robh+dt@kernel.org>
---
 drivers/of/base.c        | 33 +++++++++++++++++++++++++++++++++
 include/linux/of_graph.h |  8 ++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 36536b6..d7fa99d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2155,6 +2155,39 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 EXPORT_SYMBOL(of_graph_get_next_endpoint);
 
 /**
+ * of_graph_get_endpoint_by_regs() - get endpoint node of specific identifiers
+ * @parent: pointer to the parent device node
+ * @port_reg: identifier (value of reg property) of the parent port node
+ * @reg: identifier (value of reg property) of the endpoint node
+ *
+ * Return: An 'endpoint' node pointer which is identified by reg and at the same
+ * is the child of a port node identified by port_reg. reg and port_reg are
+ * ignored when they are -1.
+ */
+struct device_node *of_graph_get_endpoint_by_regs(
+	const struct device_node *parent, int port_reg, int reg)
+{
+	struct of_endpoint endpoint;
+	struct device_node *node, *prev_node = NULL;
+
+	while (1) {
+		node = of_graph_get_next_endpoint(parent, prev_node);
+		of_node_put(prev_node);
+		if (!node)
+			break;
+
+		of_graph_parse_endpoint(node, &endpoint);
+		if (((port_reg == -1) || (endpoint.port == port_reg)) &&
+			((reg == -1) || (endpoint.id == reg)))
+			return node;
+
+		prev_node = node;
+	}
+
+	return NULL;
+}
+
+/**
  * of_graph_get_remote_port_parent() - get remote port's parent node
  * @node: pointer to a local endpoint device_node
  *
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index befef42..e859eb7 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -31,6 +31,8 @@ int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *previous);
+struct device_node *of_graph_get_endpoint_by_regs(
+		const struct device_node *parent, int port_reg, int reg);
 struct device_node *of_graph_get_remote_port_parent(
 					const struct device_node *node);
 struct device_node *of_graph_get_remote_port(const struct device_node *node);
@@ -49,6 +51,12 @@ static inline struct device_node *of_graph_get_next_endpoint(
 	return NULL;
 }
 
+struct device_node *of_graph_get_endpoint_by_regs(
+		const struct device_node *parent, int port_reg, int reg)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_remote_port_parent(
 					const struct device_node *node)
 {
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/6] drm/exynos: mic: add MIC driver

[Hyungwon Hwang]

MIC(Mobile image compressor) is newly added IP in Exynos5433. MIC
resides between decon and mipi dsim, and compresses frame data by 50%.
With dsi, not display port, to send frame data to the panel, the
bandwidth is not enough. That is why this compressor is introduced.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
Changes for v2:
- make mic driver to be registered by exynos drm driver instead as a module                                      |      -------------------------------------------------------------------------------------------------------------
driver                                                                                                           |      -------------------------------------------------------------------------------------------------------------
- change the description of mic driver in documentation
- add module author at the top of the source file removing MODULE_OWNER,
MODULE_DESCRIPTION, MODULE_LICENSE
 .../devicetree/bindings/video/exynos-mic.txt       |  49 +++
 drivers/gpu/drm/exynos/Kconfig                     |   6 +
 drivers/gpu/drm/exynos/Makefile                    |   1 +
 drivers/gpu/drm/exynos/exynos_drm_drv.c            |   3 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h            |   1 +
 drivers/gpu/drm/exynos/exynos_drm_mic.c            | 481 +++++++++++++++++++++
 6 files changed, 541 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/exynos-mic.txt
 create mode 100644 drivers/gpu/drm/exynos/exynos_drm_mic.c

diff --git a/Documentation/devicetree/bindings/video/exynos-mic.txt b/Documentation/devicetree/bindings/video/exynos-mic.txt
new file mode 100644
index 0000000..006d072
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/exynos-mic.txt
@@ -0,0 +1,49 @@
+Device-Tree bindings for Samsung Exynos SoC mobile image compressor (MIC)
+
+MIC (mobile image compressor) resides between decon and mipi dsi. Mipi dsi is
+not capable to transfer high resoltuion frame data as decon can send. MIC
+solves this problem by compressing the frame data by 1/2 before it is transfered
+through mipi dsi. The compressed frame data must be uncompressed in the panel
+PCB.
+
+Required properties:
+- compatible: value should be "samsung,exynos5433-mic".
+- reg: physical base address and length of the MIC registers set and system
+       register of mic.
+- clocks: must include clock specifiers corresponding to entries in the
+	  clock-names property.
+- clock-names: list of clock names sorted in the same order as the clocks
+	       property. Must contain "pclk_mic0", "sclk_rgb_vclk_to_mic0".
+- ports: contains a port which is connected to decon node and dsi node.
+	 address-cells and size-cells must 1 and 0, respectively.
+- port: contains an endpoint node which is connected to the endpoint in the
+	decon node or dsi node. The reg value must be 0 and 1 respectively.
+
+Example:
+SoC specific DT entry:
+mic: mic@13930000 {
+	compatible = "samsung,exynos5433-mic";
+	reg = <0x13930000 0x48 0x13B80000 0x1010>;
+	clocks = <&cmu_disp CLK_PCLK_MIC0>,
+	       <&cmu_disp CLK_SCLK_RGB_VCLK_TO_MIC0>;
+	clock-names = "pclk_mic0", "sclk_rgb_vclk_to_mic0";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			mic_to_decon: endpoint {
+				remote-endpoint = <&decon_to_mic>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			mic_to_dsi: endpoint {
+				remote-endpoint = <&dsi_to_mic>;
+			};
+		};
+	};
+};
diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index e15cc2e..a796175 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -103,3 +103,9 @@ config DRM_EXYNOS_GSC
 	depends on DRM_EXYNOS_IPP && ARCH_EXYNOS5 && !ARCH_MULTIPLATFORM
 	help
 	  Choose this option if you want to use Exynos GSC for DRM.
+
+config DRM_EXYNOS_MIC
+	bool "Exynos DRM MIC"
+	depends on (DRM_EXYNOS && DRM_EXYNOS5433_DECON)
+	help
+	  Choose this option if you want to use Exynos MIC for DRM.
diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
index fbd084d..7de0b10 100644
--- a/drivers/gpu/drm/exynos/Makefile
+++ b/drivers/gpu/drm/exynos/Makefile
@@ -22,5 +22,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_IPP)	+= exynos_drm_ipp.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_FIMC)	+= exynos_drm_fimc.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR)	+= exynos_drm_rotator.o
 exynosdrm-$(CONFIG_DRM_EXYNOS_GSC)	+= exynos_drm_gsc.o
+exynosdrm-$(CONFIG_DRM_EXYNOS_MIC)     += exynos_drm_mic.o

 obj-$(CONFIG_DRM_EXYNOS)		+= exynosdrm.o
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 1fa0dd0..ec9984d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -562,6 +562,9 @@ static struct platform_driver *const exynos_drm_kms_drivers[] = {
 #ifdef CONFIG_DRM_EXYNOS7_DECON
 	&decon_driver,
 #endif
+#ifdef CONFIG_DRM_EXYNOS_MIC
+	&mic_driver,
+#endif
 #ifdef CONFIG_DRM_EXYNOS_DP
 	&dp_driver,
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 40996d8..9c94dc9 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -357,4 +357,5 @@ extern struct platform_driver fimc_driver;
 extern struct platform_driver rotator_driver;
 extern struct platform_driver gsc_driver;
 extern struct platform_driver ipp_driver;
+extern struct platform_driver mic_driver;
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
new file mode 100644
index 0000000..b898a2a
--- /dev/null
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -0,0 +1,481 @@
+/*
+ * Copyright (C) 2015 Samsung Electronics Co.Ltd
+ * Authors:
+ *	Hyungwon Hwang <human.hwang@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundationr
+ */
+
+#include <linux/platform_device.h>
+#include <video/of_videomode.h>
+#include <linux/of_address.h>
+#include <video/videomode.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/clk.h>
+#include <drm/drmP.h>
+
+/* Sysreg registers for MIC */
+#define DSD_CFG_MUX	0x1004
+#define MIC0_RGB_MUX	(1 << 0)
+#define MIC0_I80_MUX	(1 << 1)
+#define MIC0_ON_MUX	(1 << 5)
+
+/* MIC registers */
+#define MIC_OP				0x0
+#define MIC_IP_VER			0x0004
+#define MIC_V_TIMING_0			0x0008
+#define MIC_V_TIMING_1			0x000C
+#define MIC_IMG_SIZE			0x0010
+#define MIC_INPUT_TIMING_0		0x0014
+#define MIC_INPUT_TIMING_1		0x0018
+#define MIC_2D_OUTPUT_TIMING_0		0x001C
+#define MIC_2D_OUTPUT_TIMING_1		0x0020
+#define MIC_2D_OUTPUT_TIMING_2		0x0024
+#define MIC_3D_OUTPUT_TIMING_0		0x0028
+#define MIC_3D_OUTPUT_TIMING_1		0x002C
+#define MIC_3D_OUTPUT_TIMING_2		0x0030
+#define MIC_CORE_PARA_0			0x0034
+#define MIC_CORE_PARA_1			0x0038
+#define MIC_CTC_CTRL			0x0040
+#define MIC_RD_DATA			0x0044
+
+#define MIC_UPD_REG			(1 << 31)
+#define MIC_ON_REG			(1 << 30)
+#define MIC_TD_ON_REG			(1 << 29)
+#define MIC_BS_CHG_OUT			(1 << 16)
+#define MIC_VIDEO_TYPE(x)		(((x) & 0xf) << 12)
+#define MIC_PSR_EN			(1 << 5)
+#define MIC_SW_RST			(1 << 4)
+#define MIC_ALL_RST			(1 << 3)
+#define MIC_CORE_VER_CONTROL		(1 << 2)
+#define MIC_MODE_SEL_COMMAND_MODE	(1 << 1)
+#define MIC_MODE_SEL_MASK		(1 << 1)
+#define MIC_CORE_EN			(1 << 0)
+
+#define MIC_V_PULSE_WIDTH(x)		(((x) & 0x3fff) << 16)
+#define MIC_V_PERIOD_LINE(x)		((x) & 0x3fff)
+
+#define MIC_VBP_SIZE(x)			(((x) & 0x3fff) << 16)
+#define MIC_VFP_SIZE(x)			((x) & 0x3fff)
+
+#define MIC_IMG_V_SIZE(x)		(((x) & 0x3fff) << 16)
+#define MIC_IMG_H_SIZE(x)		((x) & 0x3fff)
+
+#define MIC_H_PULSE_WIDTH_IN(x)		(((x) & 0x3fff) << 16)
+#define MIC_H_PERIOD_PIXEL_IN(x)	((x) & 0x3fff)
+
+#define MIC_HBP_SIZE_IN(x)		(((x) & 0x3fff) << 16)
+#define MIC_HFP_SIZE_IN(x)		((x) & 0x3fff)
+
+#define MIC_H_PULSE_WIDTH_2D(x)		(((x) & 0x3fff) << 16)
+#define MIC_H_PERIOD_PIXEL_2D(x)	((x) & 0x3fff)
+
+#define MIC_HBP_SIZE_2D(x)		(((x) & 0x3fff) << 16)
+#define MIC_HFP_SIZE_2D(x)		((x) & 0x3fff)
+
+#define MIC_BS_SIZE_2D(x)	((x) & 0x3fff)
+
+enum {
+	ENDPOINT_DECON_NODE,
+	ENDPOINT_DSI_NODE,
+	NUM_ENDPOINTS
+};
+
+static char *clk_names[] = { "pclk_mic0", "sclk_rgb_vclk_to_mic0" };
+#define NUM_CLKS		ARRAY_SIZE(clk_names)
+static DEFINE_MUTEX(mic_mutex);
+
+struct exynos_mic {
+	struct device *dev;
+	void __iomem *reg;
+	void __iomem *sysreg;
+	struct clk *clks[NUM_CLKS];
+
+	bool i80_mode;
+	struct videomode vm;
+	struct drm_encoder *encoder;
+	struct drm_bridge bridge;
+
+	bool enabled;
+};
+
+static void mic_set_path(struct exynos_mic *mic, bool enable)
+{
+	u32 reg;
+
+	reg = readl(mic->sysreg + DSD_CFG_MUX);
+	if (enable)
+		reg |= MIC0_RGB_MUX | MIC0_I80_MUX | MIC0_ON_MUX;
+	else
+		reg &= ~(MIC0_RGB_MUX | MIC0_I80_MUX | MIC0_ON_MUX);
+
+	writel(reg, mic->sysreg + DSD_CFG_MUX);
+}
+
+static int mic_sw_reset(struct exynos_mic *mic)
+{
+	unsigned int retry = 100;
+	int ret;
+
+	writel(MIC_SW_RST, mic->reg + MIC_OP);
+
+	while (retry-- > 0) {
+		ret = readl(mic->reg + MIC_OP);
+		if (!(ret & MIC_SW_RST))
+			return 0;
+
+		udelay(10);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static void mic_set_porch_timing(struct exynos_mic *mic)
+{
+	struct videomode vm;
+	u32 reg;
+
+	if (!mic->i80_mode) {
+		vm = mic->vm;
+
+		reg = MIC_V_PULSE_WIDTH(vm.vsync_len) +
+			MIC_V_PERIOD_LINE(vm.vsync_len + vm.vactive +
+					vm.vback_porch + vm.vfront_porch);
+		writel(reg, mic->reg + MIC_V_TIMING_0);
+
+		reg = MIC_VBP_SIZE(vm.vback_porch) +
+						MIC_VFP_SIZE(vm.vfront_porch);
+		writel(reg, mic->reg + MIC_V_TIMING_1);
+
+		reg = MIC_V_PULSE_WIDTH(vm.hsync_len) +
+			MIC_V_PERIOD_LINE(vm.hsync_len + vm.hactive +
+					vm.hback_porch + vm.hfront_porch);
+		writel(reg, mic->reg + MIC_INPUT_TIMING_0);
+
+		reg = MIC_VBP_SIZE(vm.hback_porch) +
+						MIC_VFP_SIZE(vm.hfront_porch);
+		writel(reg, mic->reg + MIC_INPUT_TIMING_1);
+	}
+}
+
+static void mic_set_img_size(struct exynos_mic *mic)
+{
+	struct videomode *vm = &mic->vm;
+	u32 reg;
+
+	reg = MIC_IMG_H_SIZE(vm->hactive) +
+		MIC_IMG_V_SIZE(vm->vactive);
+
+	writel(reg, mic->reg + MIC_IMG_SIZE);
+}
+
+static void mic_set_output_timing(struct exynos_mic *mic)
+{
+	struct videomode vm = mic->vm;
+	u32 reg, bs_size_2d;
+
+	DRM_DEBUG("w: %u, h: %u\n", vm.hactive, vm.vactive);
+	bs_size_2d = ((vm.hactive >> 2) << 1) + (vm.vactive % 4);
+	reg = MIC_BS_SIZE_2D(bs_size_2d);
+	writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_2);
+
+	if (!mic->i80_mode) {
+		reg = MIC_H_PULSE_WIDTH_2D(vm.hsync_len) +
+			MIC_H_PERIOD_PIXEL_2D(vm.hsync_len + bs_size_2d +
+					vm.hback_porch + vm.hfront_porch);
+		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_0);
+
+		reg = MIC_HBP_SIZE_2D(vm.hback_porch) +
+			MIC_H_PERIOD_PIXEL_2D(vm.hfront_porch);
+		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_1);
+	}
+}
+
+static void mic_set_reg_on(struct exynos_mic *mic, bool enable)
+{
+	u32 reg = readl(mic->reg + MIC_OP);
+
+	if (enable) {
+		reg &= ~(MIC_MODE_SEL_MASK | MIC_CORE_VER_CONTROL | MIC_PSR_EN);
+		reg |= (MIC_CORE_EN | MIC_BS_CHG_OUT | MIC_ON_REG);
+
+		if (mic->i80_mode)
+			reg |= MIC_MODE_SEL_COMMAND_MODE;
+	} else {
+		reg &= ~MIC_CORE_EN;
+	}
+
+	reg |= MIC_UPD_REG;
+	writel(reg, mic->reg + MIC_OP);
+}
+
+static struct device_node *get_remote_node(struct device_node *from, int reg)
+{
+	struct device_node *endpoint = NULL, *remote_node = NULL;
+
+	endpoint = of_graph_get_endpoint_by_regs(from, reg, -1);
+	if (!endpoint) {
+		DRM_ERROR("mic: Failed to find remote port from %s",
+				from->full_name);
+		goto exit;
+	}
+
+	remote_node = of_graph_get_remote_port_parent(endpoint);
+	if (!remote_node) {
+		DRM_ERROR("mic: Failed to find remote port parent from %s",
+							from->full_name);
+		goto exit;
+	}
+
+exit:
+	of_node_put(endpoint);
+	return remote_node;
+}
+
+static int parse_dt(struct exynos_mic *mic)
+{
+	int ret = 0, i, j;
+	struct device_node *remote_node;
+	struct device_node *nodes[3];
+
+	/*
+	 * The order of endpoints does matter.
+	 * The first node must be for decon and the second one must be for dsi.
+	 */
+	for (i = 0, j = 0; i < NUM_ENDPOINTS; i++) {
+		remote_node = get_remote_node(mic->dev->of_node, i);
+		if (!remote_node) {
+			ret = -EPIPE;
+			goto exit;
+		}
+		nodes[j++] = remote_node;
+
+		switch (i) {
+		case ENDPOINT_DECON_NODE:
+			/* decon node */
+			if (of_get_child_by_name(remote_node,
+						"i80-if-timings"))
+				mic->i80_mode = 1;
+
+			break;
+		case ENDPOINT_DSI_NODE:
+			/* panel node */
+			remote_node = get_remote_node(remote_node, 1);
+			if (!remote_node) {
+				ret = -EPIPE;
+				goto exit;
+			}
+			nodes[j++] = remote_node;
+
+			ret = of_get_videomode(remote_node,
+							&mic->vm, 0);
+			if (ret) {
+				DRM_ERROR("mic: failed to get videomode");
+				goto exit;
+			}
+
+			break;
+		default:
+			DRM_ERROR("mic: Unknown endpoint from MIC");
+			break;
+		}
+	}
+
+exit:
+	while (--j > -1)
+		of_node_put(nodes[j]);
+
+	return ret;
+}
+
+void mic_disable(struct drm_bridge *bridge) { }
+
+void mic_post_disable(struct drm_bridge *bridge)
+{
+	struct exynos_mic *mic = bridge->driver_private;
+	int i;
+
+	mutex_lock(&mic_mutex);
+	if (!mic->enabled)
+		goto already_disabled;
+
+	mic_set_path(mic, 0);
+
+	for (i = NUM_CLKS - 1; i > -1; i--)
+		clk_disable_unprepare(mic->clks[i]);
+
+	mic->enabled = 0;
+
+already_disabled:
+	mutex_unlock(&mic_mutex);
+}
+
+void mic_pre_enable(struct drm_bridge *bridge)
+{
+	struct exynos_mic *mic = bridge->driver_private;
+	int ret, i;
+
+	mutex_lock(&mic_mutex);
+	if (mic->enabled)
+		goto already_enabled;
+
+	for (i = 0; i < NUM_CLKS; i++) {
+		ret = clk_prepare_enable(mic->clks[i]);
+		if (ret < 0) {
+			DRM_ERROR("Failed to enable clock (%s)\n",
+							clk_names[i]);
+			goto turn_off_clks;
+		}
+	}
+
+	mic_set_path(mic, 1);
+
+	ret = mic_sw_reset(mic);
+	if (ret) {
+		DRM_ERROR("Failed to reset\n");
+		goto turn_off_clks;
+	}
+
+	mic_set_porch_timing(mic);
+	mic_set_img_size(mic);
+	mic_set_output_timing(mic);
+	mic_set_reg_on(mic, 1);
+	mic->enabled = 1;
+	mutex_unlock(&mic_mutex);
+
+	return;
+
+turn_off_clks:
+	while (--i > -1)
+		clk_disable_unprepare(mic->clks[i]);
+already_enabled:
+	mutex_unlock(&mic_mutex);
+}
+
+void mic_enable(struct drm_bridge *bridge) { }
+
+void mic_destroy(struct drm_bridge *bridge)
+{
+	struct exynos_mic *mic = bridge->driver_private;
+	int i;
+
+	mutex_lock(&mic_mutex);
+	if (!mic->enabled)
+		goto already_disabled;
+
+	for (i = NUM_CLKS - 1; i > -1; i--)
+		clk_disable_unprepare(mic->clks[i]);
+
+already_disabled:
+	mutex_unlock(&mic_mutex);
+}
+
+struct drm_bridge_funcs mic_bridge_funcs = {
+	.disable = mic_disable,
+	.post_disable = mic_post_disable,
+	.pre_enable = mic_pre_enable,
+	.enable = mic_enable,
+};
+
+int exynos_mic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct exynos_mic *mic;
+	struct resource res;
+	int ret, i;
+
+	mic = devm_kzalloc(dev, sizeof(*mic), GFP_KERNEL);
+	if (!mic) {
+		DRM_ERROR("mic: Failed to allocate memory for MIC object\n");
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	mic->dev = dev;
+
+	ret = parse_dt(mic);
+	if (ret)
+		goto err;
+
+	ret = of_address_to_resource(dev->of_node, 0, &res);
+	if (ret) {
+		DRM_ERROR("mic: Failed to get mem region for MIC\n");
+		goto err;
+	}
+	mic->reg = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!mic->reg) {
+		DRM_ERROR("mic: Failed to remap for MIC\n");
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	ret = of_address_to_resource(dev->of_node, 1, &res);
+	if (ret) {
+		DRM_ERROR("mic: Failed to get mem region for MIC\n");
+		goto err;
+	}
+	mic->sysreg = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!mic->sysreg) {
+		DRM_ERROR("mic: Failed to remap for MIC\n");
+		goto err;
+	}
+
+	mic->bridge.funcs = &mic_bridge_funcs;
+	mic->bridge.of_node = dev->of_node;
+	mic->bridge.driver_private = mic;
+	ret = drm_bridge_add(&mic->bridge);
+	if (ret) {
+		DRM_ERROR("mic: Failed to add MIC to the global bridge list\n");
+		goto err;
+	}
+
+	for (i = 0; i < NUM_CLKS; i++) {
+		mic->clks[i] = of_clk_get_by_name(dev->of_node, clk_names[i]);
+		if (IS_ERR(mic->clks[i])) {
+			DRM_ERROR("mic: Failed to get clock (%s)\n",
+								clk_names[i]);
+			ret = PTR_ERR(mic->clks[i]);
+			goto err;
+		}
+	}
+
+	DRM_INFO("MIC has been probed\n");
+
+err:
+	return ret;
+}
+
+static int exynos_mic_remove(struct platform_device *pdev)
+{
+	struct exynos_mic *mic = platform_get_drvdata(pdev);
+	int i;
+
+	drm_bridge_remove(&mic->bridge);
+
+	for (i = NUM_CLKS - 1; i > -1; i--)
+		clk_put(mic->clks[i]);
+
+	return 0;
+}
+
+static const struct of_device_id exynos_mic_of_match[] = {
+	{ .compatible = "samsung,exynos5433-mic" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, exynos_mic_of_match);
+
+struct platform_driver mic_driver = {
+	.probe		= exynos_mic_probe,
+	.remove		= exynos_mic_remove,
+	.driver		= {
+		.name	= "exynos-mic",
+		.owner	= THIS_MODULE,
+		.of_match_table = exynos_mic_of_match,
+	},
+};
--
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/exynos: mic: add MIC driver

[Inki Dae]

On 2015년 03월 18일 17:16, Hyungwon Hwang wrote:
> MIC(Mobile image compressor) is newly added IP in Exynos5433. MIC
> resides between decon and mipi dsim, and compresses frame data by 50%.
> With dsi, not display port, to send frame data to the panel, the
> bandwidth is not enough. That is why this compressor is introduced.
> 
> Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
> ---
> Changes for v2:
> - make mic driver to be registered by exynos drm driver instead as a module                                      |      -------------------------------------------------------------------------------------------------------------
> driver                                                                                                           |      -------------------------------------------------------------------------------------------------------------
> - change the description of mic driver in documentation
> - add module author at the top of the source file removing MODULE_OWNER,
> MODULE_DESCRIPTION, MODULE_LICENSE
>  .../devicetree/bindings/video/exynos-mic.txt       |  49 +++
>  drivers/gpu/drm/exynos/Kconfig                     |   6 +
>  drivers/gpu/drm/exynos/Makefile                    |   1 +
>  drivers/gpu/drm/exynos/exynos_drm_drv.c            |   3 +
>  drivers/gpu/drm/exynos/exynos_drm_drv.h            |   1 +
>  drivers/gpu/drm/exynos/exynos_drm_mic.c            | 481 +++++++++++++++++++++
>  6 files changed, 541 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/exynos-mic.txt
>  create mode 100644 drivers/gpu/drm/exynos/exynos_drm_mic.c
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos-mic.txt b/Documentation/devicetree/bindings/video/exynos-mic.txt
> new file mode 100644
> index 0000000..006d072
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/exynos-mic.txt
> @@ -0,0 +1,49 @@
> +Device-Tree bindings for Samsung Exynos SoC mobile image compressor (MIC)
> +
> +MIC (mobile image compressor) resides between decon and mipi dsi. Mipi dsi is
> +not capable to transfer high resoltuion frame data as decon can send. MIC
> +solves this problem by compressing the frame data by 1/2 before it is transfered
> +through mipi dsi. The compressed frame data must be uncompressed in the panel
> +PCB.
> +
> +Required properties:
> +- compatible: value should be "samsung,exynos5433-mic".
> +- reg: physical base address and length of the MIC registers set and system
> +       register of mic.
> +- clocks: must include clock specifiers corresponding to entries in the
> +	  clock-names property.
> +- clock-names: list of clock names sorted in the same order as the clocks
> +	       property. Must contain "pclk_mic0", "sclk_rgb_vclk_to_mic0".
> +- ports: contains a port which is connected to decon node and dsi node.
> +	 address-cells and size-cells must 1 and 0, respectively.
> +- port: contains an endpoint node which is connected to the endpoint in the
> +	decon node or dsi node. The reg value must be 0 and 1 respectively.
> +
> +Example:
> +SoC specific DT entry:
> +mic: mic@13930000 {
> +	compatible = "samsung,exynos5433-mic";
> +	reg = <0x13930000 0x48 0x13B80000 0x1010>;
> +	clocks = <&cmu_disp CLK_PCLK_MIC0>,
> +	       <&cmu_disp CLK_SCLK_RGB_VCLK_TO_MIC0>;
> +	clock-names = "pclk_mic0", "sclk_rgb_vclk_to_mic0";
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +			mic_to_decon: endpoint {
> +				remote-endpoint = <&decon_to_mic>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +			mic_to_dsi: endpoint {
> +				remote-endpoint = <&dsi_to_mic>;
> +			};
> +		};
> +	};
> +};
> diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
> index e15cc2e..a796175 100644
> --- a/drivers/gpu/drm/exynos/Kconfig
> +++ b/drivers/gpu/drm/exynos/Kconfig
> @@ -103,3 +103,9 @@ config DRM_EXYNOS_GSC
>  	depends on DRM_EXYNOS_IPP && ARCH_EXYNOS5 && !ARCH_MULTIPLATFORM
>  	help
>  	  Choose this option if you want to use Exynos GSC for DRM.
> +
> +config DRM_EXYNOS_MIC
> +	bool "Exynos DRM MIC"
> +	depends on (DRM_EXYNOS && DRM_EXYNOS5433_DECON)
> +	help
> +	  Choose this option if you want to use Exynos MIC for DRM.
> diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
> index fbd084d..7de0b10 100644
> --- a/drivers/gpu/drm/exynos/Makefile
> +++ b/drivers/gpu/drm/exynos/Makefile
> @@ -22,5 +22,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_IPP)	+= exynos_drm_ipp.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_FIMC)	+= exynos_drm_fimc.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR)	+= exynos_drm_rotator.o
>  exynosdrm-$(CONFIG_DRM_EXYNOS_GSC)	+= exynos_drm_gsc.o
> +exynosdrm-$(CONFIG_DRM_EXYNOS_MIC)     += exynos_drm_mic.o
> 
>  obj-$(CONFIG_DRM_EXYNOS)		+= exynosdrm.o
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 1fa0dd0..ec9984d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -562,6 +562,9 @@ static struct platform_driver *const exynos_drm_kms_drivers[] = {
>  #ifdef CONFIG_DRM_EXYNOS7_DECON
>  	&decon_driver,
>  #endif
> +#ifdef CONFIG_DRM_EXYNOS_MIC
> +	&mic_driver,
> +#endif
>  #ifdef CONFIG_DRM_EXYNOS_DP
>  	&dp_driver,
>  #endif
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 40996d8..9c94dc9 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -357,4 +357,5 @@ extern struct platform_driver fimc_driver;
>  extern struct platform_driver rotator_driver;
>  extern struct platform_driver gsc_driver;
>  extern struct platform_driver ipp_driver;
> +extern struct platform_driver mic_driver;
>  #endif
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
> new file mode 100644
> index 0000000..b898a2a
> --- /dev/null
> +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
> @@ -0,0 +1,481 @@
> +/*
> + * Copyright (C) 2015 Samsung Electronics Co.Ltd
> + * Authors:
> + *	Hyungwon Hwang <human.hwang@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundationr
> + */
> +
> +#include <linux/platform_device.h>
> +#include <video/of_videomode.h>
> +#include <linux/of_address.h>
> +#include <video/videomode.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/clk.h>
> +#include <drm/drmP.h>
> +
> +/* Sysreg registers for MIC */
> +#define DSD_CFG_MUX	0x1004
> +#define MIC0_RGB_MUX	(1 << 0)
> +#define MIC0_I80_MUX	(1 << 1)
> +#define MIC0_ON_MUX	(1 << 5)
> +
> +/* MIC registers */
> +#define MIC_OP				0x0
> +#define MIC_IP_VER			0x0004
> +#define MIC_V_TIMING_0			0x0008
> +#define MIC_V_TIMING_1			0x000C
> +#define MIC_IMG_SIZE			0x0010
> +#define MIC_INPUT_TIMING_0		0x0014
> +#define MIC_INPUT_TIMING_1		0x0018
> +#define MIC_2D_OUTPUT_TIMING_0		0x001C
> +#define MIC_2D_OUTPUT_TIMING_1		0x0020
> +#define MIC_2D_OUTPUT_TIMING_2		0x0024
> +#define MIC_3D_OUTPUT_TIMING_0		0x0028
> +#define MIC_3D_OUTPUT_TIMING_1		0x002C
> +#define MIC_3D_OUTPUT_TIMING_2		0x0030
> +#define MIC_CORE_PARA_0			0x0034
> +#define MIC_CORE_PARA_1			0x0038
> +#define MIC_CTC_CTRL			0x0040
> +#define MIC_RD_DATA			0x0044
> +
> +#define MIC_UPD_REG			(1 << 31)
> +#define MIC_ON_REG			(1 << 30)
> +#define MIC_TD_ON_REG			(1 << 29)
> +#define MIC_BS_CHG_OUT			(1 << 16)
> +#define MIC_VIDEO_TYPE(x)		(((x) & 0xf) << 12)
> +#define MIC_PSR_EN			(1 << 5)
> +#define MIC_SW_RST			(1 << 4)
> +#define MIC_ALL_RST			(1 << 3)
> +#define MIC_CORE_VER_CONTROL		(1 << 2)
> +#define MIC_MODE_SEL_COMMAND_MODE	(1 << 1)
> +#define MIC_MODE_SEL_MASK		(1 << 1)
> +#define MIC_CORE_EN			(1 << 0)
> +
> +#define MIC_V_PULSE_WIDTH(x)		(((x) & 0x3fff) << 16)
> +#define MIC_V_PERIOD_LINE(x)		((x) & 0x3fff)
> +
> +#define MIC_VBP_SIZE(x)			(((x) & 0x3fff) << 16)
> +#define MIC_VFP_SIZE(x)			((x) & 0x3fff)
> +
> +#define MIC_IMG_V_SIZE(x)		(((x) & 0x3fff) << 16)
> +#define MIC_IMG_H_SIZE(x)		((x) & 0x3fff)
> +
> +#define MIC_H_PULSE_WIDTH_IN(x)		(((x) & 0x3fff) << 16)
> +#define MIC_H_PERIOD_PIXEL_IN(x)	((x) & 0x3fff)
> +
> +#define MIC_HBP_SIZE_IN(x)		(((x) & 0x3fff) << 16)
> +#define MIC_HFP_SIZE_IN(x)		((x) & 0x3fff)
> +
> +#define MIC_H_PULSE_WIDTH_2D(x)		(((x) & 0x3fff) << 16)
> +#define MIC_H_PERIOD_PIXEL_2D(x)	((x) & 0x3fff)
> +
> +#define MIC_HBP_SIZE_2D(x)		(((x) & 0x3fff) << 16)
> +#define MIC_HFP_SIZE_2D(x)		((x) & 0x3fff)
> +
> +#define MIC_BS_SIZE_2D(x)	((x) & 0x3fff)
> +
> +enum {
> +	ENDPOINT_DECON_NODE,
> +	ENDPOINT_DSI_NODE,
> +	NUM_ENDPOINTS
> +};
> +
> +static char *clk_names[] = { "pclk_mic0", "sclk_rgb_vclk_to_mic0" };
> +#define NUM_CLKS		ARRAY_SIZE(clk_names)
> +static DEFINE_MUTEX(mic_mutex);
> +
> +struct exynos_mic {
> +	struct device *dev;
> +	void __iomem *reg;
> +	void __iomem *sysreg;
> +	struct clk *clks[NUM_CLKS];
> +
> +	bool i80_mode;
> +	struct videomode vm;
> +	struct drm_encoder *encoder;
> +	struct drm_bridge bridge;
> +
> +	bool enabled;
> +};
> +
> +static void mic_set_path(struct exynos_mic *mic, bool enable)
> +{
> +	u32 reg;
> +
> +	reg = readl(mic->sysreg + DSD_CFG_MUX);
> +	if (enable)
> +		reg |= MIC0_RGB_MUX | MIC0_I80_MUX | MIC0_ON_MUX;
> +	else
> +		reg &= ~(MIC0_RGB_MUX | MIC0_I80_MUX | MIC0_ON_MUX);

exynos_mic has already which video mode - RGB or i80 - is used so it'd
be reasonable to select one of them to avoid confusing.

> +
> +	writel(reg, mic->sysreg + DSD_CFG_MUX);
> +}
> +
> +static int mic_sw_reset(struct exynos_mic *mic)
> +{
> +	unsigned int retry = 100;
> +	int ret;
> +
> +	writel(MIC_SW_RST, mic->reg + MIC_OP);
> +
> +	while (retry-- > 0) {
> +		ret = readl(mic->reg + MIC_OP);
> +		if (!(ret & MIC_SW_RST))
> +			return 0;
> +
> +		udelay(10);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static void mic_set_porch_timing(struct exynos_mic *mic)
> +{
> +	struct videomode vm;
> +	u32 reg;
> +
> +	if (!mic->i80_mode) {

It'd be better to check it before this function is called. This funcion
call really is unnecessary in case of i80 mode.

> +		vm = mic->vm;
> +
> +		reg = MIC_V_PULSE_WIDTH(vm.vsync_len) +
> +			MIC_V_PERIOD_LINE(vm.vsync_len + vm.vactive +
> +					vm.vback_porch + vm.vfront_porch);
> +		writel(reg, mic->reg + MIC_V_TIMING_0);
> +
> +		reg = MIC_VBP_SIZE(vm.vback_porch) +
> +						MIC_VFP_SIZE(vm.vfront_porch);
> +		writel(reg, mic->reg + MIC_V_TIMING_1);
> +
> +		reg = MIC_V_PULSE_WIDTH(vm.hsync_len) +
> +			MIC_V_PERIOD_LINE(vm.hsync_len + vm.hactive +
> +					vm.hback_porch + vm.hfront_porch);
> +		writel(reg, mic->reg + MIC_INPUT_TIMING_0);
> +
> +		reg = MIC_VBP_SIZE(vm.hback_porch) +
> +						MIC_VFP_SIZE(vm.hfront_porch);
> +		writel(reg, mic->reg + MIC_INPUT_TIMING_1);
> +	}
> +}
> +
> +static void mic_set_img_size(struct exynos_mic *mic)
> +{
> +	struct videomode *vm = &mic->vm;
> +	u32 reg;
> +
> +	reg = MIC_IMG_H_SIZE(vm->hactive) +
> +		MIC_IMG_V_SIZE(vm->vactive);
> +
> +	writel(reg, mic->reg + MIC_IMG_SIZE);
> +}
> +
> +static void mic_set_output_timing(struct exynos_mic *mic)
> +{
> +	struct videomode vm = mic->vm;
> +	u32 reg, bs_size_2d;
> +
> +	DRM_DEBUG("w: %u, h: %u\n", vm.hactive, vm.vactive);
> +	bs_size_2d = ((vm.hactive >> 2) << 1) + (vm.vactive % 4);
> +	reg = MIC_BS_SIZE_2D(bs_size_2d);
> +	writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_2);
> +
> +	if (!mic->i80_mode) {

Ditto.

> +		reg = MIC_H_PULSE_WIDTH_2D(vm.hsync_len) +
> +			MIC_H_PERIOD_PIXEL_2D(vm.hsync_len + bs_size_2d +
> +					vm.hback_porch + vm.hfront_porch);
> +		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_0);
> +
> +		reg = MIC_HBP_SIZE_2D(vm.hback_porch) +
> +			MIC_H_PERIOD_PIXEL_2D(vm.hfront_porch);
> +		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_1);
> +	}
> +}
> +
> +static void mic_set_reg_on(struct exynos_mic *mic, bool enable)
> +{
> +	u32 reg = readl(mic->reg + MIC_OP);
> +
> +	if (enable) {
> +		reg &= ~(MIC_MODE_SEL_MASK | MIC_CORE_VER_CONTROL | MIC_PSR_EN);
> +		reg |= (MIC_CORE_EN | MIC_BS_CHG_OUT | MIC_ON_REG);
> +
> +		if (mic->i80_mode)
> +			reg |= MIC_MODE_SEL_COMMAND_MODE;

Set explicitly it to video mode in case of RGB mode even though MODE_SET
field of this register has 0x0 as default.

i.e.,
               reg  &= ~MIC_MODE_SEL_COMMAND_MODE;
               if (mic->i80_mode)
                        reg |= MIC_MODE_SEL_COMMAND_MODE;

> +	} else {
> +		reg &= ~MIC_CORE_EN;
> +	}
> +
> +	reg |= MIC_UPD_REG;
> +	writel(reg, mic->reg + MIC_OP);
> +}
> +
> +static struct device_node *get_remote_node(struct device_node *from, int reg)
> +{
> +	struct device_node *endpoint = NULL, *remote_node = NULL;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(from, reg, -1);
> +	if (!endpoint) {
> +		DRM_ERROR("mic: Failed to find remote port from %s",
> +				from->full_name);
> +		goto exit;
> +	}
> +
> +	remote_node = of_graph_get_remote_port_parent(endpoint);
> +	if (!remote_node) {
> +		DRM_ERROR("mic: Failed to find remote port parent from %s",
> +							from->full_name);
> +		goto exit;
> +	}
> +
> +exit:
> +	of_node_put(endpoint);
> +	return remote_node;
> +}
> +
> +static int parse_dt(struct exynos_mic *mic)
> +{
> +	int ret = 0, i, j;
> +	struct device_node *remote_node;
> +	struct device_node *nodes[3];
> +
> +	/*
> +	 * The order of endpoints does matter.
> +	 * The first node must be for decon and the second one must be for dsi.
> +	 */
> +	for (i = 0, j = 0; i < NUM_ENDPOINTS; i++) {
> +		remote_node = get_remote_node(mic->dev->of_node, i);
> +		if (!remote_node) {
> +			ret = -EPIPE;
> +			goto exit;
> +		}
> +		nodes[j++] = remote_node;
> +
> +		switch (i) {
> +		case ENDPOINT_DECON_NODE:
> +			/* decon node */
> +			if (of_get_child_by_name(remote_node,
> +						"i80-if-timings"))
> +				mic->i80_mode = 1;
> +
> +			break;
> +		case ENDPOINT_DSI_NODE:
> +			/* panel node */
> +			remote_node = get_remote_node(remote_node, 1);
> +			if (!remote_node) {
> +				ret = -EPIPE;
> +				goto exit;
> +			}
> +			nodes[j++] = remote_node;
> +
> +			ret = of_get_videomode(remote_node,
> +							&mic->vm, 0);
> +			if (ret) {
> +				DRM_ERROR("mic: failed to get videomode");
> +				goto exit;
> +			}
> +
> +			break;
> +		default:
> +			DRM_ERROR("mic: Unknown endpoint from MIC");
> +			break;
> +		}
> +	}
> +
> +exit:
> +	while (--j > -1)
> +		of_node_put(nodes[j]);
> +
> +	return ret;
> +}
> +
> +void mic_disable(struct drm_bridge *bridge) { }

It seems that drm_encoder_disable function of drm_crtc_helper.c doesn't
check if the disable callback is NULL or not. Anyway, no problem but
this funtion could be removed with the disable callback checking later.

> +
> +void mic_post_disable(struct drm_bridge *bridge)
> +{
> +	struct exynos_mic *mic = bridge->driver_private;
> +	int i;
> +
> +	mutex_lock(&mic_mutex);
> +	if (!mic->enabled)
> +		goto already_disabled;
> +
> +	mic_set_path(mic, 0);
> +
> +	for (i = NUM_CLKS - 1; i > -1; i--)
> +		clk_disable_unprepare(mic->clks[i]);
> +
> +	mic->enabled = 0;
> +
> +already_disabled:
> +	mutex_unlock(&mic_mutex);
> +}
> +
> +void mic_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct exynos_mic *mic = bridge->driver_private;
> +	int ret, i;
> +
> +	mutex_lock(&mic_mutex);
> +	if (mic->enabled)
> +		goto already_enabled;
> +
> +	for (i = 0; i < NUM_CLKS; i++) {
> +		ret = clk_prepare_enable(mic->clks[i]);
> +		if (ret < 0) {
> +			DRM_ERROR("Failed to enable clock (%s)\n",
> +							clk_names[i]);
> +			goto turn_off_clks;
> +		}
> +	}
> +
> +	mic_set_path(mic, 1);
> +
> +	ret = mic_sw_reset(mic);
> +	if (ret) {
> +		DRM_ERROR("Failed to reset\n");
> +		goto turn_off_clks;
> +	}
> +
> +	mic_set_porch_timing(mic);
> +	mic_set_img_size(mic);
> +	mic_set_output_timing(mic);
> +	mic_set_reg_on(mic, 1);
> +	mic->enabled = 1;
> +	mutex_unlock(&mic_mutex);
> +
> +	return;
> +
> +turn_off_clks:
> +	while (--i > -1)
> +		clk_disable_unprepare(mic->clks[i]);
> +already_enabled:
> +	mutex_unlock(&mic_mutex);
> +}
> +
> +void mic_enable(struct drm_bridge *bridge) { }
> +
> +void mic_destroy(struct drm_bridge *bridge)
> +{
> +	struct exynos_mic *mic = bridge->driver_private;
> +	int i;
> +
> +	mutex_lock(&mic_mutex);
> +	if (!mic->enabled)
> +		goto already_disabled;
> +
> +	for (i = NUM_CLKS - 1; i > -1; i--)
> +		clk_disable_unprepare(mic->clks[i]);
> +
> +already_disabled:
> +	mutex_unlock(&mic_mutex);
> +}
> +
> +struct drm_bridge_funcs mic_bridge_funcs = {
> +	.disable = mic_disable,
> +	.post_disable = mic_post_disable,
> +	.pre_enable = mic_pre_enable,
> +	.enable = mic_enable,
> +};
> +
> +int exynos_mic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct exynos_mic *mic;
> +	struct resource res;
> +	int ret, i;
> +
> +	mic = devm_kzalloc(dev, sizeof(*mic), GFP_KERNEL);
> +	if (!mic) {
> +		DRM_ERROR("mic: Failed to allocate memory for MIC object\n");
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	mic->dev = dev;
> +
> +	ret = parse_dt(mic);
> +	if (ret)
> +		goto err;
> +
> +	ret = of_address_to_resource(dev->of_node, 0, &res);
> +	if (ret) {
> +		DRM_ERROR("mic: Failed to get mem region for MIC\n");
> +		goto err;
> +	}
> +	mic->reg = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!mic->reg) {
> +		DRM_ERROR("mic: Failed to remap for MIC\n");
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	ret = of_address_to_resource(dev->of_node, 1, &res);
> +	if (ret) {
> +		DRM_ERROR("mic: Failed to get mem region for MIC\n");
> +		goto err;
> +	}
> +	mic->sysreg = devm_ioremap(dev, res.start, resource_size(&res));

Consider to use syscon framework to access system registers.

> +	if (!mic->sysreg) {
> +		DRM_ERROR("mic: Failed to remap for MIC\n");
> +		goto err;
> +	}
> +
> +	mic->bridge.funcs = &mic_bridge_funcs;
> +	mic->bridge.of_node = dev->of_node;
> +	mic->bridge.driver_private = mic;
> +	ret = drm_bridge_add(&mic->bridge);
> +	if (ret) {
> +		DRM_ERROR("mic: Failed to add MIC to the global bridge list\n");
> +		goto err;
> +	}
> +
> +	for (i = 0; i < NUM_CLKS; i++) {
> +		mic->clks[i] = of_clk_get_by_name(dev->of_node, clk_names[i]);
> +		if (IS_ERR(mic->clks[i])) {
> +			DRM_ERROR("mic: Failed to get clock (%s)\n",
> +								clk_names[i]);
> +			ret = PTR_ERR(mic->clks[i]);
> +			goto err;
> +		}
> +	}
> +
> +	DRM_INFO("MIC has been probed\n");

Use DRM_DEBUG_KMS instead.

Thanks,
Inki Dae

> +
> +err:
> +	return ret;
> +}
> +
> +static int exynos_mic_remove(struct platform_device *pdev)
> +{
> +	struct exynos_mic *mic = platform_get_drvdata(pdev);
> +	int i;
> +
> +	drm_bridge_remove(&mic->bridge);
> +
> +	for (i = NUM_CLKS - 1; i > -1; i--)
> +		clk_put(mic->clks[i]);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id exynos_mic_of_match[] = {
> +	{ .compatible = "samsung,exynos5433-mic" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, exynos_mic_of_match);
> +
> +struct platform_driver mic_driver = {
> +	.probe		= exynos_mic_probe,
> +	.remove		= exynos_mic_remove,
> +	.driver		= {
> +		.name	= "exynos-mic",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = exynos_mic_of_match,
> +	},
> +};
> --
> 1.9.1
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/exynos: mic: add MIC driver

[Hyungwon Hwang]

Dear Inki dae,

On Tue, 24 Mar 2015 14:51:31 +0900
Inki Dae <inki.dae@samsung.com> wrote:

> On 2015년 03월 18일 17:16, Hyungwon Hwang wrote:
> > MIC(Mobile image compressor) is newly added IP in Exynos5433. MIC
> > resides between decon and mipi dsim, and compresses frame data by
> > 50%. With dsi, not display port, to send frame data to the panel,
> > the bandwidth is not enough. That is why this compressor is
> > introduced.
> > 
> > Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
> > ---
> > Changes for v2:
> > - make mic driver to be registered by exynos drm driver instead as
> > a module                                      |
> > -------------------------------------------------------------------------------------------------------------
> > driver
> > |
> > -------------------------------------------------------------------------------------------------------------
> > - change the description of mic driver in documentation
> > - add module author at the top of the source file removing
> > MODULE_OWNER, MODULE_DESCRIPTION, MODULE_LICENSE
> >  .../devicetree/bindings/video/exynos-mic.txt       |  49 +++
> >  drivers/gpu/drm/exynos/Kconfig                     |   6 +
> >  drivers/gpu/drm/exynos/Makefile                    |   1 +
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c            |   3 +
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h            |   1 +
> >  drivers/gpu/drm/exynos/exynos_drm_mic.c            | 481
> > +++++++++++++++++++++ 6 files changed, 541 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/video/exynos-mic.txt create mode
> > 100644 drivers/gpu/drm/exynos/exynos_drm_mic.c
> > 
> > diff --git a/Documentation/devicetree/bindings/video/exynos-mic.txt
> > b/Documentation/devicetree/bindings/video/exynos-mic.txt new file
> > mode 100644 index 0000000..006d072
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/exynos-mic.txt
> > @@ -0,0 +1,49 @@
> > +Device-Tree bindings for Samsung Exynos SoC mobile image
> > compressor (MIC) +
> > +MIC (mobile image compressor) resides between decon and mipi dsi.
> > Mipi dsi is +not capable to transfer high resoltuion frame data as
> > decon can send. MIC +solves this problem by compressing the frame
> > data by 1/2 before it is transfered +through mipi dsi. The
> > compressed frame data must be uncompressed in the panel +PCB.
> > +
> > +Required properties:
> > +- compatible: value should be "samsung,exynos5433-mic".
> > +- reg: physical base address and length of the MIC registers set
> > and system
> > +       register of mic.
> > +- clocks: must include clock specifiers corresponding to entries
> > in the
> > +	  clock-names property.
> > +- clock-names: list of clock names sorted in the same order as the
> > clocks
> > +	       property. Must contain "pclk_mic0",
> > "sclk_rgb_vclk_to_mic0". +- ports: contains a port which is
> > connected to decon node and dsi node.
> > +	 address-cells and size-cells must 1 and 0, respectively.
> > +- port: contains an endpoint node which is connected to the
> > endpoint in the
> > +	decon node or dsi node. The reg value must be 0 and 1
> > respectively. +
> > +Example:
> > +SoC specific DT entry:
> > +mic: mic@13930000 {
> > +	compatible = "samsung,exynos5433-mic";
> > +	reg = <0x13930000 0x48 0x13B80000 0x1010>;
> > +	clocks = <&cmu_disp CLK_PCLK_MIC0>,
> > +	       <&cmu_disp CLK_SCLK_RGB_VCLK_TO_MIC0>;
> > +	clock-names = "pclk_mic0", "sclk_rgb_vclk_to_mic0";
> > +
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +			mic_to_decon: endpoint {
> > +				remote-endpoint = <&decon_to_mic>;
> > +			};
> > +		};
> > +
> > +		port@1 {
> > +			reg = <1>;
> > +			mic_to_dsi: endpoint {
> > +				remote-endpoint = <&dsi_to_mic>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git a/drivers/gpu/drm/exynos/Kconfig
> > b/drivers/gpu/drm/exynos/Kconfig index e15cc2e..a796175 100644
> > --- a/drivers/gpu/drm/exynos/Kconfig
> > +++ b/drivers/gpu/drm/exynos/Kconfig
> > @@ -103,3 +103,9 @@ config DRM_EXYNOS_GSC
> >  	depends on DRM_EXYNOS_IPP && ARCH_EXYNOS5
> > && !ARCH_MULTIPLATFORM help
> >  	  Choose this option if you want to use Exynos GSC for DRM.
> > +
> > +config DRM_EXYNOS_MIC
> > +	bool "Exynos DRM MIC"
> > +	depends on (DRM_EXYNOS && DRM_EXYNOS5433_DECON)
> > +	help
> > +	  Choose this option if you want to use Exynos MIC for DRM.
> > diff --git a/drivers/gpu/drm/exynos/Makefile
> > b/drivers/gpu/drm/exynos/Makefile index fbd084d..7de0b10 100644
> > --- a/drivers/gpu/drm/exynos/Makefile
> > +++ b/drivers/gpu/drm/exynos/Makefile
> > @@ -22,5 +22,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_IPP)	+=
> > exynos_drm_ipp.o exynosdrm-$(CONFIG_DRM_EXYNOS_FIMC)	+=
> > exynos_drm_fimc.o exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR)	+=
> > exynos_drm_rotator.o exynosdrm-$(CONFIG_DRM_EXYNOS_GSC)	+=
> > exynos_drm_gsc.o +exynosdrm-$(CONFIG_DRM_EXYNOS_MIC)     +=
> > exynos_drm_mic.o
> > 
> >  obj-$(CONFIG_DRM_EXYNOS)		+= exynosdrm.o
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 1fa0dd0..ec9984d
> > 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -562,6 +562,9 @@ static struct platform_driver *const
> > exynos_drm_kms_drivers[] = { #ifdef CONFIG_DRM_EXYNOS7_DECON
> >  	&decon_driver,
> >  #endif
> > +#ifdef CONFIG_DRM_EXYNOS_MIC
> > +	&mic_driver,
> > +#endif
> >  #ifdef CONFIG_DRM_EXYNOS_DP
> >  	&dp_driver,
> >  #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 40996d8..9c94dc9
> > 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > @@ -357,4 +357,5 @@ extern struct platform_driver fimc_driver;
> >  extern struct platform_driver rotator_driver;
> >  extern struct platform_driver gsc_driver;
> >  extern struct platform_driver ipp_driver;
> > +extern struct platform_driver mic_driver;
> >  #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c
> > b/drivers/gpu/drm/exynos/exynos_drm_mic.c new file mode 100644
> > index 0000000..b898a2a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
> > @@ -0,0 +1,481 @@
> > +/*
> > + * Copyright (C) 2015 Samsung Electronics Co.Ltd
> > + * Authors:
> > + *	Hyungwon Hwang <human.hwang@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundationr
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <video/of_videomode.h>
> > +#include <linux/of_address.h>
> > +#include <video/videomode.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/clk.h>
> > +#include <drm/drmP.h>
> > +
> > +/* Sysreg registers for MIC */
> > +#define DSD_CFG_MUX	0x1004
> > +#define MIC0_RGB_MUX	(1 << 0)
> > +#define MIC0_I80_MUX	(1 << 1)
> > +#define MIC0_ON_MUX	(1 << 5)
> > +
> > +/* MIC registers */
> > +#define MIC_OP				0x0
> > +#define MIC_IP_VER			0x0004
> > +#define MIC_V_TIMING_0			0x0008
> > +#define MIC_V_TIMING_1			0x000C
> > +#define MIC_IMG_SIZE			0x0010
> > +#define MIC_INPUT_TIMING_0		0x0014
> > +#define MIC_INPUT_TIMING_1		0x0018
> > +#define MIC_2D_OUTPUT_TIMING_0		0x001C
> > +#define MIC_2D_OUTPUT_TIMING_1		0x0020
> > +#define MIC_2D_OUTPUT_TIMING_2		0x0024
> > +#define MIC_3D_OUTPUT_TIMING_0		0x0028
> > +#define MIC_3D_OUTPUT_TIMING_1		0x002C
> > +#define MIC_3D_OUTPUT_TIMING_2		0x0030
> > +#define MIC_CORE_PARA_0			0x0034
> > +#define MIC_CORE_PARA_1			0x0038
> > +#define MIC_CTC_CTRL			0x0040
> > +#define MIC_RD_DATA			0x0044
> > +
> > +#define MIC_UPD_REG			(1 << 31)
> > +#define MIC_ON_REG			(1 << 30)
> > +#define MIC_TD_ON_REG			(1 << 29)
> > +#define MIC_BS_CHG_OUT			(1 << 16)
> > +#define MIC_VIDEO_TYPE(x)		(((x) & 0xf) << 12)
> > +#define MIC_PSR_EN			(1 << 5)
> > +#define MIC_SW_RST			(1 << 4)
> > +#define MIC_ALL_RST			(1 << 3)
> > +#define MIC_CORE_VER_CONTROL		(1 << 2)
> > +#define MIC_MODE_SEL_COMMAND_MODE	(1 << 1)
> > +#define MIC_MODE_SEL_MASK		(1 << 1)
> > +#define MIC_CORE_EN			(1 << 0)
> > +
> > +#define MIC_V_PULSE_WIDTH(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_V_PERIOD_LINE(x)		((x) & 0x3fff)
> > +
> > +#define MIC_VBP_SIZE(x)			(((x) & 0x3fff) <<
> > 16) +#define MIC_VFP_SIZE(x)			((x) & 0x3fff)
> > +
> > +#define MIC_IMG_V_SIZE(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_IMG_H_SIZE(x)		((x) & 0x3fff)
> > +
> > +#define MIC_H_PULSE_WIDTH_IN(x)		(((x) & 0x3fff) <<
> > 16) +#define MIC_H_PERIOD_PIXEL_IN(x)	((x) & 0x3fff)
> > +
> > +#define MIC_HBP_SIZE_IN(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_HFP_SIZE_IN(x)		((x) & 0x3fff)
> > +
> > +#define MIC_H_PULSE_WIDTH_2D(x)		(((x) & 0x3fff) <<
> > 16) +#define MIC_H_PERIOD_PIXEL_2D(x)	((x) & 0x3fff)
> > +
> > +#define MIC_HBP_SIZE_2D(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_HFP_SIZE_2D(x)		((x) & 0x3fff)
> > +
> > +#define MIC_BS_SIZE_2D(x)	((x) & 0x3fff)
> > +
> > +enum {
> > +	ENDPOINT_DECON_NODE,
> > +	ENDPOINT_DSI_NODE,
> > +	NUM_ENDPOINTS
> > +};
> > +
> > +static char *clk_names[] = { "pclk_mic0",
> > "sclk_rgb_vclk_to_mic0" }; +#define NUM_CLKS
> > ARRAY_SIZE(clk_names) +static DEFINE_MUTEX(mic_mutex);
> > +
> > +struct exynos_mic {
> > +	struct device *dev;
> > +	void __iomem *reg;
> > +	void __iomem *sysreg;
> > +	struct clk *clks[NUM_CLKS];
> > +
> > +	bool i80_mode;
> > +	struct videomode vm;
> > +	struct drm_encoder *encoder;
> > +	struct drm_bridge bridge;
> > +
> > +	bool enabled;
> > +};
> > +
> > +static void mic_set_path(struct exynos_mic *mic, bool enable)
> > +{
> > +	u32 reg;
> > +
> > +	reg = readl(mic->sysreg + DSD_CFG_MUX);
> > +	if (enable)
> > +		reg |= MIC0_RGB_MUX | MIC0_I80_MUX | MIC0_ON_MUX;
> > +	else
> > +		reg &= ~(MIC0_RGB_MUX | MIC0_I80_MUX |
> > MIC0_ON_MUX);
> 
> exynos_mic has already which video mode - RGB or i80 - is used so it'd
> be reasonable to select one of them to avoid confusing.
> 

Yes. I agree.

> > +
> > +	writel(reg, mic->sysreg + DSD_CFG_MUX);
> > +}
> > +
> > +static int mic_sw_reset(struct exynos_mic *mic)
> > +{
> > +	unsigned int retry = 100;
> > +	int ret;
> > +
> > +	writel(MIC_SW_RST, mic->reg + MIC_OP);
> > +
> > +	while (retry-- > 0) {
> > +		ret = readl(mic->reg + MIC_OP);
> > +		if (!(ret & MIC_SW_RST))
> > +			return 0;
> > +
> > +		udelay(10);
> > +	}
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static void mic_set_porch_timing(struct exynos_mic *mic)
> > +{
> > +	struct videomode vm;
> > +	u32 reg;
> > +
> > +	if (!mic->i80_mode) {
> 
> It'd be better to check it before this function is called. This
> funcion call really is unnecessary in case of i80 mode.
> 


> > +		vm = mic->vm;
> > +
> > +		reg = MIC_V_PULSE_WIDTH(vm.vsync_len) +
> > +			MIC_V_PERIOD_LINE(vm.vsync_len +
> > vm.vactive +
> > +					vm.vback_porch +
> > vm.vfront_porch);
> > +		writel(reg, mic->reg + MIC_V_TIMING_0);
> > +
> > +		reg = MIC_VBP_SIZE(vm.vback_porch) +
> > +
> > MIC_VFP_SIZE(vm.vfront_porch);
> > +		writel(reg, mic->reg + MIC_V_TIMING_1);
> > +
> > +		reg = MIC_V_PULSE_WIDTH(vm.hsync_len) +
> > +			MIC_V_PERIOD_LINE(vm.hsync_len +
> > vm.hactive +
> > +					vm.hback_porch +
> > vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_INPUT_TIMING_0);
> > +
> > +		reg = MIC_VBP_SIZE(vm.hback_porch) +
> > +
> > MIC_VFP_SIZE(vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_INPUT_TIMING_1);
> > +	}
> > +}
> > +
> > +static void mic_set_img_size(struct exynos_mic *mic)
> > +{
> > +	struct videomode *vm = &mic->vm;
> > +	u32 reg;
> > +
> > +	reg = MIC_IMG_H_SIZE(vm->hactive) +
> > +		MIC_IMG_V_SIZE(vm->vactive);
> > +
> > +	writel(reg, mic->reg + MIC_IMG_SIZE);
> > +}
> > +
> > +static void mic_set_output_timing(struct exynos_mic *mic)
> > +{
> > +	struct videomode vm = mic->vm;
> > +	u32 reg, bs_size_2d;
> > +
> > +	DRM_DEBUG("w: %u, h: %u\n", vm.hactive, vm.vactive);
> > +	bs_size_2d = ((vm.hactive >> 2) << 1) + (vm.vactive % 4);
> > +	reg = MIC_BS_SIZE_2D(bs_size_2d);
> > +	writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_2);
> > +
> > +	if (!mic->i80_mode) {
> 
> Ditto.
> 
> > +		reg = MIC_H_PULSE_WIDTH_2D(vm.hsync_len) +
> > +			MIC_H_PERIOD_PIXEL_2D(vm.hsync_len +
> > bs_size_2d +
> > +					vm.hback_porch +
> > vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_0);
> > +
> > +		reg = MIC_HBP_SIZE_2D(vm.hback_porch) +
> > +			MIC_H_PERIOD_PIXEL_2D(vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_1);
> > +	}
> > +}
> > +
> > +static void mic_set_reg_on(struct exynos_mic *mic, bool enable)
> > +{
> > +	u32 reg = readl(mic->reg + MIC_OP);
> > +
> > +	if (enable) {
> > +		reg &= ~(MIC_MODE_SEL_MASK | MIC_CORE_VER_CONTROL
> > | MIC_PSR_EN);
> > +		reg |= (MIC_CORE_EN | MIC_BS_CHG_OUT | MIC_ON_REG);
> > +
> > +		if (mic->i80_mode)
> > +			reg |= MIC_MODE_SEL_COMMAND_MODE;
> 
> Set explicitly it to video mode in case of RGB mode even though
> MODE_SET field of this register has 0x0 as default.
> 
> i.e.,
>                reg  &= ~MIC_MODE_SEL_COMMAND_MODE;
>                if (mic->i80_mode)
>                         reg |= MIC_MODE_SEL_COMMAND_MODE;
> 
> > +	} else {
> > +		reg &= ~MIC_CORE_EN;
> > +	}
> > +
> > +	reg |= MIC_UPD_REG;
> > +	writel(reg, mic->reg + MIC_OP);
> > +}
> > +
> > +static struct device_node *get_remote_node(struct device_node
> > *from, int reg) +{
> > +	struct device_node *endpoint = NULL, *remote_node = NULL;
> > +
> > +	endpoint = of_graph_get_endpoint_by_regs(from, reg, -1);
> > +	if (!endpoint) {
> > +		DRM_ERROR("mic: Failed to find remote port from
> > %s",
> > +				from->full_name);
> > +		goto exit;
> > +	}
> > +
> > +	remote_node = of_graph_get_remote_port_parent(endpoint);
> > +	if (!remote_node) {
> > +		DRM_ERROR("mic: Failed to find remote port parent
> > from %s",
> > +
> > from->full_name);
> > +		goto exit;
> > +	}
> > +
> > +exit:
> > +	of_node_put(endpoint);
> > +	return remote_node;
> > +}
> > +
> > +static int parse_dt(struct exynos_mic *mic)
> > +{
> > +	int ret = 0, i, j;
> > +	struct device_node *remote_node;
> > +	struct device_node *nodes[3];
> > +
> > +	/*
> > +	 * The order of endpoints does matter.
> > +	 * The first node must be for decon and the second one
> > must be for dsi.
> > +	 */
> > +	for (i = 0, j = 0; i < NUM_ENDPOINTS; i++) {
> > +		remote_node = get_remote_node(mic->dev->of_node,
> > i);
> > +		if (!remote_node) {
> > +			ret = -EPIPE;
> > +			goto exit;
> > +		}
> > +		nodes[j++] = remote_node;
> > +
> > +		switch (i) {
> > +		case ENDPOINT_DECON_NODE:
> > +			/* decon node */
> > +			if (of_get_child_by_name(remote_node,
> > +						"i80-if-timings"))
> > +				mic->i80_mode = 1;
> > +
> > +			break;
> > +		case ENDPOINT_DSI_NODE:
> > +			/* panel node */
> > +			remote_node = get_remote_node(remote_node,
> > 1);
> > +			if (!remote_node) {
> > +				ret = -EPIPE;
> > +				goto exit;
> > +			}
> > +			nodes[j++] = remote_node;
> > +
> > +			ret = of_get_videomode(remote_node,
> > +							&mic->vm,
> > 0);
> > +			if (ret) {
> > +				DRM_ERROR("mic: failed to get
> > videomode");
> > +				goto exit;
> > +			}
> > +
> > +			break;
> > +		default:
> > +			DRM_ERROR("mic: Unknown endpoint from
> > MIC");
> > +			break;
> > +		}
> > +	}
> > +
> > +exit:
> > +	while (--j > -1)
> > +		of_node_put(nodes[j]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mic_disable(struct drm_bridge *bridge) { }
> 
> It seems that drm_encoder_disable function of drm_crtc_helper.c
> doesn't check if the disable callback is NULL or not. Anyway, no
> problem but this funtion could be removed with the disable callback
> checking later.
> 
> > +
> > +void mic_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct exynos_mic *mic = bridge->driver_private;
> > +	int i;
> > +
> > +	mutex_lock(&mic_mutex);
> > +	if (!mic->enabled)
> > +		goto already_disabled;
> > +
> > +	mic_set_path(mic, 0);
> > +
> > +	for (i = NUM_CLKS - 1; i > -1; i--)
> > +		clk_disable_unprepare(mic->clks[i]);
> > +
> > +	mic->enabled = 0;
> > +
> > +already_disabled:
> > +	mutex_unlock(&mic_mutex);
> > +}
> > +
> > +void mic_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct exynos_mic *mic = bridge->driver_private;
> > +	int ret, i;
> > +
> > +	mutex_lock(&mic_mutex);
> > +	if (mic->enabled)
> > +		goto already_enabled;
> > +
> > +	for (i = 0; i < NUM_CLKS; i++) {
> > +		ret = clk_prepare_enable(mic->clks[i]);
> > +		if (ret < 0) {
> > +			DRM_ERROR("Failed to enable clock (%s)\n",
> > +
> > clk_names[i]);
> > +			goto turn_off_clks;
> > +		}
> > +	}
> > +
> > +	mic_set_path(mic, 1);
> > +
> > +	ret = mic_sw_reset(mic);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to reset\n");
> > +		goto turn_off_clks;
> > +	}
> > +
> > +	mic_set_porch_timing(mic);
> > +	mic_set_img_size(mic);
> > +	mic_set_output_timing(mic);
> > +	mic_set_reg_on(mic, 1);
> > +	mic->enabled = 1;
> > +	mutex_unlock(&mic_mutex);
> > +
> > +	return;
> > +
> > +turn_off_clks:
> > +	while (--i > -1)
> > +		clk_disable_unprepare(mic->clks[i]);
> > +already_enabled:
> > +	mutex_unlock(&mic_mutex);
> > +}
> > +
> > +void mic_enable(struct drm_bridge *bridge) { }
> > +
> > +void mic_destroy(struct drm_bridge *bridge)
> > +{
> > +	struct exynos_mic *mic = bridge->driver_private;
> > +	int i;
> > +
> > +	mutex_lock(&mic_mutex);
> > +	if (!mic->enabled)
> > +		goto already_disabled;
> > +
> > +	for (i = NUM_CLKS - 1; i > -1; i--)
> > +		clk_disable_unprepare(mic->clks[i]);
> > +
> > +already_disabled:
> > +	mutex_unlock(&mic_mutex);
> > +}
> > +
> > +struct drm_bridge_funcs mic_bridge_funcs = {
> > +	.disable = mic_disable,
> > +	.post_disable = mic_post_disable,
> > +	.pre_enable = mic_pre_enable,
> > +	.enable = mic_enable,
> > +};
> > +
> > +int exynos_mic_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct exynos_mic *mic;
> > +	struct resource res;
> > +	int ret, i;
> > +
> > +	mic = devm_kzalloc(dev, sizeof(*mic), GFP_KERNEL);
> > +	if (!mic) {
> > +		DRM_ERROR("mic: Failed to allocate memory for MIC
> > object\n");
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	mic->dev = dev;
> > +
> > +	ret = parse_dt(mic);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = of_address_to_resource(dev->of_node, 0, &res);
> > +	if (ret) {
> > +		DRM_ERROR("mic: Failed to get mem region for
> > MIC\n");
> > +		goto err;
> > +	}
> > +	mic->reg = devm_ioremap(dev, res.start,
> > resource_size(&res));
> > +	if (!mic->reg) {
> > +		DRM_ERROR("mic: Failed to remap for MIC\n");
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	ret = of_address_to_resource(dev->of_node, 1, &res);
> > +	if (ret) {
> > +		DRM_ERROR("mic: Failed to get mem region for
> > MIC\n");
> > +		goto err;
> > +	}
> > +	mic->sysreg = devm_ioremap(dev, res.start,
> > resource_size(&res));
> 
> Consider to use syscon framework to access system registers.
> 
> > +	if (!mic->sysreg) {
> > +		DRM_ERROR("mic: Failed to remap for MIC\n");
> > +		goto err;
> > +	}
> > +
> > +	mic->bridge.funcs = &mic_bridge_funcs;
> > +	mic->bridge.of_node = dev->of_node;
> > +	mic->bridge.driver_private = mic;
> > +	ret = drm_bridge_add(&mic->bridge);
> > +	if (ret) {
> > +		DRM_ERROR("mic: Failed to add MIC to the global
> > bridge list\n");
> > +		goto err;
> > +	}
> > +
> > +	for (i = 0; i < NUM_CLKS; i++) {
> > +		mic->clks[i] = of_clk_get_by_name(dev->of_node,
> > clk_names[i]);
> > +		if (IS_ERR(mic->clks[i])) {
> > +			DRM_ERROR("mic: Failed to get clock
> > (%s)\n",
> > +
> > clk_names[i]);
> > +			ret = PTR_ERR(mic->clks[i]);
> > +			goto err;
> > +		}
> > +	}
> > +
> > +	DRM_INFO("MIC has been probed\n");
> 
> Use DRM_DEBUG_KMS instead.
> 
> Thanks,
> Inki Dae
> 
> > +
> > +err:
> > +	return ret;
> > +}
> > +
> > +static int exynos_mic_remove(struct platform_device *pdev)
> > +{
> > +	struct exynos_mic *mic = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	drm_bridge_remove(&mic->bridge);
> > +
> > +	for (i = NUM_CLKS - 1; i > -1; i--)
> > +		clk_put(mic->clks[i]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id exynos_mic_of_match[] = {
> > +	{ .compatible = "samsung,exynos5433-mic" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos_mic_of_match);
> > +
> > +struct platform_driver mic_driver = {
> > +	.probe		= exynos_mic_probe,
> > +	.remove		= exynos_mic_remove,
> > +	.driver		= {
> > +		.name	= "exynos-mic",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = exynos_mic_of_match,
> > +	},
> > +};
> > --
> > 1.9.1
> > 
> > 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/exynos: mic: add MIC driver

[Hyungwon Hwang]

Dear Inki dae,

Sorry for the previous mail which is not completed. I typed something
and it was the shortcut for maybe.

On Tue, 24 Mar 2015 14:51:31 +0900
Inki Dae <inki.dae@samsung.com> wrote:

> On 2015년 03월 18일 17:16, Hyungwon Hwang wrote:
> > MIC(Mobile image compressor) is newly added IP in Exynos5433. MIC
> > resides between decon and mipi dsim, and compresses frame data by
> > 50%. With dsi, not display port, to send frame data to the panel,
> > the bandwidth is not enough. That is why this compressor is
> > introduced.
> > 
> > Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
> > ---
> > Changes for v2:
> > - make mic driver to be registered by exynos drm driver instead as
> > a module                                      |
> > -------------------------------------------------------------------------------------------------------------
> > driver
> > |
> > -------------------------------------------------------------------------------------------------------------
> > - change the description of mic driver in documentation
> > - add module author at the top of the source file removing
> > MODULE_OWNER, MODULE_DESCRIPTION, MODULE_LICENSE
> >  .../devicetree/bindings/video/exynos-mic.txt       |  49 +++
> >  drivers/gpu/drm/exynos/Kconfig                     |   6 +
> >  drivers/gpu/drm/exynos/Makefile                    |   1 +
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c            |   3 +
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h            |   1 +
> >  drivers/gpu/drm/exynos/exynos_drm_mic.c            | 481
> > +++++++++++++++++++++ 6 files changed, 541 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/video/exynos-mic.txt create mode
> > 100644 drivers/gpu/drm/exynos/exynos_drm_mic.c
> > 
> > diff --git a/Documentation/devicetree/bindings/video/exynos-mic.txt
> > b/Documentation/devicetree/bindings/video/exynos-mic.txt new file
> > mode 100644 index 0000000..006d072
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/exynos-mic.txt
> > @@ -0,0 +1,49 @@
> > +Device-Tree bindings for Samsung Exynos SoC mobile image
> > compressor (MIC) +
> > +MIC (mobile image compressor) resides between decon and mipi dsi.
> > Mipi dsi is +not capable to transfer high resoltuion frame data as
> > decon can send. MIC +solves this problem by compressing the frame
> > data by 1/2 before it is transfered +through mipi dsi. The
> > compressed frame data must be uncompressed in the panel +PCB.
> > +
> > +Required properties:
> > +- compatible: value should be "samsung,exynos5433-mic".
> > +- reg: physical base address and length of the MIC registers set
> > and system
> > +       register of mic.
> > +- clocks: must include clock specifiers corresponding to entries
> > in the
> > +	  clock-names property.
> > +- clock-names: list of clock names sorted in the same order as the
> > clocks
> > +	       property. Must contain "pclk_mic0",
> > "sclk_rgb_vclk_to_mic0". +- ports: contains a port which is
> > connected to decon node and dsi node.
> > +	 address-cells and size-cells must 1 and 0, respectively.
> > +- port: contains an endpoint node which is connected to the
> > endpoint in the
> > +	decon node or dsi node. The reg value must be 0 and 1
> > respectively. +
> > +Example:
> > +SoC specific DT entry:
> > +mic: mic@13930000 {
> > +	compatible = "samsung,exynos5433-mic";
> > +	reg = <0x13930000 0x48 0x13B80000 0x1010>;
> > +	clocks = <&cmu_disp CLK_PCLK_MIC0>,
> > +	       <&cmu_disp CLK_SCLK_RGB_VCLK_TO_MIC0>;
> > +	clock-names = "pclk_mic0", "sclk_rgb_vclk_to_mic0";
> > +
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +			mic_to_decon: endpoint {
> > +				remote-endpoint = <&decon_to_mic>;
> > +			};
> > +		};
> > +
> > +		port@1 {
> > +			reg = <1>;
> > +			mic_to_dsi: endpoint {
> > +				remote-endpoint = <&dsi_to_mic>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git a/drivers/gpu/drm/exynos/Kconfig
> > b/drivers/gpu/drm/exynos/Kconfig index e15cc2e..a796175 100644
> > --- a/drivers/gpu/drm/exynos/Kconfig
> > +++ b/drivers/gpu/drm/exynos/Kconfig
> > @@ -103,3 +103,9 @@ config DRM_EXYNOS_GSC
> >  	depends on DRM_EXYNOS_IPP && ARCH_EXYNOS5
> > && !ARCH_MULTIPLATFORM help
> >  	  Choose this option if you want to use Exynos GSC for DRM.
> > +
> > +config DRM_EXYNOS_MIC
> > +	bool "Exynos DRM MIC"
> > +	depends on (DRM_EXYNOS && DRM_EXYNOS5433_DECON)
> > +	help
> > +	  Choose this option if you want to use Exynos MIC for DRM.
> > diff --git a/drivers/gpu/drm/exynos/Makefile
> > b/drivers/gpu/drm/exynos/Makefile index fbd084d..7de0b10 100644
> > --- a/drivers/gpu/drm/exynos/Makefile
> > +++ b/drivers/gpu/drm/exynos/Makefile
> > @@ -22,5 +22,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_IPP)	+=
> > exynos_drm_ipp.o exynosdrm-$(CONFIG_DRM_EXYNOS_FIMC)	+=
> > exynos_drm_fimc.o exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR)	+=
> > exynos_drm_rotator.o exynosdrm-$(CONFIG_DRM_EXYNOS_GSC)	+=
> > exynos_drm_gsc.o +exynosdrm-$(CONFIG_DRM_EXYNOS_MIC)     +=
> > exynos_drm_mic.o
> > 
> >  obj-$(CONFIG_DRM_EXYNOS)		+= exynosdrm.o
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 1fa0dd0..ec9984d
> > 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -562,6 +562,9 @@ static struct platform_driver *const
> > exynos_drm_kms_drivers[] = { #ifdef CONFIG_DRM_EXYNOS7_DECON
> >  	&decon_driver,
> >  #endif
> > +#ifdef CONFIG_DRM_EXYNOS_MIC
> > +	&mic_driver,
> > +#endif
> >  #ifdef CONFIG_DRM_EXYNOS_DP
> >  	&dp_driver,
> >  #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 40996d8..9c94dc9
> > 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > @@ -357,4 +357,5 @@ extern struct platform_driver fimc_driver;
> >  extern struct platform_driver rotator_driver;
> >  extern struct platform_driver gsc_driver;
> >  extern struct platform_driver ipp_driver;
> > +extern struct platform_driver mic_driver;
> >  #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c
> > b/drivers/gpu/drm/exynos/exynos_drm_mic.c new file mode 100644
> > index 0000000..b898a2a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
> > @@ -0,0 +1,481 @@
> > +/*
> > + * Copyright (C) 2015 Samsung Electronics Co.Ltd
> > + * Authors:
> > + *	Hyungwon Hwang <human.hwang@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundationr
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <video/of_videomode.h>
> > +#include <linux/of_address.h>
> > +#include <video/videomode.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/clk.h>
> > +#include <drm/drmP.h>
> > +
> > +/* Sysreg registers for MIC */
> > +#define DSD_CFG_MUX	0x1004
> > +#define MIC0_RGB_MUX	(1 << 0)
> > +#define MIC0_I80_MUX	(1 << 1)
> > +#define MIC0_ON_MUX	(1 << 5)
> > +
> > +/* MIC registers */
> > +#define MIC_OP				0x0
> > +#define MIC_IP_VER			0x0004
> > +#define MIC_V_TIMING_0			0x0008
> > +#define MIC_V_TIMING_1			0x000C
> > +#define MIC_IMG_SIZE			0x0010
> > +#define MIC_INPUT_TIMING_0		0x0014
> > +#define MIC_INPUT_TIMING_1		0x0018
> > +#define MIC_2D_OUTPUT_TIMING_0		0x001C
> > +#define MIC_2D_OUTPUT_TIMING_1		0x0020
> > +#define MIC_2D_OUTPUT_TIMING_2		0x0024
> > +#define MIC_3D_OUTPUT_TIMING_0		0x0028
> > +#define MIC_3D_OUTPUT_TIMING_1		0x002C
> > +#define MIC_3D_OUTPUT_TIMING_2		0x0030
> > +#define MIC_CORE_PARA_0			0x0034
> > +#define MIC_CORE_PARA_1			0x0038
> > +#define MIC_CTC_CTRL			0x0040
> > +#define MIC_RD_DATA			0x0044
> > +
> > +#define MIC_UPD_REG			(1 << 31)
> > +#define MIC_ON_REG			(1 << 30)
> > +#define MIC_TD_ON_REG			(1 << 29)
> > +#define MIC_BS_CHG_OUT			(1 << 16)
> > +#define MIC_VIDEO_TYPE(x)		(((x) & 0xf) << 12)
> > +#define MIC_PSR_EN			(1 << 5)
> > +#define MIC_SW_RST			(1 << 4)
> > +#define MIC_ALL_RST			(1 << 3)
> > +#define MIC_CORE_VER_CONTROL		(1 << 2)
> > +#define MIC_MODE_SEL_COMMAND_MODE	(1 << 1)
> > +#define MIC_MODE_SEL_MASK		(1 << 1)
> > +#define MIC_CORE_EN			(1 << 0)
> > +
> > +#define MIC_V_PULSE_WIDTH(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_V_PERIOD_LINE(x)		((x) & 0x3fff)
> > +
> > +#define MIC_VBP_SIZE(x)			(((x) & 0x3fff) <<
> > 16) +#define MIC_VFP_SIZE(x)			((x) & 0x3fff)
> > +
> > +#define MIC_IMG_V_SIZE(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_IMG_H_SIZE(x)		((x) & 0x3fff)
> > +
> > +#define MIC_H_PULSE_WIDTH_IN(x)		(((x) & 0x3fff) <<
> > 16) +#define MIC_H_PERIOD_PIXEL_IN(x)	((x) & 0x3fff)
> > +
> > +#define MIC_HBP_SIZE_IN(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_HFP_SIZE_IN(x)		((x) & 0x3fff)
> > +
> > +#define MIC_H_PULSE_WIDTH_2D(x)		(((x) & 0x3fff) <<
> > 16) +#define MIC_H_PERIOD_PIXEL_2D(x)	((x) & 0x3fff)
> > +
> > +#define MIC_HBP_SIZE_2D(x)		(((x) & 0x3fff) << 16)
> > +#define MIC_HFP_SIZE_2D(x)		((x) & 0x3fff)
> > +
> > +#define MIC_BS_SIZE_2D(x)	((x) & 0x3fff)
> > +
> > +enum {
> > +	ENDPOINT_DECON_NODE,
> > +	ENDPOINT_DSI_NODE,
> > +	NUM_ENDPOINTS
> > +};
> > +
> > +static char *clk_names[] = { "pclk_mic0",
> > "sclk_rgb_vclk_to_mic0" }; +#define NUM_CLKS
> > ARRAY_SIZE(clk_names) +static DEFINE_MUTEX(mic_mutex);
> > +
> > +struct exynos_mic {
> > +	struct device *dev;
> > +	void __iomem *reg;
> > +	void __iomem *sysreg;
> > +	struct clk *clks[NUM_CLKS];
> > +
> > +	bool i80_mode;
> > +	struct videomode vm;
> > +	struct drm_encoder *encoder;
> > +	struct drm_bridge bridge;
> > +
> > +	bool enabled;
> > +};
> > +
> > +static void mic_set_path(struct exynos_mic *mic, bool enable)
> > +{
> > +	u32 reg;
> > +
> > +	reg = readl(mic->sysreg + DSD_CFG_MUX);
> > +	if (enable)
> > +		reg |= MIC0_RGB_MUX | MIC0_I80_MUX | MIC0_ON_MUX;
> > +	else
> > +		reg &= ~(MIC0_RGB_MUX | MIC0_I80_MUX |
> > MIC0_ON_MUX);
> 
> exynos_mic has already which video mode - RGB or i80 - is used so it'd
> be reasonable to select one of them to avoid confusing.
> 

Yes. I agree.

> > +
> > +	writel(reg, mic->sysreg + DSD_CFG_MUX);
> > +}
> > +
> > +static int mic_sw_reset(struct exynos_mic *mic)
> > +{
> > +	unsigned int retry = 100;
> > +	int ret;
> > +
> > +	writel(MIC_SW_RST, mic->reg + MIC_OP);
> > +
> > +	while (retry-- > 0) {
> > +		ret = readl(mic->reg + MIC_OP);
> > +		if (!(ret & MIC_SW_RST))
> > +			return 0;
> > +
> > +		udelay(10);
> > +	}
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static void mic_set_porch_timing(struct exynos_mic *mic)
> > +{
> > +	struct videomode vm;
> > +	u32 reg;
> > +
> > +	if (!mic->i80_mode) {
> 
> It'd be better to check it before this function is called. This
> funcion call really is unnecessary in case of i80 mode.
> 

Yes. I agree.

> > +		vm = mic->vm;
> > +
> > +		reg = MIC_V_PULSE_WIDTH(vm.vsync_len) +
> > +			MIC_V_PERIOD_LINE(vm.vsync_len +
> > vm.vactive +
> > +					vm.vback_porch +
> > vm.vfront_porch);
> > +		writel(reg, mic->reg + MIC_V_TIMING_0);
> > +
> > +		reg = MIC_VBP_SIZE(vm.vback_porch) +
> > +
> > MIC_VFP_SIZE(vm.vfront_porch);
> > +		writel(reg, mic->reg + MIC_V_TIMING_1);
> > +
> > +		reg = MIC_V_PULSE_WIDTH(vm.hsync_len) +
> > +			MIC_V_PERIOD_LINE(vm.hsync_len +
> > vm.hactive +
> > +					vm.hback_porch +
> > vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_INPUT_TIMING_0);
> > +
> > +		reg = MIC_VBP_SIZE(vm.hback_porch) +
> > +
> > MIC_VFP_SIZE(vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_INPUT_TIMING_1);
> > +	}
> > +}
> > +
> > +static void mic_set_img_size(struct exynos_mic *mic)
> > +{
> > +	struct videomode *vm = &mic->vm;
> > +	u32 reg;
> > +
> > +	reg = MIC_IMG_H_SIZE(vm->hactive) +
> > +		MIC_IMG_V_SIZE(vm->vactive);
> > +
> > +	writel(reg, mic->reg + MIC_IMG_SIZE);
> > +}
> > +
> > +static void mic_set_output_timing(struct exynos_mic *mic)
> > +{
> > +	struct videomode vm = mic->vm;
> > +	u32 reg, bs_size_2d;
> > +
> > +	DRM_DEBUG("w: %u, h: %u\n", vm.hactive, vm.vactive);
> > +	bs_size_2d = ((vm.hactive >> 2) << 1) + (vm.vactive % 4);
> > +	reg = MIC_BS_SIZE_2D(bs_size_2d);
> > +	writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_2);
> > +
> > +	if (!mic->i80_mode) {
> 
> Ditto.

In this case, MIC_2D_OUTPUT_TIMING_2 must be set whether it is being
used with i80 or RGB interface. MIC_2D_OUTPUT_TIMING_{0/1/2} have
similar names, so I thought that it is good to set them in a function.
Do you think that make a new function for MIC_2D_OUTPUT_TIMING_2?

> 
> > +		reg = MIC_H_PULSE_WIDTH_2D(vm.hsync_len) +
> > +			MIC_H_PERIOD_PIXEL_2D(vm.hsync_len +
> > bs_size_2d +
> > +					vm.hback_porch +
> > vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_0);
> > +
> > +		reg = MIC_HBP_SIZE_2D(vm.hback_porch) +
> > +			MIC_H_PERIOD_PIXEL_2D(vm.hfront_porch);
> > +		writel(reg, mic->reg + MIC_2D_OUTPUT_TIMING_1);
> > +	}
> > +}
> > +
> > +static void mic_set_reg_on(struct exynos_mic *mic, bool enable)
> > +{
> > +	u32 reg = readl(mic->reg + MIC_OP);
> > +
> > +	if (enable) {
> > +		reg &= ~(MIC_MODE_SEL_MASK | MIC_CORE_VER_CONTROL
> > | MIC_PSR_EN);
> > +		reg |= (MIC_CORE_EN | MIC_BS_CHG_OUT | MIC_ON_REG);
> > +
> > +		if (mic->i80_mode)
> > +			reg |= MIC_MODE_SEL_COMMAND_MODE;
> 
> Set explicitly it to video mode in case of RGB mode even though
> MODE_SET field of this register has 0x0 as default.
> 
> i.e.,
>                reg  &= ~MIC_MODE_SEL_COMMAND_MODE;
>                if (mic->i80_mode)
>                         reg |= MIC_MODE_SEL_COMMAND_MODE;
> 

Yes. That will make the code more clear.

> > +	} else {
> > +		reg &= ~MIC_CORE_EN;
> > +	}
> > +
> > +	reg |= MIC_UPD_REG;
> > +	writel(reg, mic->reg + MIC_OP);
> > +}
> > +
> > +static struct device_node *get_remote_node(struct device_node
> > *from, int reg) +{
> > +	struct device_node *endpoint = NULL, *remote_node = NULL;
> > +
> > +	endpoint = of_graph_get_endpoint_by_regs(from, reg, -1);
> > +	if (!endpoint) {
> > +		DRM_ERROR("mic: Failed to find remote port from
> > %s",
> > +				from->full_name);
> > +		goto exit;
> > +	}
> > +
> > +	remote_node = of_graph_get_remote_port_parent(endpoint);
> > +	if (!remote_node) {
> > +		DRM_ERROR("mic: Failed to find remote port parent
> > from %s",
> > +
> > from->full_name);
> > +		goto exit;
> > +	}
> > +
> > +exit:
> > +	of_node_put(endpoint);
> > +	return remote_node;
> > +}
> > +
> > +static int parse_dt(struct exynos_mic *mic)
> > +{
> > +	int ret = 0, i, j;
> > +	struct device_node *remote_node;
> > +	struct device_node *nodes[3];
> > +
> > +	/*
> > +	 * The order of endpoints does matter.
> > +	 * The first node must be for decon and the second one
> > must be for dsi.
> > +	 */
> > +	for (i = 0, j = 0; i < NUM_ENDPOINTS; i++) {
> > +		remote_node = get_remote_node(mic->dev->of_node,
> > i);
> > +		if (!remote_node) {
> > +			ret = -EPIPE;
> > +			goto exit;
> > +		}
> > +		nodes[j++] = remote_node;
> > +
> > +		switch (i) {
> > +		case ENDPOINT_DECON_NODE:
> > +			/* decon node */
> > +			if (of_get_child_by_name(remote_node,
> > +						"i80-if-timings"))
> > +				mic->i80_mode = 1;
> > +
> > +			break;
> > +		case ENDPOINT_DSI_NODE:
> > +			/* panel node */
> > +			remote_node = get_remote_node(remote_node,
> > 1);
> > +			if (!remote_node) {
> > +				ret = -EPIPE;
> > +				goto exit;
> > +			}
> > +			nodes[j++] = remote_node;
> > +
> > +			ret = of_get_videomode(remote_node,
> > +							&mic->vm,
> > 0);
> > +			if (ret) {
> > +				DRM_ERROR("mic: failed to get
> > videomode");
> > +				goto exit;
> > +			}
> > +
> > +			break;
> > +		default:
> > +			DRM_ERROR("mic: Unknown endpoint from
> > MIC");
> > +			break;
> > +		}
> > +	}
> > +
> > +exit:
> > +	while (--j > -1)
> > +		of_node_put(nodes[j]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mic_disable(struct drm_bridge *bridge) { }
> 
> It seems that drm_encoder_disable function of drm_crtc_helper.c
> doesn't check if the disable callback is NULL or not. Anyway, no
> problem but this funtion could be removed with the disable callback
> checking later.
> 

Yes. After the checking code is inserted, it would be better to remove
this function.

> > +
> > +void mic_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct exynos_mic *mic = bridge->driver_private;
> > +	int i;
> > +
> > +	mutex_lock(&mic_mutex);
> > +	if (!mic->enabled)
> > +		goto already_disabled;
> > +
> > +	mic_set_path(mic, 0);
> > +
> > +	for (i = NUM_CLKS - 1; i > -1; i--)
> > +		clk_disable_unprepare(mic->clks[i]);
> > +
> > +	mic->enabled = 0;
> > +
> > +already_disabled:
> > +	mutex_unlock(&mic_mutex);
> > +}
> > +
> > +void mic_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct exynos_mic *mic = bridge->driver_private;
> > +	int ret, i;
> > +
> > +	mutex_lock(&mic_mutex);
> > +	if (mic->enabled)
> > +		goto already_enabled;
> > +
> > +	for (i = 0; i < NUM_CLKS; i++) {
> > +		ret = clk_prepare_enable(mic->clks[i]);
> > +		if (ret < 0) {
> > +			DRM_ERROR("Failed to enable clock (%s)\n",
> > +
> > clk_names[i]);
> > +			goto turn_off_clks;
> > +		}
> > +	}
> > +
> > +	mic_set_path(mic, 1);
> > +
> > +	ret = mic_sw_reset(mic);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to reset\n");
> > +		goto turn_off_clks;
> > +	}
> > +
> > +	mic_set_porch_timing(mic);
> > +	mic_set_img_size(mic);
> > +	mic_set_output_timing(mic);
> > +	mic_set_reg_on(mic, 1);
> > +	mic->enabled = 1;
> > +	mutex_unlock(&mic_mutex);
> > +
> > +	return;
> > +
> > +turn_off_clks:
> > +	while (--i > -1)
> > +		clk_disable_unprepare(mic->clks[i]);
> > +already_enabled:
> > +	mutex_unlock(&mic_mutex);
> > +}
> > +
> > +void mic_enable(struct drm_bridge *bridge) { }
> > +
> > +void mic_destroy(struct drm_bridge *bridge)
> > +{
> > +	struct exynos_mic *mic = bridge->driver_private;
> > +	int i;
> > +
> > +	mutex_lock(&mic_mutex);
> > +	if (!mic->enabled)
> > +		goto already_disabled;
> > +
> > +	for (i = NUM_CLKS - 1; i > -1; i--)
> > +		clk_disable_unprepare(mic->clks[i]);
> > +
> > +already_disabled:
> > +	mutex_unlock(&mic_mutex);
> > +}
> > +
> > +struct drm_bridge_funcs mic_bridge_funcs = {
> > +	.disable = mic_disable,
> > +	.post_disable = mic_post_disable,
> > +	.pre_enable = mic_pre_enable,
> > +	.enable = mic_enable,
> > +};
> > +
> > +int exynos_mic_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct exynos_mic *mic;
> > +	struct resource res;
> > +	int ret, i;
> > +
> > +	mic = devm_kzalloc(dev, sizeof(*mic), GFP_KERNEL);
> > +	if (!mic) {
> > +		DRM_ERROR("mic: Failed to allocate memory for MIC
> > object\n");
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	mic->dev = dev;
> > +
> > +	ret = parse_dt(mic);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = of_address_to_resource(dev->of_node, 0, &res);
> > +	if (ret) {
> > +		DRM_ERROR("mic: Failed to get mem region for
> > MIC\n");
> > +		goto err;
> > +	}
> > +	mic->reg = devm_ioremap(dev, res.start,
> > resource_size(&res));
> > +	if (!mic->reg) {
> > +		DRM_ERROR("mic: Failed to remap for MIC\n");
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	ret = of_address_to_resource(dev->of_node, 1, &res);
> > +	if (ret) {
> > +		DRM_ERROR("mic: Failed to get mem region for
> > MIC\n");
> > +		goto err;
> > +	}
> > +	mic->sysreg = devm_ioremap(dev, res.start,
> > resource_size(&res));
> 
> Consider to use syscon framework to access system registers.
> 

Yes. I will investigate that more and use that if it is possible.

> > +	if (!mic->sysreg) {
> > +		DRM_ERROR("mic: Failed to remap for MIC\n");
> > +		goto err;
> > +	}
> > +
> > +	mic->bridge.funcs = &mic_bridge_funcs;
> > +	mic->bridge.of_node = dev->of_node;
> > +	mic->bridge.driver_private = mic;
> > +	ret = drm_bridge_add(&mic->bridge);
> > +	if (ret) {
> > +		DRM_ERROR("mic: Failed to add MIC to the global
> > bridge list\n");
> > +		goto err;
> > +	}
> > +
> > +	for (i = 0; i < NUM_CLKS; i++) {
> > +		mic->clks[i] = of_clk_get_by_name(dev->of_node,
> > clk_names[i]);
> > +		if (IS_ERR(mic->clks[i])) {
> > +			DRM_ERROR("mic: Failed to get clock
> > (%s)\n",
> > +
> > clk_names[i]);
> > +			ret = PTR_ERR(mic->clks[i]);
> > +			goto err;
> > +		}
> > +	}
> > +
> > +	DRM_INFO("MIC has been probed\n");
> 
> Use DRM_DEBUG_KMS instead.
> 

That would be better.

Thanks for your review.

Best regards,
Hyungwon Hwang

> Thanks,
> Inki Dae
> 
> > +
> > +err:
> > +	return ret;
> > +}
> > +
> > +static int exynos_mic_remove(struct platform_device *pdev)
> > +{
> > +	struct exynos_mic *mic = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	drm_bridge_remove(&mic->bridge);
> > +
> > +	for (i = NUM_CLKS - 1; i > -1; i--)
> > +		clk_put(mic->clks[i]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id exynos_mic_of_match[] = {
> > +	{ .compatible = "samsung,exynos5433-mic" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos_mic_of_match);
> > +
> > +struct platform_driver mic_driver = {
> > +	.probe		= exynos_mic_probe,
> > +	.remove		= exynos_mic_remove,
> > +	.driver		= {
> > +		.name	= "exynos-mic",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = exynos_mic_of_match,
> > +	},
> > +};
> > --
> > 1.9.1
> > 
> > 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang]

This patch adds support for Exynos5433. The goal is achieved by
1. Getting the address of registers from driver data
2. Getting the fixed value for registers from driver data
3. Getting different number of clocks using driver data
4. Getting max frequency of pixel clock from driver data

Signed-off-by: Donghwa Lee <dh09.lee@samsung.com>
Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
Changes for v2:
- change the author of "drm/exynos: dsi: add support for Exynos5433 SoC" to
Hyungwon Hwang by the previous author's will
 .../devicetree/bindings/video/exynos_dsim.txt      |  24 +-
 drivers/gpu/drm/exynos/Kconfig                     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c            | 431 ++++++++++++++-------
 3 files changed, 313 insertions(+), 144 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_dsim.txt b/Documentation/devicetree/bindings/video/exynos_dsim.txt
index ca2b4aa..fea7718 100644
--- a/Documentation/devicetree/bindings/video/exynos_dsim.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dsim.txt
@@ -6,6 +6,7 @@ Required properties:
 		"samsung,exynos4210-mipi-dsi" /* for Exynos4 SoCs */
 		"samsung,exynos4415-mipi-dsi" /* for Exynos4415 SoC */
 		"samsung,exynos5410-mipi-dsi" /* for Exynos5410/5420/5440 SoCs */
+		"samsung,exynos5433-mipi-dsi" /* for Exynos5433 SoCs */
   - reg: physical base address and length of the registers set for the device
   - interrupts: should contain DSI interrupt
   - clocks: list of clock specifiers, must contain an entry for each required
@@ -30,10 +31,19 @@ Video interfaces:
   Device node can contain video interface port nodes according to [2].
   The following are properties specific to those nodes:

-  port node:
-    - reg: (required) can be 0 for input RGB/I80 port or 1 for DSI port;
+  port node inbound:
+    - reg: (required) must be 0.
+  port node outbound:
+    - reg: (required) must be 1.

-  endpoint node of DSI port (reg = 1):
+  endpoint node connected from mic node (reg = 0):
+    - remote-endpoint: specifies the endpoint in mic node. This node is required
+		       for Exynos5433 mipi dsi. So mic can access to panel node
+		       thoughout this dsi node.
+  endpoint node connected to panel node (reg = 1):
+    - remote-endpoint: specifies the endpoint in panel node. This node is
+		       required in all kinds of exynos mipi dsi to represent
+		       the connection between mipi dsi and panel.
     - samsung,burst-clock-frequency: specifies DSI frequency in high-speed burst
       mode
     - samsung,esc-clock-frequency: specifies DSI frequency in escape mode
@@ -72,7 +82,15 @@ Example:
 			#address-cells = <1>;
 			#size-cells = <0>;

+			port@0 {
+				reg = <0>;
+				decon_to_mic: endpoint {
+					remote-endpoint = <&mic_to_decon>;
+				};
+			};
+
 			port@1 {
+				reg = <1>;
 				dsi_ep: endpoint {
 					reg = <0>;
 					samsung,burst-clock-frequency = <500000000>;
diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index a796175..eb35a21 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -47,7 +47,7 @@ config DRM_EXYNOS_DPI

 config DRM_EXYNOS_DSI
 	bool "EXYNOS DRM MIPI-DSI driver support"
-	depends on (DRM_EXYNOS_FIMD || DRM_EXYNOS7_DECON)
+	depends on (DRM_EXYNOS_FIMD || DRM_EXYNOS5433_DECON || DRM_EXYNOS7_DECON)
 	select DRM_MIPI_DSI
 	select DRM_PANEL
 	default n
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 05fe93d..77236ad 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -33,38 +33,6 @@
 /* returns true iff both arguments logically differs */
 #define NEQV(a, b) (!(a) ^ !(b))

-#define DSIM_STATUS_REG		0x0	/* Status register */
-#define DSIM_SWRST_REG		0x4	/* Software reset register */
-#define DSIM_CLKCTRL_REG	0x8	/* Clock control register */
-#define DSIM_TIMEOUT_REG	0xc	/* Time out register */
-#define DSIM_CONFIG_REG		0x10	/* Configuration register */
-#define DSIM_ESCMODE_REG	0x14	/* Escape mode register */
-
-/* Main display image resolution register */
-#define DSIM_MDRESOL_REG	0x18
-#define DSIM_MVPORCH_REG	0x1c	/* Main display Vporch register */
-#define DSIM_MHPORCH_REG	0x20	/* Main display Hporch register */
-#define DSIM_MSYNC_REG		0x24	/* Main display sync area register */
-
-/* Sub display image resolution register */
-#define DSIM_SDRESOL_REG	0x28
-#define DSIM_INTSRC_REG		0x2c	/* Interrupt source register */
-#define DSIM_INTMSK_REG		0x30	/* Interrupt mask register */
-#define DSIM_PKTHDR_REG		0x34	/* Packet Header FIFO register */
-#define DSIM_PAYLOAD_REG	0x38	/* Payload FIFO register */
-#define DSIM_RXFIFO_REG		0x3c	/* Read FIFO register */
-#define DSIM_FIFOTHLD_REG	0x40	/* FIFO threshold level register */
-#define DSIM_FIFOCTRL_REG	0x44	/* FIFO status and control register */
-
-/* FIFO memory AC characteristic register */
-#define DSIM_PLLCTRL_REG	0x4c	/* PLL control register */
-#define DSIM_PHYACCHR_REG	0x54	/* D-PHY AC characteristic register */
-#define DSIM_PHYACCHR1_REG	0x58	/* D-PHY AC characteristic register1 */
-#define DSIM_PHYCTRL_REG	0x5c
-#define DSIM_PHYTIMING_REG	0x64
-#define DSIM_PHYTIMING1_REG	0x68
-#define DSIM_PHYTIMING2_REG	0x6c
-
 /* DSIM_STATUS */
 #define DSIM_STOP_STATE_DAT(x)		(((x) & 0xf) << 0)
 #define DSIM_STOP_STATE_CLK		(1 << 8)
@@ -128,8 +96,8 @@

 /* DSIM_MDRESOL */
 #define DSIM_MAIN_STAND_BY		(1 << 31)
-#define DSIM_MAIN_VRESOL(x)		(((x) & 0x7ff) << 16)
-#define DSIM_MAIN_HRESOL(x)		(((x) & 0X7ff) << 0)
+#define DSIM_MAIN_VRESOL(x, num_bits)	(((x) & ((1 << (num_bits)) - 1)) << 16)
+#define DSIM_MAIN_HRESOL(x, num_bits)	(((x) & ((1 << (num_bits)) - 1)) << 0)

 /* DSIM_MVPORCH */
 #define DSIM_CMD_ALLOW(x)		((x) << 28)
@@ -163,6 +131,7 @@
 #define DSIM_INT_PLL_STABLE		(1 << 31)
 #define DSIM_INT_SW_RST_RELEASE		(1 << 30)
 #define DSIM_INT_SFR_FIFO_EMPTY		(1 << 29)
+#define DSIM_INT_SFR_HDR_FIFO_EMPTY	(1 << 28)
 #define DSIM_INT_BTA			(1 << 25)
 #define DSIM_INT_FRAME_DONE		(1 << 24)
 #define DSIM_INT_RX_TIMEOUT		(1 << 21)
@@ -211,6 +180,8 @@

 /* DSIM_PHYCTRL */
 #define DSIM_PHYCTRL_ULPS_EXIT(x)	(((x) & 0x1ff) << 0)
+#define DSIM_PHYCTRL_B_DPHYCTL_VREG_LP	(1 << 30)
+#define DSIM_PHYCTRL_B_DPHYCTL_SLEW_UP	(1 << 14)

 /* DSIM_PHYTIMING */
 #define DSIM_PHYTIMING_LPX(x)		((x) << 8)
@@ -234,6 +205,12 @@
 #define DSI_XFER_TIMEOUT_MS		100
 #define DSI_RX_FIFO_EMPTY		0x30800002

+#define REG(dsi, reg)	((dsi)->reg_base + dsi->driver_data->regs[(reg)])
+
+static char *clk_names[5] = { "bus_clk", "pll_clk",
+	"phyclk_mipidphy0_bitclkdiv8", "phyclk_mipidphy0_rxclkesc0",
+	"sclk_rgb_vclk_to_dsim0" };
+
 enum exynos_dsi_transfer_type {
 	EXYNOS_DSI_TX,
 	EXYNOS_DSI_RX,
@@ -261,10 +238,15 @@ struct exynos_dsi_transfer {
 #define DSIM_STATE_CMD_LPM		BIT(2)

 struct exynos_dsi_driver_data {
+	unsigned int *regs;
 	unsigned int plltmr_reg;
-
 	unsigned int has_freqband:1;
 	unsigned int has_clklane_stop:1;
+	unsigned int num_clks;
+	unsigned int max_freq;
+	unsigned int wait_for_reset;
+	unsigned int num_bits_resol;
+	unsigned int *values;
 };

 struct exynos_dsi {
@@ -277,8 +259,7 @@ struct exynos_dsi {

 	void __iomem *reg_base;
 	struct phy *phy;
-	struct clk *pll_clk;
-	struct clk *bus_clk;
+	struct clk **clks;
 	struct regulator_bulk_data supplies[2];
 	int irq;
 	int te_gpio;
@@ -309,25 +290,186 @@ static inline struct exynos_dsi *display_to_dsi(struct exynos_drm_display *d)
 	return container_of(d, struct exynos_dsi, display);
 }

+enum regs {
+	DSIM_STATUS_REG,	/* Status register */
+	DSIM_SWRST_REG,		/* Software reset register */
+	DSIM_CLKCTRL_REG,	/* Clock control register */
+	DSIM_TIMEOUT_REG,	/* Time out register */
+	DSIM_CONFIG_REG,	/* Configuration register */
+	DSIM_ESCMODE_REG,	/* Escape mode register */
+	DSIM_MDRESOL_REG,
+	DSIM_MVPORCH_REG,	/* Main display Vporch register */
+	DSIM_MHPORCH_REG,	/* Main display Hporch register */
+	DSIM_MSYNC_REG,		/* Main display sync area register */
+	DSIM_INTSRC_REG,	/* Interrupt source register */
+	DSIM_INTMSK_REG,	/* Interrupt mask register */
+	DSIM_PKTHDR_REG,	/* Packet Header FIFO register */
+	DSIM_PAYLOAD_REG,	/* Payload FIFO register */
+	DSIM_RXFIFO_REG,	/* Read FIFO register */
+	DSIM_FIFOCTRL_REG,	/* FIFO status and control register */
+	DSIM_PLLCTRL_REG,	/* PLL control register */
+	DSIM_PHYCTRL_REG,
+	DSIM_PHYTIMING_REG,
+	DSIM_PHYTIMING1_REG,
+	DSIM_PHYTIMING2_REG,
+	NUM_REGS
+};
+static unsigned int regs[] = {
+	[DSIM_STATUS_REG] =  0x00,
+	[DSIM_SWRST_REG] =  0x04,
+	[DSIM_CLKCTRL_REG] =  0x08,
+	[DSIM_TIMEOUT_REG] =  0x0c,
+	[DSIM_CONFIG_REG] =  0x10,
+	[DSIM_ESCMODE_REG] =  0x14,
+	[DSIM_MDRESOL_REG] =  0x18,
+	[DSIM_MVPORCH_REG] =  0x1c,
+	[DSIM_MHPORCH_REG] =  0x20,
+	[DSIM_MSYNC_REG] =  0x24,
+	[DSIM_INTSRC_REG] =  0x2c,
+	[DSIM_INTMSK_REG] =  0x30,
+	[DSIM_PKTHDR_REG] =  0x34,
+	[DSIM_PAYLOAD_REG] =  0x38,
+	[DSIM_RXFIFO_REG] =  0x3c,
+	[DSIM_FIFOCTRL_REG] =  0x44,
+	[DSIM_PLLCTRL_REG] =  0x4c,
+	[DSIM_PHYCTRL_REG] =  0x5c,
+	[DSIM_PHYTIMING_REG] =  0x64,
+	[DSIM_PHYTIMING1_REG] =  0x68,
+	[DSIM_PHYTIMING2_REG] =  0x6c,
+};
+
+static unsigned int exynos5433_regs[] = {
+	[DSIM_STATUS_REG] = 0x04,
+	[DSIM_SWRST_REG] = 0x0C,
+	[DSIM_CLKCTRL_REG] = 0x10,
+	[DSIM_TIMEOUT_REG] = 0x14,
+	[DSIM_CONFIG_REG] = 0x18,
+	[DSIM_ESCMODE_REG] = 0x1C,
+	[DSIM_MDRESOL_REG] = 0x20,
+	[DSIM_MVPORCH_REG] = 0x24,
+	[DSIM_MHPORCH_REG] = 0x28,
+	[DSIM_MSYNC_REG] = 0x2C,
+	[DSIM_INTSRC_REG] = 0x34,
+	[DSIM_INTMSK_REG] = 0x38,
+	[DSIM_PKTHDR_REG] = 0x3C,
+	[DSIM_PAYLOAD_REG] = 0x40,
+	[DSIM_RXFIFO_REG] = 0x44,
+	[DSIM_FIFOCTRL_REG] = 0x4C,
+	[DSIM_PLLCTRL_REG] = 0x94,
+	[DSIM_PHYCTRL_REG] = 0xA4,
+	[DSIM_PHYTIMING_REG] = 0xB4,
+	[DSIM_PHYTIMING1_REG] = 0xB8,
+	[DSIM_PHYTIMING2_REG] = 0xBC,
+};
+
+enum values {
+	RESET_TYPE,
+	PLL_TIMER,
+	STOP_STATE_CNT,
+	PHYCTRL_ULPS_EXIT,
+	PHYCTRL_VREG_LP,
+	PHYCTRL_SLEW_UP,
+	PHYTIMING_LPX,
+	PHYTIMING_HS_EXIT,
+	PHYTIMING_CLK_PREPARE,
+	PHYTIMING_CLK_ZERO,
+	PHYTIMING_CLK_POST,
+	PHYTIMING_CLK_TRAIL,
+	PHYTIMING_HS_PREPARE,
+	PHYTIMING_HS_ZERO,
+	PHYTIMING_HS_TRAIL
+};
+
+static unsigned int values[] = {
+	[RESET_TYPE] = DSIM_SWRST,
+	[PLL_TIMER] = 500,
+	[STOP_STATE_CNT] = 0xf,
+	[PHYCTRL_ULPS_EXIT] = DSIM_PHYCTRL_ULPS_EXIT(0x0af),
+	[PHYCTRL_VREG_LP] = 0,
+	[PHYCTRL_SLEW_UP] = 0,
+	[PHYTIMING_LPX] = DSIM_PHYTIMING_LPX(0x06),
+	[PHYTIMING_HS_EXIT] = DSIM_PHYTIMING_HS_EXIT(0x0b),
+	[PHYTIMING_CLK_PREPARE] = DSIM_PHYTIMING1_CLK_PREPARE(0x07),
+	[PHYTIMING_CLK_ZERO] = DSIM_PHYTIMING1_CLK_ZERO(0x27),
+	[PHYTIMING_CLK_POST] = DSIM_PHYTIMING1_CLK_POST(0x0d),
+	[PHYTIMING_CLK_TRAIL] = DSIM_PHYTIMING1_CLK_TRAIL(0x08),
+	[PHYTIMING_HS_PREPARE] = DSIM_PHYTIMING2_HS_PREPARE(0x09),
+	[PHYTIMING_HS_ZERO] = DSIM_PHYTIMING2_HS_ZERO(0x0d),
+	[PHYTIMING_HS_TRAIL] = DSIM_PHYTIMING2_HS_TRAIL(0x0b),
+};
+
+static unsigned int exynos5433_values[] = {
+	[RESET_TYPE] = DSIM_FUNCRST,
+	[PLL_TIMER] = 22200,
+	[STOP_STATE_CNT] = 0xa,
+	[PHYCTRL_ULPS_EXIT] = DSIM_PHYCTRL_ULPS_EXIT(0x190),
+	[PHYCTRL_VREG_LP] = DSIM_PHYCTRL_B_DPHYCTL_VREG_LP,
+	[PHYCTRL_SLEW_UP] = DSIM_PHYCTRL_B_DPHYCTL_SLEW_UP,
+	[PHYTIMING_LPX] = DSIM_PHYTIMING_LPX(0x07),
+	[PHYTIMING_HS_EXIT] = DSIM_PHYTIMING_HS_EXIT(0x0c),
+	[PHYTIMING_CLK_PREPARE] = DSIM_PHYTIMING1_CLK_PREPARE(0x09),
+	[PHYTIMING_CLK_ZERO] = DSIM_PHYTIMING1_CLK_ZERO(0x2d),
+	[PHYTIMING_CLK_POST] = DSIM_PHYTIMING1_CLK_POST(0x0e),
+	[PHYTIMING_CLK_TRAIL] = DSIM_PHYTIMING1_CLK_TRAIL(0x09),
+	[PHYTIMING_HS_PREPARE] = DSIM_PHYTIMING2_HS_PREPARE(0x0b),
+	[PHYTIMING_HS_ZERO] = DSIM_PHYTIMING2_HS_ZERO(0x10),
+	[PHYTIMING_HS_TRAIL] = DSIM_PHYTIMING2_HS_TRAIL(0x0c),
+};
+
 static struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
+	.regs = regs,
 	.plltmr_reg = 0x50,
 	.has_freqband = 1,
 	.has_clklane_stop = 1,
+	.num_clks = 2,
+	.max_freq = 1000,
+	.wait_for_reset = 1,
+	.num_bits_resol = 11,
+	.values = values,
 };

 static struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
+	.regs = regs,
 	.plltmr_reg = 0x50,
 	.has_freqband = 1,
 	.has_clklane_stop = 1,
+	.num_clks = 2,
+	.max_freq = 1000,
+	.wait_for_reset = 1,
+	.num_bits_resol = 11,
+	.values = values,
 };

 static struct exynos_dsi_driver_data exynos4415_dsi_driver_data = {
+	.regs = regs,
 	.plltmr_reg = 0x58,
 	.has_clklane_stop = 1,
+	.num_clks = 2,
+	.max_freq = 1000,
+	.wait_for_reset = 1,
+	.num_bits_resol = 11,
+	.values = values,
 };

 static struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
+	.regs = regs,
 	.plltmr_reg = 0x58,
+	.num_clks = 2,
+	.max_freq = 1000,
+	.wait_for_reset = 1,
+	.num_bits_resol = 11,
+	.values = values,
+};
+
+static struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
+	.regs = exynos5433_regs,
+	.plltmr_reg = 0xa0,
+	.has_clklane_stop = 1,
+	.num_clks = 5,
+	.max_freq = 1500,
+	.wait_for_reset = 0,
+	.num_bits_resol = 12,
+	.values = exynos5433_values,
 };

 static struct of_device_id exynos_dsi_of_match[] = {
@@ -339,6 +481,8 @@ static struct of_device_id exynos_dsi_of_match[] = {
 	  .data = &exynos4415_dsi_driver_data },
 	{ .compatible = "samsung,exynos5410-mipi-dsi",
 	  .data = &exynos5_dsi_driver_data },
+	{ .compatible = "samsung,exynos5433-mipi-dsi",
+	  .data = &exynos5433_dsi_driver_data },
 	{ }
 };

@@ -361,8 +505,10 @@ static void exynos_dsi_wait_for_reset(struct exynos_dsi *dsi)

 static void exynos_dsi_reset(struct exynos_dsi *dsi)
 {
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
+
 	reinit_completion(&dsi->completed);
-	writel(DSIM_SWRST, dsi->reg_base + DSIM_SWRST_REG);
+	writel(driver_data->values[RESET_TYPE], REG(dsi, DSIM_SWRST_REG));
 }

 #ifndef MHZ
@@ -372,6 +518,7 @@ static void exynos_dsi_reset(struct exynos_dsi *dsi)
 static unsigned long exynos_dsi_pll_find_pms(struct exynos_dsi *dsi,
 		unsigned long fin, unsigned long fout, u8 *p, u16 *m, u8 *s)
 {
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
 	unsigned long best_freq = 0;
 	u32 min_delta = 0xffffffff;
 	u8 p_min, p_max;
@@ -395,7 +542,8 @@ static unsigned long exynos_dsi_pll_find_pms(struct exynos_dsi *dsi,

 			tmp = (u64)_m * fin;
 			do_div(tmp, _p);
-			if (tmp < 500 * MHZ || tmp > 1000 * MHZ)
+			if (tmp < 500 * MHZ ||
+					tmp > driver_data->max_freq * MHZ)
 				continue;

 			tmp = (u64)_m * fin;
@@ -431,15 +579,11 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
 	u16 m;
 	u32 reg;

-	clk_set_rate(dsi->pll_clk, dsi->pll_clk_rate);
-
-	fin = clk_get_rate(dsi->pll_clk);
-	if (!fin) {
-		dev_err(dsi->dev, "failed to get PLL clock frequency\n");
-		return 0;
-	}
-
-	dev_dbg(dsi->dev, "PLL input frequency: %lu\n", fin);
+	/*
+	 * The input PLL clock for MIPI DSI in Exynos5433 seems to be fixed
+	 * by OSC CLK.
+	 */
+	fin = 24 * MHZ;

 	fout = exynos_dsi_pll_find_pms(dsi, fin, freq, &p, &m, &s);
 	if (!fout) {
@@ -449,7 +593,8 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
 	}
 	dev_dbg(dsi->dev, "PLL freq %lu, (p %d, m %d, s %d)\n", fout, p, m, s);

-	writel(500, dsi->reg_base + driver_data->plltmr_reg);
+	writel(driver_data->values[PLL_TIMER],
+			dsi->reg_base + driver_data->plltmr_reg);

 	reg = DSIM_PLL_EN | DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s);

@@ -471,7 +616,7 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
 		reg |= DSIM_FREQ_BAND(band);
 	}

-	writel(reg, dsi->reg_base + DSIM_PLLCTRL_REG);
+	writel(reg, REG(dsi, DSIM_PLLCTRL_REG));

 	timeout = 1000;
 	do {
@@ -479,7 +624,7 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
 			dev_err(dsi->dev, "PLL failed to stabilize\n");
 			return 0;
 		}
-		reg = readl(dsi->reg_base + DSIM_STATUS_REG);
+		reg = readl(REG(dsi, DSIM_STATUS_REG));
 	} while ((reg & DSIM_PLL_STABLE) == 0);

 	return fout;
@@ -491,6 +636,8 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
 	unsigned long esc_div;
 	u32 reg;

+	reg = readl(REG(dsi, DSIM_STATUS_REG));
+
 	hs_clk = exynos_dsi_set_pll(dsi, dsi->burst_clk_rate);
 	if (!hs_clk) {
 		dev_err(dsi->dev, "failed to configure DSI PLL\n");
@@ -509,7 +656,7 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
 	dev_dbg(dsi->dev, "hs_clk = %lu, byte_clk = %lu, esc_clk = %lu\n",
 		hs_clk, byte_clk, esc_clk);

-	reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
+	reg = readl(REG(dsi, DSIM_CLKCTRL_REG));
 	reg &= ~(DSIM_ESC_PRESCALER_MASK | DSIM_LANE_ESC_CLK_EN_CLK
 			| DSIM_LANE_ESC_CLK_EN_DATA_MASK | DSIM_PLL_BYPASS
 			| DSIM_BYTE_CLK_SRC_MASK);
@@ -519,7 +666,8 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
 			| DSIM_LANE_ESC_CLK_EN_DATA(BIT(dsi->lanes) - 1)
 			| DSIM_BYTE_CLK_SRC(0)
 			| DSIM_TX_REQUEST_HSCLK;
-	writel(reg, dsi->reg_base + DSIM_CLKCTRL_REG);
+
+	writel(reg, REG(dsi, DSIM_CLKCTRL_REG));

 	return 0;
 }
@@ -527,22 +675,24 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
 static void exynos_dsi_set_phy_ctrl(struct exynos_dsi *dsi)
 {
 	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
+	unsigned int *values = driver_data->values;
 	u32 reg;

 	if (driver_data->has_freqband)
 		return;

 	/* B D-PHY: D-PHY Master & Slave Analog Block control */
-	reg = DSIM_PHYCTRL_ULPS_EXIT(0x0af);
-	writel(reg, dsi->reg_base + DSIM_PHYCTRL_REG);
+	reg = values[PHYCTRL_ULPS_EXIT] | values[PHYCTRL_VREG_LP] |
+		values[PHYCTRL_SLEW_UP];
+	writel(reg, REG(dsi, DSIM_PHYCTRL_REG));

 	/*
 	 * T LPX: Transmitted length of any Low-Power state period
 	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
 	 *	burst
 	 */
-	reg = DSIM_PHYTIMING_LPX(0x06) | DSIM_PHYTIMING_HS_EXIT(0x0b);
-	writel(reg, dsi->reg_base + DSIM_PHYTIMING_REG);
+	reg = values[PHYTIMING_LPX] | values[PHYTIMING_HS_EXIT];
+	writel(reg, REG(dsi, DSIM_PHYTIMING_REG));

 	/*
 	 * T CLK-PREPARE: Time that the transmitter drives the Clock Lane LP-00
@@ -557,11 +707,10 @@ static void exynos_dsi_set_phy_ctrl(struct exynos_dsi *dsi)
 	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
 	 *	the last payload clock bit of a HS transmission burst
 	 */
-	reg = DSIM_PHYTIMING1_CLK_PREPARE(0x07) |
-			DSIM_PHYTIMING1_CLK_ZERO(0x27) |
-			DSIM_PHYTIMING1_CLK_POST(0x0d) |
-			DSIM_PHYTIMING1_CLK_TRAIL(0x08);
-	writel(reg, dsi->reg_base + DSIM_PHYTIMING1_REG);
+	reg = values[PHYTIMING_CLK_PREPARE] | values[PHYTIMING_CLK_ZERO] |
+		values[PHYTIMING_CLK_POST] | values[PHYTIMING_CLK_TRAIL];
+
+	writel(reg, REG(dsi, DSIM_PHYTIMING1_REG));

 	/*
 	 * T HS-PREPARE: Time that the transmitter drives the Data Lane LP-00
@@ -572,23 +721,23 @@ static void exynos_dsi_set_phy_ctrl(struct exynos_dsi *dsi)
 	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
 	 *	state after last payload data bit of a HS transmission burst
 	 */
-	reg = DSIM_PHYTIMING2_HS_PREPARE(0x09) | DSIM_PHYTIMING2_HS_ZERO(0x0d) |
-			DSIM_PHYTIMING2_HS_TRAIL(0x0b);
-	writel(reg, dsi->reg_base + DSIM_PHYTIMING2_REG);
+	reg = values[PHYTIMING_HS_PREPARE] | values[PHYTIMING_HS_ZERO] |
+		values[PHYTIMING_HS_TRAIL];
+	writel(reg, REG(dsi, DSIM_PHYTIMING2_REG));
 }

 static void exynos_dsi_disable_clock(struct exynos_dsi *dsi)
 {
 	u32 reg;

-	reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
+	reg = readl(REG(dsi, DSIM_CLKCTRL_REG));
 	reg &= ~(DSIM_LANE_ESC_CLK_EN_CLK | DSIM_LANE_ESC_CLK_EN_DATA_MASK
 			| DSIM_ESC_CLKEN | DSIM_BYTE_CLKEN);
-	writel(reg, dsi->reg_base + DSIM_CLKCTRL_REG);
+	writel(reg, REG(dsi, DSIM_CLKCTRL_REG));

-	reg = readl(dsi->reg_base + DSIM_PLLCTRL_REG);
+	reg = readl(REG(dsi, DSIM_PLLCTRL_REG));
 	reg &= ~DSIM_PLL_EN;
-	writel(reg, dsi->reg_base + DSIM_PLLCTRL_REG);
+	writel(reg, REG(dsi, DSIM_PLLCTRL_REG));
 }

 static int exynos_dsi_init_link(struct exynos_dsi *dsi)
@@ -599,15 +748,14 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 	u32 lanes_mask;

 	/* Initialize FIFO pointers */
-	reg = readl(dsi->reg_base + DSIM_FIFOCTRL_REG);
+	reg = readl(REG(dsi, DSIM_FIFOCTRL_REG));
 	reg &= ~0x1f;
-	writel(reg, dsi->reg_base + DSIM_FIFOCTRL_REG);
+	writel(reg, REG(dsi, DSIM_FIFOCTRL_REG));

 	usleep_range(9000, 11000);

 	reg |= 0x1f;
-	writel(reg, dsi->reg_base + DSIM_FIFOCTRL_REG);
-
+	writel(reg, REG(dsi, DSIM_FIFOCTRL_REG));
 	usleep_range(9000, 11000);

 	/* DSI configuration */
@@ -666,14 +814,15 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)

 	reg |= DSIM_NUM_OF_DATA_LANE(dsi->lanes - 1);

-	writel(reg, dsi->reg_base + DSIM_CONFIG_REG);
+	writel(reg, REG(dsi, DSIM_CONFIG_REG));

 	reg |= DSIM_LANE_EN_CLK;
-	writel(reg, dsi->reg_base + DSIM_CONFIG_REG);
+	writel(reg, REG(dsi, DSIM_CONFIG_REG));

 	lanes_mask = BIT(dsi->lanes) - 1;
 	reg |= DSIM_LANE_EN(lanes_mask);
-	writel(reg, dsi->reg_base + DSIM_CONFIG_REG);
+
+	writel(reg, REG(dsi, DSIM_CONFIG_REG));

 	/*
 	 * Use non-continuous clock mode if the periparal wants and
@@ -686,7 +835,7 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 	if (driver_data->has_clklane_stop &&
 			dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
 		reg |= DSIM_CLKLANE_STOP;
-		writel(reg, dsi->reg_base + DSIM_CONFIG_REG);
+		writel(reg, REG(dsi, DSIM_CONFIG_REG));
 	}

 	/* Check clock and data lane state are stop state */
@@ -697,19 +846,19 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 			return -EFAULT;
 		}

-		reg = readl(dsi->reg_base + DSIM_STATUS_REG);
+		reg = readl(REG(dsi, DSIM_STATUS_REG));
 		if ((reg & DSIM_STOP_STATE_DAT(lanes_mask))
 		    != DSIM_STOP_STATE_DAT(lanes_mask))
 			continue;
 	} while (!(reg & (DSIM_STOP_STATE_CLK | DSIM_TX_READY_HS_CLK)));

-	reg = readl(dsi->reg_base + DSIM_ESCMODE_REG);
+	reg = readl(REG(dsi, DSIM_ESCMODE_REG));
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
-	reg |= DSIM_STOP_STATE_CNT(0xf);
-	writel(reg, dsi->reg_base + DSIM_ESCMODE_REG);
+	reg |= DSIM_STOP_STATE_CNT(driver_data->values[STOP_STATE_CNT]);
+	writel(reg, REG(dsi, DSIM_ESCMODE_REG));

 	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
-	writel(reg, dsi->reg_base + DSIM_TIMEOUT_REG);
+	writel(reg, REG(dsi, DSIM_TIMEOUT_REG));

 	return 0;
 }
@@ -717,25 +866,27 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
 {
 	struct videomode *vm = &dsi->vm;
+	unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
 	u32 reg;

 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
 		reg = DSIM_CMD_ALLOW(0xf)
 			| DSIM_STABLE_VFP(vm->vfront_porch)
 			| DSIM_MAIN_VBP(vm->vback_porch);
-		writel(reg, dsi->reg_base + DSIM_MVPORCH_REG);
+		writel(reg, REG(dsi, DSIM_MVPORCH_REG));

 		reg = DSIM_MAIN_HFP(vm->hfront_porch)
 			| DSIM_MAIN_HBP(vm->hback_porch);
-		writel(reg, dsi->reg_base + DSIM_MHPORCH_REG);
+		writel(reg, REG(dsi, DSIM_MHPORCH_REG));

 		reg = DSIM_MAIN_VSA(vm->vsync_len)
 			| DSIM_MAIN_HSA(vm->hsync_len);
-		writel(reg, dsi->reg_base + DSIM_MSYNC_REG);
+		writel(reg, REG(dsi, DSIM_MSYNC_REG));
 	}
+	reg =  DSIM_MAIN_HRESOL(vm->hactive, num_bits_resol) |
+		DSIM_MAIN_VRESOL(vm->vactive, num_bits_resol);

-	reg = DSIM_MAIN_HRESOL(vm->hactive) | DSIM_MAIN_VRESOL(vm->vactive);
-	writel(reg, dsi->reg_base + DSIM_MDRESOL_REG);
+	writel(reg, REG(dsi, DSIM_MDRESOL_REG));

 	dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive);
 }
@@ -744,12 +895,12 @@ static void exynos_dsi_set_display_enable(struct exynos_dsi *dsi, bool enable)
 {
 	u32 reg;

-	reg = readl(dsi->reg_base + DSIM_MDRESOL_REG);
+	reg = readl(REG(dsi, DSIM_MDRESOL_REG));
 	if (enable)
 		reg |= DSIM_MAIN_STAND_BY;
 	else
 		reg &= ~DSIM_MAIN_STAND_BY;
-	writel(reg, dsi->reg_base + DSIM_MDRESOL_REG);
+	writel(reg, REG(dsi, DSIM_MDRESOL_REG));
 }

 static int exynos_dsi_wait_for_hdr_fifo(struct exynos_dsi *dsi)
@@ -757,7 +908,7 @@ static int exynos_dsi_wait_for_hdr_fifo(struct exynos_dsi *dsi)
 	int timeout = 2000;

 	do {
-		u32 reg = readl(dsi->reg_base + DSIM_FIFOCTRL_REG);
+		u32 reg = readl(REG(dsi, DSIM_FIFOCTRL_REG));

 		if (!(reg & DSIM_SFR_HEADER_FULL))
 			return 0;
@@ -771,22 +922,22 @@ static int exynos_dsi_wait_for_hdr_fifo(struct exynos_dsi *dsi)

 static void exynos_dsi_set_cmd_lpm(struct exynos_dsi *dsi, bool lpm)
 {
-	u32 v = readl(dsi->reg_base + DSIM_ESCMODE_REG);
+	u32 v = readl(REG(dsi, DSIM_ESCMODE_REG));

 	if (lpm)
 		v |= DSIM_CMD_LPDT_LP;
 	else
 		v &= ~DSIM_CMD_LPDT_LP;

-	writel(v, dsi->reg_base + DSIM_ESCMODE_REG);
+	writel(v, REG(dsi, DSIM_ESCMODE_REG));
 }

 static void exynos_dsi_force_bta(struct exynos_dsi *dsi)
 {
-	u32 v = readl(dsi->reg_base + DSIM_ESCMODE_REG);
+	u32 v = readl(REG(dsi, DSIM_ESCMODE_REG));

 	v |= DSIM_FORCE_BTA;
-	writel(v, dsi->reg_base + DSIM_ESCMODE_REG);
+	writel(v, REG(dsi, DSIM_ESCMODE_REG));
 }

 static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi,
@@ -810,7 +961,7 @@ static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi,
 	while (length >= 4) {
 		reg = (payload[3] << 24) | (payload[2] << 16)
 					| (payload[1] << 8) | payload[0];
-		writel(reg, dsi->reg_base + DSIM_PAYLOAD_REG);
+		writel(reg, REG(dsi, DSIM_PAYLOAD_REG));
 		payload += 4;
 		length -= 4;
 	}
@@ -825,7 +976,7 @@ static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi,
 		/* Fall through */
 	case 1:
 		reg |= payload[0];
-		writel(reg, dsi->reg_base + DSIM_PAYLOAD_REG);
+		writel(reg, REG(dsi, DSIM_PAYLOAD_REG));
 		break;
 	case 0:
 		/* Do nothing */
@@ -848,7 +999,7 @@ static void exynos_dsi_send_to_fifo(struct exynos_dsi *dsi,
 		dsi->state ^= DSIM_STATE_CMD_LPM;
 	}

-	writel(reg, dsi->reg_base + DSIM_PKTHDR_REG);
+	writel(reg, REG(dsi, DSIM_PKTHDR_REG));

 	if (xfer->flags & MIPI_DSI_MSG_REQ_ACK)
 		exynos_dsi_force_bta(dsi);
@@ -864,7 +1015,7 @@ static void exynos_dsi_read_from_fifo(struct exynos_dsi *dsi,
 	u32 reg;

 	if (first) {
-		reg = readl(dsi->reg_base + DSIM_RXFIFO_REG);
+		reg = readl(REG(dsi, DSIM_RXFIFO_REG));

 		switch (reg & 0x3f) {
 		case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_2BYTE:
@@ -903,7 +1054,7 @@ static void exynos_dsi_read_from_fifo(struct exynos_dsi *dsi,

 	/* Receive payload */
 	while (length >= 4) {
-		reg = readl(dsi->reg_base + DSIM_RXFIFO_REG);
+		reg = readl(REG(dsi, DSIM_RXFIFO_REG));
 		payload[0] = (reg >>  0) & 0xff;
 		payload[1] = (reg >>  8) & 0xff;
 		payload[2] = (reg >> 16) & 0xff;
@@ -913,7 +1064,7 @@ static void exynos_dsi_read_from_fifo(struct exynos_dsi *dsi,
 	}

 	if (length) {
-		reg = readl(dsi->reg_base + DSIM_RXFIFO_REG);
+		reg = readl(REG(dsi, DSIM_RXFIFO_REG));
 		switch (length) {
 		case 3:
 			payload[2] = (reg >> 16) & 0xff;
@@ -932,7 +1083,7 @@ static void exynos_dsi_read_from_fifo(struct exynos_dsi *dsi,
 clear_fifo:
 	length = DSI_RX_FIFO_SIZE / 4;
 	do {
-		reg = readl(dsi->reg_base + DSIM_RXFIFO_REG);
+		reg = readl(REG(dsi, DSIM_RXFIFO_REG));
 		if (reg == DSI_RX_FIFO_EMPTY)
 			break;
 	} while (--length);
@@ -1088,23 +1239,27 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
 	struct exynos_dsi *dsi = dev_id;
 	u32 status;

-	status = readl(dsi->reg_base + DSIM_INTSRC_REG);
+	status = readl(REG(dsi, DSIM_INTSRC_REG));
 	if (!status) {
 		static unsigned long int j;
 		if (printk_timed_ratelimit(&j, 500))
 			dev_warn(dsi->dev, "spurious interrupt\n");
 		return IRQ_HANDLED;
 	}
-	writel(status, dsi->reg_base + DSIM_INTSRC_REG);
+	writel(status, REG(dsi, DSIM_INTSRC_REG));

 	if (status & DSIM_INT_SW_RST_RELEASE) {
-		u32 mask = ~(DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY);
-		writel(mask, dsi->reg_base + DSIM_INTMSK_REG);
+		u32 mask = ~(DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
+			DSIM_INT_SFR_HDR_FIFO_EMPTY | DSIM_INT_FRAME_DONE |
+			DSIM_INT_RX_ECC_ERR | DSIM_INT_SW_RST_RELEASE);
+
+		writel(mask, REG(dsi, DSIM_INTMSK_REG));
 		complete(&dsi->completed);
 		return IRQ_HANDLED;
 	}

-	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
+	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
+			DSIM_INT_FRAME_DONE | DSIM_INT_PLL_STABLE)))
 		return IRQ_HANDLED;

 	if (exynos_dsi_transfer_finish(dsi))
@@ -1142,10 +1297,13 @@ static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)

 static int exynos_dsi_init(struct exynos_dsi *dsi)
 {
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
+
 	exynos_dsi_reset(dsi);
 	exynos_dsi_enable_irq(dsi);
 	exynos_dsi_enable_clock(dsi);
-	exynos_dsi_wait_for_reset(dsi);
+	if (driver_data->wait_for_reset)
+		exynos_dsi_wait_for_reset(dsi);
 	exynos_dsi_set_phy_ctrl(dsi);
 	exynos_dsi_init_link(dsi);

@@ -1294,7 +1452,8 @@ static const struct mipi_dsi_host_ops exynos_dsi_ops = {

 static int exynos_dsi_poweron(struct exynos_dsi *dsi)
 {
-	int ret;
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
+	int ret, i;

 	ret = regulator_bulk_enable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
 	if (ret < 0) {
@@ -1302,31 +1461,24 @@ static int exynos_dsi_poweron(struct exynos_dsi *dsi)
 		return ret;
 	}

-	ret = clk_prepare_enable(dsi->bus_clk);
-	if (ret < 0) {
-		dev_err(dsi->dev, "cannot enable bus clock %d\n", ret);
-		goto err_bus_clk;
-	}
-
-	ret = clk_prepare_enable(dsi->pll_clk);
-	if (ret < 0) {
-		dev_err(dsi->dev, "cannot enable pll clock %d\n", ret);
-		goto err_pll_clk;
+	for (i = 0; i < driver_data->num_clks; i++) {
+		ret = clk_prepare_enable(dsi->clks[i]);
+		if (ret < 0)
+			goto err_clk;
 	}

 	ret = phy_power_on(dsi->phy);
 	if (ret < 0) {
 		dev_err(dsi->dev, "cannot enable phy %d\n", ret);
-		goto err_phy;
+		goto err_clk;
 	}

 	return 0;

-err_phy:
-	clk_disable_unprepare(dsi->pll_clk);
-err_pll_clk:
-	clk_disable_unprepare(dsi->bus_clk);
-err_bus_clk:
+err_clk:
+	while (--i > -1)
+		clk_disable_unprepare(dsi->clks[i]);
+
 	regulator_bulk_disable(ARRAY_SIZE(dsi->supplies), dsi->supplies);

 	return ret;
@@ -1334,7 +1486,8 @@ err_bus_clk:

 static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
 {
-	int ret;
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
+	int ret, i;

 	usleep_range(10000, 20000);

@@ -1350,8 +1503,8 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)

 	phy_power_off(dsi->phy);

-	clk_disable_unprepare(dsi->pll_clk);
-	clk_disable_unprepare(dsi->bus_clk);
+	for (i = driver_data->num_clks - 1; i > -1; i--)
+		clk_disable_unprepare(dsi->clks[i]);

 	ret = regulator_bulk_disable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
 	if (ret < 0)
@@ -1680,7 +1833,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct exynos_dsi *dsi;
-	int ret;
+	int ret, i;

 	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
 	if (!dsi)
@@ -1720,18 +1873,16 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}

-	dsi->pll_clk = devm_clk_get(dev, "pll_clk");
-	if (IS_ERR(dsi->pll_clk)) {
-		dev_info(dev, "failed to get dsi pll input clock\n");
-		ret = PTR_ERR(dsi->pll_clk);
-		goto err_del_component;
-	}
-
-	dsi->bus_clk = devm_clk_get(dev, "bus_clk");
-	if (IS_ERR(dsi->bus_clk)) {
-		dev_info(dev, "failed to get dsi bus clock\n");
-		ret = PTR_ERR(dsi->bus_clk);
-		goto err_del_component;
+	dsi->clks = devm_kzalloc(dev,
+				sizeof(*dsi->clks) * dsi->driver_data->num_clks,
+				GFP_KERNEL);
+	for (i = 0; i < dsi->driver_data->num_clks; i++) {
+		dsi->clks[i] = devm_clk_get(dev, clk_names[i]);
+		if (IS_ERR(dsi->clks[i])) {
+			dev_info(dev, "failed to get dsi pll input clock\n");
+			ret = PTR_ERR(dsi->clks[i]);
+			goto err_del_component;
+		}
 	}

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
--
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Daniel Stone]

Hi,

On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@samsung.com> wrote:
> +#define REG(dsi, reg)  ((dsi)->reg_base + dsi->driver_data->regs[(reg)])

This seems like a good change in general, but please split it up: it
makes bisection much easier if you have one patch which adds no
functionality and should have exactly the same behaviour, and then
another patch which introduces your changes.

> @@ -431,15 +579,11 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
>         u16 m;
>         u32 reg;
>
> -       clk_set_rate(dsi->pll_clk, dsi->pll_clk_rate);
> -
> -       fin = clk_get_rate(dsi->pll_clk);
> -       if (!fin) {
> -               dev_err(dsi->dev, "failed to get PLL clock frequency\n");
> -               return 0;
> -       }
> -
> -       dev_dbg(dsi->dev, "PLL input frequency: %lu\n", fin);
> +       /*
> +        * The input PLL clock for MIPI DSI in Exynos5433 seems to be fixed
> +        * by OSC CLK.
> +        */
> +       fin = 24 * MHZ;

Er, is this always true on other platforms as well? Shouldn't this be
a part of the DeviceTree description?

> @@ -509,7 +656,7 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
>         dev_dbg(dsi->dev, "hs_clk = %lu, byte_clk = %lu, esc_clk = %lu\n",
>                 hs_clk, byte_clk, esc_clk);
>
> -       reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
> +       reg = readl(REG(dsi, DSIM_CLKCTRL_REG));

Instead of this readl(REG()) pattern you have everywhere, maybe it
would be easier to introduce a dsi_read_reg(dsi, reg_enum_value)
helper, and the same for write_reg.

> @@ -1720,18 +1873,16 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>                 return -EPROBE_DEFER;
>         }
>
> -       dsi->pll_clk = devm_clk_get(dev, "pll_clk");
> -       if (IS_ERR(dsi->pll_clk)) {
> -               dev_info(dev, "failed to get dsi pll input clock\n");
> -               ret = PTR_ERR(dsi->pll_clk);
> -               goto err_del_component;
> -       }
> -
> -       dsi->bus_clk = devm_clk_get(dev, "bus_clk");
> -       if (IS_ERR(dsi->bus_clk)) {
> -               dev_info(dev, "failed to get dsi bus clock\n");
> -               ret = PTR_ERR(dsi->bus_clk);
> -               goto err_del_component;
> +       dsi->clks = devm_kzalloc(dev,
> +                               sizeof(*dsi->clks) * dsi->driver_data->num_clks,
> +                               GFP_KERNEL);
> +       for (i = 0; i < dsi->driver_data->num_clks; i++) {
> +               dsi->clks[i] = devm_clk_get(dev, clk_names[i]);
> +               if (IS_ERR(dsi->clks[i])) {
> +                       dev_info(dev, "failed to get dsi pll input clock\n");

This error message seems wrong; it should contain the name of the
actual failing clock.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang]

Dear Daniel,

On Wed, 18 Mar 2015 09:52:33 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@samsung.com>
> wrote:
> > +#define REG(dsi, reg)  ((dsi)->reg_base +
> > dsi->driver_data->regs[(reg)])
> 
> This seems like a good change in general, but please split it up: it
> makes bisection much easier if you have one patch which adds no
> functionality and should have exactly the same behaviour, and then
> another patch which introduces your changes.

Yes. I agree with you.

> 
> > @@ -431,15 +579,11 @@ static unsigned long
> > exynos_dsi_set_pll(struct exynos_dsi *dsi, u16 m;
> >         u32 reg;
> >
> > -       clk_set_rate(dsi->pll_clk, dsi->pll_clk_rate);
> > -
> > -       fin = clk_get_rate(dsi->pll_clk);
> > -       if (!fin) {
> > -               dev_err(dsi->dev, "failed to get PLL clock
> > frequency\n");
> > -               return 0;
> > -       }
> > -
> > -       dev_dbg(dsi->dev, "PLL input frequency: %lu\n", fin);
> > +       /*
> > +        * The input PLL clock for MIPI DSI in Exynos5433 seems to
> > be fixed
> > +        * by OSC CLK.
> > +        */
> > +       fin = 24 * MHZ;
> 
> Er, is this always true on other platforms as well? Shouldn't this be
> a part of the DeviceTree description?

I forgot to change the comment in development. Finally it is found that
all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I will remove
the comment, but remain the code as it is.

> 
> > @@ -509,7 +656,7 @@ static int exynos_dsi_enable_clock(struct
> > exynos_dsi *dsi) dev_dbg(dsi->dev, "hs_clk = %lu, byte_clk = %lu,
> > esc_clk = %lu\n", hs_clk, byte_clk, esc_clk);
> >
> > -       reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
> > +       reg = readl(REG(dsi, DSIM_CLKCTRL_REG));
> 
> Instead of this readl(REG()) pattern you have everywhere, maybe it
> would be easier to introduce a dsi_read_reg(dsi, reg_enum_value)
> helper, and the same for write_reg.

I think that it can make the code more readable. I agree.

> 
> > @@ -1720,18 +1873,16 @@ static int exynos_dsi_probe(struct
> > platform_device *pdev) return -EPROBE_DEFER;
> >         }
> >
> > -       dsi->pll_clk = devm_clk_get(dev, "pll_clk");
> > -       if (IS_ERR(dsi->pll_clk)) {
> > -               dev_info(dev, "failed to get dsi pll input
> > clock\n");
> > -               ret = PTR_ERR(dsi->pll_clk);
> > -               goto err_del_component;
> > -       }
> > -
> > -       dsi->bus_clk = devm_clk_get(dev, "bus_clk");
> > -       if (IS_ERR(dsi->bus_clk)) {
> > -               dev_info(dev, "failed to get dsi bus clock\n");
> > -               ret = PTR_ERR(dsi->bus_clk);
> > -               goto err_del_component;
> > +       dsi->clks = devm_kzalloc(dev,
> > +                               sizeof(*dsi->clks) *
> > dsi->driver_data->num_clks,
> > +                               GFP_KERNEL);
> > +       for (i = 0; i < dsi->driver_data->num_clks; i++) {
> > +               dsi->clks[i] = devm_clk_get(dev, clk_names[i]);
> > +               if (IS_ERR(dsi->clks[i])) {
> > +                       dev_info(dev, "failed to get dsi pll input
> > clock\n");
> 
> This error message seems wrong; it should contain the name of the
> actual failing clock.

Oh. I forgot. I will change it.

Thanks for your review. I will send it again with the changes you
suggested.

> 
> Cheers,
> Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Daniel Stone]

Hi Hyungwon,

On 19 March 2015 at 01:02, Hyungwon Hwang <human.hwang@samsung.com> wrote:
>> > +       /*
>> > +        * The input PLL clock for MIPI DSI in Exynos5433 seems to
>> > be fixed
>> > +        * by OSC CLK.
>> > +        */
>> > +       fin = 24 * MHZ;
>>
>> Er, is this always true on other platforms as well? Shouldn't this be
>> a part of the DeviceTree description?
>
> I forgot to change the comment in development. Finally it is found that
> all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I will remove
> the comment, but remain the code as it is.

Fair enough. Should pll_clk be removed from the DT description then,
if it's fixed to the oscillator?

> Thanks for your review. I will send it again with the changes you
> suggested.

Thanks very much!

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang]

Dear Daniel,

On Thu, 19 Mar 2015 01:13:21 +0000
Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw@public.gmane.org> wrote:

> Hi Hyungwon,
> 
> On 19 March 2015 at 01:02, Hyungwon Hwang <human.hwang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> wrote:
> >> > +       /*
> >> > +        * The input PLL clock for MIPI DSI in Exynos5433 seems
> >> > to be fixed
> >> > +        * by OSC CLK.
> >> > +        */
> >> > +       fin = 24 * MHZ;
> >>
> >> Er, is this always true on other platforms as well? Shouldn't this
> >> be a part of the DeviceTree description?
> >
> > I forgot to change the comment in development. Finally it is found
> > that all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I
> > will remove the comment, but remain the code as it is.
> 
> Fair enough. Should pll_clk be removed from the DT description then,
> if it's fixed to the oscillator?

Yes. It is redundant to represent pll_clk in DT, and it should be
removed.

> 
> > Thanks for your review. I will send it again with the changes you
> > suggested.
> 
> Thanks very much!
> 
> Cheers,
> Daniel

Best regards,
Hyungwon Hwang
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Andrzej Hajda]

On 03/19/2015 02:18 AM, Hyungwon Hwang wrote:
> Dear Daniel,
> 
> On Thu, 19 Mar 2015 01:13:21 +0000
> Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw@public.gmane.org> wrote:
> 
>> Hi Hyungwon,
>>
>> On 19 March 2015 at 01:02, Hyungwon Hwang <human.hwang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> wrote:
>>>>> +       /*
>>>>> +        * The input PLL clock for MIPI DSI in Exynos5433 seems
>>>>> to be fixed
>>>>> +        * by OSC CLK.
>>>>> +        */
>>>>> +       fin = 24 * MHZ;
>>>>
>>>> Er, is this always true on other platforms as well? Shouldn't this
>>>> be a part of the DeviceTree description?
>>>
>>> I forgot to change the comment in development. Finally it is found
>>> that all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I
>>> will remove the comment, but remain the code as it is.
>>
>> Fair enough. Should pll_clk be removed from the DT description then,
>> if it's fixed to the oscillator?
> 
> Yes. It is redundant to represent pll_clk in DT, and it should be
> removed.

Why do you think OSC clk determines value of pll_clk?
pll_clk is mapped to SCLK_MIPI[01] or SCLK_DSIM0 gate with few dividers
and muxes above. So at least in theory it can differ from osc clk.
Additionally this gate should be enabled so you cannot just remove it
from DT.

Regards
Andrzej

> 
>>
>>> Thanks for your review. I will send it again with the changes you
>>> suggested.
>>
>> Thanks very much!
>>
>> Cheers,
>> Daniel
> 
> Best regards,
> Hyungwon Hwang
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang]

Dear Andrej,

On Thu, 19 Mar 2015 10:32:10 +0100
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 03/19/2015 02:18 AM, Hyungwon Hwang wrote:
> > Dear Daniel,
> > 
> > On Thu, 19 Mar 2015 01:13:21 +0000
> > Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw@public.gmane.org> wrote:
> > 
> >> Hi Hyungwon,
> >>
> >> On 19 March 2015 at 01:02, Hyungwon Hwang
> >> <human.hwang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>>> +       /*
> >>>>> +        * The input PLL clock for MIPI DSI in Exynos5433 seems
> >>>>> to be fixed
> >>>>> +        * by OSC CLK.
> >>>>> +        */
> >>>>> +       fin = 24 * MHZ;
> >>>>
> >>>> Er, is this always true on other platforms as well? Shouldn't
> >>>> this be a part of the DeviceTree description?
> >>>
> >>> I forgot to change the comment in development. Finally it is found
> >>> that all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I
> >>> will remove the comment, but remain the code as it is.
> >>
> >> Fair enough. Should pll_clk be removed from the DT description
> >> then, if it's fixed to the oscillator?
> > 
> > Yes. It is redundant to represent pll_clk in DT, and it should be
> > removed.
> 
> Why do you think OSC clk determines value of pll_clk?
> pll_clk is mapped to SCLK_MIPI[01] or SCLK_DSIM0 gate with few
> dividers and muxes above. So at least in theory it can differ from
> osc clk. Additionally this gate should be enabled so you cannot just
> remove it from DT.
> 
> Regards
> Andrzej

As I found, pll clk is not SCLK_MIPI[01] but OSC CLK. SCLK_DSIM0 must
be controlled in this driver as it has been, as a gate clock of MIPI
DSI block, but not as a pll clk. SCLK_DSIM0 is not the input
clock of MIPI DPHY which provides fin in this code. So clock setting
and getting code was wrong, and must be removed.

OSC CLK is not soc-depedendant but board-dependant, even though I have
not seen any board which does not use OSC CLK by 24 MHz. It must be
parsed from board DT file, which in this case, we can use the value in
pll_clk_rate (the variable name must be renamed also).

Because ambiguous description in the technical document, I can be
wrong. Please let me know if I do not understand something. Thanks for
your comment.

Best regards,
Hyungwon Hwang

> 
> > 
> >>
> >>> Thanks for your review. I will send it again with the changes you
> >>> suggested.
> >>
> >> Thanks very much!
> >>
> >> Cheers,
> >> Daniel
> > 
> > Best regards,
> > Hyungwon Hwang
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > devicetree" in the body of a message to
> > majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Andrzej Hajda]

On 03/20/2015 06:15 AM, Hyungwon Hwang wrote:
> Dear Andrej,
>
> On Thu, 19 Mar 2015 10:32:10 +0100
> Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> On 03/19/2015 02:18 AM, Hyungwon Hwang wrote:
>>> Dear Daniel,
>>>
>>> On Thu, 19 Mar 2015 01:13:21 +0000
>>> Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw@public.gmane.org> wrote:
>>>
>>>> Hi Hyungwon,
>>>>
>>>> On 19 March 2015 at 01:02, Hyungwon Hwang
>>>> <human.hwang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>>>>>> +       /*
>>>>>>> +        * The input PLL clock for MIPI DSI in Exynos5433 seems
>>>>>>> to be fixed
>>>>>>> +        * by OSC CLK.
>>>>>>> +        */
>>>>>>> +       fin = 24 * MHZ;
>>>>>> Er, is this always true on other platforms as well? Shouldn't
>>>>>> this be a part of the DeviceTree description?
>>>>> I forgot to change the comment in development. Finally it is found
>>>>> that all exynos mipi dsi's fin is OSC clk which is 24 MHz. So I
>>>>> will remove the comment, but remain the code as it is.
>>>> Fair enough. Should pll_clk be removed from the DT description
>>>> then, if it's fixed to the oscillator?
>>> Yes. It is redundant to represent pll_clk in DT, and it should be
>>> removed.
>> Why do you think OSC clk determines value of pll_clk?
>> pll_clk is mapped to SCLK_MIPI[01] or SCLK_DSIM0 gate with few
>> dividers and muxes above. So at least in theory it can differ from
>> osc clk. Additionally this gate should be enabled so you cannot just
>> remove it from DT.
>>
>> Regards
>> Andrzej
> As I found, pll clk is not SCLK_MIPI[01] but OSC CLK. SCLK_DSIM0 must
> be controlled in this driver as it has been, as a gate clock of MIPI
> DSI block, but not as a pll clk. SCLK_DSIM0 is not the input
> clock of MIPI DPHY which provides fin in this code. So clock setting
> and getting code was wrong, and must be removed.
>
> OSC CLK is not soc-depedendant but board-dependant, even though I have
> not seen any board which does not use OSC CLK by 24 MHz. It must be
> parsed from board DT file, which in this case, we can use the value in
> pll_clk_rate (the variable name must be renamed also).
>
> Because ambiguous description in the technical document, I can be
> wrong. Please let me know if I do not understand something. Thanks for
> your comment.

After some digging I agree that documentation is quite confusing and
current code could be wrong. Anyway I wonder if it wouldn't be better
to explicitly provide input clock for DSIM, or more precisely for its PLL
instead of hardcoding 24MHz into the driver.

Another thing that bothers me is relation of DPHY_PLL in clock controller to
MIPI_DPHY in Exynos7420. There are two clocks used by MIPI_DPHY:
- "Ref Clock" pinned to SCLK_MIPIDPHY_M4 connected to OSCCLK,
- "PHY Clock" pinned to I_FOUT_DPHY_PLL connected to DPHY_PLL,

The first clock seems to be your osc clock, but what is the role of the
2nd one?

Regards
Andrzej

> Best regards,
> Hyungwon Hwang
>
>>>>> Thanks for your review. I will send it again with the changes you
>>>>> suggested.
>>>> Thanks very much!
>>>>
>>>> Cheers,
>>>> Daniel
>>> Best regards,
>>> Hyungwon Hwang
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe
>>> devicetree" in the body of a message to
>>> majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang]

Dear Andrej,

On Mon, 23 Mar 2015 10:31:58 +0100
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 03/20/2015 06:15 AM, Hyungwon Hwang wrote:
> > Dear Andrej,
> >
> > On Thu, 19 Mar 2015 10:32:10 +0100
> > Andrzej Hajda <a.hajda@samsung.com> wrote:
> >
> >> On 03/19/2015 02:18 AM, Hyungwon Hwang wrote:
> >>> Dear Daniel,
> >>>
> >>> On Thu, 19 Mar 2015 01:13:21 +0000
> >>> Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw@public.gmane.org>
> >>> wrote:
> >>>
> >>>> Hi Hyungwon,
> >>>>
> >>>> On 19 March 2015 at 01:02, Hyungwon Hwang
> >>>> <human.hwang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>>>>> +       /*
> >>>>>>> +        * The input PLL clock for MIPI DSI in Exynos5433
> >>>>>>> seems to be fixed
> >>>>>>> +        * by OSC CLK.
> >>>>>>> +        */
> >>>>>>> +       fin = 24 * MHZ;
> >>>>>> Er, is this always true on other platforms as well? Shouldn't
> >>>>>> this be a part of the DeviceTree description?
> >>>>> I forgot to change the comment in development. Finally it is
> >>>>> found that all exynos mipi dsi's fin is OSC clk which is 24
> >>>>> MHz. So I will remove the comment, but remain the code as it is.
> >>>> Fair enough. Should pll_clk be removed from the DT description
> >>>> then, if it's fixed to the oscillator?
> >>> Yes. It is redundant to represent pll_clk in DT, and it should be
> >>> removed.
> >> Why do you think OSC clk determines value of pll_clk?
> >> pll_clk is mapped to SCLK_MIPI[01] or SCLK_DSIM0 gate with few
> >> dividers and muxes above. So at least in theory it can differ from
> >> osc clk. Additionally this gate should be enabled so you cannot
> >> just remove it from DT.
> >>
> >> Regards
> >> Andrzej
> > As I found, pll clk is not SCLK_MIPI[01] but OSC CLK. SCLK_DSIM0
> > must be controlled in this driver as it has been, as a gate clock
> > of MIPI DSI block, but not as a pll clk. SCLK_DSIM0 is not the input
> > clock of MIPI DPHY which provides fin in this code. So clock setting
> > and getting code was wrong, and must be removed.
> >
> > OSC CLK is not soc-depedendant but board-dependant, even though I
> > have not seen any board which does not use OSC CLK by 24 MHz. It
> > must be parsed from board DT file, which in this case, we can use
> > the value in pll_clk_rate (the variable name must be renamed also).
> >
> > Because ambiguous description in the technical document, I can be
> > wrong. Please let me know if I do not understand something. Thanks
> > for your comment.
> 
> After some digging I agree that documentation is quite confusing and
> current code could be wrong. Anyway I wonder if it wouldn't be better
> to explicitly provide input clock for DSIM, or more precisely for its
> PLL instead of hardcoding 24MHz into the driver.

OK. I agree. It will be more explicit to get the clock rate from DT.

> 
> Another thing that bothers me is relation of DPHY_PLL in clock
> controller to MIPI_DPHY in Exynos7420. There are two clocks used by
> MIPI_DPHY:
> - "Ref Clock" pinned to SCLK_MIPIDPHY_M4 connected to OSCCLK,
> - "PHY Clock" pinned to I_FOUT_DPHY_PLL connected to DPHY_PLL,
> 
> The first clock seems to be your osc clock, but what is the role of
> the 2nd one?

Hmm, I couldn't find similar clock in Exynos5433, also I don't have
the manual for Exynos7420.

Best regards,
Hyungwon Hwang


> 
> Regards
> Andrzej
> 
> > Best regards,
> > Hyungwon Hwang
> >
> >>>>> Thanks for your review. I will send it again with the changes
> >>>>> you suggested.
> >>>> Thanks very much!
> >>>>
> >>>> Cheers,
> >>>> Daniel
> >>> Best regards,
> >>> Hyungwon Hwang
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe
> >>> devicetree" in the body of a message to
> >>> majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo
> >>> info at  http://vger.kernel.org/majordomo-info.html
> >>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang]

Dear Daniel,

On Wed, 18 Mar 2015 09:52:33 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 18 March 2015 at 08:16, Hyungwon Hwang <human.hwang@samsung.com>
> wrote:
> > +#define REG(dsi, reg)  ((dsi)->reg_base +
> > dsi->driver_data->regs[(reg)])
> 
> This seems like a good change in general, but please split it up: it
> makes bisection much easier if you have one patch which adds no
> functionality and should have exactly the same behaviour, and then
> another patch which introduces your changes.
> 

Yes. That also looks good to me.

> > @@ -431,15 +579,11 @@ static unsigned long
> > exynos_dsi_set_pll(struct exynos_dsi *dsi, u16 m;
> >         u32 reg;
> >
> > -       clk_set_rate(dsi->pll_clk, dsi->pll_clk_rate);
> > -
> > -       fin = clk_get_rate(dsi->pll_clk);
> > -       if (!fin) {
> > -               dev_err(dsi->dev, "failed to get PLL clock
> > frequency\n");
> > -               return 0;
> > -       }
> > -
> > -       dev_dbg(dsi->dev, "PLL input frequency: %lu\n", fin);
> > +       /*
> > +        * The input PLL clock for MIPI DSI in Exynos5433 seems to
> > be fixed
> > +        * by OSC CLK.
> > +        */
> > +       fin = 24 * MHZ;
> 
> Er, is this always true on other platforms as well? Shouldn't this be
> a part of the DeviceTree description?
> 
> > @@ -509,7 +656,7 @@ static int exynos_dsi_enable_clock(struct
> > exynos_dsi *dsi) dev_dbg(dsi->dev, "hs_clk = %lu, byte_clk = %lu,
> > esc_clk = %lu\n", hs_clk, byte_clk, esc_clk);
> >
> > -       reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
> > +       reg = readl(REG(dsi, DSIM_CLKCTRL_REG));
> 
> Instead of this readl(REG()) pattern you have everywhere, maybe it
> would be easier to introduce a dsi_read_reg(dsi, reg_enum_value)
> helper, and the same for write_reg.
> 

Yes. That's resonable.

> > @@ -1720,18 +1873,16 @@ static int exynos_dsi_probe(struct
> > platform_device *pdev) return -EPROBE_DEFER;
> >         }
> >
> > -       dsi->pll_clk = devm_clk_get(dev, "pll_clk");
> > -       if (IS_ERR(dsi->pll_clk)) {
> > -               dev_info(dev, "failed to get dsi pll input
> > clock\n");
> > -               ret = PTR_ERR(dsi->pll_clk);
> > -               goto err_del_component;
> > -       }
> > -
> > -       dsi->bus_clk = devm_clk_get(dev, "bus_clk");
> > -       if (IS_ERR(dsi->bus_clk)) {
> > -               dev_info(dev, "failed to get dsi bus clock\n");
> > -               ret = PTR_ERR(dsi->bus_clk);
> > -               goto err_del_component;
> > +       dsi->clks = devm_kzalloc(dev,
> > +                               sizeof(*dsi->clks) *
> > dsi->driver_data->num_clks,
> > +                               GFP_KERNEL);
> > +       for (i = 0; i < dsi->driver_data->num_clks; i++) {
> > +               dsi->clks[i] = devm_clk_get(dev, clk_names[i]);
> > +               if (IS_ERR(dsi->clks[i])) {
> > +                       dev_info(dev, "failed to get dsi pll input
> > clock\n");
> 
> This error message seems wrong; it should contain the name of the
> actual failing clock.
> 

OK.

Best regards,
Hyungwon Hwang

> Cheers,
> Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/6] drm/exynos: dsi: add support for MIC driver as a bridge

[Hyungwon Hwang]

MIC must be initilized by MIPI DSI when it is being bound.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 77236ad..e385d24 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -20,6 +20,7 @@
 #include <linux/irq.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
+#include <linux/of_graph.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
@@ -280,6 +281,7 @@ struct exynos_dsi {
 	struct list_head transfer_list;
 
 	struct exynos_dsi_driver_data *driver_data;
+	struct device_node *bridge_node;
 };
 
 #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
@@ -1787,7 +1789,22 @@ static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
 
 	ret = exynos_dsi_of_read_u32(ep, "samsung,esc-clock-frequency",
 				     &dsi->esc_clk_rate);
+	if (ret < 0)
+		goto end;
+
+	of_node_put(ep);
+
+	ep = of_graph_get_next_endpoint(node, NULL);
+	if (!ep) {
+		ret = -ENXIO;
+		goto end;
+	}
 
+	dsi->bridge_node = of_graph_get_remote_port_parent(ep);
+	if (!dsi->bridge_node) {
+		ret = -ENXIO;
+		goto end;
+	}
 end:
 	of_node_put(ep);
 
@@ -1800,6 +1817,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 	struct exynos_drm_display *display = dev_get_drvdata(dev);
 	struct exynos_dsi *dsi = display_to_dsi(display);
 	struct drm_device *drm_dev = data;
+	struct drm_bridge *bridge;
 	int ret;
 
 	ret = exynos_drm_create_enc_conn(drm_dev, display);
@@ -1809,6 +1827,12 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
+	bridge = of_drm_find_bridge(dsi->bridge_node);
+	if (bridge) {
+		display->encoder->bridge = bridge;
+		drm_bridge_attach(drm_dev, bridge);
+	}
+
 	return mipi_dsi_host_register(&dsi->dsi_host);
 }
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/6] drm/exynos: dsi: do not set TE GPIO direction by input

[Hyungwon Hwang]

On some board, TE GPIO should be configured properly thoughout pinctrl driver
as an wakeup interrupt. So this gpio should be configurable in the board's DT,
not being requested as a input pin.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index e385d24..58e0620 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1324,15 +1324,15 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
 		goto out;
 	}
 
-	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
+	ret = gpio_request(dsi->te_gpio, "te_gpio");
 	if (ret) {
 		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
 		goto out;
 	}
 
 	te_gpio_irq = gpio_to_irq(dsi->te_gpio);
-
 	irq_set_status_flags(te_gpio_irq, IRQ_NOAUTOEN);
+
 	ret = request_threaded_irq(te_gpio_irq, exynos_dsi_te_irq_handler, NULL,
 					IRQF_TRIGGER_RISING, "TE", dsi);
 	if (ret) {
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel