Re: [PATCH V3 1/7] drm/exynos: Support DP CLKCON register in FIMD driver

[Andrzej Hajda <a.hajda@samsung.com>]

On 06/27/2014 01:48 PM, Ajay kumar wrote:
> Hi Andrej,
>
> On Fri, Jun 27, 2014 at 4:52 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> Hi Ajay,
>>
>> +CC DT
>>
>> On 06/27/2014 12:12 PM, Ajay Kumar wrote:
>>> Add the missing setting for DP CLKCON register.
>>>
>>> This register is present on Exynos5 based FIMD controllers,
>>> and needs to be set if we are using DP.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>> ---
>>>  .../devicetree/bindings/video/samsung-fimd.txt     |    1 +
>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c           |   23 ++++++++++++++++++++
>>>  include/video/samsung_fimd.h                       |    4 ++++
>>>  3 files changed, 28 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/samsung-fimd.txt b/Documentation/devicetree/bindings/video/samsung-fimd.txt
>>> index 2dad41b..12f3d7a 100644
>>> --- a/Documentation/devicetree/bindings/video/samsung-fimd.txt
>>> +++ b/Documentation/devicetree/bindings/video/samsung-fimd.txt
>>> @@ -41,6 +41,7 @@ Optional Properties:
>>>  - samsung,power-domain: a phandle to FIMD power domain node.
>>>  - samsung,invert-vden: video enable signal is inverted
>>>  - samsung,invert-vclk: video clock signal is inverted
>>> +- samsung,output-type: Type of display output interface(DPI=0, DSI=1, DP=2)
>> There is no point in introducing this property. Exynos DRM have already
>> logic which creates pipeline: fimd --> DPI|DSI|DP, this logic can be
>> reused to determine display type. It can be done even without any
>> additional callbacks, just by checking if there is connector of
>> DRM_MODE_CONNECTOR_eDP type connected to fimd.
> The mapping between crtc(struct exynos_drm_manager) and encoder(struct
> exynos_drm_display)
> in exynos drm happens by matching the exynos_drm_output_type variable in each
> structure.
> exynos_drm_output_type supports 3 types: LCD, HDMI and VIDI.
> FIMD statically chooses EXYNOS_DISPLAY_TYPE_LCD as the output type,
> and both DP and MIPI statically choose the same enum EXYNOS_DISPLAY_TYPE_LCD,
> as output type.
> So, we cannot use that logic to differentiate between DP/MIPI DSI.
>
> Also, checking based on connector type doesn't hold good.
> The connector type will be DRM_MODE_CONNECTOR_LVDS in case of
> DP->LVDS or MIPI->LVDS panels!

True, I forgot about bridges. So additional callback/field is necessary.
See below.

>
> Thanks and regards,
> Ajay Kumar
>
>> Regards
>> Andrzej
>>
>>>  - display-timings: timing settings for FIMD, as described in document [1].
>>>               Can be used in case timings cannot be provided otherwise
>>>               or to override timings provided by the panel.
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> index 33161ad..aa74e90 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> @@ -72,6 +72,7 @@ struct fimd_driver_data {
>>>       unsigned int has_shadowcon:1;
>>>       unsigned int has_clksel:1;
>>>       unsigned int has_limited_fmt:1;
>>> +     unsigned int has_dp_clkcon:1;
>>>  };
>>>
>>>  static struct fimd_driver_data s3c64xx_fimd_driver_data = {
>>> @@ -88,6 +89,13 @@ static struct fimd_driver_data exynos4_fimd_driver_data = {
>>>  static struct fimd_driver_data exynos5_fimd_driver_data = {
>>>       .timing_base = 0x20000,
>>>       .has_shadowcon = 1,
>>> +     .has_dp_clkcon = 1,
>>> +};
>>> +
>>> +enum exynos_fimd_output_type {
>>> +     EXYNOS_FIMD_OUTPUT_DPI,
>>> +     EXYNOS_FIMD_OUTPUT_DSI,
>>> +     EXYNOS_FIMD_OUTPUT_DP,
>>>  };
>>>
>>>  struct fimd_win_data {
>>> @@ -125,6 +133,8 @@ struct fimd_context {
>>>       struct exynos_drm_panel_info panel;
>>>       struct fimd_driver_data *driver_data;
>>>       struct exynos_drm_display *display;
>>> +
>>> +     enum exynos_fimd_output_type exynos_fimd_output_type;
>>>  };
>>>
>>>  static const struct of_device_id fimd_driver_dt_match[] = {
>>> @@ -331,6 +341,10 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
>>>       if (clkdiv > 1)
>>>               val |= VIDCON0_CLKVAL_F(clkdiv - 1) | VIDCON0_CLKDIR;
>>>
>>> +     if (ctx->driver_data->has_dp_clkcon &&
>>> +             ctx->exynos_fimd_output_type == EXYNOS_FIMD_OUTPUT_DP)
>>> +             writel(DP_CLK_ENABLE, ctx->regs + DP_CLKCON);
>>> +
>>>       writel(val, ctx->regs + VIDCON0);

New code should not split VIDCON0 related code. It should be moved few
lines above or few lines below.
Anyway this code should be rather placed in power related functions of
dp encoder, as it enables dp. The only question
is if DP_CLKCON update can be performed after VIDCON0 update. If yes the
solution of the whole problem
seems to be simple:
- fimd should provide function fimd_set_dp_clk_gate or sth similar,
- this function should be called in exynos_dp_poweron/exynos_dp_poweroff.
I hope I have not missed anything this time.

Regards
Andrzej


>>>  }
>>>
>>> @@ -924,6 +938,7 @@ static int fimd_probe(struct platform_device *pdev)
>>>       struct device *dev = &pdev->dev;
>>>       struct fimd_context *ctx;
>>>       struct resource *res;
>>> +     u32 fimd_output_type;
>>>       int ret = -EINVAL;
>>>
>>>       ret = exynos_drm_component_add(&pdev->dev, EXYNOS_DEVICE_TYPE_CRTC,
>>> @@ -949,6 +964,14 @@ static int fimd_probe(struct platform_device *pdev)
>>>               ctx->vidcon1 |= VIDCON1_INV_VDEN;
>>>       if (of_property_read_bool(dev->of_node, "samsung,invert-vclk"))
>>>               ctx->vidcon1 |= VIDCON1_INV_VCLK;
>>> +     if (!of_property_read_u32(dev->of_node, "samsung,output-type",
>>> +                             &fimd_output_type)) {
>>> +             if ((fimd_output_type < EXYNOS_FIMD_OUTPUT_DPI) ||
>>> +                 (fimd_output_type > EXYNOS_FIMD_OUTPUT_DP))
>>> +                     dev_err(dev, "invalid output type for FIMD\n");
>>> +             else
>>> +                     ctx->exynos_fimd_output_type = fimd_output_type;
>>> +     }
>>>
>>>       ctx->bus_clk = devm_clk_get(dev, "fimd");
>>>       if (IS_ERR(ctx->bus_clk)) {
>>> diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
>>> index b039320..d8f4b0b 100644
>>> --- a/include/video/samsung_fimd.h
>>> +++ b/include/video/samsung_fimd.h
>>> @@ -435,6 +435,10 @@
>>>  #define BLENDCON_NEW_8BIT_ALPHA_VALUE                (1 << 0)
>>>  #define BLENDCON_NEW_4BIT_ALPHA_VALUE                (0 << 0)
>>>
>>> +/* Video clock enable for DP */
>>> +#define DP_CLKCON                            0x27C
>>> +#define DP_CLK_ENABLE                                0x2
>>> +
>>>  /* Notes on per-window bpp settings
>>>   *
>>>   * Value     Win0     Win1     Win2     Win3     Win 4
>>>