TI LCDC DRM driver

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the CMA helpers. Currently only the TFP410 DVI encoder is supported (tested with beaglebone + DVI cape). There are also various LCD displays, for which support can be added (as I get hw to test on), and an external i2c HDMI encoder found on some boards.

The display controller supports a single CRTC. And the encoder+ connector are split out into sub-devices. Depending on which LCD or external encoder is actually present, the appropriate output module(s) will be loaded.

v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to pick a display mode with too high memory bandwidth, and other small cleanups

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/Kconfig                |   2 +
 drivers/gpu/drm/Makefile               |   1 +
 drivers/gpu/drm/tilcdc/Kconfig         |  10 +
 drivers/gpu/drm/tilcdc/Makefile        |   8 +
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   | 597 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 605 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    | 159 +++++++++
 drivers/gpu/drm/tilcdc/tilcdc_regs.h   | 154 +++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h |  26 ++
 10 files changed, 1985 insertions(+)
 create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
 create mode 100644 drivers/gpu/drm/tilcdc/Makefile
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +20 -0

+struct tilcdc_crtc {
+	struct drm_crtc base;
+
+	const struct tilcdc_panel_info *info;
+	uint32_t dirty;
+	dma_addr_t start, end;
+	struct drm_pending_vblank_event *event;
+	int dpms;
+	wait_queue_head_t frame_done_wq;
+	bool frame_done;
+
+	/* fb currently set to scanout 0/1: */
+	struct drm_framebuffer *scanout[2];
+
+	/* for deferred fb unref's: */
+	DECLARE_KFIFO_PTR(unref_fifo, struct drm_framebuffer *);
+	struct work_struct work;
+};
+#define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
+

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +17 -0

+static void update_scanout(struct drm_crtc *crtc)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct drm_gem_cma_object *gem;
+	unsigned int depth, bpp;
+
+	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+
+	tilcdc_crtc->start = gem->paddr + fb->offsets[0] +
+			(crtc->y * fb->pitches[0]) + (crtc->x * bpp/8);
+
+	tilcdc_crtc->end = tilcdc_crtc->start +
+			(crtc->mode.vdisplay * fb->pitches[0]);
+

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +435 -0

+	}
+}
+
+static void start(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+
+	if (priv->rev == 2) {
+		tilcdc_set(dev, LCDC_CLK_RESET_REG, LCDC_CLK_MAIN_RESET);
+		msleep(1);
+		tilcdc_clear(dev, LCDC_CLK_RESET_REG, LCDC_CLK_MAIN_RESET);
+		msleep(1);
+	}
+
+	tilcdc_set(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE);
+	tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_PALETTE_LOAD_MODE(DATA_ONLY));
+	tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+}
+
+static void stop(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+
+	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+}
+
+static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+
+	WARN_ON(tilcdc_crtc->dpms == DRM_MODE_DPMS_ON);
+
+	drm_crtc_cleanup(crtc);
+	WARN_ON(!kfifo_is_empty(&tilcdc_crtc->unref_fifo));
+	kfifo_free(&tilcdc_crtc->unref_fifo);
+	kfree(tilcdc_crtc);
+}
+
+static int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
+		struct drm_framebuffer *fb,
+		struct drm_pending_vblank_event *event)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+
+	if (tilcdc_crtc->event) {
+		dev_err(dev->dev, "already pending page flip!\n");
+		return -EBUSY;
+	}
+
+	crtc->fb = fb;
+	tilcdc_crtc->event = event;
+	update_scanout(crtc);
+
+	return 0;
+}
+
+static void tilcdc_crtc_dpms(struct drm_crtc *crtc, int mode)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+
+	/* we really only care about on or off: */
+	if (mode != DRM_MODE_DPMS_ON)
+		mode = DRM_MODE_DPMS_OFF;
+
+	if (tilcdc_crtc->dpms == mode)
+		return;
+
+	tilcdc_crtc->dpms = mode;
+
+	pm_runtime_get_sync(dev->dev);
+
+	if (mode == DRM_MODE_DPMS_ON) {
+		pm_runtime_forbid(dev->dev);
+		start(crtc);
+	} else {
+		tilcdc_crtc->frame_done = false;
+		stop(crtc);
+
+		/* if necessary wait for framedone irq which will still come
+		 * before putting things to sleep..
+		 */
+		if (priv->rev == 2) {
+			int ret = wait_event_timeout(
+					tilcdc_crtc->frame_done_wq,
+					tilcdc_crtc->frame_done,
+					msecs_to_jiffies(50));
+			if (ret == 0)
+				dev_err(dev->dev, "timeout waiting for framedone\n");
+		}
+		pm_runtime_allow(dev->dev);
+	}
+
+	pm_runtime_put_sync(dev->dev);
+}
+
+static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
+		const struct drm_display_mode *mode,
+		struct drm_display_mode *adjusted_mode)
+{
+	return true;
+}
+
+static void tilcdc_crtc_prepare(struct drm_crtc *crtc)
+{
+	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+}
+
+static void tilcdc_crtc_commit(struct drm_crtc *crtc)
+{
+	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
+}
+
+static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
+		struct drm_display_mode *mode,
+		struct drm_display_mode *adjusted_mode,
+		int x, int y,
+		struct drm_framebuffer *old_fb)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	const struct tilcdc_panel_info *info = tilcdc_crtc->info;
+	uint32_t reg, hbp, hfp, hsw, vbp, vfp, vsw;
+	int ret;
+
+	ret = tilcdc_crtc_mode_valid(crtc, mode);
+	if (WARN_ON(ret))
+		return ret;
+
+	if (WARN_ON(!info))
+		return -EINVAL;
+
+	pm_runtime_get_sync(dev->dev);
+
+	/* Configure the Burst Size and fifo threshold of DMA: */
+	reg = tilcdc_read(dev, LCDC_DMA_CTRL_REG) & ~0x00000770;
+	switch (info->dma_burst_sz) {
+	case 1:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_1);
+		break;
+	case 2:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_2);
+		break;
+	case 4:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_4);
+		break;
+	case 8:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_8);
+		break;
+	case 16:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16);
+		break;
+	default:
+		return -EINVAL;
+	}
+	reg |= (info->fifo_th << 8);
+	tilcdc_write(dev, LCDC_DMA_CTRL_REG, reg);
+
+	/* Configure the AC Bias Period and Number of Transitions per Interrupt: */
+	reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG) & ~0x000fff00;
+	reg |= LCDC_AC_BIAS_FREQUENCY(info->ac_bias) |
+		LCDC_AC_BIAS_TRANSITIONS_PER_INT(info->ac_bias_intrpt);
+	tilcdc_write(dev, LCDC_RASTER_TIMING_2_REG, reg);
+
+	/* Configure timings: */
+	hbp = mode->htotal - mode->hsync_end;
+	hfp = mode->hsync_start - mode->hdisplay;
+	hsw = mode->hsync_end - mode->hsync_start;
+	vbp = mode->vtotal - mode->vsync_end;
+	vfp = mode->vsync_start - mode->vdisplay;
+	vsw = mode->vsync_end - mode->vsync_start;
+
+	DBG("%dx%d, hbp=%u, hfp=%u, hsw=%u, vbp=%u, vfp=%u, vsw=%u",
+			mode->hdisplay, mode->vdisplay, hbp, hfp, hsw, vbp, vfp, vsw);
+
+	reg = (((mode->hdisplay >> 4) - 1) << 4) |
+		((hbp & 0xff) << 24) |
+		((hfp & 0xff) << 16) |
+		((hsw & 0x3f) << 10);
+	if (priv->rev == 2)
+		reg |= (((mode->hdisplay >> 4) - 1) & 0x40) >> 3;
+	tilcdc_write(dev, LCDC_RASTER_TIMING_0_REG, reg);
+
+	reg = ((mode->vdisplay - 1) & 0x3ff) |
+		((vbp & 0xff) << 24) |
+		((vfp & 0xff) << 16) |
+		((vsw & 0x3f) << 10);
+	tilcdc_write(dev, LCDC_RASTER_TIMING_1_REG, reg);
+
+	/* Configure display type: */
+	reg = tilcdc_read(dev, LCDC_RASTER_CTRL_REG) &
+		~(LCDC_TFT_MODE | LCDC_MONO_8BIT_MODE | LCDC_MONOCHROME_MODE |
+			LCDC_V2_TFT_24BPP_MODE | LCDC_V2_TFT_24BPP_UNPACK | 0x000ff000);
+	reg |= LCDC_TFT_MODE; /* no monochrome/passive support */
+	if (info->tft_alt_mode)
+		reg |= LCDC_TFT_ALT_ENABLE;
+	if (priv->rev == 2) {
+		unsigned int depth, bpp;
+
+		drm_fb_get_bpp_depth(crtc->fb->pixel_format, &depth, &bpp);
+		switch (bpp) {
+		case 16:
+			break;
+		case 32:
+			reg |= LCDC_V2_TFT_24BPP_UNPACK;
+			/* fallthrough */
+		case 24:
+			reg |= LCDC_V2_TFT_24BPP_MODE;
+			break;
+		default:
+			dev_err(dev->dev, "invalid pixel format\n");
+			return -EINVAL;
+		}
+	}
+	reg |= info->fdd < 12;
+	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
+
+	if (info->invert_pxl_clk)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
+
+	if (info->sync_ctrl)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
+
+	if (info->sync_edge)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
+
+	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);
+
+	if (info->raster_order)
+		tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
+
+
+	update_scanout(crtc);
+	tilcdc_crtc_update_clk(crtc);
+
+	pm_runtime_put_sync(dev->dev);
+
+	return 0;
+}
+
+static int tilcdc_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
+		struct drm_framebuffer *old_fb)
+{
+	update_scanout(crtc);
+	return 0;
+}
+
+static void tilcdc_crtc_load_lut(struct drm_crtc *crtc)
+{
+}
+
+static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
+		.destroy        = tilcdc_crtc_destroy,
+		.set_config     = drm_crtc_helper_set_config,
+		.page_flip      = tilcdc_crtc_page_flip,
+};
+
+static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
+		.dpms           = tilcdc_crtc_dpms,
+		.mode_fixup     = tilcdc_crtc_mode_fixup,
+		.prepare        = tilcdc_crtc_prepare,
+		.commit         = tilcdc_crtc_commit,
+		.mode_set       = tilcdc_crtc_mode_set,
+		.mode_set_base  = tilcdc_crtc_mode_set_base,
+		.load_lut       = tilcdc_crtc_load_lut,
+};
+
+int tilcdc_crtc_max_width(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	int max_width = 0;
+
+	if (priv->rev == 1)
+		max_width = 1024;
+	else if (priv->rev == 2)
+		max_width = 2048;
+
+	return max_width;
+}
+
+int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode)
+{
+	struct tilcdc_drm_private *priv = crtc->dev->dev_private;
+	unsigned int bandwidth;
+
+	if (mode->hdisplay > tilcdc_crtc_max_width(crtc))
+		return MODE_VIRTUAL_X;
+
+	/* width must be multiple of 16 */
+	if (mode->hdisplay & 0xf)
+		return MODE_VIRTUAL_X;
+
+	if (mode->vdisplay > 2048)
+		return MODE_VIRTUAL_Y;
+
+	/* filter out modes that would require too much memory bandwidth: */
+	bandwidth = mode->hdisplay * mode->vdisplay * drm_mode_vrefresh(mode);
+	if (bandwidth > priv->max_bandwidth)
+		return MODE_BAD;
+
+	return MODE_OK;
+}
+
+void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
+		const struct tilcdc_panel_info *info)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	tilcdc_crtc->info = info;
+}
+
+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	int dpms = tilcdc_crtc->dpms;
+	unsigned int lcd_clk, div;
+	int ret;
+
+	pm_runtime_get_sync(dev->dev);
+
+	if (dpms == DRM_MODE_DPMS_ON)
+		tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+
+	/* in raster mode, minimum divisor is 2: */
+	ret = clk_set_rate(priv->disp_clk, crtc->mode.clock * 1000 * 2);
+	if (ret) {
+		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
+				crtc->mode.clock);
+		goto out;
+	}
+
+	lcd_clk = clk_get_rate(priv->clk);
+	div = lcd_clk / (crtc->mode.clock * 1000);
+
+	DBG("lcd_clk=%u, mode clock=%d, div=%u", lcd_clk, crtc->mode.clock, div);
+	DBG("fck=%lu, dpll_disp_ck=%lu", clk_get_rate(priv->clk), clk_get_rate(priv->disp_clk));
+
+	/* Configure the LCD clock divisor. */
+	tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+			LCDC_RASTER_MODE);
+
+	if (priv->rev == 2)
+		tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+				LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+				LCDC_V2_CORE_CLK_EN);
+
+	if (dpms == DRM_MODE_DPMS_ON)
+		tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
+
+out:
+	pm_runtime_put_sync(dev->dev);
+}
+
+irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	uint32_t stat = tilcdc_read_irqstatus(dev);
+
+	if ((stat & LCDC_SYNC_LOST) && (stat & LCDC_FIFO_UNDERFLOW)) {
+		stop(crtc);
+		dev_err(dev->dev, "error: %08x\n", stat);
+		tilcdc_clear_irqstatus(dev, stat);
+		start(crtc);
+	} else if (stat & LCDC_PL_LOAD_DONE) {
+		tilcdc_clear_irqstatus(dev, stat);
+	} else {
+		struct drm_pending_vblank_event *event;
+		unsigned long flags;
+		uint32_t dirty = tilcdc_crtc->dirty & stat;
+
+		tilcdc_clear_irqstatus(dev, stat);
+
+		if (dirty & LCDC_END_OF_FRAME0)
+			set_scanout(crtc, 0);
+
+		if (dirty & LCDC_END_OF_FRAME1)
+			set_scanout(crtc, 1);
+
+		drm_handle_vblank(dev, 0);
+
+		spin_lock_irqsave(&dev->event_lock, flags);
+		event = tilcdc_crtc->event;
+		tilcdc_crtc->event = NULL;
+		if (event)
+			drm_send_vblank_event(dev, 0, event);
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+
+		if (dirty && !tilcdc_crtc->dirty)
+			drm_vblank_put(dev, 0);
+	}
+
+	if (priv->rev == 2) {
+		if (stat & LCDC_FRAME_DONE) {
+			tilcdc_crtc->frame_done = true;
+			wake_up(&tilcdc_crtc->frame_done_wq);
+		}
+		tilcdc_write(dev, LCDC_END_OF_INT_IND_REG, 0);
+	}
+
+	return IRQ_HANDLED;
+}
+
+void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_pending_vblank_event *event;
+	struct drm_device *dev = crtc->dev;
+	unsigned long flags;
+
+	/* Destroy the pending vertical blanking event associated with the
+	 * pending page flip, if any, and disable vertical blanking interrupts.

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +31 -0

+/*
+ * Power management:
+ */
+
+#if CONFIG_PM_SLEEP
+static int tilcdc_pm_suspend(struct device *dev)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct tilcdc_drm_private *priv = ddev->dev_private;
+	unsigned i, n = 0;
+
+	drm_kms_helper_poll_disable(ddev);
+
+	/* Save register state: */
+	for (i = 0; i < ARRAY_SIZE(registers); i++)
+		if (registers[i].save && (priv->rev >= registers[i].rev))
+			priv->saved_register[n++] = tilcdc_read(ddev, registers[i].reg);
+
+	return 0;
+}
+
+static int tilcdc_pm_resume(struct device *dev)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct tilcdc_drm_private *priv = ddev->dev_private;
+	unsigned i, n = 0;
+
+	/* Restore register state: */
+	for (i = 0; i < ARRAY_SIZE(registers); i++)
+		if (registers[i].save && (priv->rev >= registers[i].rev))
+			tilcdc_write(ddev, registers[i].reg, priv->saved_register[n++]);

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the CMA helpers. Currently only the TFP410 DVI encoder is supported (tested with beaglebone + DVI cape). There are also various LCD displays, for which support can be added (as I get hw to test on), and an external i2c HDMI encoder found on some boards.

The display controller supports a single CRTC. And the encoder+ connector are split out into sub-devices. Depending on which LCD or external encoder is actually present, the appropriate output module(s) will be loaded.

v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to pick a display mode with too high memory bandwidth, and other small cleanups

Signed-off-by Rob Clark <[email protected]>

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the CMA helpers. Currently only the TFP410 DVI encoder is supported (tested with beaglebone + DVI cape). There are also various LCD displays, for which support can be added (as I get hw to test on), and an external i2c HDMI encoder found on some boards.

The display controller supports a single CRTC. And the encoder+ connector are split out into sub-devices. Depending on which LCD or external encoder is actually present, the appropriate output module(s) will be loaded.

v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to pick a display mode with too high memory bandwidth, and other small cleanups

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/Kconfig                |   2 +
 drivers/gpu/drm/Makefile               |   1 +
 drivers/gpu/drm/tilcdc/Kconfig         |  10 +
 drivers/gpu/drm/tilcdc/Makefile        |   8 +
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   | 597 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 605 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    | 159 +++++++++
 drivers/gpu/drm/tilcdc/tilcdc_regs.h   | 154 +++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h |  26 ++
 10 files changed, 1985 insertions(+)

 create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
 create mode 100644 drivers/gpu/drm/tilcdc/Makefile
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h

drivers/gpu/drm/tilcdc/Kconfig new file +10 -0

new file mode 100644
index 0000000..ee9b592
@@ -0,0 +1,10 @@
+config DRM_TILCDC
+	tristate "DRM Support for TI LCDC Display Controller"
+	depends on DRM && OF
+	select DRM_KMS_HELPER
+	select DRM_KMS_CMA_HELPER
+	select DRM_GEM_CMA_HELPER
+	help
+	  Choose this option if you have an TI SoC with LCDC display
+	  controller, for example AM33xx in beagle-bone, DA8xx, or
+	  OMAP-L1xx.  This driver replaces the FB_DA8XX fbdev driver.

drivers/gpu/drm/tilcdc/Makefile new file +0 -0

new file mode 100644
index 0000000..1359cc2

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +25 -0

+static struct drm_driver tilcdc_driver = {
+	.driver_features    = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
+	.load               = tilcdc_load,
+	.unload             = tilcdc_unload,
+	.preclose           = tilcdc_preclose,
+	.lastclose          = tilcdc_lastclose,
+	.irq_handler        = tilcdc_irq,
+	.irq_preinstall     = tilcdc_irq_preinstall,
+	.irq_postinstall    = tilcdc_irq_postinstall,
+	.irq_uninstall      = tilcdc_irq_uninstall,
+	.get_vblank_counter = drm_vblank_count,
+	.enable_vblank      = tilcdc_enable_vblank,
+	.disable_vblank     = tilcdc_disable_vblank,
+	.gem_free_object    = drm_gem_cma_free_object,
+	.gem_vm_ops         = &drm_gem_cma_vm_ops,
+	.dumb_create        = drm_gem_cma_dumb_create,
+	.dumb_map_offset    = drm_gem_cma_dumb_map_offset,
+	.dumb_destroy       = drm_gem_cma_dumb_destroy,
+#ifdef CONFIG_DEBUG_FS
+	.debugfs_init       = tilcdc_debugfs_init,
+	.debugfs_cleanup    = tilcdc_debugfs_cleanup,
+#endif
+	.fops               = &fops,
+	.name               = "tilcdc",
+	.desc               = "TI LCD Controller DRM",

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +11 -0

+static struct of_device_id tilcdc_of_match[] = {
+		{ .compatible = "ti,am33xx-tilcdc", },
+		{ },
+};
+MODULE_DEVICE_TABLE(of, tilcdc_of_match);
+
+static struct platform_driver tilcdc_platform_driver = {
+	.probe      = tilcdc_pdev_probe,
+	.remove     = tilcdc_pdev_remove,
+	.driver     = {
+		.owner  = THIS_MODULE,

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +16 -0

+	return platform_driver_register(&tilcdc_platform_driver);
+}
+
+static void __exit tilcdc_drm_fini(void)
+{
+	DBG("fini");
+	tilcdc_tfp410_fini();
+	platform_driver_unregister(&tilcdc_platform_driver);
+}
+
+module_init(tilcdc_drm_init);
+module_exit(tilcdc_drm_fini);
+
+MODULE_AUTHOR("Rob Clark <[email protected]");
+MODULE_DESCRIPTION("TI LCD Controller DRM Driver");
+MODULE_LICENSE("GPL");

Hi Rob,

As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below.

Quoted 32 lines · On Tue, 22 Jan 2013 16:36:22 -0600 Rob Clark <[email protected]> wrote:

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the CMA helpers. Currently only the TFP410 DVI encoder is supported (tested with beaglebone + DVI cape). There are also various LCD displays, for which support can be added (as I get hw to test on), and an external i2c HDMI encoder found on some boards.

The display controller supports a single CRTC. And the encoder+ connector are split out into sub-devices. Depending on which LCD or external encoder is actually present, the appropriate output module(s) will be loaded.

v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to pick a display mode with too high memory bandwidth, and other small cleanups

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/Kconfig                |   2 +
 drivers/gpu/drm/Makefile               |   1 +
 drivers/gpu/drm/tilcdc/Kconfig         |  10 +
 drivers/gpu/drm/tilcdc/Makefile        |   8 +
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   | 597 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 605 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    | 159 +++++++++
 drivers/gpu/drm/tilcdc/tilcdc_regs.h   | 154 +++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h |  26 ++
 10 files changed, 1985 insertions(+)

Quoted 8 lines

 create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
 create mode 100644 drivers/gpu/drm/tilcdc/Makefile
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h

        [snip]

Quoted 22 lines

drivers/gpu/drm/tilcdc/Kconfig new file +10 -0

new file mode 100644
index 0000000..ee9b592
@@ -0,0 +1,10 @@
+config DRM_TILCDC
+     tristate "DRM Support for TI LCDC Display Controller"
+     depends on DRM && OF
+     select DRM_KMS_HELPER
+     select DRM_KMS_CMA_HELPER
+     select DRM_GEM_CMA_HELPER
+     help
+       Choose this option if you have an TI SoC with LCDC display
+       controller, for example AM33xx in beagle-bone, DA8xx, or
+       OMAP-L1xx.  This driver replaces the FB_DA8XX fbdev driver.

drivers/gpu/drm/tilcdc/Makefile new file +0 -0

new file mode 100644
index 0000000..1359cc2

drivers/gpu/drm/tilcdc/Makefile line 6

Quoted 6 lines

drivers/gpu/drm/tilcdc/Makefile new file +6 -0

@@ -0,0 +1,8 @@
+ccflags-y := -Iinclude/drm -Werror
+
+tilcdc-y := \
+     tilcdc_crtc.o \
+     tilcdc_tfp410.o \
+     tilcdc_drv.o

As I understand, this means that the 3 objects will go into the resident kernel.

I though that the device tree was created for Linux distributors who might generate generic ARM kernels, the choice of the drivers being done according the local device tree.

From this point of vue, it would be nicer to have 2 separate modules:
tilcdc and tfp410, tilcdc_crtc being included in one of these ones (I did not look deep enough at the code to know which).

        [snip]

drivers/gpu/drm/tilcdc/tilcdc_drv.c

Quoted 6 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +0 -0

new file mode 100644
index 0000000..cf1fddc
@@ -0,0 +1,605 @@

        [snip]

Quoted 25 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +25 -0

+static struct drm_driver tilcdc_driver = {
+     .driver_features    = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
+     .load               = tilcdc_load,
+     .unload             = tilcdc_unload,
+     .preclose           = tilcdc_preclose,
+     .lastclose          = tilcdc_lastclose,
+     .irq_handler        = tilcdc_irq,
+     .irq_preinstall     = tilcdc_irq_preinstall,
+     .irq_postinstall    = tilcdc_irq_postinstall,
+     .irq_uninstall      = tilcdc_irq_uninstall,
+     .get_vblank_counter = drm_vblank_count,
+     .enable_vblank      = tilcdc_enable_vblank,
+     .disable_vblank     = tilcdc_disable_vblank,
+     .gem_free_object    = drm_gem_cma_free_object,
+     .gem_vm_ops         = &drm_gem_cma_vm_ops,
+     .dumb_create        = drm_gem_cma_dumb_create,
+     .dumb_map_offset    = drm_gem_cma_dumb_map_offset,
+     .dumb_destroy       = drm_gem_cma_dumb_destroy,
+#ifdef CONFIG_DEBUG_FS
+     .debugfs_init       = tilcdc_debugfs_init,
+     .debugfs_cleanup    = tilcdc_debugfs_cleanup,
+#endif
+     .fops               = &fops,
+     .name               = "tilcdc",
+     .desc               = "TI LCD Controller DRM",

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 509

Quoted 10 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +10 -0

+     .date               = "20121205",
+     .major              = 1,
+     .minor              = 0,
+};
+
+/*
+ * Power management:
+ */
+
+#if CONFIG_PM_SLEEP

Should be:

#ifdef CONFIG_PM_SLEEP

        [snip]

Quoted 15 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +15 -0

+static struct of_device_id tilcdc_of_match[] = {
+             { .compatible = "ti,am33xx-tilcdc", },
+             { },
+};
+MODULE_DEVICE_TABLE(of, tilcdc_of_match);
+
+static struct platform_driver tilcdc_platform_driver = {
+     .probe      = tilcdc_pdev_probe,
+     .remove     = tilcdc_pdev_remove,
+     .driver     = {
+             .owner  = THIS_MODULE,
+             .name   = "tilcdc",
+             .pm     = &tilcdc_pm_ops,
+             .of_match_table = tilcdc_of_match,
+     },

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 589

Quoted 6 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +6 -0

+};
+
+static int __init tilcdc_drm_init(void)
+{
+     DBG("init");
+     tilcdc_tfp410_init();

This function may be called twice if "tilcdc,tfp410" is in the DT.

Quoted 13 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +13 -0

+     return platform_driver_register(&tilcdc_platform_driver);
+}
+
+static void __exit tilcdc_drm_fini(void)
+{
+     DBG("fini");
+     tilcdc_tfp410_fini();
+     platform_driver_unregister(&tilcdc_platform_driver);
+}
+
+module_init(tilcdc_drm_init);
+module_exit(tilcdc_drm_fini);
+

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 605

Quoted 3 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +3 -0

+MODULE_AUTHOR("Rob Clark <[email protected]");
+MODULE_DESCRIPTION("TI LCD Controller DRM Driver");
+MODULE_LICENSE("GPL");

--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

Quoted 4 lines · Jean-Francois Moine

Hi Rob,

As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below.

Just fyi, you can re-use the nxp-tda998x part independently of tilcdc (just in case that wasn't clear).

Quoted 8 lines · Rob Clark

Quoted 4 lines · Jean-Francois Moine

Hi Rob,

As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below.

Just fyi, you can re-use the nxp-tda998x part independently of tilcdc (just in case that wasn't clear).

Great, this chip is also used on the cubox too.

The one thing I wonder is how you deal with the VSYNC/HSYNC polarities that are provided to the TDA9988x chip. On the cubox, I have to adjust the mode parameters such that the polarities are fixed up thusly:

        adjusted->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC |
                             DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC |
                             DRM_MODE_FLAG_PCSYNC | DRM_MODE_FLAG_NCSYNC);

        /* The TDA19988 always requires negative VSYNC? */
        adjusted->flags |= DRM_MODE_FLAG_NVSYNC;

        /* The TDA19988 requires positive HSYNC on 1080p or 720p */
        if ((adjusted->hdisplay == 1920 && adjusted->vdisplay == 1080) ||
            (adjusted->hdisplay == 1280 && adjusted->vdisplay == 720))
                adjusted->flags |= DRM_MODE_FLAG_PHSYNC;
        else
                adjusted->flags |= DRM_MODE_FLAG_NHSYNC;

        return true;

for these resolutions to be displayed correctly.

Also, when the only output is the HDMI device, reporting the display "disconnected" and without any modes seems to cause problems - I have to report "unknown" status when there's nothing connected, and also provide a default (720p) mode when no EDID is received.

Quoted 11 lines · Russell King - ARM Linux

Quoted 8 lines · Rob Clark

Quoted 4 lines · Jean-Francois Moine

Hi Rob,

As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below.

Just fyi, you can re-use the nxp-tda998x part independently of tilcdc (just in case that wasn't clear).

Great, this chip is also used on the cubox too.

cool, it would be great if other platforms could benefit from this as well.. the out-of-tree nxp driver is just horrid ;-)

Quoted 18 lines

The one thing I wonder is how you deal with the VSYNC/HSYNC polarities that are provided to the TDA9988x chip. On the cubox, I have to adjust the mode parameters such that the polarities are fixed up thusly:

        adjusted->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC |
                             DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC |
                             DRM_MODE_FLAG_PCSYNC | DRM_MODE_FLAG_NCSYNC);

        /* The TDA19988 always requires negative VSYNC? */
        adjusted->flags |= DRM_MODE_FLAG_NVSYNC;

        /* The TDA19988 requires positive HSYNC on 1080p or 720p */
        if ((adjusted->hdisplay == 1920 && adjusted->vdisplay == 1080) ||
            (adjusted->hdisplay == 1280 && adjusted->vdisplay == 720))
                adjusted->flags |= DRM_MODE_FLAG_PHSYNC;
        else
                adjusted->flags |= DRM_MODE_FLAG_NHSYNC;

Quoted 3 lines

        return true;

for these resolutions to be displayed correctly.

hmm, I didn't come across this. Although 1080p seems to be a bit more than what the little board I'm working on can drive. I didn't have to do any special fixup for 720p..

I wonder if you are having to do that with the nxp out of tree driver?
Maybe it is related to how that driver it is configuring the hw? It
would be interesting if you hit the same issue w/ the i2c encoder slave version.

At any rate, if it turns out to be needed, we can add this in tda998x_encoder_mode_fixup().

Quoted 4 lines

Also, when the only output is the HDMI device, reporting the display "disconnected" and without any modes seems to cause problems - I have to report "unknown" status when there's nothing connected, and also provide a default (720p) mode when no EDID is received.

hmm, also did not run into any issues here on my end. What sort of problems did you hit?

BR,
-R

Hi Rob,

Quoted 4 lines · Rob Clark

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the

+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c line 461

Quoted 2 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +2 -0

+     /* in raster mode, minimum divisor is 2: */
+     ret = clk_set_rate(priv->disp_clk, crtc->mode.clock * 1000 * 2);

These things can better be handled with divider clock having a minimum value (being discussed with Mike on how exactly it should be) and instead of setting rate over a platform specific clock, you can set rate over lcd clock using SET_RATE_PARENT at platform level, more below,

drivers/gpu/drm/tilcdc/tilcdc_crtc.c line 481

Quoted 8 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +8 -0

+     /* Configure the LCD clock divisor. */
+     tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+                     LCDC_RASTER_MODE);
+
+     if (priv->rev == 2)
+             tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+                             LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+                             LCDC_V2_CORE_CLK_EN);

Mike was suggesting to model the above using gate/divider/composite clocks of CCF in the FB driver.

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 197

Quoted 13 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +13 -0

+     priv->clk = clk_get(dev->dev, "fck");
+     if (IS_ERR(priv->clk)) {
+             dev_err(dev->dev, "failed to get functional clock\n");
+             ret = -ENODEV;
+             goto fail;
+     }
+
+     priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
+     if (IS_ERR(priv->clk)) {
+             dev_err(dev->dev, "failed to get display clock\n");
+             ret = -ENODEV;
+             goto fail;
+     }

"dpll_disp_ck" is a platform specific matter, driver should not be handling platform specifics. And this won't work on DaVinci, you can probably make use of

my series "[PATCH v2 0/4] ARM: AM335x: LCDC platform support"

or something similar to overcome this.

Regards
Afzal

Quoted 7 lines · Afzal

Quoted 4 lines · Rob Clark

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the

+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)

Quoted 8 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c line 461

Quoted 2 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +2 -0

+     /* in raster mode, minimum divisor is 2: */
+     ret = clk_set_rate(priv->disp_clk, crtc->mode.clock * 1000 * 2);

These things can better be handled with divider clock having a minimum value (being discussed with Mike on how exactly it should be) and instead of setting rate over a platform specific clock, you can set rate over lcd clock using SET_RATE_PARENT at platform level, more below,

I looked at that patch you were proposing for da8xx-fb.. to be honest, it did not seem simpler to me, so I was sort of failing to see the benefit..

Quoted 26 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c line 481

Quoted 8 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +8 -0

+     /* Configure the LCD clock divisor. */
+     tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+                     LCDC_RASTER_MODE);
+
+     if (priv->rev == 2)
+             tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+                             LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+                             LCDC_V2_CORE_CLK_EN);

Mike was suggesting to model the above using gate/divider/composite clocks of CCF in the FB driver.

Quoted 13 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +13 -0

+     priv->clk = clk_get(dev->dev, "fck");
+     if (IS_ERR(priv->clk)) {
+             dev_err(dev->dev, "failed to get functional clock\n");
+             ret = -ENODEV;
+             goto fail;
+     }
+
+     priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
+     if (IS_ERR(priv->clk)) {
+             dev_err(dev->dev, "failed to get display clock\n");
+             ret = -ENODEV;
+             goto fail;
+     }

Quoted 3 lines

"dpll_disp_ck" is a platform specific matter, driver should not be handling platform specifics. And this won't work on DaVinci, you can probably make use of

meaning the clock has a different name on davinci, or? Presumably there must be some clock generating the pixel clock for the display, but I confess to not being too familiar with the details on davinci..

Hi Rob,

Quoted 16 lines · Rob Clark

Quoted 15 lines · Afzal

drivers/gpu/drm/tilcdc/tilcdc_crtc.c line 461

Quoted 7 lines · Rob Clark

A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +3 -0

+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
 
+     /* in raster mode, minimum divisor is 2: */
+     ret = clk_set_rate(priv->disp_clk, crtc->mode.clock * 1000 * 2);

These things can better be handled with divider clock having a minimum value (being discussed with Mike on how exactly it should be) and instead of setting rate over a platform specific clock, you can set rate over lcd clock using SET_RATE_PARENT at platform level, more below,

Quoted 3 lines

I looked at that patch you were proposing for da8xx-fb.. to be honest, it did not seem simpler to me, so I was sort of failing to see the benefit..

It's not about being simple, but not doing the wrong way, here you are relying on a platform specific clock in a driver, think about the case where same IP is used on another platform. Either way it is not a good thing to handle platform specific details (about disp_clk) in driver.

And Mike mentioned that one of design goals of CCF is to model these kinds of clocks in IP's.

Quoted 30 lines

Quoted 29 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c line 481

Quoted 8 lines

drivers/gpu/drm/tilcdc/tilcdc_crtc.c new file +8 -0

+     /* Configure the LCD clock divisor. */
+     tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+                     LCDC_RASTER_MODE);
+
+     if (priv->rev == 2)
+             tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+                             LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+                             LCDC_V2_CORE_CLK_EN);

Mike was suggesting to model the above using gate/divider/composite clocks of CCF in the FB driver.

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 197

Quoted 13 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +13 -0

+     priv->clk = clk_get(dev->dev, "fck");
+     if (IS_ERR(priv->clk)) {
+             dev_err(dev->dev, "failed to get functional clock\n");
+             ret = -ENODEV;
+             goto fail;
+     }
+
+     priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
+     if (IS_ERR(priv->clk)) {
+             dev_err(dev->dev, "failed to get display clock\n");
+             ret = -ENODEV;
+             goto fail;
+     }

"dpll_disp_ck" is a platform specific matter, driver should not be handling platform specifics. And this won't work on DaVinci, you can probably make use of

Quoted 3 lines

meaning the clock has a different name on davinci, or? Presumably there must be some clock generating the pixel clock for the display, but I confess to not being too familiar with the details on davinci..

The only option for the driver in DaVinci is to configure clock rate using the divider of LCDC IP, it has "fck", which gives a rate decided by its ancestors.

Regards
Afzal

Quoted 4 lines · Afzal

It's not about being simple, but not doing the wrong way, here you are relying on a platform specific clock in a driver, think about the case where same IP is used on another platform. Either way it is not a good thing to handle platform specific details (about disp_clk) in driver.

Right, but I was trying to understand what was the benefit that the added complexity is. I didn't realize on davinci that you are limited

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 192

Quoted 3 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +2 -0

+     priv->clk = clk_get(dev->dev, "fck");
 
+     priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");

at the moment all this discussion is a bit moot. I'd propose leaving the driver as it is for now, because that at least makes it useful on am33xx. And when CCF and davinci have the needed support in place,

Hi Rob,

Quoted 9 lines · Rob Clark

Quoted 4 lines · Afzal

It's not about being simple, but not doing the wrong way, here you are relying on a platform specific clock in a driver, think about the case where same IP is used on another platform. Either way it is not a good thing to handle platform specific details (about disp_clk) in driver.

Right, but I was trying to understand what was the benefit that the added complexity is. I didn't realize on davinci that you are limited

Here I am referring to usage of disp_clk,

Driver is not supposed to do platform hacks - here you are trying to configure something (PLL) in your driver which is not part of LCDC IP. And LCDC IP is not aware of PLL which is a platform specific matter (existent only in AM335x), it is only aware of functional clock.

You can set the rate on "fck" (functional clock) in AM335x using my patch, "ARM: AM33XX: clock: SET_RATE_PARENT in lcd path", and there would not be any need for driver to be aware of platform specific PLL.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c line 192

Quoted 3 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c new file +2 -0

+     priv->clk = clk_get(dev->dev, "fck");
 
+     priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");

at the moment all this discussion is a bit moot. I'd propose leaving the driver as it is for now, because that at least makes it useful on am33xx. And when CCF and davinci have the needed support in place,

Let's forget about leveraging CCF in driver, but sane solution w.r.t PLL configuration would be to do as mentioned above.

Regards
Afzal

Driver for the NXP TDA998X i2c hdmi encoder slave.

v1: original
v2: fix npix/nline programming

Signed-off-by Rob Clark <[email protected]>

Driver for the NXP TDA998X i2c hdmi encoder slave.

v1: original
v2: fix npix/nline programming

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/i2c/Makefile      |   3 +
 drivers/gpu/drm/i2c/tda998x_drv.c | 908 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 911 insertions(+)
 create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.c

drivers/gpu/drm/i2c/tda998x_drv.c new file +5 -0

+static void __exit
+tda998x_exit(void)
+{
+	DBG("");
+	drm_i2c_encoder_unregister(&tda998x_driver);

Quoted 11 lines · On Tue, 22 Jan 2013 16:36:23 -0600 Rob Clark <[email protected]> wrote:

Driver for the NXP TDA998X i2c hdmi encoder slave.

v1: original
v2: fix npix/nline programming

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/i2c/Makefile      |   3 +
 drivers/gpu/drm/i2c/tda998x_drv.c | 908 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 911 insertions(+)
 create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.c

        [snip]

drivers/gpu/drm/i2c/tda998x_drv.c

Quoted 6 lines

drivers/gpu/drm/i2c/tda998x_drv.c new file +0 -0

new file mode 100644
index 0000000..02054e8
@@ -0,0 +1,908 @@

        [snip]

Quoted 9 lines

drivers/gpu/drm/i2c/tda998x_drv.c new file +9 -0

+static void __exit
+tda998x_exit(void)
+{
+     DBG("");
+     drm_i2c_encoder_unregister(&tda998x_driver);
+}
+
+MODULE_DESCRIPTION("NXP Semiconductors TDA998X TMDS transmitter driver");
+

drivers/gpu/drm/i2c/tda998x_drv.c line 908

Quoted 6 lines

drivers/gpu/drm/i2c/tda998x_drv.c new file +6 -0

+MODULE_AUTHOR("Rob Clark <[email protected]");
+MODULE_DESCRIPTION("NXP Semiconductors TDA998X HDMI Encoder");
+MODULE_LICENSE("GPL");
+
+module_init(tda998x_init);
+module_exit(tda998x_exit);

There are 2 MODULE_DESCRIPTION().

--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

Driver for the NXP TDA998X i2c hdmi encoder slave.

v1: original
v2: fix npix/nline programming

Signed-off-by Rob Clark <[email protected]>

Quoted 6 lines · Rob Clark

Driver for the NXP TDA998X i2c hdmi encoder slave.

v1: original
v2: fix npix/nline programming

Signed-off-by Rob Clark <[email protected]>

Just one bikeshed, otherwise

Reviewed-by Daniel Vetter <[email protected]>

[cut]

drivers/gpu/drm/i2c/tda998x_drv.c line 387

Quoted 11 lines

drivers/gpu/drm/i2c/tda998x_drv.c new file +11 -0

+static void
+reg_set(struct drm_encoder *encoder, uint16_t reg, uint8_t val)
+{
+     reg_write(encoder, reg, reg_read(encoder, reg) | val);
+}
+
+static void
+reg_clear(struct drm_encoder *encoder, uint16_t reg, uint8_t val)
+{
+     reg_write(encoder, reg, reg_read(encoder, reg) & ~val);
+}

What about drivers/base/regmap? I haven't looked to closely yet and never used it in code, but there's a presentation [1] and it sounds like it provides some nice (and more important standardized) helper stuff for debug, tracing, ...

Since encoder slave drivers tend to be utterly boring register bashing and we expect tons of time, I think high levels of standardization would be really useful. Care to look into this a bit?

Cheers, Daniel

1: http://free-electrons.com/blog/fosdem2012-videos/
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Add output panel driver for i2c encoder slaves.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/tilcdc/Kconfig        |  12 ++
 drivers/gpu/drm/tilcdc/Makefile       |   1 +
 drivers/gpu/drm/tilcdc/tilcdc_drv.c   |   5 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c | 380 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_slave.h |  26 +++
 5 files changed, 423 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h

drivers/gpu/drm/tilcdc/Kconfig +2 -0

index ee9b592..99beca2 100644
@@ -8,3 +8,15 @@ config DRM_TILCDC
 	  Choose this option if you have an TI SoC with LCDC display
 	  controller, for example AM33xx in beagle-bone, DA8xx, or
 	  OMAP-L1xx.  This driver replaces the FB_DA8XX fbdev driver.
+
+menu "I2C encoder or helper chips"

drivers/gpu/drm/tilcdc/Makefile +1 -0

index 1359cc2..aa9097e 100644
@@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm -Werror
 tilcdc-y := \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
+	tilcdc_slave.o \
 	tilcdc_drv.o
 
 obj-$(CONFIG_DRM_TILCDC)	+= tilcdc.o

drivers/gpu/drm/tilcdc/tilcdc_drv.c +4 -1

index cf1fddc..ca76dbe 100644
@@ -20,6 +20,7 @@
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
 #include "tilcdc_tfp410.h"
+#include "tilcdc_slave.h"
 
 #include "drm_fb_helper.h"
 

@@ -587,6 +588,7 @@ static int __init tilcdc_drm_init(void)
 {
 	DBG("init");
 	tilcdc_tfp410_init();
+	tilcdc_slave_init();
 	return platform_driver_register(&tilcdc_platform_driver);
 }
 

@@ -594,10 +596,11 @@ static void __exit tilcdc_drm_fini(void)
 {
 	DBG("fini");
 	tilcdc_tfp410_fini();
+	tilcdc_slave_fini();
 	platform_driver_unregister(&tilcdc_platform_driver);
 }
 
-module_init(tilcdc_drm_init);
+late_initcall(tilcdc_drm_init);
 module_exit(tilcdc_drm_fini);
 
 MODULE_AUTHOR("Rob Clark <[email protected]");

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +47 -0

new file mode 100644
index 0000000..b6f3e63
@@ -0,0 +1,380 @@
+/*
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Rob Clark <[email protected]>
+ *
+ * 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 Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/i2c.h>
+#include <linux/of_i2c.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/consumer.h>
+#include <drm/drm_encoder_slave.h>
+
+#include "tilcdc_drv.h"
+
+struct slave_module {
+	struct tilcdc_module base;
+	struct i2c_adapter *i2c;
+};
+#define to_slave_module(x) container_of(x, struct slave_module, base)
+
+static const struct tilcdc_panel_info slave_info = {
+		.min_bpp                = 16,
+		.max_bpp                = 16,
+		.bpp                    = 16,
+		.ac_bias                = 255,
+		.ac_bias_intrpt         = 0,
+		.dma_burst_sz           = 16,
+		.fdd                    = 0x80,
+		.tft_alt_mode           = 0,
+		.stn_565_mode           = 0,
+		.mono_8bit_mode         = 0,
+		.sync_edge              = 0,
+		.sync_ctrl              = 1,
+		.raster_order           = 0,
+};
+

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +50 -0

+
+static inline struct drm_encoder_slave_funcs *
+get_slave_funcs(struct drm_encoder *enc)
+{
+	return to_encoder_slave(enc)->slave_funcs;
+}
+
+static void slave_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct slave_encoder *slave_encoder = to_slave_encoder(encoder);
+	if (get_slave_funcs(encoder))
+		get_slave_funcs(encoder)->destroy(encoder);
+	drm_encoder_cleanup(encoder);
+	kfree(slave_encoder);
+}
+
+static void slave_encoder_prepare(struct drm_encoder *encoder)
+{
+	drm_i2c_encoder_prepare(encoder);
+	tilcdc_crtc_set_panel_info(encoder->crtc, &slave_info);
+}
+
+static const struct drm_encoder_funcs slave_encoder_funcs = {
+		.destroy        = slave_encoder_destroy,
+};
+
+static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
+		.dpms           = drm_i2c_encoder_dpms,
+		.mode_fixup     = drm_i2c_encoder_mode_fixup,
+		.prepare        = slave_encoder_prepare,
+		.commit         = drm_i2c_encoder_commit,
+		.mode_set       = drm_i2c_encoder_mode_set,
+		.save           = drm_i2c_encoder_save,
+		.restore        = drm_i2c_encoder_restore,
+};
+
+static const struct i2c_board_info info = {
+		I2C_BOARD_INFO("tda998x", 0x70)
+};
+
+static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
+		struct slave_module *mod)
+{
+	struct slave_encoder *slave_encoder;
+	struct drm_encoder *encoder;
+	int ret;
+
+	slave_encoder = kzalloc(sizeof(*slave_encoder), GFP_KERNEL);
+	if (!slave_encoder) {
+		dev_err(dev->dev, "allocation failed\n");

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +102 -0

+	if (ret)
+		goto fail;
+
+	drm_encoder_helper_add(encoder, &slave_encoder_helper_funcs);
+
+	ret = drm_i2c_encoder_init(dev, to_encoder_slave(encoder), mod->i2c, &info);
+	if (ret)
+		goto fail;
+
+	return encoder;
+
+fail:
+	slave_encoder_destroy(encoder);
+	return NULL;
+}
+
+/*
+ * Connector:
+ */
+
+struct slave_connector {
+	struct drm_connector base;
+
+	struct drm_encoder *encoder;  /* our connected encoder */
+	struct slave_module *mod;
+};
+#define to_slave_connector(x) container_of(x, struct slave_connector, base)
+
+static void slave_connector_destroy(struct drm_connector *connector)
+{
+	struct slave_connector *slave_connector = to_slave_connector(connector);
+	drm_connector_cleanup(connector);
+	kfree(slave_connector);
+}
+
+static enum drm_connector_status slave_connector_detect(
+		struct drm_connector *connector,
+		bool force)
+{
+	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
+	return get_slave_funcs(encoder)->detect(encoder, connector);
+}
+
+static int slave_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
+	return get_slave_funcs(encoder)->get_modes(encoder, connector);
+}
+
+static int slave_connector_mode_valid(struct drm_connector *connector,
+		  struct drm_display_mode *mode)
+{
+	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
+	struct tilcdc_drm_private *priv = connector->dev->dev_private;
+	int ret;
+
+	ret = tilcdc_crtc_mode_valid(priv->crtc, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	return get_slave_funcs(encoder)->mode_valid(encoder, mode);
+}
+
+static struct drm_encoder *slave_connector_best_encoder(
+		struct drm_connector *connector)
+{
+	struct slave_connector *slave_connector = to_slave_connector(connector);
+	return slave_connector->encoder;
+}
+
+static int slave_connector_set_property(struct drm_connector *connector,
+		struct drm_property *property, uint64_t value)
+{
+	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
+	return get_slave_funcs(encoder)->set_property(encoder,
+			connector, property, value);
+}
+
+static const struct drm_connector_funcs slave_connector_funcs = {
+	.destroy            = slave_connector_destroy,
+	.dpms               = drm_helper_connector_dpms,
+	.detect             = slave_connector_detect,
+	.fill_modes         = drm_helper_probe_single_connector_modes,
+	.set_property       = slave_connector_set_property,
+};
+
+static const struct drm_connector_helper_funcs slave_connector_helper_funcs = {
+	.get_modes          = slave_connector_get_modes,
+	.mode_valid         = slave_connector_mode_valid,
+	.best_encoder       = slave_connector_best_encoder,
+};
+
+static struct drm_connector *slave_connector_create(struct drm_device *dev,
+		struct slave_module *mod, struct drm_encoder *encoder)
+{
+	struct slave_connector *slave_connector;
+	struct drm_connector *connector;
+	int ret;
+
+	slave_connector = kzalloc(sizeof(*slave_connector), GFP_KERNEL);
+	if (!slave_connector) {
+		dev_err(dev->dev, "allocation failed\n");

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +131 -0

+	drm_connector_helper_add(connector, &slave_connector_helper_funcs);
+
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
+
+	connector->interlace_allowed = 0;
+	connector->doublescan_allowed = 0;
+
+	get_slave_funcs(encoder)->create_resources(encoder, connector);
+
+	ret = drm_mode_connector_attach_encoder(connector, encoder);
+	if (ret)
+		goto fail;
+
+	drm_sysfs_connector_add(connector);
+
+	return connector;
+
+fail:
+	slave_connector_destroy(connector);
+	return NULL;
+}
+
+/*
+ * Module:
+ */
+
+static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
+{
+	struct slave_module *slave_mod = to_slave_module(mod);
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+
+	encoder = slave_encoder_create(dev, slave_mod);
+	if (!encoder)
+		return -ENOMEM;
+
+	connector = slave_connector_create(dev, slave_mod, encoder);
+	if (!connector)
+		return -ENOMEM;
+
+	priv->encoders[priv->num_encoders++] = encoder;
+	priv->connectors[priv->num_connectors++] = connector;
+
+	return 0;
+}
+
+static void slave_destroy(struct tilcdc_module *mod)
+{
+	struct slave_module *slave_mod = to_slave_module(mod);
+
+	tilcdc_module_cleanup(mod);
+	kfree(slave_mod);
+}
+
+static const struct tilcdc_module_ops slave_module_ops = {
+		.modeset_init = slave_modeset_init,
+		.destroy = slave_destroy,
+};
+
+/*
+ * Device:
+ */
+
+static struct of_device_id slave_of_match[];
+
+static int slave_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *i2c_node;
+	struct slave_module *slave_mod;
+	struct tilcdc_module *mod;
+	struct pinctrl *pinctrl;
+	uint32_t i2c_phandle;
+	int ret = -EINVAL;
+
+	/* bail out early if no DT data: */
+	if (!node) {
+		dev_err(&pdev->dev, "device-tree data is missing\n");
+		return -ENXIO;
+	}
+
+	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
+	if (!slave_mod)
+		return -ENOMEM;
+
+	mod = &slave_mod->base;
+
+	tilcdc_module_init(mod, "slave", &slave_module_ops);
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "pins are not configured\n");
+
+	if (of_property_read_u32(node, "i2c", &i2c_phandle)) {
+		dev_err(&pdev->dev, "could not get i2c bus phandle\n");
+		goto fail;
+	}
+
+	i2c_node = of_find_node_by_phandle(i2c_phandle);
+	if (!i2c_node) {
+		dev_err(&pdev->dev, "could not get i2c bus node\n");
+		goto fail;
+	}
+
+	slave_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
+	if (!slave_mod->i2c) {
+		dev_err(&pdev->dev, "could not get i2c\n");
+		goto fail;
+	}
+
+	of_node_put(i2c_node);
+
+	return 0;
+
+fail:
+	slave_destroy(mod);
+	return ret;
+}
+
+static int slave_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct of_device_id slave_of_match[] = {
+		{ .compatible = "tilcdc,slave", },
+		{ },
+};
+MODULE_DEVICE_TABLE(of, slave_of_match);

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +10 -0

+
+int __init tilcdc_slave_init(void)
+{
+	return platform_driver_register(&slave_driver);
+}
+
+void __exit tilcdc_slave_fini(void)
+{
+	platform_driver_unregister(&slave_driver);
+}

drivers/gpu/drm/tilcdc/tilcdc_slave.h new file +26 -0

new file mode 100644
index 0000000..2f85048
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Rob Clark <[email protected]>
+ *
+ * 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 Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __TILCDC_SLAVE_H__
+#define __TILCDC_SLAVE_H__
+
+/* sub-module for i2c slave encoder output */
+
+int tilcdc_slave_init(void);
+void tilcdc_slave_fini(void);
+
+#endif /* __TILCDC_SLAVE_H__ */

Signature · 6 lines

1.8.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 3 lines · Rob Clark

Add output panel driver for i2c encoder slaves.

Signed-off-by Rob Clark <[email protected]>

A few questions below, otherwise

Reviewed-by Daniel Vetter <[email protected]>

Quoted 1 line

---

Quoted 14 lines

drivers/gpu/drm/tilcdc/Kconfig +6 -0

index ee9b592..99beca2 100644
@@ -8,3 +8,15 @@ config DRM_TILCDC
        Choose this option if you have an TI SoC with LCDC display
        controller, for example AM33xx in beagle-bone, DA8xx, or
        OMAP-L1xx.  This driver replaces the FB_DA8XX fbdev driver.
+
+menu "I2C encoder or helper chips"
+     depends on DRM && DRM_KMS_HELPER && I2C
+
+config DRM_I2C_NXP_TDA998X
+     tristate "NXP Semiconductors TDA998X HDMI encoder"

drivers/gpu/drm/tilcdc/Kconfig config DRM_TILCDC · line 22

Quoted 6 lines

drivers/gpu/drm/tilcdc/Kconfig +6 -0

+     default m if DRM_TILCDC
+     help
+       Support for NXP Semiconductors TDA998X HDMI encoders.
+
+endmenu
+

Shouldn't that hunk be in patch 2?

Quoted 5 lines

drivers/gpu/drm/tilcdc/Kconfig +5 -0

+/*
+ * Encoder:
+ */
+
+struct slave_encoder {

drivers/gpu/drm/tilcdc/Kconfig

Quoted 4 lines

drivers/gpu/drm/tilcdc/Kconfig +4 -0

+     struct drm_encoder_slave base;
+     struct slave_module *mod;
+};
+#define to_slave_encoder(x) container_of(to_encoder_slave(x), struct slave_encoder, base)

Since you have a 1:1:1 relationship between module/drm_encoder, the drm_encoder_slave and the connector I'd just smash this all into one struct. Pure bikeshed though ;-)

Quoted 18 lines

drivers/gpu/drm/tilcdc/Kconfig +18 -0

+static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
+             struct slave_module *mod)
+{
+     struct slave_encoder *slave_encoder;
+     struct drm_encoder *encoder;
+     int ret;
+
+     slave_encoder = kzalloc(sizeof(*slave_encoder), GFP_KERNEL);
+     if (!slave_encoder) {
+             dev_err(dev->dev, "allocation failed\n");
+             return NULL;
+     }
+
+     slave_encoder->mod = mod;
+
+     encoder = &slave_encoder->base.base;
+     encoder->possible_crtcs = 1;
+

drivers/gpu/drm/tilcdc/Kconfig

Quoted 2 lines

drivers/gpu/drm/tilcdc/Kconfig +2 -0

+     ret = drm_encoder_init(dev, encoder, &slave_encoder_funcs,
+                     DRM_MODE_ENCODER_LVDS);

DRM_MODE_ENCODER_TMDS? Although I guess adding a new kind of multi-function encoder type would make more sense and also useful in other places. E.g. i915-sdvo/dvo just set meaningless types for multi-function encoders (i.e. more than one connector on the same output), namely the type which matches the connector last initalized.

Quoted 16 lines

drivers/gpu/drm/tilcdc/Kconfig +16 -0

+static struct drm_connector *slave_connector_create(struct drm_device *dev,
+             struct slave_module *mod, struct drm_encoder *encoder)
+{
+     struct slave_connector *slave_connector;
+     struct drm_connector *connector;
+     int ret;
+
+     slave_connector = kzalloc(sizeof(*slave_connector), GFP_KERNEL);
+     if (!slave_connector) {
+             dev_err(dev->dev, "allocation failed\n");
+             return NULL;
+     }
+
+     slave_connector->encoder = encoder;
+     slave_connector->mod = mod;
+

drivers/gpu/drm/tilcdc/Kconfig

Quoted 4 lines

drivers/gpu/drm/tilcdc/Kconfig +4 -0

+     connector = &slave_connector->base;
+
+     drm_connector_init(dev, connector, &slave_connector_funcs,
+                     DRM_MODE_CONNECTOR_HDMIA);

Shouldn't we get the connector type from the drm_encoder_slave somehow? Works here for now, so just food for thought for future encoder slave refactorings and infrastructure work.

Quoted 10 lines

drivers/gpu/drm/tilcdc/Kconfig +10 -0

+static struct of_device_id slave_of_match[] = {
+             { .compatible = "tilcdc,slave", },
+             { },
+};
+MODULE_DEVICE_TABLE(of, slave_of_match);
+
+struct platform_driver slave_driver = {
+     .probe = slave_probe,
+     .remove = slave_remove,
+     .driver = {

drivers/gpu/drm/tilcdc/Kconfig

Quoted 5 lines

drivers/gpu/drm/tilcdc/Kconfig +5 -0

+             .owner = THIS_MODULE,
+             .name = "slave",
+             .of_match_table = slave_of_match,
+     },
+};

No idea how this devicetree matching stuff is supposed to work, but I guess this needs to be updated in the devictree docs like the base match.

Add output panel driver for i2c encoder slaves.

Signed-off-by Rob Clark <[email protected]>

Quoted 3 lines · Rob Clark

Add output panel driver for i2c encoder slaves.

Signed-off-by Rob Clark <[email protected]>

Found some more stuff ...

[cut]

drivers/gpu/drm/tilcdc/tilcdc_slave.c line 92

Quoted 9 lines

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +9 -0

+static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
+             .dpms           = drm_i2c_encoder_dpms,
+             .mode_fixup     = drm_i2c_encoder_mode_fixup,
+             .prepare        = slave_encoder_prepare,
+             .commit         = drm_i2c_encoder_commit,
+             .mode_set       = drm_i2c_encoder_mode_set,
+             .save           = drm_i2c_encoder_save,
+             .restore        = drm_i2c_encoder_restore,
+};

I couldn't find these wrappers anywhere ...

drivers/gpu/drm/tilcdc/tilcdc_slave.c line 96

Quoted 4 lines

drivers/gpu/drm/tilcdc/tilcdc_slave.c new file +4 -0

+
+static const struct i2c_board_info info = {
+             I2C_BOARD_INFO("tda998x", 0x70)
+};

Shouldn't there be some of/devicetree thing to tell us which one to load?

-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Add an output panel driver for LCD panels. Tested with LCD3 cape on beaglebone.

v1: original
v2: s/of_find_node_by_name()/of_get_child_by_name()/ from Pantelis
Antoniou
v3: add backlight support
v4: rebase to latest of video timing helpers

Signed-off-by Rob Clark <[email protected]>

drivers/gpu/drm/tilcdc/tilcdc_panel.c new file +5 -0

+static int panel_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct panel_connector *panel_connector = to_panel_connector(connector);
+	struct display_timings *timings = panel_connector->mod->timings;

drivers/gpu/drm/tilcdc/tilcdc_panel.c new file +54 -0

+
+	/* optional: */
+	info->tft_alt_mode      = of_property_read_bool(info_np, "tft-alt-mode");
+	info->stn_565_mode      = of_property_read_bool(info_np, "stn-565-mode");
+	info->mono_8bit_mode    = of_property_read_bool(info_np, "mono-8bit-mode");
+	info->invert_pxl_clk    = of_property_read_bool(info_np, "invert-pxl-clk");
+
+	if (of_property_read_u32(info_np, "max-bpp", &info->max_bpp))
+		info->max_bpp = info->bpp;
+	if (of_property_read_u32(info_np, "min-bpp", &info->min_bpp))
+		info->min_bpp = info->bpp;
+
+	if (ret) {
+		pr_err("%s: error reading panel-info properties\n", __func__);
+		kfree(info);
+		return NULL;
+	}
+
+	return info;
+}
+
+static struct of_device_id panel_of_match[];
+
+static int panel_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct panel_module *panel_mod;
+	struct tilcdc_module *mod;
+	struct pinctrl *pinctrl;
+	int ret = -EINVAL;
+
+
+	/* bail out early if no DT data: */
+	if (!node) {
+		dev_err(&pdev->dev, "device-tree data is missing\n");
+		return -ENXIO;
+	}
+
+	panel_mod = kzalloc(sizeof(*panel_mod), GFP_KERNEL);
+	if (!panel_mod)
+		return -ENOMEM;
+
+	mod = &panel_mod->base;
+
+	tilcdc_module_init(mod, "panel", &panel_module_ops);
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "pins are not configured\n");
+
+
+	panel_mod->timings = of_get_display_timings(node);
+	if (!panel_mod->timings) {
+		dev_err(&pdev->dev, "could not get panel timings\n");

Quoted 10 lines · Rob Clark

Add an output panel driver for LCD panels. Tested with LCD3 cape on beaglebone.

v1: original
v2: s/of_find_node_by_name()/of_get_child_by_name()/ from Pantelis
Antoniou
v3: add backlight support
v4: rebase to latest of video timing helpers

Signed-off-by Rob Clark <[email protected]>

So given that I'm utterly lacking clue about all things of (which seems to be where all the magic in this patch lies) I'm just gonna ask a few funny questions.

[cut]

Quoted 10 lines

drivers/gpu/drm/tilcdc/tilcdc_panel.c new file +10 -0

+static int panel_connector_get_modes(struct drm_connector *connector)
+{
+     struct drm_device *dev = connector->dev;
+     struct panel_connector *panel_connector = to_panel_connector(connector);
+     struct display_timings *timings = panel_connector->mod->timings;
+     int i;
+
+     for (i = 0; i < timings->num_timings; i++) {
+             struct drm_display_mode *mode = drm_mode_create(dev);
+             struct videomode vm;

drivers/gpu/drm/tilcdc/tilcdc_panel.c line 179

Quoted 5 lines

drivers/gpu/drm/tilcdc/tilcdc_panel.c new file +5 -0

+
+             if (videomode_from_timing(timings, &vm, i))
+                     break;
+
+             drm_display_mode_from_videomode(&vm, mode);

Why do we need to jump through the intermediate videomode thing here? Is that a deficiency of the of/videomode stuff?

[cut]

drivers/gpu/drm/tilcdc/tilcdc_panel.c line 339

Quoted 9 lines

drivers/gpu/drm/tilcdc/tilcdc_panel.c new file +9 -0

+     ret |= of_property_read_u32(info_np, "ac-bias", &info->ac_bias);
+     ret |= of_property_read_u32(info_np, "ac-bias-intrpt", &info->ac_bias_intrpt);
+     ret |= of_property_read_u32(info_np, "dma-burst-sz", &info->dma_burst_sz);
+     ret |= of_property_read_u32(info_np, "bpp", &info->bpp);
+     ret |= of_property_read_u32(info_np, "fdd", &info->fdd);
+     ret |= of_property_read_u32(info_np, "sync-edge", &info->sync_edge);
+     ret |= of_property_read_u32(info_np, "sync-ctrl", &info->sync_ctrl);
+     ret |= of_property_read_u32(info_np, "raster-order", &info->raster_order);
+     ret |= of_property_read_u32(info_np, "fifo-th", &info->fifo_th);

Shouldn't these values all be documented somewhere in the devictree docs? Or are they somewhat standardized?

 drivers/gpu/drm/Kconfig                |   2 +
 drivers/gpu/drm/Makefile               |   1 +
 drivers/gpu/drm/i2c/Makefile           |   3 +
 drivers/gpu/drm/i2c/tda998x_drv.c      | 908 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/Kconfig         |  25 +
 drivers/gpu/drm/tilcdc/Makefile        |  10 +
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   | 597 ++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 611 ++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    | 159 ++++++
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 443 ++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_panel.h  |  26 +
 drivers/gpu/drm/tilcdc/tilcdc_regs.h   | 154 ++++++
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  | 380 ++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_slave.h  |  26 +
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h |  26 +
 16 files changed, 3794 insertions(+)
 create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.c
 create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
 create mode 100644 drivers/gpu/drm/tilcdc/Makefile
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c

 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_panel.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_panel.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h