Hi, YT:

One comment inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

We need to acquire mutex before using the resources, and need to release it after finished. So we don't need to write registers in the blanking period.

Signed-off-by: YT Shen yt.shen@mediatek.com

drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 75 +++++++++++++++++++------------ drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 22 +++++++++ drivers/gpu/drm/mediatek/mtk_drm_ddp.h | 2 + drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + 4 files changed, 71 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index 24aa3ba..80d9641 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -315,6 +315,42 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) pm_runtime_put(drm->dev); }

+static void mtk_crtc_ddp_config(struct drm_crtc *crtc) +{

• struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
• struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
• struct mtk_ddp_comp *ovl = mtk_crtc->ddp_comp[0];
• unsigned int i;
• /*

• * TODO: instead of updating the registers here, we should prepare
  

• * working registers in atomic_commit and let the hardware command
  

• * queue update module registers on vblank.
  

• */
  

• if (state->pending_config) {

• mtk_ddp_comp_config(ovl, state->pending_width,
  

• 		    state->pending_height,
  

• 		    state->pending_vrefresh);
  

• state->pending_config = false;
  

• }
• if (mtk_crtc->pending_planes) {

• for (i = 0; i < OVL_LAYER_NR; i++) {
  

• 	struct drm_plane *plane = &mtk_crtc->planes[i].base;
  

• 	struct mtk_plane_state *plane_state;
  

• 	plane_state = to_mtk_plane_state(plane->state);
  

• 	if (plane_state->pending.config) {
  

• 		mtk_ddp_comp_layer_config(ovl, i, plane_state);
  

• 		plane_state->pending.config = false;
  

• 	}
  

• }
  

• mtk_crtc->pending_planes = false;
  

• }

+}

static void mtk_drm_crtc_enable(struct drm_crtc *crtc) { struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc); @@ -391,6 +427,7 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);

• struct mtk_drm_private *priv = crtc->dev->dev_private; unsigned int pending_planes = 0; int i;

@@ -409,6 +446,12 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc, } if (pending_planes) mtk_crtc->pending_planes = true;

• if (priv->data->shadow_register) {

• mtk_disp_mutex_acquire(mtk_crtc->mutex);
  

• mtk_crtc_ddp_config(crtc);
  

• mtk_disp_mutex_release(mtk_crtc->mutex);
  

• }

}

static const struct drm_crtc_funcs mtk_crtc_funcs = { @@ -453,36 +496,10 @@ err_cleanup_crtc: void mtk_crtc_ddp_irq(struct drm_crtc *crtc, struct mtk_ddp_comp *ovl) { struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);

• struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
• unsigned int i;

• struct mtk_drm_private *priv = crtc->dev->dev_private;

• /*

• * TODO: instead of updating the registers here, we should prepare
  

• * working registers in atomic_commit and let the hardware command
  

• * queue update module registers on vblank.
  

• */
  

• if (state->pending_config) {

• mtk_ddp_comp_config(ovl, state->pending_width,
  

• 		    state->pending_height,
  

• 		    state->pending_vrefresh);
  

• state->pending_config = false;
  

• }
• if (mtk_crtc->pending_planes) {

• for (i = 0; i < OVL_LAYER_NR; i++) {
  

• 	struct drm_plane *plane = &mtk_crtc->planes[i].base;
  

• 	struct mtk_plane_state *plane_state;
  

• 	plane_state = to_mtk_plane_state(plane->state);
  

• 	if (plane_state->pending.config) {
  

• 		mtk_ddp_comp_layer_config(ovl, i, plane_state);
  

• 		plane_state->pending.config = false;
  

• 	}
  

• }
  

• mtk_crtc->pending_planes = false;
  

• }

• if (!priv->data->shadow_register)

• mtk_crtc_ddp_config(crtc);
  
  

  mtk_drm_finish_page_flip(mtk_crtc);

} diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c index 8030769..fa53806 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c @@ -12,6 +12,7 @@ */

#include <linux/clk.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/platform_device.h> @@ -32,6 +33,7 @@ #define DISP_REG_CONFIG_MMSYS_CG_CON0 0x100

#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n)) +#define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n)) #define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n)) #define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * (n)) #define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n)) @@ -300,6 +302,26 @@ void mtk_disp_mutex_disable(struct mtk_disp_mutex *mutex) writel(0, ddp->regs + DISP_REG_MUTEX_EN(mutex->id)); }

+void mtk_disp_mutex_acquire(struct mtk_disp_mutex *mutex) +{

• struct mtk_ddp *ddp = container_of(mutex, struct mtk_ddp,

• 			   mutex[mutex->id]);
  

• u32 tmp;
• writel(1, ddp->regs + DISP_REG_MUTEX_EN(mutex->id));
• writel(1, ddp->regs + DISP_REG_MUTEX(mutex->id));
• readl_poll_timeout_atomic(ddp->regs + DISP_REG_MUTEX(mutex->id), tmp,

• 		  tmp & 0x2, 1, 10000);
  

Nothing to do with timeout? I think it should print error message here or reset HW to recover it.

+}

+void mtk_disp_mutex_release(struct mtk_disp_mutex *mutex) +{

• struct mtk_ddp *ddp = container_of(mutex, struct mtk_ddp,

• 			   mutex[mutex->id]);
  

• writel(0, ddp->regs + DISP_REG_MUTEX(mutex->id));

+}

static int mtk_ddp_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h index 92c1175..f9a7991 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h @@ -37,5 +37,7 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex, enum mtk_ddp_comp_id id); void mtk_disp_mutex_unprepare(struct mtk_disp_mutex *mutex); void mtk_disp_mutex_put(struct mtk_disp_mutex *mutex); +void mtk_disp_mutex_acquire(struct mtk_disp_mutex *mutex); +void mtk_disp_mutex_release(struct mtk_disp_mutex *mutex);

#endif /* MTK_DRM_DDP_H */ diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index fa0b106..94f8b66 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h @@ -33,6 +33,7 @@ struct mtk_mmsys_driver_data { unsigned int main_len; const enum mtk_ddp_comp_id *ext_path; unsigned int ext_len;

• bool shadow_register;

};

struct mtk_drm_private {

Hi Philipp, CK,

Thanks for the review.

On Mon, 2016-07-18 at 10:36 +0200, Philipp Zabel wrote:

Hi CK, YT,

Am Montag, den 18.07.2016, 14:32 +0800 schrieb CK Hu:

...

Hi, YT:

One comment inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

...

We need to acquire mutex before using the resources, and need to release it after finished. So we don't need to write registers in the blanking period.

Signed-off-by: YT Shen yt.shen@mediatek.com

drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 75 +++++++++++++++++++------------ drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 22 +++++++++ drivers/gpu/drm/mediatek/mtk_drm_ddp.h | 2 + drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + 4 files changed, 71 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index 24aa3ba..80d9641 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -315,6 +315,42 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) pm_runtime_put(drm->dev); }

+static void mtk_crtc_ddp_config(struct drm_crtc *crtc) +{

• struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
• struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
• struct mtk_ddp_comp *ovl = mtk_crtc->ddp_comp[0];
• unsigned int i;
• /*

• * TODO: instead of updating the registers here, we should prepare
  

• * working registers in atomic_commit and let the hardware command
  

• * queue update module registers on vblank.
  

• */
  

• if (state->pending_config) {

• mtk_ddp_comp_config(ovl, state->pending_width,
  

• 		    state->pending_height,
  

• 		    state->pending_vrefresh);
  

• state->pending_config = false;
  

• }
• if (mtk_crtc->pending_planes) {

• for (i = 0; i < OVL_LAYER_NR; i++) {
  

• 	struct drm_plane *plane = &mtk_crtc->planes[i].base;
  

• 	struct mtk_plane_state *plane_state;
  

• 	plane_state = to_mtk_plane_state(plane->state);
  

• 	if (plane_state->pending.config) {
  

• 		mtk_ddp_comp_layer_config(ovl, i, plane_state);
  

• 		plane_state->pending.config = false;
  

• 	}
  

• }
  

• mtk_crtc->pending_planes = false;
  

• }

+}

static void mtk_drm_crtc_enable(struct drm_crtc *crtc) { struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc); @@ -391,6 +427,7 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);

• struct mtk_drm_private *priv = crtc->dev->dev_private; unsigned int pending_planes = 0; int i;

@@ -409,6 +446,12 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc, } if (pending_planes) mtk_crtc->pending_planes = true;

• if (priv->data->shadow_register) {

• mtk_disp_mutex_acquire(mtk_crtc->mutex);
  

• mtk_crtc_ddp_config(crtc);
  

• mtk_disp_mutex_release(mtk_crtc->mutex);
  

• }

}

static const struct drm_crtc_funcs mtk_crtc_funcs = { @@ -453,36 +496,10 @@ err_cleanup_crtc: void mtk_crtc_ddp_irq(struct drm_crtc *crtc, struct mtk_ddp_comp *ovl) { struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);

• struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
• unsigned int i;

• struct mtk_drm_private *priv = crtc->dev->dev_private;

• /*

• * TODO: instead of updating the registers here, we should prepare
  

• * working registers in atomic_commit and let the hardware command
  

• * queue update module registers on vblank.
  

• */
  

• if (state->pending_config) {

• mtk_ddp_comp_config(ovl, state->pending_width,
  

• 		    state->pending_height,
  

• 		    state->pending_vrefresh);
  

• state->pending_config = false;
  

• }
• if (mtk_crtc->pending_planes) {

• for (i = 0; i < OVL_LAYER_NR; i++) {
  

• 	struct drm_plane *plane = &mtk_crtc->planes[i].base;
  

• 	struct mtk_plane_state *plane_state;
  

• 	plane_state = to_mtk_plane_state(plane->state);
  

• 	if (plane_state->pending.config) {
  

• 		mtk_ddp_comp_layer_config(ovl, i, plane_state);
  

• 		plane_state->pending.config = false;
  

• 	}
  

• }
  

• mtk_crtc->pending_planes = false;
  

• }

• if (!priv->data->shadow_register)

• mtk_crtc_ddp_config(crtc);
  
  

  mtk_drm_finish_page_flip(mtk_crtc);

} diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c index 8030769..fa53806 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c @@ -12,6 +12,7 @@ */

#include <linux/clk.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/platform_device.h> @@ -32,6 +33,7 @@ #define DISP_REG_CONFIG_MMSYS_CG_CON0 0x100

#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n)) +#define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n)) #define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n)) #define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * (n)) #define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n)) @@ -300,6 +302,26 @@ void mtk_disp_mutex_disable(struct mtk_disp_mutex *mutex) writel(0, ddp->regs + DISP_REG_MUTEX_EN(mutex->id)); }

+void mtk_disp_mutex_acquire(struct mtk_disp_mutex *mutex) +{

• struct mtk_ddp *ddp = container_of(mutex, struct mtk_ddp,

• 			   mutex[mutex->id]);
  

• u32 tmp;
• writel(1, ddp->regs + DISP_REG_MUTEX_EN(mutex->id));
• writel(1, ddp->regs + DISP_REG_MUTEX(mutex->id));
• readl_poll_timeout_atomic(ddp->regs + DISP_REG_MUTEX(mutex->id), tmp,

• 		  tmp & 0x2, 1, 10000);
  

Please add a descriptive #define for the 0x2 bit of the DISP_REG_MUTEX registers.

OK, will do.

...

Nothing to do with timeout? I think it should print error message here or reset HW to recover it.

I will print error messages here for future debugging.

I assume that if mtk_disp_mutex_acquire is called while the hardware has the mutex (during the shadow register update), this should wait until the update is complete, since it can't back out from trying to acquire the lock. Is that the case?

The acquire/release is handshake interface between software and hardware to guarantee hardware receives a complete setting. If software wants to modify registers, it needs set a request and wait acknowledge. And after everything is done, software needs to notify hardware. As far as I know, this timeout means hardware is wrong, eg: no output signals.

regards Philipp

Hi, YT:

One comment inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

This patch add support for the Mediatek MT2701 DISP subsystem. There is only one OVL engine in MT2701.

Signed-off-by: YT Shen yt.shen@mediatek.com

drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 6 ++++ drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 6 ++++ drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 41 +++++++++++++++++++++++++++ drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 7 +++++ drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 31 ++++++++++++++++++++ 6 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index eb5c05e..1da0a71 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -286,11 +286,17 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev) return 0; }

+static const struct mtk_ddp_comp_driver_data mt2701_ovl_driver_data = {

• .ovl = {0x0040, 1 << 12, 0}

+};

static const struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = { .ovl = {0x0f40, 0, 1 << 12} };

static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {

• { .compatible = "mediatek,mt2701-disp-ovl",

•  .data = &mt2701_ovl_driver_data},
  

  { .compatible = "mediatek,mt8173-disp-ovl", .data = &mt8173_ovl_driver_data}, {},

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c index fb0db50..506a353 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c @@ -225,11 +225,17 @@ static int mtk_disp_rdma_remove(struct platform_device *pdev) return 0; }

+static const struct mtk_ddp_comp_driver_data mt2701_rdma_driver_data = {

• .rdma_fifo_pseudo_size = SZ_4K,

+};

static const struct mtk_ddp_comp_driver_data mt8173_rdma_driver_data = { .rdma_fifo_pseudo_size = SZ_8K, };

static const struct of_device_id mtk_disp_rdma_driver_dt_match[] = {

• { .compatible = "mediatek,mt2701-disp-rdma",

•  .data = &mt2701_rdma_driver_data},
  

  { .compatible = "mediatek,mt8173-disp-rdma", .data = &mt8173_rdma_driver_data}, {},

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c index fa53806..ee0326a 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c @@ -32,6 +32,10 @@ #define DISP_REG_CONFIG_DISP_RDMA1_MOUT_EN 0x0c8 #define DISP_REG_CONFIG_MMSYS_CG_CON0 0x100

+#define DISP_REG_CONFIG_DISP_OVL_MOUT_EN 0x030 +#define DISP_REG_CONFIG_OUT_SEL 0x04c +#define DISP_REG_CONFIG_DSI_SEL 0x050

#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n)) #define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n)) #define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n)) @@ -54,6 +58,13 @@ #define MT8173_MUTEX_MOD_DISP_PWM1 BIT(24) #define MT8173_MUTEX_MOD_DISP_OD BIT(25)

+#define MT2701_MUTEX_MOD_DISP_OVL BIT(3) +#define MT2701_MUTEX_MOD_DISP_WDMA BIT(6) +#define MT2701_MUTEX_MOD_DISP_COLOR BIT(7) +#define MT2701_MUTEX_MOD_DISP_BLS BIT(9) +#define MT2701_MUTEX_MOD_DISP_RDMA0 BIT(10) +#define MT2701_MUTEX_MOD_DISP_RDMA1 BIT(12)

#define MUTEX_SOF_SINGLE_MODE 0 #define MUTEX_SOF_DSI0 1 #define MUTEX_SOF_DSI1 2 @@ -69,6 +80,10 @@ #define DPI0_SEL_IN_RDMA1 0x1 #define COLOR1_SEL_IN_OVL1 0x1

+#define OVL_MOUT_EN_RDMA 0x1 +#define BLS_TO_DSI_RDMA1_TO_DPI1 0x8 +#define DSI_SEL_IN_BLS 0x0

struct mtk_disp_mutex { int id; bool claimed; @@ -82,6 +97,15 @@ struct mtk_ddp { const unsigned int *mutex_mod; };

+static const unsigned int mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = {

• [DDP_COMPONENT_BLS] = MT2701_MUTEX_MOD_DISP_BLS,
• [DDP_COMPONENT_COLOR0] = MT2701_MUTEX_MOD_DISP_COLOR,
• [DDP_COMPONENT_OVL0] = MT2701_MUTEX_MOD_DISP_OVL,
• [DDP_COMPONENT_RDMA0] = MT2701_MUTEX_MOD_DISP_RDMA0,
• [DDP_COMPONENT_RDMA1] = MT2701_MUTEX_MOD_DISP_RDMA1,
• [DDP_COMPONENT_WDMA0] = MT2701_MUTEX_MOD_DISP_WDMA,

+};

static const unsigned int mt8173_mutex_mod[DDP_COMPONENT_ID_MAX] = { [DDP_COMPONENT_AAL] = MT8173_MUTEX_MOD_DISP_AAL, [DDP_COMPONENT_COLOR0] = MT8173_MUTEX_MOD_DISP_COLOR0, @@ -109,6 +133,9 @@ static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur, if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_COLOR0) { *addr = DISP_REG_CONFIG_DISP_OVL0_MOUT_EN; value = OVL0_MOUT_EN_COLOR0;

• } else if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_RDMA0) {

• *addr = DISP_REG_CONFIG_DISP_OVL_MOUT_EN;
  

• value = OVL_MOUT_EN_RDMA;
  

  } else if (cur == DDP_COMPONENT_OD && next == DDP_COMPONENT_RDMA0) { *addr = DISP_REG_CONFIG_DISP_OD_MOUT_EN; value = OD_MOUT_EN_RDMA0;

@@ -146,6 +173,9 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur, } else if (cur == DDP_COMPONENT_OVL1 && next == DDP_COMPONENT_COLOR1) { *addr = DISP_REG_CONFIG_DISP_COLOR1_SEL_IN; value = COLOR1_SEL_IN_OVL1;

• } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {

• *addr = DISP_REG_CONFIG_DSI_SEL;
  

• value = DSI_SEL_IN_BLS;
  

  } else { value = 0; }

@@ -153,6 +183,14 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur, return value; }

+static void mtk_ddp_sout_sel(void __iomem *config_regs,

• 	    enum mtk_ddp_comp_id cur, enum mtk_ddp_comp_id next)
  

+{

• if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0)

• writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
  

• 	       config_regs + DISP_REG_CONFIG_OUT_SEL);
  

+}

I think all the connection related modification should be an independent patch because it is common code. Like the shadow register patch (https://patchwork.kernel.org/patch/9231531/) , even MT8173 do not have that feature, we still split it out of this patch because it's not MT2701-only feature. I think these connection may happen in some MTK soc., so the connection related modification should not be placed in a MT2701 patch.

void mtk_ddp_add_comp_to_path(void __iomem *config_regs, enum mtk_ddp_comp_id cur, enum mtk_ddp_comp_id next) @@ -165,6 +203,8 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs, writel_relaxed(reg, config_regs + addr); }

• mtk_ddp_sout_sel(config_regs, cur, next);
• value = mtk_ddp_sel_in(cur, next, &addr); if (value) { reg = readl_relaxed(config_regs + addr) | value;

@@ -362,6 +402,7 @@ static int mtk_ddp_remove(struct platform_device *pdev) }

static const struct of_device_id ddp_driver_dt_match[] = {

• { .compatible = "mediatek,mt2701-disp-mutex", .data = mt2701_mutex_mod}, { .compatible = "mediatek,mt8173-disp-mutex", .data = mt8173_mutex_mod}, {},

}; diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index 4b4e449..465819b 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c @@ -112,6 +112,7 @@ struct mtk_ddp_comp_match {

static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { [DDP_COMPONENT_AAL] = { MTK_DISP_AAL, 0, NULL },

• [DDP_COMPONENT_BLS] = { MTK_DISP_PWM, 0, NULL }, [DDP_COMPONENT_COLOR0] = { MTK_DISP_COLOR, 0, &ddp_color }, [DDP_COMPONENT_COLOR1] = { MTK_DISP_COLOR, 1, &ddp_color }, [DDP_COMPONENT_DPI0] = { MTK_DPI, 0, NULL },

@@ -130,11 +131,17 @@ static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { [DDP_COMPONENT_WDMA1] = { MTK_DISP_WDMA, 1, NULL }, };

+static const struct mtk_ddp_comp_driver_data mt2701_color_driver_data = {

• .color_offset = 0x0f00,

+};

static const struct mtk_ddp_comp_driver_data mt8173_color_driver_data = { .color_offset = 0x0c00, };

static const struct of_device_id mtk_disp_color_driver_dt_match[] = {

• { .compatible = "mediatek,mt2701-disp-color",

•  .data = &mt2701_color_driver_data},
  

  { .compatible = "mediatek,mt8173-disp-color", .data = &mt8173_color_driver_data}, {},

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h index 53065c7..0850aa4 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h @@ -40,6 +40,7 @@ enum mtk_ddp_comp_type {

enum mtk_ddp_comp_id { DDP_COMPONENT_AAL,

• DDP_COMPONENT_BLS, DDP_COMPONENT_COLOR0, DDP_COMPONENT_COLOR1, DDP_COMPONENT_DPI0,

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 80b4f54..13cf160 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -109,6 +109,19 @@ static const struct drm_mode_config_funcs mtk_drm_mode_config_funcs = { .atomic_commit = mtk_atomic_commit, };

+static const enum mtk_ddp_comp_id mt2701_mtk_ddp_main[] = {

• DDP_COMPONENT_OVL0,
• DDP_COMPONENT_RDMA0,
• DDP_COMPONENT_COLOR0,
• DDP_COMPONENT_BLS,
• DDP_COMPONENT_DSI0,

+};

+static const enum mtk_ddp_comp_id mt2701_mtk_ddp_ext[] = {

• DDP_COMPONENT_RDMA1,
• DDP_COMPONENT_DPI0,

+};

static const enum mtk_ddp_comp_id mt8173_mtk_ddp_main[] = { DDP_COMPONENT_OVL0, DDP_COMPONENT_COLOR0, @@ -128,6 +141,14 @@ static const enum mtk_ddp_comp_id mt8173_mtk_ddp_ext[] = { DDP_COMPONENT_DPI0, };

+static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {

• .main_path = mt2701_mtk_ddp_main,
• .main_len = ARRAY_SIZE(mt2701_mtk_ddp_main),
• .ext_path = mt2701_mtk_ddp_ext,
• .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
• .shadow_register = true,

+};

static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { .main_path = mt8173_mtk_ddp_main, .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), @@ -331,16 +352,24 @@ static const struct component_master_ops mtk_drm_ops = { };

static const struct of_device_id mtk_ddp_comp_dt_ids[] = {

• { .compatible = "mediatek,mt2701-disp-ovl", .data = (void *)MTK_DISP_OVL }, { .compatible = "mediatek,mt8173-disp-ovl", .data = (void *)MTK_DISP_OVL },
• { .compatible = "mediatek,mt2701-disp-rdma", .data = (void *)MTK_DISP_RDMA }, { .compatible = "mediatek,mt8173-disp-rdma", .data = (void *)MTK_DISP_RDMA },
• { .compatible = "mediatek,mt2701-disp-wdma", .data = (void *)MTK_DISP_WDMA }, { .compatible = "mediatek,mt8173-disp-wdma", .data = (void *)MTK_DISP_WDMA },
• { .compatible = "mediatek,mt2701-disp-color", .data = (void *)MTK_DISP_COLOR }, { .compatible = "mediatek,mt8173-disp-color", .data = (void *)MTK_DISP_COLOR }, { .compatible = "mediatek,mt8173-disp-aal", .data = (void *)MTK_DISP_AAL}, { .compatible = "mediatek,mt8173-disp-gamma", .data = (void *)MTK_DISP_GAMMA, }, { .compatible = "mediatek,mt8173-disp-ufoe", .data = (void *)MTK_DISP_UFOE },
• { .compatible = "mediatek,mt2701-dsi", .data = (void *)MTK_DSI }, { .compatible = "mediatek,mt8173-dsi", .data = (void *)MTK_DSI },
• { .compatible = "mediatek,mt2701-dpi", .data = (void *)MTK_DPI }, { .compatible = "mediatek,mt8173-dpi", .data = (void *)MTK_DPI },
• { .compatible = "mediatek,mt2701-disp-mutex", .data = (void *)MTK_DISP_MUTEX }, { .compatible = "mediatek,mt8173-disp-mutex", .data = (void *)MTK_DISP_MUTEX },
• { .compatible = "mediatek,mt2701-disp-pwm", .data = (void *)MTK_DISP_PWM }, { .compatible = "mediatek,mt8173-disp-pwm", .data = (void *)MTK_DISP_PWM }, { .compatible = "mediatek,mt8173-disp-od", .data = (void *)MTK_DISP_OD }, { }

@@ -514,6 +543,8 @@ static SIMPLE_DEV_PM_OPS(mtk_drm_pm_ops, mtk_drm_sys_suspend, mtk_drm_sys_resume);

static const struct of_device_id mtk_drm_of_ids[] = {

• { .compatible = "mediatek,mt2701-mmsys",

•  .data = &mt2701_mmsys_driver_data},
  

  { .compatible = "mediatek,mt8173-mmsys", .data = &mt8173_mmsys_driver_data}, { }

Hi, YT:

Some comments inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

This patch add support for the Mediatek MT2701 DISP subsystem. There is only one OVL engine in MT2701.

Signed-off-by: YT Shen yt.shen@mediatek.com

drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 6 ++++ drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 6 ++++ drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 41 +++++++++++++++++++++++++++ drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 7 +++++ drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 31 ++++++++++++++++++++ 6 files changed, 92 insertions(+)

[snip...]

static const struct of_device_id mtk_ddp_comp_dt_ids[] = {

• { .compatible = "mediatek,mt2701-disp-ovl", .data = (void *)MTK_DISP_OVL }, { .compatible = "mediatek,mt8173-disp-ovl", .data = (void *)MTK_DISP_OVL },
• { .compatible = "mediatek,mt2701-disp-rdma", .data = (void *)MTK_DISP_RDMA }, { .compatible = "mediatek,mt8173-disp-rdma", .data = (void *)MTK_DISP_RDMA },
• { .compatible = "mediatek,mt2701-disp-wdma", .data = (void *)MTK_DISP_WDMA },

Is WDMA different from MT8173 to MT2701. If they are the same, you need not to add compatible of 'mediatek,mt2701-disp-wdma' because use 'mediatek,mt8173-disp-wdma' is enough.

{ .compatible = "mediatek,mt8173-disp-wdma", .data = (void *)MTK_DISP_WDMA },

• { .compatible = "mediatek,mt2701-disp-color", .data = (void *)MTK_DISP_COLOR }, { .compatible = "mediatek,mt8173-disp-color", .data = (void *)MTK_DISP_COLOR }, { .compatible = "mediatek,mt8173-disp-aal", .data = (void *)MTK_DISP_AAL}, { .compatible = "mediatek,mt8173-disp-gamma", .data = (void *)MTK_DISP_GAMMA, }, { .compatible = "mediatek,mt8173-disp-ufoe", .data = (void *)MTK_DISP_UFOE },
• { .compatible = "mediatek,mt2701-dsi", .data = (void *)MTK_DSI }, { .compatible = "mediatek,mt8173-dsi", .data = (void *)MTK_DSI },
• { .compatible = "mediatek,mt2701-dpi", .data = (void *)MTK_DPI },

Is DPI different from MT8173 to MT2701. If they are the same, you need not to add compatible of 'mediatek,mt2701-dpi' because use 'mediatek,mt8173-dpi' is enough.

{ .compatible = "mediatek,mt8173-dpi", .data = (void *)MTK_DPI },

• { .compatible = "mediatek,mt2701-disp-mutex", .data = (void *)MTK_DISP_MUTEX }, { .compatible = "mediatek,mt8173-disp-mutex", .data = (void *)MTK_DISP_MUTEX },
• { .compatible = "mediatek,mt2701-disp-pwm", .data = (void *)MTK_DISP_PWM },

Is pwm different from MT8173 to MT2701. If they are the same, you need not to add compatible of 'mediatek,mt2701-disp-pwm' because use 'mediatek,mt8173-disp-pwm' is enough.

{ .compatible = "mediatek,mt8173-disp-pwm", .data = (void *)MTK_DISP_PWM }, { .compatible = "mediatek,mt8173-disp-od", .data = (void *)MTK_DISP_OD }, { } @@ -514,6 +543,8 @@ static SIMPLE_DEV_PM_OPS(mtk_drm_pm_ops, mtk_drm_sys_suspend, mtk_drm_sys_resume);

static const struct of_device_id mtk_drm_of_ids[] = {

• { .compatible = "mediatek,mt2701-mmsys",

•  .data = &mt2701_mmsys_driver_data},
  

  { .compatible = "mediatek,mt8173-mmsys", .data = &mt8173_mmsys_driver_data}, { }

Regards, CK

Hi CK,

On Tue, 2016-07-19 at 13:39 +0800, CK Hu wrote:

Hi, YT:

Some comments inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

...

From: shaoming chen shaoming.chen@mediatek.com

add dsi interrupt control

Signed-off-by: shaoming chen shaoming.chen@mediatek.com

drivers/gpu/drm/mediatek/mtk_dsi.c | 130 ++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 2d808e5..de5ad7f 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -18,6 +18,7 @@ #include <drm/drm_panel.h> #include <linux/clk.h> #include <linux/component.h> +#include <linux/irq.h> #include <linux/of.h> #include <linux/of_platform.h> #include <linux/of_graph.h> @@ -32,6 +33,13 @@

#define DSI_START 0x00

+#define DSI_INTEN 0x08

+#define DSI_INTSTA 0x0c +#define LPRX_RD_RDY_INT_FLAG BIT(0) +#define CMD_DONE_INT_FLAG BIT(1) +#define DSI_BUSY BIT(31)

Why need LPRX_RD_RDY_INT_FLAG, CMD_DONE_INT_FLAG, and DSI_BUSY? Maybe these three should be moved to other patch.

OK.

...

#define DSI_CON_CTRL 0x10 #define DSI_RESET BIT(0) #define DSI_EN BIT(1) @@ -74,6 +82,9 @@

#define DSI_HSTX_CKL_WC 0x64

+#define DSI_RACK 0x84 +#define RACK BIT(0)

#define DSI_PHY_LCCON 0x104 #define LC_HS_TX_EN BIT(0) #define LC_ULPM_EN BIT(1) @@ -134,6 +145,18 @@ struct mtk_dsi { struct videomode vm; int refcount; bool enabled;

• int irq_num, irq_data;

+};

+enum {

• DSI_INT_SLEEPOUT_DONE_FLAG = BIT(6),
• DSI_INT_VM_CMD_DONE_FLAG = BIT(5),
• DSI_INT_EXT_TE_RDY_FLAG = BIT(4),
• DSI_INT_VM_DONE_FLAG = BIT(3),
• DSI_INT_TE_RDY_FLAG = BIT(2),
• DSI_INT_CMD_DONE_FLAG = BIT(1),
• DSI_INT_LPRX_RD_RDY_FLAG = BIT(0),
• DSI_INT_ALL_BITS = (0x7f)

};

I think you should use '#define' instead of 'enum'. The code would be like below, and these definition should be moved to after DSI_INTEN or DSI_INTSTA.

#define DSI_INT_LPRX_RD_RDY_FLAG BIT(0) #define DSI_INT_CMD_DONE_FLAG BIT(1) #define DSI_INT_TE_RDY_FLAG BIT(2) #define DSI_INT_VM_DONE_FLAG BIT(3) #define DSI_INT_EXT_TE_RDY_FLAG BIT(4) #define DSI_INT_VM_CMD_DONE_FLAG BIT(5) #define DSI_INT_SLEEPOUT_DONE_FLAG BIT(6) #define DSI_INT_ALL_BITS (DSI_INT_LPRX_RD_RDY_FLAG | \ DSI_INT_CMD_DONE_FLAG | \ DSI_INT_TE_RDY_FLAG | \ DSI_INT_VM_DONE_FLAG | \ DSI_INT_EXT_TE_RDY_FLAG | \ DSI_INT_VM_CMD_DONE_FLAG | \ DSI_INT_SLEEPOUT_DONE_FLAG)

I will update this part in the next version.

...

static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e) @@ -440,6 +463,94 @@ static void mtk_dsi_start(struct mtk_dsi *dsi) writel(1, dsi->regs + DSI_START); }

+static void mtk_dsi_set_interrupt_enable(struct mtk_dsi *dsi) +{

• u32 inten = DSI_INT_ALL_BITS;
• if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO)

• inten &= ~(DSI_INT_TE_RDY_FLAG | DSI_INT_EXT_TE_RDY_FLAG);
  

• writel(inten, dsi->regs + DSI_INTEN);

+}

+static void mtk_dsi_irq_wakeup(struct mtk_dsi *dsi, u32 irq_bit) +{

• dsi->irq_data |= irq_bit;

+}

+static irqreturn_t mtk_dsi_irq(int irq, void *dev_id) +{

• struct mtk_dsi *dsi = dev_id;
• u32 status, tmp;
• status = readl(dsi->regs + DSI_INTSTA);
• if (status & DSI_INT_LPRX_RD_RDY_FLAG) {

• /* write clear RD_RDY interrupt */
  

• /* write clear RD_RDY interrupt must be before DSI_RACK */
  

• /* because CMD_DONE will raise after DSI_RACK, */
  

• /* so write clear RD_RDY after that will clear CMD_DONE too */
  

• do {
  

• 	/* send read ACK */
  

• 	mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK);
  

• 	tmp = readl(dsi->regs + DSI_INTSTA);
  

• } while (tmp & DSI_BUSY);
  

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_LPRX_RD_RDY_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_LPRX_RD_RDY_FLAG);
  

• }
• if (status & DSI_INT_CMD_DONE_FLAG) {

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_CMD_DONE_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_CMD_DONE_FLAG);
  

• }
• if (status & DSI_INT_TE_RDY_FLAG) {

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_TE_RDY_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_TE_RDY_FLAG);
  

• }
• if (status & DSI_INT_VM_DONE_FLAG) {

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_VM_DONE_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_VM_DONE_FLAG);
  

• }
• if (status & DSI_INT_EXT_TE_RDY_FLAG) {

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_EXT_TE_RDY_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_EXT_TE_RDY_FLAG);
  

• }
• if (status & DSI_INT_VM_CMD_DONE_FLAG) {

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_VM_CMD_DONE_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_VM_CMD_DONE_FLAG);
  

• }
• if (status & DSI_INT_SLEEPOUT_DONE_FLAG) {

• mtk_dsi_mask(dsi, DSI_INTSTA, DSI_INT_SLEEPOUT_DONE_FLAG, 0);
  

• mtk_dsi_irq_wakeup(dsi, DSI_INT_SLEEPOUT_DONE_FLAG);
  

• }

All these 'if' statement do the same thing and I think these could be merge as:

status = readl(dsi->regs + DSI_INTSTA) & DSI_INT_ALL_BITS); if (status) { mtk_dsi_mask(dsi, DSI_INTSTA, status, 0); mtk_dsi_irq_wakeup(dsi, status); }

Will do.

...

• return IRQ_HANDLED;

+}

+static s32 mtk_dsi_wait_for_irq_timeout(struct mtk_dsi *dsi, u32 irq_bit,

• 			u32 timeout_ms)
  

+{

• while (timeout_ms--) {

• if (dsi->irq_data & irq_bit) {
  

• 	dsi->irq_data &= ~irq_bit;
  

• 	return 0;
  

• }
  

• usleep_range(1000, 1100);
  

• }

I think it's better to use wait_event() or something else. Polling with sleep could not react immediately.

Will check.

...

• dsi->irq_data = 0;

Why clear all bit after wait one bit timeout?

Will check.

...

• return -1;

+}

static void mtk_dsi_poweroff(struct mtk_dsi *dsi) { if (WARN_ON(dsi->refcount == 0)) @@ -488,6 +599,7 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)

dsi_ps_control_vact(dsi); dsi_config_vdo_timing(dsi);

• mtk_dsi_set_interrupt_enable(dsi);

  dsi_set_mode(dsi); dsi_clk_hs_mode(dsi, 1);

@@ -883,6 +995,24 @@ static int mtk_dsi_probe(struct platform_device *pdev) return ret; }

• dsi->irq_num = platform_get_irq(pdev, 0);

You use irq_num only in this function, why keep it in dsi structure?

OK, will remove it from dsi structure.

Regards, yt.shen

...

• if (dsi->irq_num < 0) {

• dev_err(&pdev->dev, "failed to request dsi irq resource\n");
  

• ret = dsi->irq_num;
  

• return -EPROBE_DEFER;
  

• }

• irq_set_status_flags(dsi->irq_num, IRQ_TYPE_LEVEL_LOW);

• ret = devm_request_irq(&pdev->dev, dsi->irq_num, mtk_dsi_irq,

• 	       IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
  

• if (ret) {

• dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
  

• return -EPROBE_DEFER;
  

• }

• dsi->irq_data = 0;

• dev_info(dev, "dsi irq num is 0x%x\n", dsi->irq_num);

• platform_set_drvdata(pdev, dsi);

  return component_add(&pdev->dev, &mtk_dsi_component_ops);

Regards, CK

Hi, YT:

Some comments inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

From: shaoming chen shaoming.chen@mediatek.com

add dsi read/write commands for transfer function

Signed-off-by: shaoming chen shaoming.chen@mediatek.com

drivers/gpu/drm/mediatek/mtk_dsi.c | 322 ++++++++++++++++++++++++++++++++++++ 1 file changed, 322 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index de5ad7f..1f99894 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -24,6 +24,7 @@ #include <linux/of_graph.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> +#include <video/mipi_display.h> #include <video/videomode.h>

#include "mtk_drm_ddp_comp.h" @@ -80,8 +81,16 @@ #define DSI_HBP_WC 0x54 #define DSI_HFP_WC 0x58

+#define DSI_CMDQ_SIZE 0x60 +#define CMDQ_SIZE 0x3f

#define DSI_HSTX_CKL_WC 0x64

+#define DSI_RX_DATA0 0x74 +#define DSI_RX_DATA1 0x78 +#define DSI_RX_DATA2 0x7c +#define DSI_RX_DATA3 0x80

#define DSI_RACK 0x84 #define RACK BIT(0)

@@ -117,8 +126,25 @@ #define CLK_HS_POST (0xff << 8) #define CLK_HS_EXIT (0xff << 16)

+#define DSI_CMDQ0 0x180

#define NS_TO_CYCLE(n, c) ((n) / (c) + (((n) % (c)) ? 1 : 0))

+#define MTK_DSI_HOST_IS_READ(type) \

• ((type == MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM) || \
• (type == MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM) || \
• (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \
• (type == MIPI_DSI_DCS_READ))

+#define MTK_DSI_HOST_IS_WRITE(type) \

• ((type == MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM) || \
• (type == MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM) || \
• (type == MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM) || \
• (type == MIPI_DSI_DCS_SHORT_WRITE) || \
• (type == MIPI_DSI_DCS_SHORT_WRITE_PARAM) || \
• (type == MIPI_DSI_GENERIC_LONG_WRITE) || \
• (type == MIPI_DSI_DCS_LONG_WRITE))

struct phy;

struct mtk_dsi { @@ -148,6 +174,38 @@ struct mtk_dsi { int irq_num, irq_data; };

+struct dsi_cmd_t0 {

• u8 config;
• u8 type;
• u8 data0;
• u8 data1;

+};

+struct dsi_cmd_t2 {

• u8 config;
• u8 type;
• u16 wc16;
• u8 *pdata;

+};

+struct dsi_rx_data {

• u8 byte0;
• u8 byte1;
• u8 byte2;
• u8 byte3;

+};

+struct dsi_tx_cmdq {

• u8 byte0;
• u8 byte1;
• u8 byte2;
• u8 byte3;

+};

+struct dsi_tx_cmdq_regs {

• struct dsi_tx_cmdq data[128];

+};

enum { DSI_INT_SLEEPOUT_DONE_FLAG = BIT(6), DSI_INT_VM_CMD_DONE_FLAG = BIT(5), @@ -858,9 +916,273 @@ static int mtk_dsi_host_detach(struct mipi_dsi_host *host, return 0; }

+static void mtk_dsi_set_cmdq(void __iomem *reg, u32 mask, u32 data) +{

• u32 temp = readl(reg);
• writel((temp & ~mask) | (data & mask), reg);

+}

+static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi) +{

• u32 timeout_ms = 500000; /* total 1s ~ 2s timeout */
• while (timeout_ms--) {

• if (!(readl(dsi->regs + DSI_INTSTA) & DSI_BUSY))
  

• 	break;
  

• usleep_range(2, 4);
  

• }
• if (timeout_ms == 0) {

• dev_info(dsi->dev, "polling dsi wait not busy timeout!\n");
  

• mtk_dsi_enable(dsi);
  

• mtk_dsi_reset_engine(dsi);
  

• }

+}

+static ssize_t mtk_dsi_host_read_cmd(struct mtk_dsi *dsi,

• 		     const struct mipi_dsi_msg *msg)
  

+{

• u8 max_try_count = 5;
• u32 recv_data_cnt, tmp_val;
• u32 recv_data0, recv_data1, recv_data2, recv_data3;
• struct dsi_rx_data read_data0, read_data1, read_data2, read_data3;
• struct dsi_cmd_t0 t0;
• s32 ret;
• u8 *buffer = msg->rx_buf;
• u8 buffer_size = msg->rx_len;
• u8 packet_type;
• if (readl(dsi->regs + DSI_MODE_CTRL) & 0x03) {

• dev_info(dsi->dev, "dsi engine is not command mode\n");
  

• return -1;
  

• }
• if (!buffer) {

• dev_info(dsi->dev, "dsi receive buffer size may be NULL\n");
  

• return -1;
  

• }
• do {

• if (max_try_count == 0) {
  

• 	dev_info(dsi->dev, "dsi engine read counter has been maxinum\n");
  

• 	return -1;
  

• }
  

• max_try_count--;
  

• recv_data_cnt = 0;
  

• mtk_dsi_wait_for_idle(dsi);
  

• t0.config = 0x04;
  

• t0.data0 = *((u8 *)(msg->tx_buf));
  

• if (buffer_size < 0x3)
  

It's better to use '3' instead of '0x3'.

• 	t0.type = MIPI_DSI_DCS_READ;
  

• else
  

• 	t0.type = MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM;
  

• t0.data1 = 0;
  

• tmp_val = (t0.data1 << 24) | (t0.data0 << 16) | (t0.type << 8) |
  

• 	t0.config;
  

Why need 'struct dsi_cmd_t0 t0'? I think code is more simple when get rid of this structure. The code looks like:

tmp_val = (data1 << 24) | (data0 << 16) | (type << 8) | config;

• writel(tmp_val, dsi->regs + DSI_CMDQ0);
  

• mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE, 1);
  

• mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK);
  

• mtk_dsi_mask(dsi, DSI_INTSTA, LPRX_RD_RDY_INT_FLAG,
  

• 	     ~LPRX_RD_RDY_INT_FLAG);
  

• mtk_dsi_mask(dsi, DSI_INTSTA, CMD_DONE_INT_FLAG,
  

• 	     ~CMD_DONE_INT_FLAG);
  

I think interrupt status should be cleared in irq handler. So you need not to clear it here.

• mtk_dsi_mask(dsi, DSI_INTEN, DSI_INT_LPRX_RD_RDY_FLAG,
  

• 	     DSI_INT_LPRX_RD_RDY_FLAG);
  

• mtk_dsi_mask(dsi, DSI_INTEN, DSI_INT_CMD_DONE_FLAG,
  

• 	     DSI_INT_CMD_DONE_FLAG);
  

Why enable cmd_done interrupt? You just need interrupt of LPRX_RD_RDY.

• mtk_dsi_start(dsi);
  

• dev_info(dsi->dev, "Start polling DSI read ready!!!\n");
  

•  /* 2s timeout*/
  

• ret = mtk_dsi_wait_for_irq_timeout(dsi,
  

• 				   DSI_INT_LPRX_RD_RDY_FLAG,
  

• 				   2000);
  

• if (ret) {
  

• 	dev_info(dsi->dev, "Polling DSI read ready timeout!!!\n");
  

• 	mtk_dsi_enable(dsi);
  

• 	mtk_dsi_reset_engine(dsi);
  

• 	return ret;
  

• }
  

• dev_info(dsi->dev, "End polling DSI read ready!!!\n");
  

• mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK);
  

• mtk_dsi_mask(dsi, DSI_INTSTA, LPRX_RD_RDY_INT_FLAG,
  

• 	     ~LPRX_RD_RDY_INT_FLAG);
  

• recv_data0 = readl(dsi->regs + DSI_RX_DATA0);
  

• recv_data1 = readl(dsi->regs + DSI_RX_DATA1);
  

• recv_data2 = readl(dsi->regs + DSI_RX_DATA2);
  

• recv_data3 = readl(dsi->regs + DSI_RX_DATA3);
  

• read_data0 = *((struct dsi_rx_data *)(&recv_data0));
  

• read_data1 = *((struct dsi_rx_data *)(&recv_data1));
  

• read_data2 = *((struct dsi_rx_data *)(&recv_data2));
  

• read_data3 = *((struct dsi_rx_data *)(&recv_data3));
  

You only use read_data0. read_data1, read_data2, and read_data3 are just for debug. Remove the debug only modification if they are not necessary. Or move them to another patch to make this patch clear.

• ret = readl(dsi->regs + DSI_CMDQ_SIZE);
  

• dev_info(dsi->dev, "DSI_CMDQ_SIZE : 0x%x\n", ret & CMDQ_SIZE);
  

• ret = readl(dsi->regs + DSI_CMDQ0);
  

• dev_info(dsi->dev, "DSI_CMDQ_DATA0 : 0x%x\n",
  

• 	 ret & 0xff);
  

• dev_info(dsi->dev, "DSI_CMDQ_DATA1 : 0x%x\n",
  

• 	 (ret >> 8) & 0xff);
  

• dev_info(dsi->dev, "DSI_CMDQ_DATA2 : 0x%x\n",
  

• 	 (ret >> 16) & 0xff);
  

• dev_info(dsi->dev, "DSI_CMDQ_DATA3 : 0x%x\n",
  

• 	 (ret >> 24) & 0xff);
  

• dev_info(dsi->dev, "DSI_RX_DATA0: 0x%x\n", recv_data0);
  

• dev_info(dsi->dev, "DSI_RX_DATA1: 0x%x\n", recv_data1);
  

• dev_info(dsi->dev, "DSI_RX_DATA2: 0x%x\n", recv_data2);
  

• dev_info(dsi->dev, "DSI_RX_DATA3: 0x%x\n", recv_data3);
  

• dev_info(dsi->dev, "read_data0: %x,%x,%x,%x\n",
  

• 	 read_data0.byte0, read_data0.byte1, read_data0.byte2,
  

• 	 read_data0.byte3);
  

• dev_info(dsi->dev, "read_data1: %x,%x,%x,%x\n",
  

• 	 read_data1.byte0, read_data1.byte1, read_data1.byte2,
  

• 	 read_data1.byte3);
  

• dev_info(dsi->dev, "read_data2: %x,%x,%x,%x\n",
  

• 	 read_data2.byte0, read_data2.byte1, read_data2.byte2,
  

• 	 read_data2.byte3);
  

• dev_info(dsi->dev, "read_data3: %x,%x,%x,%x\n",
  

• 	 read_data3.byte0, read_data3.byte1, read_data3.byte2,
  

• 	 read_data3.byte3);
  

• packet_type = read_data0.byte0;
  

• dev_info(dsi->dev, "DSI read packet_type is 0x%x\n",
  

• 	 packet_type);
  

• if (packet_type == 0x1a || packet_type == 0x1c) {
  

• 	void *read_tmp = (void *)&recv_data1;
  

• 	recv_data_cnt = read_data0.byte1 +
  

• 			read_data0.byte2 * 16;
  

• 	if (recv_data_cnt > 10)
  

• 		recv_data_cnt = 10;
  

Why do you drop data over size of 10? Isn't it an error?

• 	if (recv_data_cnt > buffer_size)
  

• 		recv_data_cnt = buffer_size;
  

Isn't it an error while you drop data over the buffer size?

• 	memcpy(buffer, read_tmp, recv_data_cnt);
  

• } else {
  

• 	/* short  packet */
  

• 	recv_data_cnt = 2;
  

• 	if (recv_data_cnt > buffer_size)
  

• 		recv_data_cnt = buffer_size;
  

Isn't it an error while you drop data over the buffer size?

• 	memcpy(buffer, &read_data0.byte1, 2);
  

If buffer_size is 1, why do you still copy 2 bytes?

• }
  

• } while (packet_type != 0x1c && packet_type != 0x21 &&

• packet_type != 0x22 && packet_type != 0x1a);
  

Could you give these magic number a descriptive name?

• dev_info(dsi->dev, "dsi get %d byte data from the panel address(0x%x)\n",

•  recv_data_cnt, *((u8 *)(msg->tx_buf)));
  

• return recv_data_cnt;

+}

+static ssize_t mtk_dsi_host_write_cmd(struct mtk_dsi *dsi,

• 		      const struct mipi_dsi_msg *msg)
  

+{

• u32 i;
• u32 goto_addr, mask_para, set_para, reg_val;
• void __iomem *cmdq_reg;
• struct dsi_cmd_t0 t0;
• struct dsi_cmd_t2 t2;
• const char *tx_buf = msg->tx_buf;
• struct dsi_tx_cmdq_regs *dsi_cmd_reg;
• dsi_cmd_reg = (struct dsi_tx_cmdq_regs *)(dsi->regs + DSI_CMDQ0);
• mtk_dsi_wait_for_idle(dsi);
• if (msg->tx_len > 2) {

• t2.config = 2;
  

• t2.type = msg->type;
  

• t2.wc16 = msg->tx_len;
  

• reg_val = (t2.wc16 << 16) | (t2.type << 8) | t2.config;
  

Why need 'struct dsi_cmd_t2 t2'? I think code is more simple when get rid of this structure. The code looks like:

reg_val = (wc16 << 16) | (type << 8) | config;

• writel(reg_val, &dsi_cmd_reg->data[0]);
  

• goto_addr = (u32)(&dsi_cmd_reg->data[1].byte0);
  

• mask_para = (0xff << ((goto_addr & 0x3) * 8));
  

• set_para = (tx_buf[0] << ((goto_addr & 0x3) * 8));
  

• cmdq_reg = (void __iomem *)(goto_addr & (~0x3));
  

• mtk_dsi_set_cmdq(cmdq_reg, mask_para, set_para);
  

I think you can merge this setting into 'for loop' and the loop look like this:

for (i = 0; i < msg->tx_len; i++) { goto_addr = (u32)(&dsi_cmd_reg->data[1].byte0) + i; mask_para = (0xff << ((goto_addr & 0x3) * 8)); set_para = (tx_buf[i] << ((goto_addr & 0x3) * 8)); cmdq_reg = (void __iomem *)(goto_addr & (~0x3)); mtk_dsi_set_cmdq(cmdq_reg, mask_para, set_para); }

• for (i = 1; i < msg->tx_len; i++) {
  

• 	goto_addr = (u32)(&dsi_cmd_reg->data[1].byte1) + i - 1;
  

• 	mask_para = (0xff << ((goto_addr & 0x3) * 8));
  

• 	set_para = (tx_buf[i] << ((goto_addr & 0x3) * 8));
  

• 	cmdq_reg = (void __iomem *)(goto_addr & (~0x3));
  

• 	mtk_dsi_set_cmdq(cmdq_reg, mask_para, set_para);
  

• }
  

• mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE,
  

• 	     2 + (msg->tx_len - 1) / 4);
  

I think it is better to change formula of DSI_CMDQ_SIZE to '1 + (msg->tx_len + 3) / 4'. The '1' means the first cmd which describe the size of tx_buf, and the rest means the other cmds for body of tx_buf.

• } else {

• t0.config = 0;
  

• t0.data0 = tx_buf[0];
  

• if (msg->tx_len == 2) {
  

• 	t0.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
  

• 	t0.data1 = tx_buf[1];
  

• } else {
  

• 	t0.type = MIPI_DSI_DCS_SHORT_WRITE;
  

• 	t0.data1 = 0;
  

• }
  

• reg_val = (t0.data1 << 24) | (t0.data0 << 16) | (t0.type << 8) |
  

• 	   t0.config;
  

• writel(reg_val, &dsi_cmd_reg->data[0]);
  

• mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE, 1);
  

• }
• mtk_dsi_start(dsi);
• mtk_dsi_wait_for_idle(dsi);
• return 0;

+}

+static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host,

• 		     const struct mipi_dsi_msg *msg)
  

+{

• struct mtk_dsi *dsi = host_to_dsi(host);
• u8 type = msg->type;
• ssize_t ret = 0;
• if (MTK_DSI_HOST_IS_READ(type))

• ret = mtk_dsi_host_read_cmd(dsi, msg);
  

• else if (MTK_DSI_HOST_IS_WRITE(type))

• ret = mtk_dsi_host_write_cmd(dsi, msg);
  

• return ret;

+}

static const struct mipi_dsi_host_ops mtk_dsi_ops = { .attach = mtk_dsi_host_attach, .detach = mtk_dsi_host_detach,

• .transfer = mtk_dsi_host_transfer,

};

static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)

Regards, CK

Hi CK,

On Wed, 2016-07-20 at 14:27 +0800, CK Hu wrote:

Hi, YT:

Some comments inline.

On Fri, 2016-07-15 at 18:07 +0800, YT Shen wrote:

...

From: shaoming chen shaoming.chen@mediatek.com

add dsi and mipi tx driver for mipi panel support

Signed-off-by: shaoming chen shaoming.chen@mediatek.com

drivers/gpu/drm/mediatek/mtk_dsi.c | 169 ++++++++++++++++++++++---------- drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 82 +++++++++++----- 2 files changed, 173 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 1f99894..4eae63c 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -29,9 +29,6 @@

#include "mtk_drm_ddp_comp.h"

-#define DSI_VIDEO_FIFO_DEPTH (1920 / 4) -#define DSI_HOST_FIFO_DEPTH 64

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

#define DSI_START 0x00

#define DSI_INTEN 0x08 @@ -55,7 +52,7 @@ #define MIX_MODE BIT(17)

#define DSI_TXRX_CTRL 0x18 -#define VC_NUM (2 << 0) +#define VC_NUM BIT(1)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

#define LANE_NUM (0xf << 2) #define DIS_EOT BIT(6) #define NULL_EN BIT(7) @@ -94,6 +91,8 @@ #define DSI_RACK 0x84 #define RACK BIT(0)

+#define DSI_MEM_CONTI 0x90

#define DSI_PHY_LCCON 0x104 #define LC_HS_TX_EN BIT(0) #define LC_ULPM_EN BIT(1) @@ -126,6 +125,10 @@ #define CLK_HS_POST (0xff << 8) #define CLK_HS_EXIT (0xff << 16)

+#define DSI_VM_CMD_CON 0x130 +#define VM_CMD_EN BIT(0) +#define TS_VFP_EN BIT(5)

The bitwise definition should add one more tab to align other bitwise definition in this file.

Will do.

...

#define DSI_CMDQ0 0x180

#define NS_TO_CYCLE(n, c) ((n) / (c) + (((n) % (c)) ? 1 : 0)) @@ -239,11 +242,11 @@ static void mtk_dsi_mask(struct mtk_dsi *dsi, u32 offset, u32 mask, u32 data) writel((temp & ~mask) | (data & mask), dsi->regs + offset); }

-static void dsi_phy_timconfig(struct mtk_dsi *dsi) +static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ u32 timcon0, timcon1, timcon2, timcon3;

• unsigned int ui, cycle_time;
• unsigned int lpx;

• u32 ui, cycle_time;
• u32 lpx;

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

ui = 1000 / dsi->data_rate + 0x01; cycle_time = 8000 / dsi->data_rate + 0x01; @@ -273,7 +276,7 @@ static void mtk_dsi_disable(struct mtk_dsi *dsi) mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, 0); }

-static void mtk_dsi_reset(struct mtk_dsi *dsi) +static void mtk_dsi_reset_engine(struct mtk_dsi *dsi) { mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_RESET, DSI_RESET); mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_RESET, 0); @@ -293,7 +296,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) * mipi_ratio is mipi clk coefficient for balance the pixel clk in mipi. * we set mipi_ratio is 1.05. */

• dsi->data_rate = dsi->vm.pixelclock * 3 * 21 / (1 * 1000 * 10);

• dsi->data_rate = dsi->vm.pixelclock * 12 * 21;

• dsi->data_rate /= (dsi->lanes * 1000 * 10);

• dev_info(dev, "set mipitx's data rate: %dMHz\n", dsi->data_rate);

  ret = clk_set_rate(dsi->hs_clk, dsi->data_rate * 1000000); if (ret < 0) {

@@ -315,10 +320,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) goto err_disable_engine_clk; }

• mtk_dsi_enable(dsi);
• mtk_dsi_reset(dsi);
• dsi_phy_timconfig(dsi);
• return 0;

err_disable_engine_clk: @@ -330,33 +331,33 @@ err_refcount: return ret; }

-static void dsi_clk_ulp_mode_enter(struct mtk_dsi *dsi) +static void mtk_dsi_clk_ulp_mode_enter(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_HS_TX_EN, 0);

• mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, 0);

• mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, LC_ULPM_EN);

}

-static void dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi) +static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, 0); mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_WAKEUP_EN, LC_WAKEUP_EN); mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_WAKEUP_EN, 0); }

-static void dsi_lane0_ulp_mode_enter(struct mtk_dsi *dsi) +static void mtk_dsi_lane0_ulp_mode_enter(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_HS_TX_EN, 0);

• mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, 0);

• mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, LD0_ULPM_EN);

}

-static void dsi_lane0_ulp_mode_leave(struct mtk_dsi *dsi) +static void mtk_dsi_lane0_ulp_mode_leave(struct mtk_dsi *dsi) { mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, 0); mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_WAKEUP_EN, LD0_WAKEUP_EN); mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_WAKEUP_EN, 0); }

-static bool dsi_clk_hs_state(struct mtk_dsi *dsi) +static bool mtk_dsi_clk_hs_state(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ u32 tmp_reg1;

@@ -364,15 +365,15 @@ static bool dsi_clk_hs_state(struct mtk_dsi *dsi) return ((tmp_reg1 & LC_HS_TX_EN) == 1) ? true : false; }

-static void dsi_clk_hs_mode(struct mtk_dsi *dsi, bool enter) +static void mtk_dsi_clk_hs_mode(struct mtk_dsi *dsi, bool enter)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{

• if (enter && !dsi_clk_hs_state(dsi))

• if (enter && !mtk_dsi_clk_hs_state(dsi)) mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_HS_TX_EN, LC_HS_TX_EN);

• else if (!enter && dsi_clk_hs_state(dsi))

• else if (!enter && mtk_dsi_clk_hs_state(dsi)) mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_HS_TX_EN, 0);

}

-static void dsi_set_mode(struct mtk_dsi *dsi) +static void mtk_dsi_set_mode(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ u32 vid_mode = CMD_MODE;

@@ -382,12 +383,22 @@ static void dsi_set_mode(struct mtk_dsi *dsi) if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) vid_mode = BURST_MODE;

• else
  

• 	vid_mode = SYNC_EVENT_MODE;
  

  }

  writel(vid_mode, dsi->regs + DSI_MODE_CTRL);

}

-static void dsi_ps_control_vact(struct mtk_dsi *dsi) +static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi) +{

• writel(0x3c, dsi->regs + DSI_MEM_CONTI);
• mtk_dsi_mask(dsi, DSI_VM_CMD_CON, VM_CMD_EN, VM_CMD_EN);
• mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);

+}

+static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi) { struct videomode *vm = &dsi->vm; u32 dsi_buf_bpp, ps_wc; @@ -421,7 +432,7 @@ static void dsi_ps_control_vact(struct mtk_dsi *dsi) writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC); }

-static void dsi_rxtx_control(struct mtk_dsi *dsi) +static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{ u32 tmp_reg;

@@ -443,12 +454,15 @@ static void dsi_rxtx_control(struct mtk_dsi *dsi) break; }

• tmp_reg |= (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) << 6;
• tmp_reg |= (dsi->mode_flags & MIPI_DSI_MODE_EOT_PACKET) >> 3;
• writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);

}

-static void dsi_ps_control(struct mtk_dsi *dsi) +static void mtk_dsi_ps_control(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{

• unsigned int dsi_tmp_buf_bpp;

• u32 dsi_tmp_buf_bpp; u32 tmp_reg;

  switch (dsi->format) {

@@ -478,12 +492,12 @@ static void dsi_ps_control(struct mtk_dsi *dsi) writel(tmp_reg, dsi->regs + DSI_PSCTRL); }

-static void dsi_config_vdo_timing(struct mtk_dsi *dsi) +static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

{

• unsigned int horizontal_sync_active_byte;
• unsigned int horizontal_backporch_byte;
• unsigned int horizontal_frontporch_byte;
• unsigned int dsi_tmp_buf_bpp;

• u32 horizontal_sync_active_byte;
• u32 horizontal_backporch_byte;
• u32 horizontal_frontporch_byte;
• u32 dsi_tmp_buf_bpp;

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

struct videomode *vm = &dsi->vm;

@@ -512,7 +526,7 @@ static void dsi_config_vdo_timing(struct mtk_dsi *dsi) writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC); writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);

• dsi_ps_control(dsi);

• mtk_dsi_ps_control(dsi);

}

static void mtk_dsi_start(struct mtk_dsi *dsi) @@ -521,6 +535,19 @@ static void mtk_dsi_start(struct mtk_dsi *dsi) writel(1, dsi->regs + DSI_START); }

+static void mtk_dsi_stop(struct mtk_dsi *dsi) +{

• writel(0, dsi->regs + DSI_START);

+}

+static void mtk_dsi_set_cmd_mode(struct mtk_dsi *dsi) +{

• u32 tmp_reg1;
• tmp_reg1 = CMD_MODE;
• writel(tmp_reg1, dsi->regs + DSI_MODE_CTRL);

+}

static void mtk_dsi_set_interrupt_enable(struct mtk_dsi *dsi) { u32 inten = DSI_INT_ALL_BITS; @@ -609,6 +636,21 @@ static s32 mtk_dsi_wait_for_irq_timeout(struct mtk_dsi *dsi, u32 irq_bit, return -1; }

+static void mtk_dsi_switch_to_cmd_mode(struct mtk_dsi *dsi) +{

• s32 ret = 0;
• mtk_dsi_set_cmd_mode(dsi);
• ret = mtk_dsi_wait_for_irq_timeout(dsi, DSI_INT_VM_DONE_FLAG, 500);
• if (ret != 0) {

• dev_info(dsi->dev, "dsi wait engine idle timeout\n");
  

• mtk_dsi_enable(dsi);
  

• mtk_dsi_reset_engine(dsi);
  

• }

+}

static void mtk_dsi_poweroff(struct mtk_dsi *dsi) { if (WARN_ON(dsi->refcount == 0)) @@ -617,8 +659,19 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi) if (--dsi->refcount != 0) return;

• dsi_lane0_ulp_mode_enter(dsi);
• dsi_clk_ulp_mode_enter(dsi);

• mtk_dsi_switch_to_cmd_mode(dsi);

• if (dsi->panel) {

• if (drm_panel_unprepare(dsi->panel)) {
  

• 	DRM_ERROR("failed to unprepare the panel\n");
  

• 	return;
  

• }
  

• }

• mtk_dsi_reset_engine(dsi);

• mtk_dsi_lane0_ulp_mode_enter(dsi);

• mtk_dsi_clk_ulp_mode_enter(dsi);

  mtk_dsi_disable(dsi);

@@ -635,32 +688,40 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi) if (dsi->enabled) return;

• if (dsi->panel) {

• if (drm_panel_prepare(dsi->panel)) {
  

• 	DRM_ERROR("failed to setup the panel\n");
  

• 	return;
  

• }
  

• }

• ret = mtk_dsi_poweron(dsi); if (ret < 0) { DRM_ERROR("failed to power on dsi\n"); return; }

• dsi_rxtx_control(dsi);

• usleep_range(20000, 21000);

• dsi_clk_ulp_mode_leave(dsi);
• dsi_lane0_ulp_mode_leave(dsi);
• dsi_clk_hs_mode(dsi, 0);
• dsi_set_mode(dsi);
• dsi_ps_control_vact(dsi);
• dsi_config_vdo_timing(dsi);

• mtk_dsi_rxtx_control(dsi);
• mtk_dsi_phy_timconfig(dsi);
• mtk_dsi_ps_control_vact(dsi);
• mtk_dsi_set_vm_cmd(dsi);
• mtk_dsi_config_vdo_timing(dsi); mtk_dsi_set_interrupt_enable(dsi);

• dsi_set_mode(dsi);
• dsi_clk_hs_mode(dsi, 1);

• mtk_dsi_enable(dsi);

• mtk_dsi_clk_ulp_mode_leave(dsi);

• mtk_dsi_lane0_ulp_mode_leave(dsi);

• mtk_dsi_clk_hs_mode(dsi, 0);

• if (dsi->panel) {

• if (drm_panel_prepare(dsi->panel)) {
  

• 	DRM_ERROR("failed to prepare the panel\n");
  

• 	return;
  

• }
  

• if (drm_panel_enable(dsi->panel)) {
  

• 	DRM_ERROR("failed to enable the panel\n");
  

• 	return;
  

• }
  

• }

• mtk_dsi_set_mode(dsi);

• mtk_dsi_clk_hs_mode(dsi, 1);

  mtk_dsi_start(dsi);

@@ -679,6 +740,7 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi) } }

• mtk_dsi_stop(dsi); mtk_dsi_poweroff(dsi);

  dsi->enabled = false;

@@ -1351,6 +1413,7 @@ static int mtk_dsi_remove(struct platform_device *pdev) }

static const struct of_device_id mtk_dsi_of_match[] = {

• { .compatible = "mediatek,mt2701-dsi" },

This is not related to 'mipi panel support'. So move this modification to another patch.

We will split common part to another patch.

...

{ .compatible = "mediatek,mt8173-dsi" }, { }, }; diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c index cf8f38d..3bffc40 100644 --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c @@ -16,6 +16,7 @@ #include <linux/delay.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/phy/phy.h>

@@ -87,6 +88,9 @@

#define MIPITX_DSI_PLL_CON2 0x58

+#define MIPITX_DSI_PLL_TOP 0x64 +#define RG_DSI_MPPLL_PRESERVE (0xff << 8)

#define MIPITX_DSI_PLL_PWR 0x68 #define RG_DSI_MPPLL_SDM_PWR_ON BIT(0) #define RG_DSI_MPPLL_SDM_ISO_EN BIT(1) @@ -123,10 +127,32 @@ #define SW_LNT2_HSTX_PRE_OE BIT(24) #define SW_LNT2_HSTX_OE BIT(25)

+struct mtk_mipitx_data {

• const u32 data;

+};

+static const struct mtk_mipitx_data mt2701_mipitx_data = {

• .data = (3 << 8)

+};

+static const struct mtk_mipitx_data mt8173_mipitx_data = {

• .data = (0 << 8)

+};

+static const struct of_device_id mtk_mipi_tx_match[] = {

• { .compatible = "mediatek,mt2701-mipi-tx",

•  .data = &mt2701_mipitx_data },
  

• { .compatible = "mediatek,mt8173-mipi-tx",

•  .data = &mt8173_mipitx_data },
  

• {},

+};

This looks more like 'mt2701 support' rather than 'mipi panel support'. So move this modification to another patch.

We will split common part to another patch.

...

struct mtk_mipi_tx { struct device *dev; void __iomem *regs;

• unsigned int data_rate;

• u32 data_rate;
• const struct mtk_mipitx_data *driver_data;
• struct clk_hw pll_hw; struct clk *pll;

}; @@ -163,12 +189,14 @@ static void mtk_mipi_tx_update_bits(struct mtk_mipi_tx *mipi_tx, u32 offset, static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw) { struct mtk_mipi_tx *mipi_tx = mtk_mipi_tx_from_clk_hw(hw);

• unsigned int txdiv, txdiv0, txdiv1;

• u8 txdiv, txdiv0, txdiv1; u64 pcw;

  dev_dbg(mipi_tx->dev, "prepare: %u Hz\n", mipi_tx->data_rate);

• if (mipi_tx->data_rate >= 500000000) {

• if (mipi_tx->data_rate > 1250000000) {

• return -EINVAL;
  

• } else if (mipi_tx->data_rate >= 500000000) { txdiv = 1; txdiv0 = 0; txdiv1 = 0;

@@ -192,6 +220,10 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw) return -EINVAL; }

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_TOP_CON,

• 		RG_DSI_LNT_IMP_CAL_CODE | RG_DSI_LNT_HS_BIAS_EN,
  

• 		(8 << 4) | RG_DSI_LNT_HS_BIAS_EN);
  

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_BG_CON, RG_DSI_VOUT_MSK | RG_DSI_BG_CKEN | RG_DSI_BG_CORE_EN,

@@ -201,24 +233,18 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)

usleep_range(30, 100);

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_TOP_CON,

• 		RG_DSI_LNT_IMP_CAL_CODE | RG_DSI_LNT_HS_BIAS_EN,
  

• 		(8 << 4) | RG_DSI_LNT_HS_BIAS_EN);
  

• mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_CON,

• 	     RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN);
  

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_CON,

• 		RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN,
  

• 		RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN);
  
  

  mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_PWR, RG_DSI_MPPLL_SDM_PWR_ON | RG_DSI_MPPLL_SDM_ISO_EN, RG_DSI_MPPLL_SDM_PWR_ON);

• mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON0,

• 	       RG_DSI_MPPLL_PLL_EN);
  

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON0,

• 		RG_DSI_MPPLL_TXDIV0 | RG_DSI_MPPLL_TXDIV1 |
  

• 		RG_DSI_MPPLL_PREDIV,
  

• 		RG_DSI_MPPLL_PREDIV | RG_DSI_MPPLL_TXDIV0 |
  

• 		RG_DSI_MPPLL_TXDIV1 | RG_DSI_MPPLL_POSDIV,
  	(txdiv0 << 3) | (txdiv1 << 5));
  
  

  /*

@@ -233,15 +259,21 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw) 26000000); writel(pcw, mipi_tx->regs + MIPITX_DSI_PLL_CON2);

• mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_PLL_CON1,

• 	     RG_DSI_MPPLL_SDM_FRA_EN);
  

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON1,

• 		RG_DSI_MPPLL_SDM_FRA_EN,
  

• 		RG_DSI_MPPLL_SDM_FRA_EN);
  
  

• mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_PLL_CON0, RG_DSI_MPPLL_PLL_EN);

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON0,

• 		RG_DSI_MPPLL_PLL_EN, RG_DSI_MPPLL_PLL_EN);
  
  

  usleep_range(20, 100);

  mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON1,

• 	       RG_DSI_MPPLL_SDM_SSC_EN);
  

• 			RG_DSI_MPPLL_SDM_SSC_EN);
  

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_TOP,

• 		RG_DSI_MPPLL_PRESERVE,
  

• 		mipi_tx->driver_data->data);
  
  

  return 0;

} @@ -255,6 +287,10 @@ static void mtk_mipi_tx_pll_unprepare(struct clk_hw *hw) mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON0, RG_DSI_MPPLL_PLL_EN);

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_TOP,

• 		RG_DSI_MPPLL_PRESERVE,
  

• 		mipi_tx->driver_data->data);
  

• mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_PWR, RG_DSI_MPPLL_SDM_ISO_EN | RG_DSI_MPPLL_SDM_PWR_ON,

@@ -310,7 +346,7 @@ static const struct clk_ops mtk_mipi_tx_pll_ops = { static int mtk_mipi_tx_power_on_signal(struct phy *phy) { struct mtk_mipi_tx *mipi_tx = phy_get_drvdata(phy);

• unsigned int reg;

• u32 reg;

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

for (reg = MIPITX_DSI_CLOCK_LANE; reg <= MIPITX_DSI_DATA_LANE3; reg += 4) @@ -341,7 +377,7 @@ static int mtk_mipi_tx_power_on(struct phy *phy) static void mtk_mipi_tx_power_off_signal(struct phy *phy) { struct mtk_mipi_tx *mipi_tx = phy_get_drvdata(phy);

• unsigned int reg;

• u32 reg;

I think this should be moved to a 'cleaning up and refine' patch.

OK.

...

mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_TOP_CON, RG_DSI_PAD_TIE_LOW_EN); @@ -391,6 +427,7 @@ static int mtk_mipi_tx_probe(struct platform_device *pdev) if (!mipi_tx) return -ENOMEM;

• mipi_tx->driver_data = of_device_get_match_data(dev); mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); mipi_tx->regs = devm_ioremap_resource(dev, mem); if (IS_ERR(mipi_tx->regs)) {

@@ -448,11 +485,6 @@ static int mtk_mipi_tx_remove(struct platform_device *pdev) return 0; }

-static const struct of_device_id mtk_mipi_tx_match[] = {

• { .compatible = "mediatek,mt8173-mipi-tx", },
• {},

-};

struct platform_driver mtk_mipi_tx_driver = { .probe = mtk_mipi_tx_probe, .remove = mtk_mipi_tx_remove,

Regards, CK