[PATCH 1/3] drm: exynos: hdmi: add exynos5 support to mixer driver
Joonyoung Shim
jy0922.shim at samsung.com
Wed Sep 12 20:34:24 PDT 2012
On 09/13/2012 11:53 AM, Inki Dae wrote:
>
>> -----Original Message-----
>> From: Joonyoung Shim [mailto:jy0922.shim at samsung.com]
>> Sent: Thursday, September 13, 2012 10:44 AM
>> To: Rahul Sharma
>> Cc: dri-devel at lists.freedesktop.org; sw0312.kim at samsung.com;
>> inki.dae at samsung.com; kyungmin.park at samsung.com; prashanth.g at samsung.com;
>> joshi at samsung.com; s.shirish at samsung.com; fahad.k at samsung.com;
>> l.krishna at samsung.com; r.sh.open at gmail.com
>> Subject: Re: [PATCH 1/3] drm: exynos: hdmi: add exynos5 support to mixer
>> driver
>>
>> Hi, Rahul.
>>
>> On 09/12/2012 09:08 PM, Rahul Sharma wrote:
>>> Added support for exynos5 to drm mixer driver. Exynos5 works
>>> with dt enabled while in exynos4 mixer device information can
>>> be passed either way (dt or plf data). This situation is taken
>>> cared.
>>>
>>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
>>> Signed-off-by: Shirish S <s.shirish at samsung.com>
>>> Signed-off-by: Fahad Kunnathadi <fahad.k at samsung.com>
>>> ---
>>> drivers/gpu/drm/exynos/exynos_mixer.c | 153
>> ++++++++++++++++++++++++++++++---
>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
>>> 2 files changed, 142 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
>> b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 8a43ee1..7d04a40 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -71,6 +71,7 @@ struct mixer_resources {
>>> struct clk *sclk_mixer;
>>> struct clk *sclk_hdmi;
>>> struct clk *sclk_dac;
>>> + bool is_soc_exynos5;
>>> };
>>>
>>> struct mixer_context {
>>> @@ -251,7 +252,8 @@ static void mixer_vsync_set_update(struct
>> mixer_context *ctx, bool enable)
>>> mixer_reg_writemask(res, MXR_STATUS, enable ?
>>> MXR_STATUS_SYNC_ENABLE : 0, MXR_STATUS_SYNC_ENABLE);
>>>
>>> - vp_reg_write(res, VP_SHADOW_UPDATE, enable ?
>>> + if (!res->is_soc_exynos5)
>>> + vp_reg_write(res, VP_SHADOW_UPDATE, enable ?
>>> VP_SHADOW_UPDATE_ENABLE : 0);
>>> }
>>>
>>> @@ -615,15 +617,21 @@ static void mixer_win_reset(struct mixer_context
>> *ctx)
>>> val = MXR_GRP_CFG_ALPHA_VAL(0);
>>> mixer_reg_write(res, MXR_VIDEO_CFG, val);
>>>
>>> - /* configuration of Video Processor Registers */
>>> - vp_win_reset(ctx);
>>> - vp_default_filter(res);
>>> + if (!res->is_soc_exynos5) {
>>> + /* configuration of Video Processor Registers */
>>> + vp_win_reset(ctx);
>>> + vp_default_filter(res);
>>> + }
>>>
>>> /* disable all layers */
>>> mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE);
>>> mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE);
>>> mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE);
>>>
>>> + /* enable vsync interrupt after mixer reset*/
>>> + mixer_reg_writemask(res, MXR_INT_EN, MXR_INT_EN_VSYNC,
>>> + MXR_INT_EN_VSYNC);
>>> +
>>> mixer_vsync_set_update(ctx, true);
>>> spin_unlock_irqrestore(&res->reg_slock, flags);
>>> }
>>> @@ -645,7 +653,8 @@ static void mixer_poweron(struct mixer_context *ctx)
>>> pm_runtime_get_sync(ctx->dev);
>>>
>>> clk_enable(res->mixer);
>>> - clk_enable(res->vp);
>>> + if (!res->is_soc_exynos5)
>>> + clk_enable(res->vp);
>>> clk_enable(res->sclk_mixer);
>>>
>>> mixer_reg_write(res, MXR_INT_EN, ctx->int_en);
>>> @@ -666,7 +675,8 @@ static void mixer_poweroff(struct mixer_context
> *ctx)
>>> ctx->int_en = mixer_reg_read(res, MXR_INT_EN);
>>>
>>> clk_disable(res->mixer);
>>> - clk_disable(res->vp);
>>> + if (!res->is_soc_exynos5)
>>> + clk_disable(res->vp);
>>> clk_disable(res->sclk_mixer);
>>>
>>> pm_runtime_put_sync(ctx->dev);
>>> @@ -797,11 +807,16 @@ static void mixer_win_mode_set(void *ctx,
>>> static void mixer_win_commit(void *ctx, int win)
>>> {
>>> struct mixer_context *mixer_ctx = ctx;
>>> + struct mixer_resources *res = &mixer_ctx->mixer_res;
>>>
>>> DRM_DEBUG_KMS("[%d] %s, win: %d\n", __LINE__, __func__, win);
>>>
>>> - if (win > 1)
>>> - vp_video_buffer(mixer_ctx, win);
>>> + if (!res->is_soc_exynos5) {
>>> + if (win > 1)
>>> + vp_video_buffer(mixer_ctx, win);
>>> + else
>>> + mixer_graph_buffer(mixer_ctx, win);
>>> + }
>>> else
>>> mixer_graph_buffer(mixer_ctx, win);
>>> }
>>> @@ -888,6 +903,12 @@ static irqreturn_t mixer_irq_handler(int irq, void
>> *arg)
>>> /* handling VSYNC */
>>> if (val & MXR_INT_STATUS_VSYNC) {
>>> + /* layer update mandatory for exynos5 soc,and not present
>>> + * in exynos4 */
>>> + if (res->is_soc_exynos5)
>>> + mixer_reg_writemask(res, MXR_CFG, ~0,
>>> + MXR_CFG_LAYER_UPDATE);
>>> +
>>> /* interlace scan need to check shadow register */
>>> if (ctx->interlace) {
>>> base = mixer_reg_read(res, MXR_GRAPHIC_BASE(0));
>>> @@ -919,8 +940,81 @@ out:
>>> return IRQ_HANDLED;
>>> }
>>>
>>> -static int __devinit mixer_resources_init(struct
>> exynos_drm_hdmi_context *ctx,
>>> - struct platform_device *pdev)
>>> +static int __devinit mixer_resources_init_exynos5(
>>> + struct exynos_drm_hdmi_context *ctx,
>>> + struct platform_device *pdev)
>>> +{
>>> + struct mixer_context *mixer_ctx = ctx->ctx;
>>> + struct device *dev = &pdev->dev;
>>> + struct mixer_resources *mixer_res = &mixer_ctx->mixer_res;
>>> + struct resource *res;
>>> + int ret;
>>> +
>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>> +
>>> + mixer_res->is_soc_exynos5 = true;
>>> + spin_lock_init(&mixer_res->reg_slock);
>>> +
>>> + mixer_res->mixer = clk_get(dev, "mixer");
>>> + if (IS_ERR_OR_NULL(mixer_res->mixer)) {
>>> + dev_err(dev, "failed to get clock 'mixer'\n");
>>> + ret = -ENODEV;
>>> + goto fail;
>>> + }
>>> +
>>> + mixer_res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
>>> + if (IS_ERR_OR_NULL(mixer_res->sclk_hdmi)) {
>>> + dev_err(dev, "failed to get clock 'sclk_hdmi'\n");
>>> + ret = -ENODEV;
>>> + goto fail;
>>> + }
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (res == NULL) {
>>> + dev_err(dev, "get memory resource failed.\n");
>>> + ret = -ENXIO;
>>> + goto fail;
>>> + }
>>> +
>>> + mixer_res->mixer_regs = devm_ioremap(&pdev->dev, res->start,
>>> + resource_size(res));
>>> + if (mixer_res->mixer_regs == NULL) {
>>> + dev_err(dev, "register mapping failed.\n");
>>> + ret = -ENXIO;
>>> + goto fail;
>>> + }
>>> +
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> + if (res == NULL) {
>>> + dev_err(dev, "get interrupt resource failed.\n");
>>> + ret = -ENXIO;
>>> + goto fail;
>>> + }
>>> +
>>> + ret = devm_request_irq(&pdev->dev, res->start, mixer_irq_handler,
>>> + 0, "drm_mixer",
> ctx);
>>> + if (ret) {
>>> + dev_err(dev, "request interrupt failed.\n");
>>> + goto fail;
>>> + }
>>> + mixer_res->irq = res->start;
>>> +
>>> + return 0;
>>> +
>>> +fail:
>>> + if (!IS_ERR_OR_NULL(mixer_res->sclk_dac))
>>> + clk_put(mixer_res->sclk_dac);
>>> + if (!IS_ERR_OR_NULL(mixer_res->sclk_hdmi))
>>> + clk_put(mixer_res->sclk_hdmi);
>>> + if (!IS_ERR_OR_NULL(mixer_res->mixer))
>>> + clk_put(mixer_res->mixer);
>>> + return ret;
>>> +}
>>> +
>>> +static int __devinit mixer_resources_init_exynos4(
>>> + struct exynos_drm_hdmi_context *ctx,
>>> + struct platform_device *pdev)
>>> {
>>> struct mixer_context *mixer_ctx = ctx->ctx;
>>> struct device *dev = &pdev->dev;
>>> @@ -928,6 +1022,10 @@ static int __devinit mixer_resources_init(struct
>> exynos_drm_hdmi_context *ctx,
>>> struct resource *res;
>>> int ret;
>>>
>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>> +
>>> + mixer_res->is_soc_exynos5 = false;
>>> +
>>> spin_lock_init(&mixer_res->reg_slock);
>>>
>>> mixer_res->mixer = clk_get(dev, "mixer");
>>> @@ -1028,10 +1126,17 @@ static int __devinit mixer_probe(struct
>> platform_device *pdev)
>>> struct device *dev = &pdev->dev;
>>> struct exynos_drm_hdmi_context *drm_hdmi_ctx;
>>> struct mixer_context *ctx;
>>> + bool is_soc_exynos5 = false;
>>> int ret;
>>>
>>> dev_info(dev, "probe start\n");
>>>
>>> + if (pdev->dev.of_node &&
>>> + of_device_is_compatible(pdev->dev.of_node,
>>> + "samsung,exynos5-mixer")) {
>>> + is_soc_exynos5 = true;
>>> + }
>>> +
>>> drm_hdmi_ctx = devm_kzalloc(&pdev->dev, sizeof(*drm_hdmi_ctx),
>>> GFP_KERNEL);
>>> if (!drm_hdmi_ctx) {
>>> @@ -1053,7 +1158,10 @@ static int __devinit mixer_probe(struct
>> platform_device *pdev)
>>> platform_set_drvdata(pdev, drm_hdmi_ctx);
>>>
>>> /* acquire resources: regs, irqs, clocks */
>>> - ret = mixer_resources_init(drm_hdmi_ctx, pdev);
>>> + if (is_soc_exynos5)
>>> + ret = mixer_resources_init_exynos5(drm_hdmi_ctx, pdev);
>>> + else
>>> + ret = mixer_resources_init_exynos4(drm_hdmi_ctx, pdev);
>> I don't like this. These share many same codes.
>>
> Please, share mixer_resources_init function. I think we could reuse same
> codes such things related to mixer clock, sclk_hdmi, io resource and irq.
>
>>> if (ret)
>>> goto fail;
>>>
>>> @@ -1064,7 +1172,6 @@ static int __devinit mixer_probe(struct
>> platform_device *pdev)
>>> return 0;
>>>
>>> -
>>> fail:
>>> dev_info(dev, "probe failed\n");
>>> return ret;
>>> @@ -1093,12 +1200,32 @@ static int mixer_suspend(struct device *dev)
>>>
>>> static SIMPLE_DEV_PM_OPS(mixer_pm_ops, mixer_suspend, NULL);
>>>
>>> +static struct platform_device_id mixer_driver_types[] = {
>>> + {
>>> + .name = "exynos5-mixer",
>>> + }, {
>>> + .name = "exynos4-mixer",
>>> + }, {
>>> + /* end node */
>>> + }
>>> +};
>>> +
>> These names should consider devname of clock.
>>
>>> +static struct of_device_id mixer_match_types[] = {
>>> + {
>>> + .compatible = "samsung,exynos5-mixer",
>>> + }, {
>>> + /* end node */
>>> + }
>>> +};
>>> +
>>> struct platform_driver mixer_driver = {
>>> .driver = {
>>> - .name = "s5p-mixer",
>>> + .name = "exynos-mixer",
>>> .owner = THIS_MODULE,
>>> .pm = &mixer_pm_ops,
>>> + .of_match_table = mixer_match_types,
>>> },
>>> .probe = mixer_probe,
>>> .remove = __devexit_p(mixer_remove),
>>> + .id_table = mixer_driver_types,
>>> };
>>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h
>> b/drivers/gpu/drm/exynos/regs-mixer.h
>>> index fd2f4d1..0491ad8 100644
>>> --- a/drivers/gpu/drm/exynos/regs-mixer.h
>>> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
>>> @@ -69,6 +69,7 @@
>>> (((val) << (low_bit)) & MXR_MASK(high_bit, low_bit))
>>>
>>> /* bits for MXR_STATUS */
>>> +#define MXR_STATUS_SOFT_RESET (1 << 8)
>>> #define MXR_STATUS_16_BURST (1 << 7)
>>> #define MXR_STATUS_BURST_MASK (1 << 7)
>>> #define MXR_STATUS_BIG_ENDIAN (1 << 3)
>>> @@ -77,6 +78,7 @@
>>> #define MXR_STATUS_REG_RUN (1 << 0)
>>>
>>> /* bits for MXR_CFG */
>>> +#define MXR_CFG_LAYER_UPDATE (1 << 31)
>>> #define MXR_CFG_RGB601_0_255 (0 << 9)
>>> #define MXR_CFG_RGB601_16_235 (1 << 9)
>>> #define MXR_CFG_RGB709_0_255 (2 << 9)
>> Overall, i think to use is_soc_exynos5 causes too many if statements.
>> Let's look other good idea to solve basically.
>>
> For this, you could check mixer version reading MIXER_VER register but not
> check hdmi. actually hdmi has no any register we can check version. this
> would be the reason we used is_v13 variable to identify exynos4210 and
> exynos4412 SoC. I tend to agree with Rahul. I will merge it as is if there
> is no any better way and next we can fix it later. any opinions?
>
Let's think to disassociate hdmi and mixer. I have plan to unify to one
many things of exynos hdmi. Above problem occurs because exynos5 doesn't
have video processor ip. Even if we use a field such is_soc_exynos5, the
is_soc_exynos5 is unsuitable naming if other exynos SoC also doesn't
have video processor ip.
More information about the dri-devel
mailing list