On Sun, Jan 29, 2017 at 01:24:21PM +0000, John Keeping wrote:

With atomic modesetting the hardware will be powered off when the mode_set function is called. We should configure the hardware in the enable function, which is the atomic version of "commit" so let's use the enable hook rather than commit while we're at it.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com

v3:

• Squash together with the commit to s/commit/enable/

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 49 +++++++++++++++------------------- 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index d9aa382bb629..bbd992299f73 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -819,34 +819,8 @@ static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode) { struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);

• int ret;

  dsi->mode = adjusted_mode;

• ret = dw_mipi_dsi_get_lane_bps(dsi);

• if (ret < 0)

• return;
  

• if (clk_prepare_enable(dsi->pclk)) {

• dev_err(dsi->dev, "%s: Failed to enable pclk\n", __func__);
  

• return;
  

• }

• dw_mipi_dsi_init(dsi);

• dw_mipi_dsi_dpi_config(dsi, mode);

• dw_mipi_dsi_packet_handler_config(dsi);

• dw_mipi_dsi_video_mode_config(dsi);

• dw_mipi_dsi_video_packet_config(dsi, mode);

• dw_mipi_dsi_command_mode_config(dsi);

• dw_mipi_dsi_line_timer_config(dsi);

• dw_mipi_dsi_vertical_timing_config(dsi);

• dw_mipi_dsi_dphy_timing_config(dsi);

• dw_mipi_dsi_dphy_interface_config(dsi);

• dw_mipi_dsi_clear_err(dsi);

• if (drm_panel_prepare(dsi->panel))

• dev_err(dsi->dev, "failed to prepare panel\n");
  

• clk_disable_unprepare(dsi->pclk);

}

static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) @@ -875,17 +849,36 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) clk_disable_unprepare(dsi->pclk); }

-static void dw_mipi_dsi_encoder_commit(struct drm_encoder *encoder) +static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) { struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder); int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder); u32 val;

• int ret;

• ret = dw_mipi_dsi_get_lane_bps(dsi);

• if (ret < 0)

• return;
  
  

  if (clk_prepare_enable(dsi->pclk)) { dev_err(dsi->dev, "%s: Failed to enable pclk\n", __func__); return; }

• dw_mipi_dsi_init(dsi);

• dw_mipi_dsi_dpi_config(dsi, dsi->mode);

• dw_mipi_dsi_packet_handler_config(dsi);

• dw_mipi_dsi_video_mode_config(dsi);

• dw_mipi_dsi_video_packet_config(dsi, dsi->mode);

• dw_mipi_dsi_command_mode_config(dsi);

• dw_mipi_dsi_line_timer_config(dsi);

• dw_mipi_dsi_vertical_timing_config(dsi);

• dw_mipi_dsi_dphy_timing_config(dsi);

• dw_mipi_dsi_dphy_interface_config(dsi);

• dw_mipi_dsi_clear_err(dsi);

• if (drm_panel_prepare(dsi->panel))

• dev_err(dsi->dev, "failed to prepare panel\n");
  

• dw_mipi_dsi_phy_init(dsi); dw_mipi_dsi_wait_for_two_frames(dsi);

@@ -933,7 +926,7 @@ dw_mipi_dsi_encoder_atomic_check(struct drm_encoder *encoder,

static struct drm_encoder_helper_funcs dw_mipi_dsi_encoder_helper_funcs = {

• .commit = dw_mipi_dsi_encoder_commit,

• .enable = dw_mipi_dsi_encoder_enable, .mode_set = dw_mipi_dsi_encoder_mode_set, .disable = dw_mipi_dsi_encoder_disable, .atomic_check = dw_mipi_dsi_encoder_atomic_check,

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:22PM +0000, John Keeping wrote:

This shows that we only use the mode from the enable function and prepares us to remove the "mode" field and the mode_set hook in the next commit.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

New in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 41 ++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index bbd992299f73..cdbd25087e83 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -330,11 +330,11 @@ static int max_mbps_to_testdin(unsigned int max_mbps)

• The controller should generate 2 frames before
• preparing the peripheral.

*/ -static void dw_mipi_dsi_wait_for_two_frames(struct dw_mipi_dsi *dsi) +static void dw_mipi_dsi_wait_for_two_frames(struct drm_display_mode *mode) { int refresh, two_frames;

• refresh = drm_mode_vrefresh(dsi->mode);

• refresh = drm_mode_vrefresh(mode); two_frames = DIV_ROUND_UP(MSEC_PER_SEC, refresh) * 2; msleep(two_frames);

} @@ -459,7 +459,8 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) return ret; }

-static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi) +static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,

• 		    struct drm_display_mode *mode)
  

{ unsigned int i, pre; unsigned long mpclk, pllref, tmp; @@ -474,7 +475,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi) return bpp; }

• mpclk = DIV_ROUND_UP(dsi->mode->clock, MSEC_PER_SEC);

• mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC); if (mpclk) { /* take 1 / 0.9, since mbps must big than bandwidth of RGB */ tmp = mpclk * (bpp / dsi->lanes) * 10 / 9;

@@ -742,43 +743,44 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)

/* Get lane byte clock cycles. */ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,

• 			   struct drm_display_mode *mode,
  		   u32 hcomponent)
  

{ u32 frac, lbcc;

lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;

• frac = lbcc % dsi->mode->clock;
• lbcc = lbcc / dsi->mode->clock;

• frac = lbcc % mode->clock;

• lbcc = lbcc / mode->clock; if (frac) lbcc++;

  return lbcc;

}

-static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi) +static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi,

• 			  struct drm_display_mode *mode)
  

{ u32 htotal, hsa, hbp, lbcc;

• struct drm_display_mode *mode = dsi->mode;

  htotal = mode->htotal; hsa = mode->hsync_end - mode->hsync_start; hbp = mode->htotal - mode->hsync_end;

• lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, htotal);

• lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, htotal); dsi_write(dsi, DSI_VID_HLINE_TIME, lbcc);

• lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, hsa);

• lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hsa); dsi_write(dsi, DSI_VID_HSA_TIME, lbcc);

• lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, hbp);

• lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hbp); dsi_write(dsi, DSI_VID_HBP_TIME, lbcc);

}

-static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi) +static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi,

• 			       struct drm_display_mode *mode)
  

{ u32 vactive, vsa, vfp, vbp;

• struct drm_display_mode *mode = dsi->mode;

  vactive = mode->vdisplay; vsa = mode->vsync_end - mode->vsync_start;

@@ -852,11 +854,12 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) { struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);

• struct drm_display_mode *mode = dsi->mode; int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder); u32 val; int ret;

• ret = dw_mipi_dsi_get_lane_bps(dsi);

• ret = dw_mipi_dsi_get_lane_bps(dsi, mode); if (ret < 0) return;

@@ -866,13 +869,13 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) }

dw_mipi_dsi_init(dsi);

• dw_mipi_dsi_dpi_config(dsi, dsi->mode);

• dw_mipi_dsi_dpi_config(dsi, mode); dw_mipi_dsi_packet_handler_config(dsi); dw_mipi_dsi_video_mode_config(dsi);

• dw_mipi_dsi_video_packet_config(dsi, dsi->mode);

• dw_mipi_dsi_video_packet_config(dsi, mode); dw_mipi_dsi_command_mode_config(dsi);

• dw_mipi_dsi_line_timer_config(dsi);
• dw_mipi_dsi_vertical_timing_config(dsi);

• dw_mipi_dsi_line_timer_config(dsi, mode);
• dw_mipi_dsi_vertical_timing_config(dsi, mode); dw_mipi_dsi_dphy_timing_config(dsi); dw_mipi_dsi_dphy_interface_config(dsi); dw_mipi_dsi_clear_err(dsi);

@@ -880,7 +883,7 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) dev_err(dsi->dev, "failed to prepare panel\n");

dw_mipi_dsi_phy_init(dsi);

• dw_mipi_dsi_wait_for_two_frames(dsi);

• dw_mipi_dsi_wait_for_two_frames(mode);

  dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_VID_MODE); drm_panel_enable(dsi->panel);

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:23PM +0000, John Keeping wrote:

This is not needed since we can access the mode via the CRTC from the enable hook. Also remove the "mode" field that is no longer used.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

New in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index cdbd25087e83..bd92e58b64f3 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -286,7 +286,6 @@ struct dw_mipi_dsi { u32 format; u16 input_div; u16 feedback_div;

• struct drm_display_mode *mode;

  const struct dw_mipi_dsi_plat_data *pdata;

}; @@ -816,15 +815,6 @@ static void dw_mipi_dsi_clear_err(struct dw_mipi_dsi *dsi) dsi_write(dsi, DSI_INT_MSK1, 0); }

-static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder,

• 			struct drm_display_mode *mode,
  

• 			struct drm_display_mode *adjusted_mode)
  

-{

• struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
• dsi->mode = adjusted_mode;

-}

static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) { struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder); @@ -854,7 +844,7 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) { struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);

• struct drm_display_mode *mode = dsi->mode;

• struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder); u32 val; int ret;

@@ -930,7 +920,6 @@ dw_mipi_dsi_encoder_atomic_check(struct drm_encoder *encoder, static struct drm_encoder_helper_funcs dw_mipi_dsi_encoder_helper_funcs = { .enable = dw_mipi_dsi_encoder_enable,

• .mode_set = dw_mipi_dsi_encoder_mode_set, .disable = dw_mipi_dsi_encoder_disable, .atomic_check = dw_mipi_dsi_encoder_atomic_check,

};

2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:24PM +0000, John Keeping wrote:

In a couple of places here we use "val" for the value that is about to be written to a register but then reuse the same variable for the value of a status register before we get around to writing it. Rename the value to be written to so that we write the value we intend to and not what we have just read from the status register.

Oh my.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Tested-by: Chris Zhong zyw@rock-chips.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

Unchanged in v3 Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index bd92e58b64f3..4cbbbcb619b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -542,9 +542,10 @@ static int dw_mipi_dsi_host_detach(struct mipi_dsi_host *host, return 0; }

-static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 val) +static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) { int ret;

• u32 val;

  ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS, val, !(val & GEN_CMD_FULL), 1000,

@@ -554,7 +555,7 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 val) return ret; }

• dsi_write(dsi, DSI_GEN_HDR, val);

• dsi_write(dsi, DSI_GEN_HDR, hdr_val);

  ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS, val, val & (GEN_CMD_EMPTY | GEN_PLD_W_EMPTY),

@@ -587,8 +588,9 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, { const u32 *tx_buf = msg->tx_buf; int len = msg->tx_len, pld_data_bytes = sizeof(*tx_buf), ret;

• u32 val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);

• u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type); u32 remainder = 0;

• u32 val;

  if (msg->tx_len < 3) { dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",

@@ -617,7 +619,7 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, } }

• return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);

• return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);

}

static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,

2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:25PM +0000, John Keeping wrote:

We want to check that both the GEN_CMD_EMPTY and GEN_PLD_W_EMPTY bits are set so we can't just check "val & mask" because that will be true if either bit is set.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 4cbbbcb619b7..4be1ff3a42bb 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -545,7 +545,7 @@ static int dw_mipi_dsi_host_detach(struct mipi_dsi_host *host, static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) { int ret;

• u32 val;

• u32 val, mask;

  ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS, val, !(val & GEN_CMD_FULL), 1000,

@@ -557,8 +557,9 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)

dsi_write(dsi, DSI_GEN_HDR, hdr_val);

• mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY; ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS,

• 		 val, val & (GEN_CMD_EMPTY | GEN_PLD_W_EMPTY),
  

• 		 val, (val & mask) == mask,
  	 1000, CMD_PKT_STATUS_TIMEOUT_US);
  

  if (ret < 0) { dev_err(dsi->dev, "failed to write command FIFO\n");

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:26PM +0000, John Keeping wrote:

As a side-effect of this, encode the endianness explicitly rather than casting a u16.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 4be1ff3a42bb..2e6ad4591ebf 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) {

• const u16 *tx_buf = msg->tx_buf;
• u32 val = GEN_HDATA(*tx_buf) | GEN_HTYPE(msg->type);

• const u8 *tx_buf = msg->tx_buf;
• u32 val = GEN_HTYPE(msg->type);
• if (msg->tx_len > 0)

• val |= GEN_HDATA(tx_buf[0]);
  

• if (msg->tx_len > 1)

• val |= GEN_HDATA(tx_buf[1] << 8);
  

You should probably update the mask inside GEN_HDATA to mask off 8 bits instead of 16.

Sean

if (msg->tx_len > 2) { dev_err(dsi->dev, "too long tx buf length %zu for short write\n", -- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Jan 30, 2017 at 06:16:36PM +0000, John Keeping wrote:

On Mon, 30 Jan 2017 13:01:46 -0500, Sean Paul wrote:

...

On Sun, Jan 29, 2017 at 01:24:26PM +0000, John Keeping wrote:

...

As a side-effect of this, encode the endianness explicitly rather than casting a u16.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 4be1ff3a42bb..2e6ad4591ebf 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) {

• const u16 *tx_buf = msg->tx_buf;
• u32 val = GEN_HDATA(*tx_buf) | GEN_HTYPE(msg->type);

• const u8 *tx_buf = msg->tx_buf;
• u32 val = GEN_HTYPE(msg->type);
• if (msg->tx_len > 0)

• val |= GEN_HDATA(tx_buf[0]);
  

• if (msg->tx_len > 1)

• val |= GEN_HDATA(tx_buf[1] << 8);  
  

You should probably update the mask inside GEN_HDATA to mask off 8 bits instead of 16.

Won't that mask off the data written by "tx_buf[1] << 8"?

I would move the shift outside the mask, ie:

val |= GEN_HDATA(tx_buf[1]) << 8;

Sean

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Jan 31, 2017 at 11:45:48AM +0000, John Keeping wrote:

On Mon, 30 Jan 2017 15:09:55 -0500, Sean Paul wrote:

...

On Mon, Jan 30, 2017 at 06:16:36PM +0000, John Keeping wrote:

...

On Mon, 30 Jan 2017 13:01:46 -0500, Sean Paul wrote:

...

On Sun, Jan 29, 2017 at 01:24:26PM +0000, John Keeping wrote:

...

As a side-effect of this, encode the endianness explicitly rather than casting a u16.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 4be1ff3a42bb..2e6ad4591ebf 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) {

• const u16 *tx_buf = msg->tx_buf;
• u32 val = GEN_HDATA(*tx_buf) | GEN_HTYPE(msg->type);

• const u8 *tx_buf = msg->tx_buf;
• u32 val = GEN_HTYPE(msg->type);
• if (msg->tx_len > 0)

• val |= GEN_HDATA(tx_buf[0]);
  

• if (msg->tx_len > 1)

• val |= GEN_HDATA(tx_buf[1] << 8);    
  

You should probably update the mask inside GEN_HDATA to mask off 8 bits instead of 16.

Won't that mask off the data written by "tx_buf[1] << 8"?

I would move the shift outside the mask, ie:

val |= GEN_HDATA(tx_buf[1]) << 8;

I can do that, but that doesn't seem to match the intention of the macros which are about encoding the placement and size of fields within registers.

Maybe it would be clearer to do:

u16 data = 0;

if (msg->tx_len > 0)
    data |= tx_buf[0];
if (msg->tx_len > 1)
    data |= tx_buf[1];

Yep, this looks good to me, but I think you need:

data |= tx_buf[1] << 8;

Sean

val = GEN_HDATA(data) | GEN_HTYPE(msg->type);

? _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:27PM +0000, John Keeping wrote:

As an aid to debugging.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 2e6ad4591ebf..92dbc3e56603 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -644,7 +644,8 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, ret = dw_mipi_dsi_dcs_long_write(dsi, msg); break; default:

• dev_err(dsi->dev, "unsupported message type\n");
  

• dev_err(dsi->dev, "unsupported message type 0x%02x\n",
  

• 	msg->type);
  

  ret = -EINVAL; }

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:28PM +0000, John Keeping wrote:

Instead of always sending commands in LP mode, respect the MIPI_DSI_MSG_USE_LPM flag to decide how to send each message. Also request acks if MIPI_DSI_MSG_REQ_ACK is set.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 92dbc3e56603..15d33c3c8cb7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -542,6 +542,19 @@ static int dw_mipi_dsi_host_detach(struct mipi_dsi_host *host, return 0; }

+static void dw_mipi_message_config(struct dw_mipi_dsi *dsi,

• 		   const struct mipi_dsi_msg *msg)
  

+{

• u32 val = 0;
• if (msg->flags & MIPI_DSI_MSG_REQ_ACK)

• val |= EN_ACK_RQST;
  

• if (msg->flags & MIPI_DSI_MSG_USE_LPM)

• val |= CMD_MODE_ALL_LP;
  

• dsi_write(dsi, DSI_CMD_MODE_CFG, val);

+}

static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) { int ret; @@ -634,6 +647,8 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, struct dw_mipi_dsi *dsi = host_to_dsi(host); int ret;

• dw_mipi_message_config(dsi, msg);
• switch (msg->type) { case MIPI_DSI_DCS_SHORT_WRITE: case MIPI_DSI_DCS_SHORT_WRITE_PARAM:

@@ -745,7 +760,6 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi) { dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000)); dsi_write(dsi, DSI_BTA_TO_CNT, 0xd00);

• dsi_write(dsi, DSI_CMD_MODE_CFG, CMD_MODE_ALL_LP); dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);

}

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:29PM +0000, John Keeping wrote:

Requesting the HS clock from the PHY before we initialize it causes an invalid signal to be sent out since the input clock is not yet configured. The PHY databook suggests only asserting this signal when performing HS transfers, so let's do that.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 15d33c3c8cb7..03fc096fe1bd 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -545,13 +545,15 @@ static int dw_mipi_dsi_host_detach(struct mipi_dsi_host *host, static void dw_mipi_message_config(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) {

• bool lpm = msg->flags & MIPI_DSI_MSG_USE_LPM; u32 val = 0;

  if (msg->flags & MIPI_DSI_MSG_REQ_ACK) val |= EN_ACK_RQST;

• if (msg->flags & MIPI_DSI_MSG_USE_LPM)

• if (lpm) val |= CMD_MODE_ALL_LP;

• dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS); dsi_write(dsi, DSI_CMD_MODE_CFG, val);

}

@@ -693,6 +695,7 @@ static void dw_mipi_dsi_set_mode(struct dw_mipi_dsi *dsi, dsi_write(dsi, DSI_PWR_UP, RESET); dsi_write(dsi, DSI_MODE_CFG, ENABLE_VIDEO_MODE); dw_mipi_dsi_video_mode_config(dsi);

• dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
  

  dsi_write(dsi, DSI_PWR_UP, POWERUP); }

} @@ -710,7 +713,6 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) | PHY_RSTZ | PHY_SHUTDOWNZ); dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVIDSION(10) | TX_ESC_CLK_DIVIDSION(7));

• dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);

}

static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,

2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:30PM +0000, John Keeping wrote:

By dereferencing the MIPI command buffer as a u32* we rely on it being correctly aligned on ARM, but this may not be the case. Copy it into a stack variable that will be correctly aligned.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 03fc096fe1bd..ddbc037e7ced 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -607,10 +607,10 @@ static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi, static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) {

• const u32 *tx_buf = msg->tx_buf;
• int len = msg->tx_len, pld_data_bytes = sizeof(*tx_buf), ret;

• const u8 *tx_buf = msg->tx_buf;
• int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret; u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);

• u32 remainder = 0;

• u32 remainder; u32 val;

  if (msg->tx_len < 3) {

@@ -621,12 +621,14 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,

while (DIV_ROUND_UP(len, pld_data_bytes)) { if (len < pld_data_bytes) {

• 	remainder = 0;
  memcpy(&remainder, tx_buf, len);
  dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
  len = 0;
  

  } else {

• 	dsi_write(dsi, DSI_GEN_PLD_DATA, *tx_buf);
  

• 	tx_buf++;
  

• 	memcpy(&remainder, tx_buf, pld_data_bytes);
  

• 	dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
  

• 	tx_buf += pld_data_bytes;
  len -= pld_data_bytes;
  

  }

You can clean this up further by removing a couple of the locals, the conditional and the division:

while(len < msg->tx_len) { size_t to_write = MIN(msg->tx_len - len, sizeof(remainder));

memcpy(&remainder, msg->tx_buf + len, to_write); dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); len += to_write;

... }

Sean

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Jan 31, 2017 at 11:56:18AM +0000, John Keeping wrote:

On Mon, 30 Jan 2017 15:08:11 -0500, Sean Paul wrote:

...

On Sun, Jan 29, 2017 at 01:24:30PM +0000, John Keeping wrote:

...

By dereferencing the MIPI command buffer as a u32* we rely on it being correctly aligned on ARM, but this may not be the case. Copy it into a stack variable that will be correctly aligned.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 03fc096fe1bd..ddbc037e7ced 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -607,10 +607,10 @@ static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi, static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) {

• const u32 *tx_buf = msg->tx_buf;
• int len = msg->tx_len, pld_data_bytes = sizeof(*tx_buf), ret;

• const u8 *tx_buf = msg->tx_buf;
• int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret; u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);

• u32 remainder = 0;

• u32 remainder; u32 val;

  if (msg->tx_len < 3) {

@@ -621,12 +621,14 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,

while (DIV_ROUND_UP(len, pld_data_bytes)) { if (len < pld_data_bytes) {

• 	remainder = 0;
  memcpy(&remainder, tx_buf, len);
  dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
  len = 0;
  

  } else {

• 	dsi_write(dsi, DSI_GEN_PLD_DATA, *tx_buf);
  

• 	tx_buf++;
  

• 	memcpy(&remainder, tx_buf, pld_data_bytes);
  

• 	dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
  

• 	tx_buf += pld_data_bytes;
  len -= pld_data_bytes;
  

  }

You can clean this up further by removing a couple of the locals, the conditional and the division:

while(len < msg->tx_len) { size_t to_write = MIN(msg->tx_len - len, sizeof(remainder));

    memcpy(&remainder, msg->tx_buf + len, to_write);
    dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
    len += to_write;

    ...

}

That's a nice simplification, but it's more than I was trying to do with this patch. I can add a cleanup patch on top to simplify the function, but I don't have that much time to spend on this at the moment and I'd rather not block this fix because the original code structure could be improved.

Ok. I'm inclined to argue that writing your response probably took more time than the copy/paste to fix it, but *shrug*.

Sean

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:31PM +0000, John Keeping wrote:

Some panels need to be configured with commands sent over the MIPI link, which they will do in the prepare hook. Call this after the PHY has been initialized so that we are able to send commands to the panel.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index ddbc037e7ced..7ada6d8ed143 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -896,12 +896,14 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) dw_mipi_dsi_dphy_timing_config(dsi); dw_mipi_dsi_dphy_interface_config(dsi); dw_mipi_dsi_clear_err(dsi);

• if (drm_panel_prepare(dsi->panel))

• dev_err(dsi->dev, "failed to prepare panel\n");
  
  

  dw_mipi_dsi_phy_init(dsi); dw_mipi_dsi_wait_for_two_frames(mode);

• dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_CMD_MODE);
• if (drm_panel_prepare(dsi->panel))

• dev_err(dsi->dev, "failed to prepare panel\n");
  

• dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_VID_MODE); drm_panel_enable(dsi->panel);

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:32PM +0000, John Keeping wrote:

Panel drivers may want to sent commands during the disable function, for example MIPI_DCS_SET_DISPLAY_OFF before the video signal ends. In order to send commands we need to write to registers, so pclk must be enabled.

While changing this, remove the unnecessary code after the panel unprepare call which seems to be a workaround for a specific panel and thus belongs in the panel driver.

Do you know which panel? If the panel driver is upstream, we should make sure we migrate this hack before removing it here. If it's downstream somewhere,

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 7ada6d8ed143..290282e86d16 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -846,24 +846,16 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder) { struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);

• drm_panel_disable(dsi->panel);
• if (clk_prepare_enable(dsi->pclk)) { dev_err(dsi->dev, "%s: Failed to enable pclk\n", __func__); return; }

• drm_panel_disable(dsi->panel);
• dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_CMD_MODE); drm_panel_unprepare(dsi->panel);

• dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_VID_MODE);

• /*

• * This is necessary to make sure the peripheral will be driven
  

• * normally when the display is enabled again later.
  

• */
  

• msleep(120);

• dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_CMD_MODE); dw_mipi_dsi_disable(dsi); clk_disable_unprepare(dsi->pclk);

}

2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, 30 Jan 2017 15:25:10 -0500, Sean Paul wrote:

On Sun, Jan 29, 2017 at 01:24:33PM +0000, John Keeping wrote:

...

This clock rate is derived from the PHY PLL, so it should be calculated dynamically. Use the same calculation as the vendor kernel to derive the escape clock speed.

Nit below, but

Reviewed-by: Sean Paul seanpaul@chromium.org

...

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Improve the commit message a bit
• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 290282e86d16..c2e0ba96e0a0 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -710,11 +710,13 @@ static void dw_mipi_dsi_disable(struct dw_mipi_dsi *dsi)

static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) {

Nit: It would be nice to add a comment to the effect of "You are not meant to understand this, it comes from the vendor kernel"

Actually, I think my commit message was misleading. I think I do understand the calculation, although the TRM is not particularly clear about it. TX_ESC_CLK_DIVISION is described as:

the division factor for the TX_Escape clock source (lanebyteclk). The value 0 and 1 stop the TX_ESC clock generation

Now lanebyteclk is (dsi->lane_mbps >> 3) since lane_mbps is the lane bit clock. The maximum escape mode clock from the MIPI specification is 20MHz, so we end up needing

lanebyteclk / esc_clk_division < 20

thus:

esc_clk_division > lanebyteclk / 20

and we want esc_clk_division >= 2 to avoid disabling the clock generation.

I'll add a comment to this effect.

...

• u32 esc_clk_division = (dsi->lane_mbps >> 3) / 20 + 1;
• dsi_write(dsi, DSI_PWR_UP, RESET); dsi_write(dsi, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK | PHY_RSTZ | PHY_SHUTDOWNZ); dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVIDSION(10) |

•   TX_ESC_CLK_DIVIDSION(7));
  

•   TX_ESC_CLK_DIVIDSION(esc_clk_division));
  

}

static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,

2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:34PM +0000, John Keeping wrote:

Also don't power up the DSI host at this point since this is not necessary in order to configure the PHY and we do so later when selecting video or command mode.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index c2e0ba96e0a0..5b3068e9e8db 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -397,7 +397,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) return testdin; }

• dsi_write(dsi, DSI_PWR_UP, POWERUP);

• /* Start by clearing PHY state */

• dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);

• dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLR);

• dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);

  dw_mipi_dsi_phy_write(dsi, 0x10, BYPASS_VCO_RANGE | VCO_RANGE_CON_SEL(vco) |

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:35PM +0000, John Keeping wrote:

The bias, bandgap and PLL should all be configured before we enable them.

Do you know why the test codes are hard-coded magic? It'd be nice to make some sense of them in a future patch.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com

v3:

• Squash together two patches that both affect initialization order of the PHY

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 5b3068e9e8db..cfe7e4ba305c 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -413,12 +413,17 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)

dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin));

• dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN);

• dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);

• dw_mipi_dsi_phy_write(dsi, 0x22, LOW_PROGRAM_EN |

• 			 BIASEXTR_SEL(BIASEXTR_127_7));
  

• dw_mipi_dsi_phy_write(dsi, 0x22, HIGH_PROGRAM_EN |

• 			 BANDGAP_SEL(BANDGAP_96_10));
  
  

  dw_mipi_dsi_phy_write(dsi, 0x20, POWER_CONTROL | INTERNAL_REG_CURRENT | BIAS_BLOCK_ON | BANDGAP_ON);

@@ -429,10 +434,6 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) SETRD_MAX | POWER_MANAGE | TER_RESISTORS_ON);

• dw_mipi_dsi_phy_write(dsi, 0x22, LOW_PROGRAM_EN |

• 			 BIASEXTR_SEL(BIASEXTR_127_7));
  

• dw_mipi_dsi_phy_write(dsi, 0x22, HIGH_PROGRAM_EN |

• 			 BANDGAP_SEL(BANDGAP_96_10));
  
  

  dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | 0xf); dw_mipi_dsi_phy_write(dsi, 0x71, THS_PRE_PROGRAM_EN | 0x55);

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, 30 Jan 2017 16:57:36 -0500, Sean Paul wrote:

On Sun, Jan 29, 2017 at 01:24:36PM +0000, John Keeping wrote:

...

These values are specified as constant time periods but the PHY configuration is in terms of the current lane byte clock so using constant values guarantees that the timings will be outside the specification with some display configurations.

Derive the necessary configuration from the byte clock in order to ensure that the PHY configuration is correct.

Signed-off-by: John Keeping john@metanate.com

v3:

• Wrap some long lines

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 39 ++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index cfe7e4ba305c..85edf6dd2bac 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -383,6 +383,26 @@ static void dw_mipi_dsi_phy_write(struct dw_mipi_dsi *dsi, u8 test_code, dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLK | PHY_UNTESTCLR); }

+/**

• • ns2bc - Nanoseconds to byte clock cycles
• */

+static inline unsigned int ns2bc(struct dw_mipi_dsi *dsi, int ns) +{

• unsigned long byte_clk_khz = dsi->lane_mbps * MSEC_PER_SEC / 8;

Why multiply by 1000 (MSEC_PER_SEC) only to immediately divide by 1000?

...

• return (ns * (byte_clk_khz / 1000) + 999) / 1000;

Can you replace this whole function with:

return DIV_ROUND_UP(ns * dsi->lane_mbps / 8, 1000);

Yes, I'll make this simplification.

...

+}

+/**

• • ns2ui - Nanoseconds to UI time periods
• */

+static inline unsigned int ns2ui(struct dw_mipi_dsi *dsi, int ns) +{

• unsigned long byte_clk_khz = dsi->lane_mbps * MSEC_PER_SEC;
• return (ns * (byte_clk_khz / 1000) + 999) / 1000;

Same remarks here.

Sean

...

+}

static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) { int ret, testdin, vco, val; @@ -434,10 +454,21 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) SETRD_MAX | POWER_MANAGE | TER_RESISTORS_ON);

• dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | 0xf);
• dw_mipi_dsi_phy_write(dsi, 0x71, THS_PRE_PROGRAM_EN | 0x55);
• dw_mipi_dsi_phy_write(dsi, 0x72, THS_ZERO_PROGRAM_EN | 0xa);

• dw_mipi_dsi_phy_write(dsi, 0x60, TLP_PROGRAM_EN | ns2bc(dsi, 500));

• dw_mipi_dsi_phy_write(dsi, 0x61, THS_PRE_PROGRAM_EN | ns2ui(dsi, 40));

• dw_mipi_dsi_phy_write(dsi, 0x62, THS_ZERO_PROGRAM_EN | ns2bc(dsi, 300));

• dw_mipi_dsi_phy_write(dsi, 0x63, THS_PRE_PROGRAM_EN | ns2ui(dsi, 100));

• dw_mipi_dsi_phy_write(dsi, 0x64, BIT(5) | ns2bc(dsi, 100));

• dw_mipi_dsi_phy_write(dsi, 0x65, BIT(5) | (ns2bc(dsi, 60) + 7));

• dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | ns2bc(dsi, 500));

• dw_mipi_dsi_phy_write(dsi, 0x71,

• 	      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 50) + 5));
  

• dw_mipi_dsi_phy_write(dsi, 0x72,

• 	      THS_ZERO_PROGRAM_EN | (ns2bc(dsi, 140) + 2));
  

• dw_mipi_dsi_phy_write(dsi, 0x73,

• 	      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 60) + 8));
  

• dw_mipi_dsi_phy_write(dsi, 0x74, BIT(5) | ns2bc(dsi, 100));

  dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK | PHY_UNRSTZ | PHY_UNSHUTDOWNZ);

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:37PM +0000, John Keeping wrote:

The multiplication ratio for the PLL is required to be even due to the use of a "by 2 pre-scaler". Currently we are likely to end up with an odd multiplier even though there is an equivalent set of parameters with an even multiplier.

For example, using the 324MHz bit rate with a reference clock of 24MHz we end up with M = 27, N = 2 whereas the example in the PHY databook gives M = 54, N = 4 for this bit rate and reference clock.

By walking down through the available multiplier instead of up we are more likely to hit an even multiplier. With the above example we do now get M = 54, N = 4 as given by the databook.

While doing this, change the loop limits to encode the actual limits on the divisor, which are:

40MHz >= (pllref / N) >= 5MHz

Signed-off-by: John Keeping john@metanate.com

Unchanged in v3 Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 85edf6dd2bac..dcb66a21e1f1 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -522,7 +522,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC); tmp = pllref;

• for (i = 1; i < 6; i++) {

• for (i = pllref / 5; i > (pllref / 40); i--) {

I've convinced myself that this is right, but it took reading through the commit message a few times. I think this code would benefit greatly from a comment so readers don't need to go through git history.

With that,

Reviewed-by: Sean Paul seanpaul@chromium.org

pre = pllref / i;
if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
	tmp = target_mbps % pre;

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:38PM +0000, John Keeping wrote:

As the documentation for readx_poll_timeout says, we want to use the specialized macro for readl rather than using the generic version directly.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index dcb66a21e1f1..be395c3c5c06 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -474,14 +474,14 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) PHY_UNRSTZ | PHY_UNSHUTDOWNZ);

• ret = readx_poll_timeout(readl, dsi->base + DSI_PHY_STATUS,

• ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, val, val & LOCK, 1000, PHY_STATUS_TIMEOUT_US); if (ret < 0) { dev_err(dsi->dev, "failed to wait for phy lock state\n"); return ret; }

• ret = readx_poll_timeout(readl, dsi->base + DSI_PHY_STATUS,

• ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, val, val & STOP_STATE_CLK_LANE, 1000, PHY_STATUS_TIMEOUT_US); if (ret < 0) {

@@ -597,7 +597,7 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) int ret; u32 val, mask;

• ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS,

• ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS, val, !(val & GEN_CMD_FULL), 1000, CMD_PKT_STATUS_TIMEOUT_US); if (ret < 0) {

@@ -608,7 +608,7 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) dsi_write(dsi, DSI_GEN_HDR, hdr_val);

mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY;

• ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS,

• ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS, val, (val & mask) == mask, 1000, CMD_PKT_STATUS_TIMEOUT_US); if (ret < 0) {

@@ -667,7 +667,7 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, len -= pld_data_bytes; }

• ret = readx_poll_timeout(readl, dsi->base + DSI_CMD_PKT_STATUS,
  

• ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
  		 val, !(val & GEN_PLD_W_FULL), 1000,
  		 CMD_PKT_STATUS_TIMEOUT_US);
  

  if (ret < 0) {

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:39PM +0000, John Keeping wrote:

This matches other drivers.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com

Unchanged in v3 Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index be395c3c5c06..f5b15377ef85 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -774,9 +774,9 @@ static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi, break; }

• if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))

• if (mode->flags & DRM_MODE_FLAG_NVSYNC) val |= VSYNC_ACTIVE_LOW;

• if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))

• if (mode->flags & DRM_MODE_FLAG_NHSYNC) val |= HSYNC_ACTIVE_LOW;

  dsi_write(dsi, DSI_DPI_VCID, DPI_VID(dsi->channel));

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:40PM +0000, John Keeping wrote:

When connected to the MIPI DSI output, we need to use N{H,V}SYNC for the internal connection but these flags are meaningless for DSI panels. Switch the test so that we do not set the P{H,V}SYNC bits unless the mode requires it.

Reviewed-by: Sean Paul seanpaul@chromium.org

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Mark Yao mark.yao@rock-chips.com

---

v3:

• Add Mark's Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c7eba305c488..67aefc6d4e9a 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -933,8 +933,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc) }

pin_pol = 0x8;

• pin_pol |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
• pin_pol |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);

• pin_pol |= (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) ? 1 : 0;

• pin_pol |= (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC) ? (1 << 1) : 0; VOP_CTRL_SET(vop, pin_pol, pin_pol);

  switch (s->output_type) {

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, Jan 29, 2017 at 01:24:41PM +0000, John Keeping wrote:

This ensures that the output resolution is known before fbcon loads.

Signed-off-by: John Keeping john@metanate.com

Unchanged in v3 Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index f5b15377ef85..5bad92e2370e 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -1176,10 +1176,17 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,

dsi->dsi_host.ops = &dw_mipi_dsi_host_ops; dsi->dsi_host.dev = dev;

• return mipi_dsi_host_register(&dsi->dsi_host);

• ret = mipi_dsi_host_register(&dsi->dsi_host);
• if (!ret && !dsi->panel) {

• mipi_dsi_host_unregister(&dsi->dsi_host);
  

• drm_encoder_cleanup(&dsi->encoder);
  

• drm_connector_cleanup(&dsi->connector);
  

Move the host registration up before dw_mipi_dsi_register() to avoid having to clean up the encoder and connector?

• ret = -EPROBE_DEFER;
  

• }

err_pllref:

• clk_disable_unprepare(dsi->pllref_clk);

• if (ret)

I personally think it's cleaner to explicitly goto in the error conditional (or in this case, the defer conditional) and have a return 0; right before the err_* labels. Then you don't need to worry about a) checking ret in all of your cleanups and b) someone adding code above the labels that you don't intend to run.

Sean

• clk_disable_unprepare(dsi->pllref_clk);
  

  return ret;

}

-- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi John

On 02/01/2017 03:22 AM, Sean Paul wrote:

On Sun, Jan 29, 2017 at 01:24:42PM +0000, John Keeping wrote:

Reviewed-by: Sean Paul seanpaul@chromium.org

...

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 5bad92e2370e..58cb8ace2fe8 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -82,6 +82,7 @@ #define FRAME_BTA_ACK BIT(14) #define ENABLE_LOW_POWER (0x3f << 8) #define ENABLE_LOW_POWER_MASK (0x3f << 8) +#define VID_MODE_TYPE_NON_BURST_SYNC_EVENTS 0x1 #define VID_MODE_TYPE_BURST_SYNC_PULSES 0x2

This field indicates the video mode transmission type as follows: 00: Non-burst with sync pulses 01: Non-burst with sync events 10 and 11: Burst mode

So, I think define the macro like this is better:

#define VID_MODE_TYPE_NON_BURST_SYNC_PULSES 0x0 #define VID_MODE_TYPE_NON_BURST_SYNC_EVENTS 0x1 #define VID_MODE_TYPE_BURST 0x2

...

#define VID_MODE_TYPE_MASK 0x3

@@ -286,6 +287,7 @@ struct dw_mipi_dsi { u32 format; u16 input_div; u16 feedback_div;

• unsigned long mode_flags;

  const struct dw_mipi_dsi_plat_data *pdata; };

@@ -551,15 +553,10 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, return -EINVAL; }

• if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) ||

•    !(device->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) {
  

• dev_err(dsi->dev, "device mode is unsupported\n");
  

• return -EINVAL;
  

• }
• dsi->lanes = device->lanes; dsi->channel = device->channel; dsi->format = device->format;

• dsi->mode_flags = device->mode_flags; dsi->panel = of_drm_find_panel(device->dev.of_node); if (dsi->panel) return drm_panel_attach(dsi->panel, &dsi->connector);

@@ -716,7 +713,12 @@ static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi) { u32 val;

• val = VID_MODE_TYPE_BURST_SYNC_PULSES | ENABLE_LOW_POWER;

• val = ENABLE_LOW_POWER;
• if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)

• val |= VID_MODE_TYPE_BURST_SYNC_PULSES;
  

• else if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE))

• val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;
  

if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) val |= VID_MODE_TYPE_BURST; else if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) val |= VID_MODE_TYPE_NON_BURST_SYNC_PULSES; else val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;

...

dsi_write(dsi, DSI_VID_MODE_CFG, val); } -- 2.11.0.197.gb556de5.dirty

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi John

On 01/29/2017 09:24 PM, John Keeping wrote:

In order to fully reset the state of the MIPI controller we must assert this reset.

This is slightly more complicated than it could be in order to maintain compatibility with device trees that do not specify the reset property.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 58cb8ace2fe8..cf3ca6b0cbdb 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/regmap.h> +#include <linux/reset.h> #include <linux/mfd/syscon.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> @@ -1124,6 +1125,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, of_match_device(dw_mipi_dsi_dt_ids, dev); const struct dw_mipi_dsi_plat_data *pdata = of_id->data; struct platform_device *pdev = to_platform_device(dev);

• struct reset_control *apb_rst; struct drm_device *drm = data; struct dw_mipi_dsi *dsi; struct resource *res;

@@ -1162,6 +1164,34 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, return ret; }

• /*

• * Note that the reset was not defined in the initial device tree, so
  

• * we have to be prepared for it not being found.
  

• */
  

• apb_rst = devm_reset_control_get(dev, "apb");
• if (IS_ERR(apb_rst)) {

• if (PTR_ERR(apb_rst) == -ENODEV) {
  

According to [0], I think it should be -ENOENT here.

[0] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=...

commit 3d81216fde465e76c5eae98f61d3666163634395 Author: Alban Bedel albeu@free.fr Date: Tue Sep 1 17:28:31 2015 +0200

reset: Fix of_reset_control_get() for consistent return values

When of_reset_control_get() is called without connection ID it returns -ENOENT when the 'resets' property doesn't exists or is an empty entry. However when a connection ID is given it returns -EINVAL when the 'resets' property doesn't exists or the requested name can't be found. This is because the error code returned by of_property_match_string() is just passed down as an index to of_parse_phandle_with_args(), which then returns -EINVAL.

To get a consistent return value with both code paths we must return -ENOENT when of_property_match_string() fails.

Signed-off-by: Alban Bedel albeu@free.fr Signed-off-by: Philipp Zabel p.zabel@pengutronix.de

• 	apb_rst = NULL;
  

• } else {
  

• 	dev_err(dev, "Unable to get reset control: %d\n", ret);
  

• 	return PTR_ERR(apb_rst);
  

• }
  

• }
• if (apb_rst) {

• ret = clk_prepare_enable(dsi->pclk);
  

• if (ret) {
  

• 	dev_err(dev, "%s: Failed to enable pclk\n", __func__);
  

• 	return ret;
  

• }
  

• reset_control_assert(apb_rst);
  

• usleep_range(10, 20);
  

• reset_control_deassert(apb_rst);
  

• clk_disable_unprepare(dsi->pclk);
  

• }
• ret = clk_prepare_enable(dsi->pllref_clk); if (ret) { dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);

Hi John

On 02/15/2017 08:39 PM, John Keeping wrote:

On Wed, 15 Feb 2017 11:38:45 +0800, Chris Zhong wrote:

...

On 01/29/2017 09:24 PM, John Keeping wrote:

...

In order to fully reset the state of the MIPI controller we must assert this reset.

This is slightly more complicated than it could be in order to maintain compatibility with device trees that do not specify the reset property.

Signed-off-by: John Keeping john@metanate.com Reviewed-by: Chris Zhong zyw@rock-chips.com

---

v3:

• Add Chris' Reviewed-by

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 58cb8ace2fe8..cf3ca6b0cbdb 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/regmap.h> +#include <linux/reset.h> #include <linux/mfd/syscon.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> @@ -1124,6 +1125,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, of_match_device(dw_mipi_dsi_dt_ids, dev); const struct dw_mipi_dsi_plat_data *pdata = of_id->data; struct platform_device *pdev = to_platform_device(dev);

• struct reset_control *apb_rst; struct drm_device *drm = data; struct dw_mipi_dsi *dsi; struct resource *res;

@@ -1162,6 +1164,34 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, return ret; }

• /*

• * Note that the reset was not defined in the initial device tree, so
  

• * we have to be prepared for it not being found.
  

• */
  

• apb_rst = devm_reset_control_get(dev, "apb");
• if (IS_ERR(apb_rst)) {

• if (PTR_ERR(apb_rst) == -ENODEV) {
  

According to [0], I think it should be -ENOENT here.

Nice catch, I'll fix this.

...

[0] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=...

commit 3d81216fde465e76c5eae98f61d3666163634395 Author: Alban Bedel albeu@free.fr Date: Tue Sep 1 17:28:31 2015 +0200

  reset: Fix of_reset_control_get() for consistent return values

  When of_reset_control_get() is called without connection ID it returns
  -ENOENT when the 'resets' property doesn't exists or is an empty entry.
  However when a connection ID is given it returns -EINVAL when the

'resets' property doesn't exists or the requested name can't be found. This is because the error code returned by of_property_match_string() is just passed down as an index to of_parse_phandle_with_args(), which then returns -EINVAL.

  To get a consistent return value with both code paths we must return
  -ENOENT when of_property_match_string() fails.

  Signed-off-by: Alban Bedel <albeu@free.fr>
  Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

...

• 	apb_rst = NULL;
  

• } else {
  

• 	dev_err(dev, "Unable to get reset control: %d\n", ret);
  

Also, we can not get error number from "ret" here.

...

...

• 	return PTR_ERR(apb_rst);
  

• }
  

• }
• if (apb_rst) {

• ret = clk_prepare_enable(dsi->pclk);
  

• if (ret) {
  

• 	dev_err(dev, "%s: Failed to enable pclk\n", __func__);
  

• 	return ret;
  

• }
  

• reset_control_assert(apb_rst);
  

• usleep_range(10, 20);
  

• reset_control_deassert(apb_rst);
  

• clk_disable_unprepare(dsi->pclk);
  

• }
• ret = clk_prepare_enable(dsi->pllref_clk); if (ret) { dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);

On Mon, 30 Jan 2017 10:26:11 -0500, Sean Paul wrote:

On Sun, Jan 29, 2017 at 01:24:44PM +0000, John Keeping wrote:

...

I haven't found any method for getting the length of a response, so this just uses the requested rx_len

Signed-off-by: John Keeping john@metanate.com

v3:

• Fix checkpatch warnings

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 56 ++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index cf3ca6b0cbdb..cc58ada75425 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -678,6 +678,56 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); }

+static int dw_mipi_dsi_dcs_read(struct dw_mipi_dsi *dsi,

• 		const struct mipi_dsi_msg *msg)
  

+{

• const u8 *tx_buf = msg->tx_buf;
• u8 *rx_buf = msg->rx_buf;
• size_t i;
• int ret, val;
• dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA);
• dsi_write(dsi, DSI_GEN_HDR,

•   GEN_HDATA(tx_buf[0]) | GEN_HTYPE(msg->type));
  

• ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,

• 		 val, !(val & GEN_RD_CMD_BUSY), 1000,
  

• 		 CMD_PKT_STATUS_TIMEOUT_US);
  

• if (ret < 0) {

• dev_err(dsi->dev, "failed to read command response\n");
  

• return ret;
  

• }
• for (i = 0; i < msg->rx_len;) {

• u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA);
  

• while (i < msg->rx_len) {
  

• 	rx_buf[i] = pld & 0xff;
  

• 	pld >>= 8;
  

• 	i++;
  

• }
  

• }

AFAICT, the outer for loop just initializes i and ensures msg->rx_len is non-zero?

I think the following would be easier to read (and safe against the case where msg->rx_len > sizeof(pld) (even though this shouldn't happen according to DCS spec)).

if (msg->rx_len > 0) { u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); memcpy(rx_buf, &pld, MIN(msg->rx_len, sizeof(pld)); }

I think the intent was to handle rx_len > 4, but the patch is obvously completely broken regarding that. As far as I can tell, rx_len is limited by the maximum return packet size which can be any value up to the maximum size of a long packet, so we may need to read from the FIFO multiple times.

The loop should be something like this:

for (i = 0; i < msg->rx_len;) { u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); int j;

for (j = 0; j < 4 && i < msg->rx_len; i++, j++) { rx_buf[i] = pld & 0xff; pld >>= 8; } }

I have successfully read 5 bytes from a DSI display using this code, but I'm tempted to just drop this patch since I only used it for debugging while bringing up a new panel.

On Mon, 30 Jan 2017 15:16:09 -0500, Sean Paul wrote:

On Mon, Jan 30, 2017 at 06:14:27PM +0000, John Keeping wrote:

...

On Mon, 30 Jan 2017 10:26:11 -0500, Sean Paul wrote:

...

On Sun, Jan 29, 2017 at 01:24:44PM +0000, John Keeping wrote:

...

I haven't found any method for getting the length of a response, so this just uses the requested rx_len

Signed-off-by: John Keeping john@metanate.com

v3:

• Fix checkpatch warnings

Unchanged in v2

drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 56 ++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index cf3ca6b0cbdb..cc58ada75425 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -678,6 +678,56 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); }

+static int dw_mipi_dsi_dcs_read(struct dw_mipi_dsi *dsi,

• 		const struct mipi_dsi_msg *msg)
  

+{

• const u8 *tx_buf = msg->tx_buf;
• u8 *rx_buf = msg->rx_buf;
• size_t i;
• int ret, val;
• dsi_write(dsi, DSI_PCKHDL_CFG, EN_CRC_RX | EN_ECC_RX | EN_BTA);
• dsi_write(dsi, DSI_GEN_HDR,

•   GEN_HDATA(tx_buf[0]) | GEN_HTYPE(msg->type));
  

• ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,

• 		 val, !(val & GEN_RD_CMD_BUSY), 1000,
  

• 		 CMD_PKT_STATUS_TIMEOUT_US);
  

• if (ret < 0) {

• dev_err(dsi->dev, "failed to read command response\n");
  

• return ret;
  

• }
• for (i = 0; i < msg->rx_len;) {

• u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA);
  

• while (i < msg->rx_len) {
  

• 	rx_buf[i] = pld & 0xff;
  

• 	pld >>= 8;
  

• 	i++;
  

• }
  

• }

AFAICT, the outer for loop just initializes i and ensures msg->rx_len is non-zero?

I think the following would be easier to read (and safe against the case where msg->rx_len > sizeof(pld) (even though this shouldn't happen according to DCS spec)).

if (msg->rx_len > 0) { u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); memcpy(rx_buf, &pld, MIN(msg->rx_len, sizeof(pld)); }

I think the intent was to handle rx_len > 4, but the patch is obvously completely broken regarding that. As far as I can tell, rx_len is limited by the maximum return packet size which can be any value up to the maximum size of a long packet, so we may need to read from the FIFO multiple times.

The loop should be something like this:

for (i = 0; i < msg->rx_len;) { u32 pld = dsi_read(dsi, DSI_GEN_PLD_DATA); int j;

for (j = 0; j < 4 && i < msg->rx_len; i++, j++) {
	rx_buf[i] = pld & 0xff;
	pld >>= 8;
}

}

Short packets should never exceed 32 bits, so I don't think you need to add the nested loop.

The read response is not restricted to a short packet. I have a panel that documents a read request that returns up to 64KiB, admittedly with a continuation command and the panels I have seem to only be programmed to return 5 bytes of meaningful data, but they do return all of those bytes in a single read response.