From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: Re: [RFC PATCH] drm/exynos: Add DECON driver Date: Tue, 25 Nov 2014 22:29:47 +0900 Message-ID: <5474844B.9040306@samsung.com> References: <1412945316-26877-1-git-send-email-ajaykumar.rs@samsung.com> <5447C5AB.9090900@samsung.com> <5457527A.7070007@samsung.com> <54747D54.3000302@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mailout3.samsung.com ([203.254.224.33]:9889 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752541AbaKYN3v (ORCPT ); Tue, 25 Nov 2014 08:29:51 -0500 In-reply-to: Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: Ajay kumar Cc: Ajay Kumar , "dri-devel@lists.freedesktop.org" , "linux-samsung-soc@vger.kernel.org" , "devicetree@vger.kernel.org" , Joonyoung Shim , Kukjin Kim , Sean Paul , Jingoo Han , Pannaga Bhushan Reddy Patel , Prashanth G , "cpgs ." On 2014=EB=85=84 11=EC=9B=94 25=EC=9D=BC 22:08, Ajay kumar wrote: > Hi Inki, >=20 > On Tue, Nov 25, 2014 at 6:30 PM, Inki Dae wrot= e: >> On 2014=EB=85=84 11=EC=9B=94 25=EC=9D=BC 21:17, Ajay kumar wrote: >>> ping. >>> >> >> You'd need to clean up clocks and fix up binding file. And then let'= s >> have review in more details. I wish that other people also give you >> their reviews. > Nice to hear. Earlier, you mentioned that its good if FIMD driver its= elf > is modified to support Exynos7 DECON. So, what is your take now? > 1) Should I add it in FIMD driver itself? > We may need to add lot of driver_data > for that, since offsets are much different. > 2) Or, create two seperate register level files for Exynos5 and Exyno= s7? > 3) Or the current way - Entirely different driver This one, 3), for now because they, Exynos4, Exynos543x and Exynos7, are much different each other. So for next version of your patch, you'd need to change the driver name to exynos7-decon or what you want so tha= t each driver can be entirely separated in SoC name somehow. i.e., - exynos_drm_fimd covers Exynos64xx, Exynos3250, all Exynos4 series and Exynos5250 ~ 5422 SoC. - exynos5-decon covers Exynos5430 and Exynos5433 SoC. - exynos7-decon covers Exynos7 and maybe later SoC. After that, let's consider how we can integrate these drivers later. Thanks, Inki Dae >=20 >> Anyway, below is my answer. >> >> Thanks, >> Inki Dae >> >> >>> On Tue, Nov 11, 2014 at 10:08 PM, Ajay kumar w= rote: >>>> Hi Inki, >>>> >>>> On Mon, Nov 3, 2014 at 3:31 PM, Inki Dae wr= ote: >>>>> >>>>> Hi, >>>>> >>>>> Fortunately, I could get the user manual for Exynos7420. Below ar= e my >>>>> comments. >>>>> >>>>> Thanks, >>>>> Inki Dae >>>>> >>>>> On 2014=EB=85=84 10=EC=9B=94 23=EC=9D=BC 01:34, Ajay kumar wrote: >>>>>> On Wed, Oct 22, 2014 at 8:26 PM, Inki Dae = wrote: >>>>>>> >>>>>>> Thanks for contribution. >>>>>>> >>>>>>> It seems reasonable that you separate device drivers into FIMD = and DECON >>>>>>> because many registers of them have many different offsets and = fields. >>>>>>> However, there may be a good solution that we can combine commo= n sets of >>>>>>> these drivers later. >>>>>> Yes, this is the main reason behind sending this as RFC patch. >>>>>> I want to know what's the best way to do this. >>>>>> FIMD, 5433 DECON and Exynos7 DECON - all are different. >>>>>> Also, in Exynos7 DECON-INT is same as DECON-EXT(Mixer). >>>>>> So, even I am not sure how the driver layouts should be! >>>>> >>>>> Please, make sure Exynos SoC name, Exynos7410 or Exynos7420. In m= y >>>>> understanding, Exynos7 doesn't mean one real SoC. >>>> We shall use Exynos7 as per the discussion. >> >> Just for the time being. > Ok. >=20 >>>> >>>>>> >>>>>>> Below are my comments. >>>>>>> >>>>>>> Thanks, >>>>>>> Inki Dae >>>>>>> >>>>>>> On 2014=EB=85=84 10=EC=9B=94 10=EC=9D=BC 21:48, Ajay Kumar wrot= e: >>>>>>>> This series is based on exynos-drm-next branch of Inki Dae's t= ree at: >>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exyn= os.git >>>>>>>> >>>>>>>> DECON(Display and Enhancement Controller) is the new IP >>>>>>>> in exynos7 SOC for generating video signals using pixel data. >>>>>>> >>>>>>> DECON was used since Exynos5430. And is Exynos5433 different fr= om >>>>>>> Exynos7? If so, could I get the Exynos7 user manual (TRM) for r= eview? >>>>>> Yes, Exynos5433 DECON is very much different than Exynos7 DECON. >>>>> >>>>> Do not use Exynos7 word and use Exynos7410 or Exynos7420 instead. >>>> Again, we shall use Exynos7. >>>> >>>>>> I will see how manual can be arranged. >>>>>> >>>>>>>> >>>>>>>> DECON driver can be used to drive 2 different interfaces on Ex= ynos7: >>>>>>>> DECON-INT(video controller) and DECON-EXT(Mixer for HDMI) >>>>>>>> >>>>>>>> The existing FIMD driver code was used as a template to create >>>>>>>> DECON driver. Only DECON-INT is supported as of now, and >>>>>>>> DECON-EXT support will be added later. >>>>>>>> >>>>>>>> Signed-off-by: Akshu Agrawal >>>>>>>> Signed-off-by: Ajay Kumar >>>>>>>> --- >>>>>>>> .../devicetree/bindings/video/exynos-decon.txt | 68 ++ >>>>>>>> drivers/gpu/drm/exynos/Kconfig | 11 +- >>>>>>>> drivers/gpu/drm/exynos/Makefile | 1 + >>>>>>>> drivers/gpu/drm/exynos/exynos_drm_decon.c | 1086 >>>>>>> ++++++++++++++++++++ >>>>>>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 17 +- >>>>>>>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 11 + >>>>>>>> include/video/samsung_decon.h | 346 +++= ++++ >>>>>>>> 7 files changed, 1537 insertions(+), 3 deletions(-) >>>>>>>> create mode 100644 >>>>>>> Documentation/devicetree/bindings/video/exynos-decon.txt >>>>>>>> create mode 100644 drivers/gpu/drm/exynos/exynos_drm_decon.c >>>>>>>> create mode 100644 include/video/samsung_decon.h >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/video/exynos-de= con.txt >>>>>>> b/Documentation/devicetree/bindings/video/exynos-decon.txt >>>>>>>> new file mode 100644 >>>>>>>> index 0000000..e865650 >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/video/exynos-decon.txt >>>>>>>> @@ -0,0 +1,68 @@ >>>>>>>> +Device-Tree bindings for Samsung Exynos7 SoC display controll= er (DECON) >>>>>>>> + >>>>>>>> +DECON (Display and Enhancement Controller) is the Display Con= troller >>>>>>> for the >>>>>>>> +Exynos7 series of SoCs which transfers the image data from a = video memory >>>>>>>> +buffer to an external LCD interface. >>>>>>>> + >>>>>>>> +Required properties: >>>>>>>> +- compatible: value should be "samsung,exynos7-decon"; >>>>>>> >>>>>>> If exynos5433 was just renamed to exynos7 then, it should be on= e of the >>>>>>> following: >>>>>>> (a) "samsung,exynos5430-decon" for Display and enhancem= ent controller >>>>>>> IP for Exynos5430 >>>>>>> (b) "samsung,exynos7" for Display and enhancement contr= oller IP for Exynos7 >>>>>>> >>>>>>> Or, >>>>>>> (a) "samsung,exynos5430-decon" for Display and enhancem= ent controller >>>>>>> IP for Exynos5430 >>>>>>> >>>>>>> (b) "samsung,exynos5433-decon" for Display and enhancem= ent controller >>>>>>> IP for Exynos5433 >>>>>>> (c) "samsung,exynos7" for Display and enhancement contr= oller IP for Exynos7 >>>>>> Eventually, we will end up here. >>>>>> >>>>>>> >>>>>>>> + >>>>>>>> +- reg: physical base address and length of the DECON register= s set. >>>>>>>> + >>>>>>>> +- interrupt-parent: should be the phandle of the decon contro= ller's >>>>>>>> + parent interrupt controller. >>>>>>>> + >>>>>>>> +- interrupts: should contain a list of all DECON IP block int= errupts >>>>>>> in the >>>>>>>> + order: FIFO Level, VSYNC, LCD_SYSTEM. The inter= rupt specifier >>>>>>>> + format depends on the interrupt controller used= =2E >>>>>>>> + >>>>>>>> +- interrupt-names: should contain the interrupt names: "fifo"= , "vsync", >>>>>>>> + "lcd_sys", in the same order as they were listed in the = interrupts >>>>>>>> + property. >>>>>>>> + >>>>>>>> +- pinctrl-0: pin control group to be used for this controller= =2E >>>>>>>> + >>>>>>>> +- pinctrl-names: must contain a "default" entry. >>>>>>>> + >>>>>>>> +- clocks: must include clock specifiers corresponding to entr= ies in the >>>>>>>> + clock-names property. >>>>>>>> + >>>>>>>> +- clock-names: list of clock names sorted in the same order a= s the clocks >>>>>>>> + property. Must contain "pclk_decon0", "aclk_de= con0", >>>>>>>> + "decon0_eclk", "decon0_vclk", "sclk_dsd", aclk_lh= _disp0", >>>>>>>> + "aclk_disp", "aclk_lh_disp1". >>>>>>> >>>>>>> Should DECON driver really control above all clocks? I think it= 's enough >>>>>>> that DECON driver controls only lcd and bus clocks, and others = could be >>>>>>> configured by boot-loader or by calling clk_set_rate. >>>>>> Yes, even I am not sure of the clocks. I have copied these clock= s from intrnal >>>>>> android code. >>>>>>>> + >>>>>>>> +Optional Properties: >>>>>>>> +- samsung,power-domain: a phandle to DECON power domain node. >>>>>>> >>>>>>> You are missing many properties, >>>>>>> samsung,invert-vden >>>>>>> samsung,invert-vclk >>>>>> These are not present in Exynos7 DECON! >>>>> >>>>> What does the CRCCTRL register mean? That definitely indicates ab= ove >>>>> properties. >>>> Hmm, even I am not sure about CRCCTRL register. >>>> Currently, we don't actually set CRC on, and still I can see the d= isplay >>>> on a MIPI DSI panel. >> >> It would be nice that you can check this register about what is the = purpose. > Ok. I will check them. >=20 >>>> May be, I will know better once I bring up DP interface on Exynos7= =2E >>>> >>>>>> >>>>>>> display-timings >>>>>>> ... >>>>>>> refer to below document, >>>>>>> Documentation/devicetree/bindings/video/samsung-fimd.txt >>>>>>> >>>>>>>> + >>>>>>>> +Example: >>>>>>>> + >>>>>>>> +SoC specific DT entry: >>>>>>>> + >>>>>>>> + decon@13930000 { >>>>> >>>>> In case of Exynos7420, the base address of the DECON controller i= s >>>>> 0x13950000. Does 0x1393000 mean the one for Exynos7410 >>>> There is DECON-INT(0x13930000) and DECON-EXT(0x13950000) >>>> DECON-INT is the display controller, but DECON-EXT replaces mixer = in Exynos7. >>>> >>>>>>>> + compatible =3D "samsung,exynos7-decon"; >>>>> >>>>> Therefore, it should be "samsung,exynos7410-decon" or >>>>> "samsung,exynos7420-decon" >>>> "samsung,exynos7-decon" >>>> >>>>>>>> + interrupt-parent =3D <&combiner>; >>>>>>>> + reg =3D <0x13930000 0x1000>; >>>>>>>> + interrupt-names =3D "lcd_sys", "vsync", "fifo"; >>>>>>>> + interrupts =3D <0 188 0>, <0 189 0>, <0 190 0>; >>>>>>>> + clocks =3D <&clock_disp PCLK_DECON_INT>, >>>>>>>> + <&clock_disp ACLK_DECON_INT>, >>>>>>>> + <&clock_disp SCLK_DECON_INT_ECLK>, >>>>>>>> + <&clock_disp SCLK_DECON_INT_EXTCLKPLL>, >>>>>>>> + <&clock_disp SCLK_DSD>, >>>>>>>> + <&clock_bus0 ACLK_LH_DISP0>, >>>>>>>> + <&clock_disp ACLK_CP_DISP>, >>>>>>>> + <&clock_bus0 ACLK_LH_DISP1>; >>>>>>>> + clock-names =3D "pclk_decon0", "aclk_decon0", "d= econ0_eclk", >>>>>>>> + "decon0_vclk", "sclk_dsd", "aclk= _lh_disp0", >>>>>>>> + "aclk_disp", "aclk_lh_disp1"; >>>>>>>> + status =3D "disabled"; >>>>>>>> + }; >>>>>>>> + >>>>>>>> +Board specific DT entry: >>>>>>>> + >>>>>>>> + decon@13930000 { >>>>>>>> + pinctrl-0 =3D <&lcd_clk &pwm1_out>; >>>>>>>> + pinctrl-names =3D "default"; >>>>>>>> + status =3D "okay"; >>>>>>>> + }; >>>>>>>> diff --git a/drivers/gpu/drm/exynos/Kconfig >>>>>>> b/drivers/gpu/drm/exynos/Kconfig >>>>>>>> index fd1c070..89275ea 100644 >>>>>>>> --- a/drivers/gpu/drm/exynos/Kconfig >>>>>>>> +++ b/drivers/gpu/drm/exynos/Kconfig >>>>>>>> @@ -31,6 +31,13 @@ config DRM_EXYNOS_FIMD >>>>>>>> help >>>>>>>> Choose this option if you want to use Exynos FIMD for = DRM. >>>>>>>> >>>>>>>> +config DRM_EXYNOS_DECON >>>>>>>> + bool "Exynos DRM DECON" >>>>>>>> + depends on DRM_EXYNOS >>>>>>>> + select FB_MODE_HELPERS >>>>>>>> + help >>>>>>>> + Choose this option if you want to use Exynos DECON for= DRM. >>>>>>>> + >>>>>>>> config DRM_EXYNOS_DPI >>>>>>>> bool "EXYNOS DRM parallel output support" >>>>>>>> depends on DRM_EXYNOS_FIMD >>>>>>>> @@ -41,7 +48,7 @@ config DRM_EXYNOS_DPI >>>>>>>> >>>>>>>> config DRM_EXYNOS_DSI >>>>>>>> bool "EXYNOS DRM MIPI-DSI driver support" >>>>>>>> - depends on DRM_EXYNOS_FIMD >>>>>>>> + depends on (DRM_EXYNOS_FIMD || DRM_EXYNOS_DECON) >>>>>>>> select DRM_MIPI_DSI >>>>>>>> select DRM_PANEL >>>>>>>> default n >>>>>>>> @@ -50,7 +57,7 @@ config DRM_EXYNOS_DSI >>>>>>>> >>>>>>>> config DRM_EXYNOS_DP >>>>>>>> bool "EXYNOS DRM DP driver support" >>>>>>>> - depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN346= 0=3Dn || >>>>>>> DRM_PTN3460=3Dy || DRM_PTN3460=3DDRM_EXYNOS) >>>>>>>> + depends on (DRM_EXYNOS_FIMD || DRM_EXYNOS_DECON) && ARCH= _EXYNOS && >>>>>>> (DRM_PTN3460=3Dn || DRM_PTN3460=3Dy || DRM_PTN3460=3DDRM_EXYNOS= ) >>>>>>>> default DRM_EXYNOS >>>>>>>> select DRM_PANEL >>>>>>>> help >>>>>>>> diff --git a/drivers/gpu/drm/exynos/Makefile >>>>>>> b/drivers/gpu/drm/exynos/Makefile >>>>>>>> index 33ae365..c3282ac 100644 >>>>>>>> --- a/drivers/gpu/drm/exynos/Makefile >>>>>>>> +++ b/drivers/gpu/drm/exynos/Makefile >>>>>>>> @@ -11,6 +11,7 @@ exynosdrm-y :=3D exynos_drm_drv.o exynos_drm= _encoder.o \ >>>>>>>> exynosdrm-$(CONFIG_DRM_EXYNOS_IOMMU) +=3D exynos_drm_iommu.o >>>>>>>> exynosdrm-$(CONFIG_DRM_EXYNOS_DMABUF) +=3D exynos_drm_dmabuf.= o >>>>>>>> exynosdrm-$(CONFIG_DRM_EXYNOS_FIMD) +=3D exynos_drm_fimd.o >>>>>>>> +exynosdrm-$(CONFIG_DRM_EXYNOS_DECON) +=3D exynos_drm_decon.o >>>>>>>> exynosdrm-$(CONFIG_DRM_EXYNOS_DPI) +=3D exynos_drm_dpi.o >>>>>>>> exynosdrm-$(CONFIG_DRM_EXYNOS_DSI) +=3D exynos_drm_dsi.o >>>>>>>> exynosdrm-$(CONFIG_DRM_EXYNOS_DP) +=3D exynos_dp_core.o ex= ynos_dp_reg.o >>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_decon.c >>>>>>> b/drivers/gpu/drm/exynos/exynos_drm_decon.c >>>>>>>> new file mode 100644 >>>>>>>> index 0000000..5ac4557 >>>>>>>> --- /dev/null >>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_decon.c >>>>>>>> @@ -0,0 +1,1086 @@ >>>>>>>> +/* exynos_drm_decon.c >>>>>>>> + * >>>>>>>> + * Copyright (C) 2014 Samsung Electronics Co.Ltd >>>>>>>> + * Authors: >>>>>>>> + * Akshu Agarwal >>>>>>>> + * Ajay Kumar >>>>>>>> + * >>>>>>>> + * This program is free software; you can redistribute it an= d/or >>>>>>> modify it >>>>>>>> + * under the terms of the GNU General Public License as pu= blished >>>>>>> by the >>>>>>>> + * Free Software Foundation; either version 2 of the Licens= e, or >>>>>>> (at your >>>>>>>> + * option) any later version. >>>>>>>> + * >>>>>>>> + */ >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> + >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> +#include >>>>>>>> + >>>>>>>> +#include