[PATCH v3 00/24] drm/rockchip: MIPI fixes & improvements

[PATCH v3 00/24] drm/rockchip: MIPI fixes & improvements

[John Keeping]

This re-roll mostly just gather up reviewed-by tags, although I have
also wrapped some long lines and squashed together some commits as
suggested by Chris Zhong.

Version 2 was posted here:
https://www.spinics.net/lists/arm-kernel/msg556683.html

John Keeping (24):
  drm/rockchip: dw-mipi-dsi: don't configure hardware in mode_set for
    MIPI
  drm/rockchip: dw-mipi-dsi: pass mode in where needed
  drm/rockchip: dw-mipi-dsi: remove mode_set hook
  drm/rockchip: dw-mipi-dsi: fix command header writes
  drm/rockchip: dw-mipi-dsi: fix generic packet status check
  drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf
  drm/rockchip: dw-mipi-dsi: include bad value in error message
  drm/rockchip: dw-mipi-dsi: respect message flags
  drm/rockchip: dw-mipi-dsi: only request HS clock when required
  drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned
  drm/rockchip: dw-mipi-dsi: prepare panel after phy init
  drm/rockchip: dw-mipi-dsi: allow commands in panel_disable
  drm/rockchip: dw-mipi-dsi: fix escape clock rate
  drm/rockchip: dw-mipi-dsi: ensure PHY is reset
  drm/rockchip: dw-mipi-dsi: configure PHY before enabling
  drm/rockchip: dw-mipi-dsi: properly configure PHY timing
  drm/rockchip: dw-mipi-dsi: improve PLL configuration
  drm/rockchip: dw-mipi-dsi: use specific poll helper
  drm/rockchip: dw-mipi-dsi: use positive check for N{H,V}SYNC
  drm/rockchip: vop: test for P{H,V}SYNC
  drm/rockchip: dw-mipi-dsi: defer probe if panel is not loaded
  drm/rockchip: dw-mipi-dsi: support non-burst modes
  drm/rockchip: dw-mipi-dsi: add reset control
  drm/rockchip: dw-mipi-dsi: support read commands

 drivers/gpu/drm/rockchip/dw-mipi-dsi.c      | 348 +++++++++++++++++++---------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   4 +-
 2 files changed, 245 insertions(+), 107 deletions(-)

-- 
2.11.0.197.gb556de5.dirty

[PATCH v3 01/24] drm/rockchip: dw-mipi-dsi: don't configure hardware in mode_set for MIPI

[John Keeping]

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.

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

[PATCH v3 02/24] drm/rockchip: dw-mipi-dsi: pass mode in where needed

[John Keeping]

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.

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

[PATCH v3 03/24] drm/rockchip: dw-mipi-dsi: remove mode_set hook

[John Keeping]

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.

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

[PATCH v3 04/24] drm/rockchip: dw-mipi-dsi: fix command header writes

[John Keeping]

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.

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

[PATCH v3 05/24] drm/rockchip: dw-mipi-dsi: fix generic packet status check

[John Keeping]

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.

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

[PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf

[John Keeping]

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);
 
 	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

[PATCH v3 07/24] drm/rockchip: dw-mipi-dsi: include bad value in error message

[John Keeping]

As an aid to debugging.

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

[PATCH v3 08/24] drm/rockchip: dw-mipi-dsi: respect message flags

[John Keeping]

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.

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

[PATCH v3 09/24] drm/rockchip: dw-mipi-dsi: only request HS clock when required

[John Keeping]

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.

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

[PATCH v3 10/24] drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned

[John Keeping]

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;
 		}
 
-- 
2.11.0.197.gb556de5.dirty

[PATCH v3 11/24] drm/rockchip: dw-mipi-dsi: prepare panel after phy init

[John Keeping]

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.

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

[PATCH v3 12/24] drm/rockchip: dw-mipi-dsi: allow commands in panel_disable

[John Keeping]

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.

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

[PATCH v3 13/24] drm/rockchip: dw-mipi-dsi: fix escape clock rate

[John Keeping]

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.

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)
 {
+	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

[PATCH v3 14/24] drm/rockchip: dw-mipi-dsi: ensure PHY is reset

[John Keeping]

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.

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

[PATCH v3 15/24] drm/rockchip: dw-mipi-dsi: configure PHY before enabling

[John Keeping]

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

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

[PATCH v3 16/24] drm/rockchip: dw-mipi-dsi: properly configure PHY timing

[John Keeping]

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;
+
+	return (ns * (byte_clk_khz / 1000) + 999) / 1000;
+}
+
+/**
+ * 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;
+}
+
 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

[PATCH v3 17/24] drm/rockchip: dw-mipi-dsi: improve PLL configuration

[John Keeping]

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--) {
 		pre = pllref / i;
 		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
 			tmp = target_mbps % pre;
-- 
2.11.0.197.gb556de5.dirty

[PATCH v3 18/24] drm/rockchip: dw-mipi-dsi: use specific poll helper

[John Keeping]

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

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

[PATCH v3 19/24] drm/rockchip: dw-mipi-dsi: use positive check for N{H,V}SYNC

[John Keeping]

This matches other drivers.

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

[PATCH v3 20/24] drm/rockchip: vop: test for P{H,V}SYNC

[John Keeping]

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.

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

[PATCH v3 21/24] drm/rockchip: dw-mipi-dsi: defer probe if panel is not loaded

[John Keeping]

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);
+		ret = -EPROBE_DEFER;
+	}
 
 err_pllref:
-	clk_disable_unprepare(dsi->pllref_clk);
+	if (ret)
+		clk_disable_unprepare(dsi->pllref_clk);
 	return ret;
 }
 
-- 
2.11.0.197.gb556de5.dirty

[PATCH v3 22/24] drm/rockchip: dw-mipi-dsi: support non-burst modes

[John Keeping]

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
 #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;
 
 	dsi_write(dsi, DSI_VID_MODE_CFG, val);
 }
-- 
2.11.0.197.gb556de5.dirty

[PATCH v3 23/24] drm/rockchip: dw-mipi-dsi: add reset control

[John Keeping]

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) {
+			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__);
-- 
2.11.0.197.gb556de5.dirty

[PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands

[John Keeping]

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++;
+		}
+	}
+
+	return msg->rx_len;
+}
+
+static int dw_mipi_dsi_set_max_return_packet_size(struct dw_mipi_dsi *dsi,
+						  size_t len)
+{
+	u8 val[] = { len & 0xff, (len >> 8) & 0xff };
+	struct mipi_dsi_msg msg = {
+		.channel = dsi->channel,
+		.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
+		.tx_buf = val,
+		.tx_len = 2,
+	};
+
+	if (len > 0xffff)
+		return -EINVAL;
+
+	return dw_mipi_dsi_dcs_short_write(dsi, &msg);
+}
+
 static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
 					 const struct mipi_dsi_msg *msg)
 {
@@ -695,6 +745,12 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
 	case MIPI_DSI_DCS_LONG_WRITE:
 		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
 		break;
+	case MIPI_DSI_DCS_READ:
+		ret = dw_mipi_dsi_set_max_return_packet_size(dsi, msg->rx_len);
+		if (ret < 0)
+			return ret;
+		ret = dw_mipi_dsi_dcs_read(dsi, msg);
+		break;
 	default:
 		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
 			msg->type);
-- 
2.11.0.197.gb556de5.dirty

Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands

[Sean Paul]

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));
}


> +
> +	return msg->rx_len;
> +}
> +
> +static int dw_mipi_dsi_set_max_return_packet_size(struct dw_mipi_dsi *dsi,
> +						  size_t len)
> +{
> +	u8 val[] = { len & 0xff, (len >> 8) & 0xff };
> +	struct mipi_dsi_msg msg = {
> +		.channel = dsi->channel,
> +		.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
> +		.tx_buf = val,
> +		.tx_len = 2,
> +	};
> +
> +	if (len > 0xffff)
> +		return -EINVAL;
> +
> +	return dw_mipi_dsi_dcs_short_write(dsi, &msg);
> +}
> +
>  static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>  					 const struct mipi_dsi_msg *msg)
>  {
> @@ -695,6 +745,12 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>  	case MIPI_DSI_DCS_LONG_WRITE:
>  		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
>  		break;
> +	case MIPI_DSI_DCS_READ:
> +		ret = dw_mipi_dsi_set_max_return_packet_size(dsi, msg->rx_len);
> +		if (ret < 0)
> +			return ret;
> +		ret = dw_mipi_dsi_dcs_read(dsi, msg);
> +		break;
>  	default:
>  		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
>  			msg->type);
> -- 
> 2.11.0.197.gb556de5.dirty
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 01/24] drm/rockchip: dw-mipi-dsi: don't configure hardware in mode_set for MIPI

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 03/24] drm/rockchip: dw-mipi-dsi: remove mode_set hook

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 02/24] drm/rockchip: dw-mipi-dsi: pass mode in where needed

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 04/24] drm/rockchip: dw-mipi-dsi: fix command header writes

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 05/24] drm/rockchip: dw-mipi-dsi: fix generic packet status check

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 07/24] drm/rockchip: dw-mipi-dsi: include bad value in error message

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands

[John Keeping]

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.

Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf

[John Keeping]

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"?

Re: [PATCH v3 08/24] drm/rockchip: dw-mipi-dsi: respect message flags

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 09/24] drm/rockchip: dw-mipi-dsi: only request HS clock when required

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 10/24] drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands

[Sean Paul]

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.

Sean


> 
> 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.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 11/24] drm/rockchip: dw-mipi-dsi: prepare panel after phy init

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 12/24] drm/rockchip: dw-mipi-dsi: allow commands in panel_disable

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 13/24] drm/rockchip: dw-mipi-dsi: fix escape clock rate

[Sean Paul]

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"

> +	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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 14/24] drm/rockchip: dw-mipi-dsi: ensure PHY is reset

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 15/24] drm/rockchip: dw-mipi-dsi: configure PHY before enabling

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 16/24] drm/rockchip: dw-mipi-dsi: properly configure PHY timing

[Sean Paul]

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);

> +}
> +
> +/**
> + * 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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf

[John Keeping]

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];

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

?

Re: [PATCH v3 10/24] drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned

[John Keeping]

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.

Re: [PATCH v3 12/24] drm/rockchip: dw-mipi-dsi: allow commands in panel_disable

[John Keeping]

On Mon, 30 Jan 2017 15:19:53 -0500, Sean Paul wrote:

> 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,

I'm just going by the comment in the code that this patch deletes and
the fact that this delay was not needed on any of the three panels I
tested.

Given the way the modes change, I think this should be a 120ms disable
delay if the affected panel is supported by the simple-panel driver.

> 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  
> 

Re: [PATCH v3 15/24] drm/rockchip: dw-mipi-dsi: configure PHY before enabling

[John Keeping]

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

> 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.

I just kept with the existing style of the code, but it should be
straightforward to add some defines with sensible names.

> 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  
> 

Re: [PATCH v3 16/24] drm/rockchip: dw-mipi-dsi: properly configure PHY timing

[John Keeping]

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  
> 

Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands

[John Keeping]

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.

Re: [PATCH v3 24/24] drm/rockchip: dw-mipi-dsi: support read commands

[Sean Paul]

On Tue, Jan 31, 2017 at 12:41:47PM +0000, John Keeping wrote:
> 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.

Ah, apologies for the misunderstanding. In that case, I think you can get away
with replacing the inner loop in your snippet with a memcpy and call it a day.

Sean

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 10/24] drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 18/24] drm/rockchip: dw-mipi-dsi: use specific poll helper

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 17/24] drm/rockchip: dw-mipi-dsi: improve PLL configuration

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 19/24] drm/rockchip: dw-mipi-dsi: use positive check for N{H, V}SYNC

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 20/24] drm/rockchip: vop: test for P{H,V}SYNC

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 21/24] drm/rockchip: dw-mipi-dsi: defer probe if panel is not loaded

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 22/24] drm/rockchip: dw-mipi-dsi: support non-burst modes

[Sean Paul]

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
>  #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;
>  
>  	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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 23/24] drm/rockchip: dw-mipi-dsi: add reset control

[Sean Paul]

On Sun, Jan 29, 2017 at 01:24:43PM +0000, 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.
> 

I always find it a little grating to see a device managed resource given to a
local variable that is used immediately and only once. However, I think this
might just be one of my twitches. So now that I've aired my grievance, 

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 | 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) {
> +			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__);
> -- 
> 2.11.0.197.gb556de5.dirty
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 13/24] drm/rockchip: dw-mipi-dsi: fix escape clock rate

[John Keeping]

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  
> 

Re: [PATCH v3 21/24] drm/rockchip: dw-mipi-dsi: defer probe if panel is not loaded

[John Keeping]

On Tue, 31 Jan 2017 14:21:17 -0500, Sean Paul wrote:

> 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?

No, mipi_dsi_host_register() has to be called after the connector is
registered because it is likely to result in a call to
dw_mipi_dsi_host_attach() which attaches a panel to the 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.

Agreed.  I'll change this to use a goto if we hit the EPROBE_DEFER case
and keep all the cleanup together.

> > +		clk_disable_unprepare(dsi->pllref_clk);
> >  	return ret;
> >  }

Re: [PATCH v3 23/24] drm/rockchip: dw-mipi-dsi: add reset control

[Chris Zhong]

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=3d81216fde465e76c5eae98f61d3666163634395

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__);

Re: [PATCH v3 23/24] drm/rockchip: dw-mipi-dsi: add reset control

[John Keeping]

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=3d81216fde465e76c5eae98f61d3666163634395
> 
> 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__);  
> 
> 

Re: [PATCH v3 23/24] drm/rockchip: dw-mipi-dsi: add reset control

[Chris Zhong]

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=3d81216fde465e76c5eae98f61d3666163634395
>>
>> 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__);
>>
>
>

Re: [PATCH v3 22/24] drm/rockchip: dw-mipi-dsi: support non-burst modes

[Chris Zhong]

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

Re: [PATCH v3 23/24] drm/rockchip: dw-mipi-dsi: add reset control

[John Keeping]

On Thu, 16 Feb 2017 10:12:33 +0800, Chris Zhong wrote:

> 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=3d81216fde465e76c5eae98f61d3666163634395
> >>
> >> 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.

Good point, I'll fix this.


John

Re: [PATCH v3 22/24] drm/rockchip: dw-mipi-dsi: support non-burst modes

[John Keeping]

On Thu, 16 Feb 2017 11:01:46 +0800, Chris Zhong wrote:

> 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;

OK, this is definitely clearer now that I've forgotten most of the
datasheet; without your definitions at the top it's not clear that
VID_MODE_TYPE_BURST_SYNC_PULSES is zero.