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.
