Hi Sergei,

Thank you for the patch.

On Friday 29 Apr 2016 00:03:29 Sergei Shtylyov wrote:

Renesas R-Car SoCs include the timing controller (TCON) that can directly drive LCDs. It receives the H/VSYNC, etc. from the Display Unit (DU) and converts them to the set of signals that a LCD panel can understand (the RBG signals are effectively passed thru). TCON has a set of registers that we need to program based on the video mode timings, so we're adding a DU encoder driver doing that...

Based on a large patch by Andrey Gusakov.

Signed-off-by: Andrey Gusakov andrey.gusakov@cogentembedded.com Signed-off-by: Sergei Shtylyov sergei.shtylyov@cogentembedded.com

---

drivers/gpu/drm/rcar-du/Kconfig | 6 drivers/gpu/drm/rcar-du/Makefile | 1 drivers/gpu/drm/rcar-du/rcar_du_drv.h | 4 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 9 + drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 3 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 4 drivers/gpu/drm/rcar-du/rcar_du_tconenc.c | 184 ++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_tconenc.h | 37 ++++++ drivers/gpu/drm/rcar-du/rcar_tcon_regs.h | 70 +++++++++++ 9 files changed, 318 insertions(+)

Index: linux/drivers/gpu/drm/rcar-du/Kconfig

--- linux.orig/drivers/gpu/drm/rcar-du/Kconfig +++ linux/drivers/gpu/drm/rcar-du/Kconfig @@ -24,6 +24,12 @@ config DRM_RCAR_LVDS help Enable support for the R-Car Display Unit embedded LVDS encoders.

+config DRM_RCAR_TCON

• bool "R-Car DU TCON Encoder Support"
• depends on DRM_RCAR_DU
• help

•  Enable support for the R-Car Display Unit embedded TCON encoders.
  

config DRM_RCAR_VSP bool "R-Car DU VSP Compositor Support" depends on DRM_RCAR_DU Index: linux/drivers/gpu/drm/rcar-du/Makefile =================================================================== --- linux.orig/drivers/gpu/drm/rcar-du/Makefile +++ linux/drivers/gpu/drm/rcar-du/Makefile @@ -10,6 +10,7 @@ rcar-du-drm-y := rcar_du_crtc.o \ rcar-du-drm-$(CONFIG_DRM_RCAR_HDMI) += rcar_du_hdmicon.o \ rcar_du_hdmienc.o rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_lvdsenc.o +rcar-du-drm-$(CONFIG_DRM_RCAR_TCON) += rcar_du_tconenc.o

rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o

Index: linux/drivers/gpu/drm/rcar-du/rcar_du_drv.h

--- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ linux/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -59,6 +59,7 @@ struct rcar_du_output_routing {

• @num_crtcs: total number of CRTCs
• @routes: array of CRTC to output routes, indexed by output

(RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders

• • @num_tcon: number of internal TCON encoders
  */

struct rcar_du_device_info { unsigned int gen; @@ -67,11 +68,13 @@ struct rcar_du_device_info { unsigned int num_crtcs; struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX]; unsigned int num_lvds;

• unsigned int num_tcon;

};

#define RCAR_DU_MAX_CRTCS 4 #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2) #define RCAR_DU_MAX_LVDS 2 +#define RCAR_DU_MAX_TCON 1 #define RCAR_DU_MAX_VSPS 4

struct rcar_du_device { @@ -99,6 +102,7 @@ struct rcar_du_device { unsigned int vspd1_sink;

struct rcar_du_lvdsenc *lvds[RCAR_DU_MAX_LVDS];

• struct rcar_du_tconenc *tcon[RCAR_DU_MAX_TCON];

  struct { wait_queue_head_t wait;

Index: linux/drivers/gpu/drm/rcar-du/rcar_du_encoder.c

--- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ linux/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -24,6 +24,7 @@ #include "rcar_du_kms.h" #include "rcar_du_lvdscon.h" #include "rcar_du_lvdsenc.h" +#include "rcar_du_tconenc.h" #include "rcar_du_vgacon.h"

/* ------------------------------------------------------------------------ @@ -48,6 +49,8 @@ static void rcar_du_encoder_disable(stru

if (renc->lvds) rcar_du_lvdsenc_enable(renc->lvds, encoder->crtc, false);

• if (renc->tcon)

• rcar_du_tconenc_enable(renc->tcon, encoder->crtc, false);
  

}

static void rcar_du_encoder_enable(struct drm_encoder *encoder) @@ -56,6 +59,8 @@ static void rcar_du_encoder_enable(struc

if (renc->lvds) rcar_du_lvdsenc_enable(renc->lvds, encoder->crtc, true);

• if (renc->tcon)

• rcar_du_tconenc_enable(renc->tcon, encoder->crtc, true);
  

}

static int rcar_du_encoder_atomic_check(struct drm_encoder *encoder, @@ -142,6 +147,10 @@ int rcar_du_encoder_init(struct rcar_du_ renc->lvds = rcdu->lvds[1]; break;

• case RCAR_DU_OUTPUT_TCON:

• renc->tcon = rcdu->tcon[0];
  

• break;
  

• default: break; }

Index: linux/drivers/gpu/drm/rcar-du/rcar_du_encoder.h

--- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_encoder.h +++ linux/drivers/gpu/drm/rcar-du/rcar_du_encoder.h @@ -20,6 +20,7 @@ struct rcar_du_device; struct rcar_du_hdmienc; struct rcar_du_lvdsenc; +struct rcar_du_tconenc;

enum rcar_du_encoder_type { RCAR_DU_ENCODER_UNUSED = 0, @@ -27,6 +28,7 @@ enum rcar_du_encoder_type { RCAR_DU_ENCODER_VGA, RCAR_DU_ENCODER_LVDS, RCAR_DU_ENCODER_HDMI,

• RCAR_DU_ENCODER_TCON,

};

struct rcar_du_encoder { @@ -34,6 +36,7 @@ struct rcar_du_encoder { enum rcar_du_output output; struct rcar_du_hdmienc *hdmi; struct rcar_du_lvdsenc *lvds;

• struct rcar_du_tconenc *tcon;

};

#define to_rcar_encoder(e) \ Index: linux/drivers/gpu/drm/rcar-du/rcar_du_kms.c =================================================================== --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ linux/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -27,6 +27,7 @@ #include "rcar_du_encoder.h" #include "rcar_du_kms.h" #include "rcar_du_lvdsenc.h" +#include "rcar_du_tconenc.h" #include "rcar_du_regs.h" #include "rcar_du_vsp.h"

@@ -619,6 +620,9 @@ int rcar_du_modeset_init(struct rcar_du_ ret = rcar_du_lvdsenc_init(rcdu); if (ret < 0) return ret;

• ret = rcar_du_tconenc_init(rcdu);

• if (ret < 0)

• return ret;
  
  

  ret = rcar_du_encoders_init(rcdu); if (ret < 0)

Index: linux/drivers/gpu/drm/rcar-du/rcar_du_tconenc.c

--- /dev/null +++ linux/drivers/gpu/drm/rcar-du/rcar_du_tconenc.c @@ -0,0 +1,184 @@ +/*

• • rcar_du_tconenc.c -- R-Car Display Unit TCON Encoder
• • Copyright (C) 2015-2016 Cogent Embedded, Inc.

source@cogentembedded.com

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.
• */

+#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <drm/drm_encoder_slave.h>

+#include "rcar_du_drv.h" +#include "rcar_du_encoder.h" +#include "rcar_du_tconenc.h" +#include "rcar_tcon_regs.h"

+struct rcar_du_tconenc {

• struct rcar_du_device *dev;
• unsigned int index;
• void __iomem *mmio;
• struct clk *clock;
• bool enabled;

+};

+static void rcar_tcon_write(struct rcar_du_tconenc *tcon, u32 reg, u32 data) +{

• iowrite32(data, tcon->mmio + reg);

+}

+static int rcar_du_tconenc_start(struct rcar_du_tconenc *tcon,

• 		 struct rcar_du_crtc *rcrtc)
  

+{

• const struct drm_display_mode *mode = &rcrtc->crtc.mode;
• int ret;
• if (tcon->enabled)

Now that the DU driver implements the atomic API the DRM/KMS core is supposed to only enable/disable CRTCs and encoders when needed. Can this still happen ? Same comment for the stop function.

• return 0;
  

• ret = clk_prepare_enable(tcon->clock);
• if (ret < 0)

• return ret;
  

• /* Update */
• rcar_tcon_write(tcon, OUT_V_LATCH, OUTCNT_VEN);
• rcar_tcon_write(tcon, TCON_V_LATCH, TCON_VEN);

Aren't you supposed to set those bits after modifying the registers only, not before ?

• /* Signals:

• * R-Car	Display
  

• * QCLK		SSC (Source Driver Clock Input)
  

• * QSTH		SSP (Source Scanning Signal Left/Right)
  

• * QSTB		SOE (Source Driver Output Enable)
  

• * QCPV		GOE (Gate Driver Output Enable)
  

• * QPOLA	POL (Polarity Control Signal)
  

• * QPOLB	GSC (Gate Driver Scanning Clock)
  

• * QSTVA	GSP (Gate Scanning Start Signal Up/Down)
  

• * QSTVB	nope
  

• */
  

• /* Setup timings */
• rcar_tcon_write(tcon, TCON_TIM, TCON_TIMING(50, 0));
• /* Horizontal timings */
• rcar_tcon_write(tcon, TCON_TIM_STH1, TCON_TIMING(mode->htotal -

• 					 mode->hsync_start - 1,
  

• 					 1));
  

• rcar_tcon_write(tcon, TCON_TIM_STH2, TCON_SEL_STH_SP_HS);
• rcar_tcon_write(tcon, TCON_TIM_STB1, TCON_TIMING(mode->htotal -

• 					 mode->hsync_start +
  

• 					 mode->hdisplay + 6,
  

• 					 3));
  

• rcar_tcon_write(tcon, TCON_TIM_STB2, TCON_SEL_STB_LP_HE);
• rcar_tcon_write(tcon, TCON_TIM_POLB1,

• 	TCON_TIMING(mode->htotal - mode->hsync_start +
  

• 		    mode->hdisplay - 8, mode->htotal / 2));
  

• rcar_tcon_write(tcon, TCON_TIM_POLB2,

• 	TCON_SEL_POLB | TCON_INV | TCON_MD_NORM);
  

• rcar_tcon_write(tcon, TCON_TIM_CPV1,

• 	TCON_TIMING(mode->htotal - mode->hsync_start +
  

• 		    mode->hdisplay - 33, 50));
  

• rcar_tcon_write(tcon, TCON_TIM_CPV2, TCON_SEL_CPV_GCK);
• rcar_tcon_write(tcon, TCON_TIM_POLA1,

• 	TCON_TIMING(mode->htotal - mode->hsync_start +
  

• 		    mode->hdisplay + 1, 39));
  

• rcar_tcon_write(tcon, TCON_TIM_POLA2,

• 	TCON_SEL_POLA | TCON_INV | TCON_MD_1X1);
  

• /* Vertical timings */
• rcar_tcon_write(tcon, TCON_TIM_STVA1,

• 	TCON_TIMING(mode->vtotal - mode->vsync_start, 1));
  

• rcar_tcon_write(tcon, TCON_TIM_STVA2, TCON_SEL_STVA_VS);
• rcar_tcon_write(tcon, TCON_TIM_STVB1, TCON_TIMING(0, 0));
• rcar_tcon_write(tcon, TCON_TIM_STVB2, TCON_SEL_STVB_VE);
• /* Data clocked out on the falling edge of QCLK, latched in LCD on

• * the rising edge
  

• */
  

• rcar_tcon_write(tcon, OUT_CLK_PHASE, OUTCNT_LCD_EDGE);

I can't verify all those settings as I have no idea how TCON operates. However, it looks like we have lots of magic numbers, and I have a feeling those actually depend on the panel model. Am I wrong ?

• /* Update */
• rcar_tcon_write(tcon, OUT_V_LATCH, OUTCNT_VEN);
• rcar_tcon_write(tcon, TCON_V_LATCH, TCON_VEN);
• tcon->enabled = true;
• return 0;

+}

+static void rcar_du_tconenc_stop(struct rcar_du_tconenc *tcon) +{

• if (!tcon->enabled)

• return;
  

• clk_disable_unprepare(tcon->clock);
• tcon->enabled = false;

+}

+int rcar_du_tconenc_enable(struct rcar_du_tconenc *tcon, struct drm_crtc *crtc,

• 	   bool enable)
  

+{

• if (!enable) {

• rcar_du_tconenc_stop(tcon);
  

• return 0;
  

• } else if (crtc) {

• struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
  

• return rcar_du_tconenc_start(tcon, rcrtc);
  

• } else

• return -EINVAL;
  

Missing { } around the last branch. Is this needed though, can enable be true and crtc NULL ? If so, under which circumstances ?

+}

+static int rcar_du_tconenc_get_resources(struct rcar_du_tconenc *tcon,

• 			 struct platform_device *pdev)
  

+{

• struct resource *mem;
• char name[7];
• sprintf(name, "tcon.%u", tcon->index);
• mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
• tcon->mmio = devm_ioremap_resource(&pdev->dev, mem);
• if (IS_ERR(tcon->mmio))

• return PTR_ERR(tcon->mmio);
  

• tcon->clock = devm_clk_get(&pdev->dev, name);
• if (IS_ERR(tcon->clock)) {

• dev_err(&pdev->dev, "failed to get clock for %s\n", name);
  

• return PTR_ERR(tcon->clock);
  

• }

I wasn't very proud of my very similar implementation of the LVDS encoder code. It's a separate IP core, it should have been modeled by a separate DT node. I can't really ask you to do so for TCON given that I haven't done it for LVDS though, but I suspect we'll pay the price at some point (for instance when we'll have an SoC with the DU and TCON in separate power domains). If you have a clever idea to enhance this, please let me know.

• return 0;

+}

+int rcar_du_tconenc_init(struct rcar_du_device *rcdu) +{

• struct platform_device *pdev = to_platform_device(rcdu->dev);
• struct rcar_du_tconenc *tcon;
• unsigned int i;
• int ret;
• for (i = 0; i < rcdu->info->num_tcon; ++i) {

• tcon = devm_kzalloc(&pdev->dev, sizeof(*tcon), GFP_KERNEL);
  

• if (tcon == NULL)
  

• 	return -ENOMEM;
  

• tcon->dev = rcdu;
  

• tcon->index = i;
  

• tcon->enabled = false;
  

• ret = rcar_du_tconenc_get_resources(tcon, pdev);
  

• if (ret < 0)
  

• 	return ret;
  

• rcdu->tcon[i] = tcon;
  

• }
• return 0;

+} Index: linux/drivers/gpu/drm/rcar-du/rcar_du_tconenc.h =================================================================== --- /dev/null +++ linux/drivers/gpu/drm/rcar-du/rcar_du_tconenc.h @@ -0,0 +1,37 @@ +/*

• • rcar_du_tconenc.h -- R-Car Display Unit TCON Encoder
• • Copyright (C) 2015-2016 CogentEmbedded, Inc. source@cogentembedded.com
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.
• */

+#ifndef __RCAR_DU_TCONENC_H__ +#define __RCAR_DU_TCONENC_H__

+#include <linux/io.h> +#include <linux/module.h>

+struct rcar_drm_crtc; +struct rcar_du_tconenc;

+#if IS_ENABLED(CONFIG_DRM_RCAR_TCON) +int rcar_du_tconenc_init(struct rcar_du_device *rcdu); +int rcar_du_tconenc_enable(struct rcar_du_tconenc *tcon,

• 	   struct drm_crtc *crtc, bool enable);
  

+#else +static inline int rcar_du_tconenc_init(struct rcar_du_device *rcdu) +{

• return 0;

+} +static inline int rcar_du_tconenc_enable(struct rcar_du_tconenc *tcon,

• 			 struct drm_crtc *crtc, bool enable)
  

+{

• return 0;

+} +#endif

+#endif /* __RCAR_DU_TCONENC_H__ */ Index: linux/drivers/gpu/drm/rcar-du/rcar_tcon_regs.h =================================================================== --- /dev/null +++ linux/drivers/gpu/drm/rcar-du/rcar_tcon_regs.h @@ -0,0 +1,70 @@ +/*

• • rcar_tcon_regs.h -- R-Car TCON Registers Definitions
• • Copyright (C) 2015-2016 CogentEmbedded, Inc. source@cogentembedded.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 Foundation.
• */

+#ifndef __RCAR_TCON_REGS_H__ +#define __RCAR_TCON_REGS_H__

+#define OUT_V_LATCH 0x0000 +#define OUTCNT_VEN (1 << 0)

+#define OUT_CLK_PHASE 0x0024 +#define OUTCNT_LCD_EDGE (1 << 8) /* If set, LCDOUT[23:0] are */

• 				/* output on the falling edge */
  

• 				/* of QCLK */
  

+#define TCON_V_LATCH 0x0280 +#define TCON_VEN (1 << 0)

+#define TCON_TIM 0x0284

+/* Synced to VSYNC */ +#define TCON_TIM_STVA1 0x0288 +#define TCON_TIM_STVA2 0x028c +#define TCON_TIM_STVB1 0x0290 +#define TCON_TIM_STVB2 0x0294

+/* Synced to HSYNC */ +#define TCON_TIM_STH1 0x0298 +#define TCON_TIM_STH2 0x029c +#define TCON_TIM_STB1 0x02a0 +#define TCON_TIM_STB2 0x02a4 +#define TCON_TIM_CPV1 0x02a8 +#define TCON_TIM_CPV2 0x02ac +#define TCON_TIM_POLA1 0x02b0 +#define TCON_TIM_POLA2 0x02b4 +#define TCON_TIM_POLB1 0x02b8 +#define TCON_TIM_POLB2 0x02bc +#define TCON_TIM_DE 0x02c0

For the sake of completeness, could you define the TCON_DE_INV bit ?

+/* Common definitions for all TCON_TIM_*1 registers */ +#define TCON_TIMING(start, width) (((start) << 16) | ((width) << 0))

+/* Common definitions for all TCON_TIM_*2 registers */ +#define TCON_INV (1 << 4) +/* Output signal select */ +#define TCON_SEL_STVA_VS 0

Could you write this as (0 << 0) (and some for the other bits below) to make it clearer that those are bit values ?

+#define TCON_SEL_STVB_VE 1 +#define TCON_SEL_STH_SP_HS 2 +#define TCON_SEL_STB_LP_HE 3 +#define TCON_SEL_CPV_GCK 4 +#define TCON_SEL_POLA 5 +#define TCON_SEL_POLB 6 +#define TCON_SEL_DE 7

I'd add a blank line here.

+/* Definitions for HSYNC-related TIM registers */

I'd list the registers explicitly here, or would add a short comment after each of them above to tell what they control.

+#define TCON_HS_SEL (1 << 8) /* If set, horizontal sync */

• 				/* signal reference after the */
  

• 				/* offset */
  

And a blank linke here too.

+/* Polarity generation mode */

Could you mention that this is applicable to POLA2 and POLB2 only ?

+#define TCON_MD_NORM (0 << 12) +#define TCON_MD_1X1 (1 << 12) +#define TCON_MD_1X2 (2 << 12) +#define TCON_MD_2X2 (3 << 12)

+#endif /* __RCAR_TCON_REGS_H__ */