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 at cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov at 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 at 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 at
> 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 at
> 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__ */
--
Regards,
Laurent Pinchart
