[PATCH v13 03/14] drm/mediatek: Add DSI sub driver
On Tue, Mar 15, 2016 at 7:49 PM, Philipp Zabel wrote: > > Hi Daniel, > > Am Mittwoch, den 09.03.2016, 22:07 +0800 schrieb Daniel Kurtz: > > Hi Philipp, CK, > > > > Some small comments. > > Nothing that couldn't be addressed after merging, if you prefer. > > > > On Tue, Mar 8, 2016 at 9:27 PM, Philipp Zabel > > wrote: > [...] > > > +static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > > +{ > > > + struct device *dev = dsi->dev; > > > + int ret; > > > + > > > + if (++dsi->refcount != 1) > > > + return 0; > > > > What is the point of this refcount? > > I believe dsi->enabled already ensures poweron/poweroff calls are paired. > > mtk_dsi_poweron() is called from both mtk_dsi_encoder_enable() and > mtk_dsi_ddp_start() and enables just enough of the DSI to provide power > and the pixel clock. The reason is that the DSI also provides the pixel > clock to the rest of the pipeline elements. > > dsi->enabled only tracks the whole DSI being active after the call of > mtk_dsi_encoder_enable(). > > [...] > > > +static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct > > > mtk_dsi *dsi) > > > +{ > > > + int ret; > > > + > > > + ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs, > > > + DRM_MODE_ENCODER_DSI, NULL); > > > + if (ret) { > > > + DRM_ERROR("Failed to encoder init to drm\n"); > > > + return ret; > > > + } > > > + drm_encoder_helper_add(&dsi->encoder, > > > &mtk_dsi_encoder_helper_funcs); > > > + > > > + /* > > > +* Currently display data paths are statically assigned to a crtc > > > each. > > > +* crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0 > > > +*/ > > > + dsi->encoder.possible_crtcs = 1; > > > + > > > + /* Pre-empt DP connector creation if there's a bridge */ > > > + ret = mtk_drm_attach_bridge(dsi->bridge, &dsi->encoder); > > > + if (!ret) > > > + return 0; > > > > nit: the above valid early termination of this function here is a bit > > unusual. > > It might be more clear if the "connector init" part below was split out > > into its > > own helper function. > > Good point, will do that. > > [...] > > > +static int mtk_dsi_remove(struct platform_device *pdev) > > > +{ > > > + struct mtk_dsi *dsi = platform_get_drvdata(pdev); > > > + > > > + mtk_output_dsi_disable(dsi); > > > > This one looks unmatched. > > It is, indeed we should let the drm core disable the encoder before the > driver is removed. > > [...] > > > +static int mtk_dsi_resume(struct device *dev) > > > +{ > > > + struct mtk_dsi *dsi; > > > + > > > + dsi = dev_get_drvdata(dev); > > > + > > > + mtk_output_dsi_enable(dsi); > > > + DRM_DEBUG_DRIVER("dsi resume success!\n"); > > > + > > > + return 0; > > > +} > > > +#endif > > > +static SIMPLE_DEV_PM_OPS(mtk_dsi_pm_ops, mtk_dsi_suspend, > > > mtk_dsi_resume); > > > > I don't think we want PM ops. > > The MTK DRM driver disables/enables encoders during suspend/resume. > > Yes, and that will also allow to merge mtk_output_dsi_disable() into > mtk_dsi_encoder_disable(), too. > > [...] > > > +static unsigned long mtk_mipi_tx_pll_recalc_rate(struct clk_hw *hw, > > > +unsigned long > > > parent_rate) > > > +{ > > > + struct mtk_mipi_tx *mipi_tx = container_of(hw, struct mtk_mipi_tx, > > > + pll_hw); > > > > An inline function / macro would help here. > > Ok. > > [...] > > > +static void mtk_mipi_tx_power_off_signal(struct phy *phy) > > > +{ > > > + struct mtk_mipi_tx *mipi_tx = phy_get_drvdata(phy); > > > + > > > + mtk_mipi_tx_mask(mipi_tx, MIPITX_DSI_TOP_CON, > > > RG_DSI_PAD_TIE_LOW_EN, > > > +RG_DSI_PAD_TIE_LOW_EN); > > > > As mentioned in the HDMI review, _set_bits() / _clr_bits() is more readable > > than > > _mask() if we are just setting/clearing groups of bits. > > I don't think > mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_TOP_CON, > RG_DSI_PAD_TIE_LOW_EN); > is that much easier to read. How about calling the function > mtk_mipi_tx_update_bits instead? Actually, the important part here was to remove the redundant 'value' parameter, and instead have dedicated clear / set functions that operate on just the mask. > > regards > Philipp >
[PATCH v13 03/14] drm/mediatek: Add DSI sub driver
Hi Daniel, Am Mittwoch, den 09.03.2016, 22:07 +0800 schrieb Daniel Kurtz: > Hi Philipp, CK, > > Some small comments. > Nothing that couldn't be addressed after merging, if you prefer. > > On Tue, Mar 8, 2016 at 9:27 PM, Philipp Zabel > wrote: [...] > > +static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > +{ > > + struct device *dev = dsi->dev; > > + int ret; > > + > > + if (++dsi->refcount != 1) > > + return 0; > > What is the point of this refcount? > I believe dsi->enabled already ensures poweron/poweroff calls are paired. mtk_dsi_poweron() is called from both mtk_dsi_encoder_enable() and mtk_dsi_ddp_start() and enables just enough of the DSI to provide power and the pixel clock. The reason is that the DSI also provides the pixel clock to the rest of the pipeline elements. dsi->enabled only tracks the whole DSI being active after the call of mtk_dsi_encoder_enable(). [...] > > +static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi > > *dsi) > > +{ > > + int ret; > > + > > + ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs, > > + DRM_MODE_ENCODER_DSI, NULL); > > + if (ret) { > > + DRM_ERROR("Failed to encoder init to drm\n"); > > + return ret; > > + } > > + drm_encoder_helper_add(&dsi->encoder, > > &mtk_dsi_encoder_helper_funcs); > > + > > + /* > > +* Currently display data paths are statically assigned to a crtc > > each. > > +* crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0 > > +*/ > > + dsi->encoder.possible_crtcs = 1; > > + > > + /* Pre-empt DP connector creation if there's a bridge */ > > + ret = mtk_drm_attach_bridge(dsi->bridge, &dsi->encoder); > > + if (!ret) > > + return 0; > > nit: the above valid early termination of this function here is a bit unusual. > It might be more clear if the "connector init" part below was split out into > its > own helper function. Good point, will do that. [...] > > +static int mtk_dsi_remove(struct platform_device *pdev) > > +{ > > + struct mtk_dsi *dsi = platform_get_drvdata(pdev); > > + > > + mtk_output_dsi_disable(dsi); > > This one looks unmatched. It is, indeed we should let the drm core disable the encoder before the driver is removed. [...] > > +static int mtk_dsi_resume(struct device *dev) > > +{ > > + struct mtk_dsi *dsi; > > + > > + dsi = dev_get_drvdata(dev); > > + > > + mtk_output_dsi_enable(dsi); > > + DRM_DEBUG_DRIVER("dsi resume success!\n"); > > + > > + return 0; > > +} > > +#endif > > +static SIMPLE_DEV_PM_OPS(mtk_dsi_pm_ops, mtk_dsi_suspend, mtk_dsi_resume); > > I don't think we want PM ops. > The MTK DRM driver disables/enables encoders during suspend/resume. Yes, and that will also allow to merge mtk_output_dsi_disable() into mtk_dsi_encoder_disable(), too. [...] > > +static unsigned long mtk_mipi_tx_pll_recalc_rate(struct clk_hw *hw, > > +unsigned long parent_rate) > > +{ > > + struct mtk_mipi_tx *mipi_tx = container_of(hw, struct mtk_mipi_tx, > > + pll_hw); > > An inline function / macro would help here. Ok. [...] > > +static void mtk_mipi_tx_power_off_signal(struct phy *phy) > > +{ > > + struct mtk_mipi_tx *mipi_tx = phy_get_drvdata(phy); > > + > > + mtk_mipi_tx_mask(mipi_tx, MIPITX_DSI_TOP_CON, RG_DSI_PAD_TIE_LOW_EN, > > +RG_DSI_PAD_TIE_LOW_EN); > > As mentioned in the HDMI review, _set_bits() / _clr_bits() is more readable > than > _mask() if we are just setting/clearing groups of bits. I don't think mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_TOP_CON, RG_DSI_PAD_TIE_LOW_EN); is that much easier to read. How about calling the function mtk_mipi_tx_update_bits instead? regards Philipp
[PATCH v13 03/14] drm/mediatek: Add DSI sub driver
Hi Philipp, CK, Some small comments. Nothing that couldn't be addressed after merging, if you prefer. On Tue, Mar 8, 2016 at 9:27 PM, Philipp Zabel wrote: > From: CK Hu > > This patch add a drm encoder/connector driver for the MIPI DSI function > block of the Mediatek display subsystem and a phy driver for the MIPI TX > D-PHY control module. > > Signed-off-by: Jitao Shi > Signed-off-by: Philipp Zabel > -- > drivers/gpu/drm/mediatek/Kconfig | 2 + > drivers/gpu/drm/mediatek/Makefile | 4 +- > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 + > drivers/gpu/drm/mediatek/mtk_dsi.c | 942 > + > drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 487 + > 6 files changed, 1438 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dsi.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_mipi_tx.c > > diff --git a/drivers/gpu/drm/mediatek/Kconfig > b/drivers/gpu/drm/mediatek/Kconfig > index 8dad892..0c49a94 100644 > --- a/drivers/gpu/drm/mediatek/Kconfig > +++ b/drivers/gpu/drm/mediatek/Kconfig > @@ -3,6 +3,8 @@ config DRM_MEDIATEK > depends on DRM > depends on ARCH_MEDIATEK || (ARM && COMPILE_TEST) > select DRM_KMS_HELPER > + select DRM_MIPI_DSI > + select DRM_PANEL > select IOMMU_DMA > select MTK_SMI > help > diff --git a/drivers/gpu/drm/mediatek/Makefile > b/drivers/gpu/drm/mediatek/Makefile > index d4bde7c..e781db5a 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -6,6 +6,8 @@ mediatek-drm-y := mtk_disp_ovl.o \ > mtk_drm_drv.o \ > mtk_drm_fb.o \ > mtk_drm_gem.o \ > - mtk_drm_plane.o > + mtk_drm_plane.o \ > + mtk_dsi.o \ > + mtk_mipi_tx.o > > obj-$(CONFIG_DRM_MEDIATEK) += mediatek-drm.o > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index 8a21ca7..4fcc0e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -551,6 +551,8 @@ static struct platform_driver * const mtk_drm_drivers[] = > { > &mtk_drm_platform_driver, > &mtk_disp_ovl_driver, > &mtk_disp_rdma_driver, > + &mtk_dsi_driver, > + &mtk_mipi_tx_driver, > }; > > static int __init mtk_drm_init(void) > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > index efb744c..161a362 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > @@ -50,5 +50,7 @@ struct mtk_drm_private { > > extern struct platform_driver mtk_disp_ovl_driver; > extern struct platform_driver mtk_disp_rdma_driver; > +extern struct platform_driver mtk_dsi_driver; > +extern struct platform_driver mtk_mipi_tx_driver; > > #endif /* MTK_DRM_DRV_H */ > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > new file mode 100644 > index 000..463d389 > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -0,0 +1,942 @@ > +/* > + * Copyright (c) 2015 MediaTek Inc. > + * > + * 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "mtk_drm_ddp_comp.h" > + > +#define DSI_VIDEO_FIFO_DEPTH (1920 / 4) > +#define DSI_HOST_FIFO_DEPTH64 > + > +#define DSI_START 0x00 > + > +#define DSI_CON_CTRL 0x10 > +#define DSI_RESET BIT(0) > +#define DSI_EN BIT(1) > + > +#define DSI_MODE_CTRL 0x14 > +#define MODE (3) > +#define CMD_MODE 0 > +#define SYNC_PULSE_MODE1 > +#define SYNC_EVENT_MODE2 > +#define BURST_MODE 3 > +#define FRM_MODE BIT(16) > +#define MIX_MODE BIT(17) > + > +#define DSI_TXRX_CTRL 0x18 > +#define VC_NUM (2 << 0) > +#define LANE_NUM (0xf << 2) > +#define DIS_EOTBIT(6) > +#define NULL_ENBIT(7) > +#define TE_FREERUN BIT(8) > +#define EXT_TE_EN BIT(9) > +#define EXT_TE_EDGE
[PATCH v13 03/14] drm/mediatek: Add DSI sub driver
From: CK Hu This patch add a drm encoder/connector driver for the MIPI DSI function block of the Mediatek display subsystem and a phy driver for the MIPI TX D-PHY control module. Signed-off-by: Jitao Shi Signed-off-by: Philipp Zabel -- drivers/gpu/drm/mediatek/Kconfig | 2 + drivers/gpu/drm/mediatek/Makefile | 4 +- drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 + drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 + drivers/gpu/drm/mediatek/mtk_dsi.c | 942 + drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 487 + 6 files changed, 1438 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/mediatek/mtk_dsi.c create mode 100644 drivers/gpu/drm/mediatek/mtk_mipi_tx.c diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig index 8dad892..0c49a94 100644 --- a/drivers/gpu/drm/mediatek/Kconfig +++ b/drivers/gpu/drm/mediatek/Kconfig @@ -3,6 +3,8 @@ config DRM_MEDIATEK depends on DRM depends on ARCH_MEDIATEK || (ARM && COMPILE_TEST) select DRM_KMS_HELPER + select DRM_MIPI_DSI + select DRM_PANEL select IOMMU_DMA select MTK_SMI help diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile index d4bde7c..e781db5a 100644 --- a/drivers/gpu/drm/mediatek/Makefile +++ b/drivers/gpu/drm/mediatek/Makefile @@ -6,6 +6,8 @@ mediatek-drm-y := mtk_disp_ovl.o \ mtk_drm_drv.o \ mtk_drm_fb.o \ mtk_drm_gem.o \ - mtk_drm_plane.o + mtk_drm_plane.o \ + mtk_dsi.o \ + mtk_mipi_tx.o obj-$(CONFIG_DRM_MEDIATEK) += mediatek-drm.o diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 8a21ca7..4fcc0e0 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -551,6 +551,8 @@ static struct platform_driver * const mtk_drm_drivers[] = { &mtk_drm_platform_driver, &mtk_disp_ovl_driver, &mtk_disp_rdma_driver, + &mtk_dsi_driver, + &mtk_mipi_tx_driver, }; static int __init mtk_drm_init(void) diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index efb744c..161a362 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h @@ -50,5 +50,7 @@ struct mtk_drm_private { extern struct platform_driver mtk_disp_ovl_driver; extern struct platform_driver mtk_disp_rdma_driver; +extern struct platform_driver mtk_dsi_driver; +extern struct platform_driver mtk_mipi_tx_driver; #endif /* MTK_DRM_DRV_H */ diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c new file mode 100644 index 000..463d389 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -0,0 +1,942 @@ +/* + * Copyright (c) 2015 MediaTek Inc. + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "mtk_drm_ddp_comp.h" + +#define DSI_VIDEO_FIFO_DEPTH (1920 / 4) +#define DSI_HOST_FIFO_DEPTH64 + +#define DSI_START 0x00 + +#define DSI_CON_CTRL 0x10 +#define DSI_RESET BIT(0) +#define DSI_EN BIT(1) + +#define DSI_MODE_CTRL 0x14 +#define MODE (3) +#define CMD_MODE 0 +#define SYNC_PULSE_MODE1 +#define SYNC_EVENT_MODE2 +#define BURST_MODE 3 +#define FRM_MODE BIT(16) +#define MIX_MODE BIT(17) + +#define DSI_TXRX_CTRL 0x18 +#define VC_NUM (2 << 0) +#define LANE_NUM (0xf << 2) +#define DIS_EOTBIT(6) +#define NULL_ENBIT(7) +#define TE_FREERUN BIT(8) +#define EXT_TE_EN BIT(9) +#define EXT_TE_EDGEBIT(10) +#define MAX_RTN_SIZE (0xf << 12) +#define HSTX_CKLP_EN BIT(16) + +#define DSI_PSCTRL 0x1c +#define DSI_PS_WC 0x3fff +#define DSI_PS_SEL (3 << 16) +#define PACKED_PS_16BIT_RGB565 (0 << 16) +#define LOOSELY_PS_18BIT_RGB666(1 << 16) +#define PACKED_PS_18BIT_RGB666 (2 << 16) +#define PACKED_PS_24BIT_RG