subject:"\[PATCH v13 03\/14\] drm\/mediatek\: Add DSI sub driver"

[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

2016-03-15 Thread Daniel Kurtz

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, >encoder, _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(>encoder, 
> > > _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, >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

2016-03-15 Thread Philipp Zabel

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, >encoder, _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(>encoder, 
> > _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, >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

2016-03-09 Thread 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:
> 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[] = 
> {
> _drm_platform_driver,
> _disp_ovl_driver,
> _disp_rdma_driver,
> +   _dsi_driver,
> +   _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)
>

[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

2016-03-08 Thread Philipp Zabel

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[] = {
_drm_platform_driver,
_disp_ovl_driver,
_disp_rdma_driver,
+   _dsi_driver,
+   _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

[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

4 matches

Site Navigation

Mail list logo

Footer information