[PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[Nickey Yang]

This patch correct Feedback divider setting:
1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
2、Due to the use of a "by 2 pre-scaler," the range of the
feedback multiplication Feedback divider is limited to even
division numbers, and Feedback divider must be greater than
12, less than 1000.
3、Make the previously configured Feedback divider(LSB)
factors effective

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 9a20b9d..52698b7 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -228,7 +228,7 @@
 #define LOW_PROGRAM_EN		0
 #define HIGH_PROGRAM_EN		BIT(7)
 #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
-#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
+#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
 #define PLL_LOOP_DIV_EN		BIT(5)
 #define PLL_INPUT_DIV_EN	BIT(4)
 
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_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);
@@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(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;
-	unsigned int m = 1, n = 1, target_mbps = 1000;
+	unsigned long mpclk, tmp;
+	unsigned int target_mbps = 1000;
 	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
 	int bpp;
+	unsigned long best_freq = 0;
+	unsigned long fvco_min, fvco_max, fin ,fout;
+	unsigned int min_prediv, max_prediv;
+	unsigned int _prediv, uninitialized_var(best_prediv);
+	unsigned long _fbdiv, uninitialized_var(best_fbdiv);
+	unsigned long min_delta = UINT_MAX;
 
 	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
 	if (bpp < 0) {
@@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 			dev_err(dsi->dev, "DPHY clock frequency is out of range\n");
 	}
 
-	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
-	tmp = pllref;
-
-	/*
-	 * The limits on the PLL divisor are:
-	 *
-	 *	5MHz <= (pllref / n) <= 40MHz
-	 *
-	 * we walk over these values in descreasing order so that if we hit
-	 * an exact match for target_mbps it is more likely that "m" will be
-	 * even.
-	 *
-	 * TODO: ensure that "m" is even after this loop.
-	 */
-	for (i = pllref / 5; i > (pllref / 40); i--) {
-		pre = pllref / i;
-		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
-			tmp = target_mbps % pre;
-			n = i;
-			m = target_mbps / pre;
+	fin = clk_get_rate(dsi->pllref_clk);
+	fout = target_mbps * USEC_PER_SEC;
+
+	/* constraint: 5Mhz < Fref / N < 40MHz */
+	min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
+	max_prediv = fin / (5 * USEC_PER_SEC);
+
+	/* constraint: 80MHz < Fvco < 1500Mhz */
+	fvco_min = 80 * USEC_PER_SEC;
+	fvco_max = 1500 * USEC_PER_SEC;
+
+	for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
+		u32 delta;
+		/* Fvco = Fref * M / N */
+		tmp = fout * _prediv;
+		do_div(tmp, fin);
+		_fbdiv = tmp;
+		/*
+		 * Due to the use of a "by 2 pre-scaler," the range of the
+		 * feedback multiplication value M is limited to even division
+		 * numbers, and m must be greater than 12, less than 1000.
+		 */
+		if (_fbdiv < 12 || _fbdiv > 1000)
+			continue;
+
+		if (_fbdiv % 2)
+			++_fbdiv;
+
+		tmp = (u64)_fbdiv * fin;
+		do_div(tmp, _prediv);
+		if (tmp < fvco_min || tmp > fvco_max)
+			continue;
+
+		delta = abs(fout - tmp);
+		if (delta < min_delta) {
+			best_prediv = _prediv;
+			best_fbdiv = _fbdiv;
+			min_delta = delta;
+			best_freq = tmp;
 		}
-		if (tmp == 0)
-			break;
 	}
 
-	dsi->lane_mbps = pllref / n * m;
-	dsi->input_div = n;
-	dsi->feedback_div = m;
+	if (best_freq) {
+		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
+		dsi->input_div = best_prediv;
+		dsi->feedback_div = best_fbdiv;
+	}
 
 	return 0;
 }
-- 
1.9.1

[PATCH 2/7] drm/rockchip/dsi: add dual mipi channel support

[Nickey Yang]

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c      | 353 ++++++++++++++++++++--------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   2 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   3 +
 4 files changed, 257 insertions(+), 102 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 52698b7..9265b7f 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -42,6 +42,17 @@
 #define RK3399_GRF_SOC_CON22		0x6258
 #define RK3399_GRF_DSI_MODE		0xffff0000
 
+/* disable turndisable, forcetxstopmode, forcerxmode, enable */
+#define RK3399_GRF_SOC_CON23		0x625c
+#define RK3399_GRF_DSI1_MODE1		0xffff0000
+#define RK3399_GRF_DSI1_ENABLE		0x000f000f
+/* disable basedir and enable clk*/
+#define RK3399_GRF_SOC_CON24		0x6260
+#define RK3399_TXRX_MASTERSLAVEZ	BIT(7)
+#define RK3399_TXRX_ENABLECLK		BIT(6)
+#define RK3399_TXRX_BASEDIR		BIT(5)
+#define RK3399_GRF_DSI1_MODE2		0x00e00080
+
 #define DSI_VERSION			0x00
 #define DSI_PWR_UP			0x04
 #define RESET				0
@@ -282,6 +293,12 @@ struct dw_mipi_dsi_plat_data {
 	u32 grf_switch_reg;
 	u32 grf_dsi0_mode;
 	u32 grf_dsi0_mode_reg;
+	u32 grf_dsi1_mode;
+	u32 grf_dsi1_mode_reg1;
+	u32 dsi1_basedir;
+	u32 dsi1_masterslavez;
+	u32 dsi1_enableclk;
+	u32 grf_dsi1_mode_reg2;
 	unsigned int flags;
 	unsigned int max_data_lanes;
 };
@@ -300,6 +317,12 @@ struct dw_mipi_dsi {
 	struct clk *pclk;
 	struct clk *phy_cfg_clk;
 
+	/* dual-channel */
+	struct dw_mipi_dsi *master;
+	struct dw_mipi_dsi *slave;
+	struct device_node *panel_node;
+	int id;
+
 	int dpms_mode;
 	unsigned int lane_mbps; /* per lane */
 	u32 channel;
@@ -526,6 +549,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 	unsigned int target_mbps = 1000;
 	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
 	int bpp;
+	int lanes = dsi->lanes;
 	unsigned long best_freq = 0;
 	unsigned long fvco_min, fvco_max, fin ,fout;
 	unsigned int min_prediv, max_prediv;
@@ -540,10 +564,13 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 		return bpp;
 	}
 
+	if (dsi->slave || dsi->master)
+		lanes = dsi->lanes * 2;
+
 	mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC);
 	if (mpclk) {
 		/* take 1 / 0.8, since mbps must big than bandwidth of RGB */
-		tmp = mpclk * (bpp / dsi->lanes) * 10 / 8;
+		tmp = mpclk * (bpp / lanes) * 10 / 8;
 		if (tmp < max_mbps)
 			target_mbps = tmp;
 		else
@@ -605,17 +632,26 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
 				   struct mipi_dsi_device *device)
 {
 	struct dw_mipi_dsi *dsi = host_to_dsi(host);
+	int lanes = dsi->slave ? device->lanes / 2 : device->lanes;
 
-	if (device->lanes > dsi->pdata->max_data_lanes) {
+	if (lanes > dsi->pdata->max_data_lanes) {
 		dev_err(dsi->dev, "the number of data lanes(%u) is too many\n",
-			device->lanes);
+			lanes);
 		return -EINVAL;
 	}
 
-	dsi->lanes = device->lanes;
+	dsi->lanes = lanes;
 	dsi->channel = device->channel;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
+
+	if (dsi->slave) {
+		dsi->slave->lanes = lanes;
+		dsi->slave->channel = device->channel;
+		dsi->slave->format = device->format;
+		dsi->slave->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);
@@ -745,15 +781,22 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
 	int ret;
 
 	dw_mipi_message_config(dsi, msg);
+	if (dsi->slave)
+		dw_mipi_message_config(dsi->slave, msg);
 
 	switch (msg->type) {
 	case MIPI_DSI_DCS_SHORT_WRITE:
 	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
 	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
+	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
 		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
+		if (dsi->slave)
+			ret = dw_mipi_dsi_dcs_short_write(dsi->slave, msg);
 		break;
 	case MIPI_DSI_DCS_LONG_WRITE:
 		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
+		if (dsi->slave)
+			ret = dw_mipi_dsi_dcs_long_write(dsi->slave, msg);
 		break;
 	default:
 		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
@@ -827,6 +870,64 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
 		  TX_ESC_CLK_DIVIDSION(esc_clk_division));
 }
 
+static void rockchip_dsi_grf_config(struct dw_mipi_dsi *dsi, int vop_id)
+{
+	const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata;
+	int val = 0;
+	int ret;
+
+	/*
+	 * For the RK3399, the clk of grf must be enabled before writing grf
+	 * register. And for RK3288 or other soc, this grf_clk must be NULL,
+	 * the clk_prepare_enable return true directly.
+	 */
+	ret = clk_prepare_enable(dsi->grf_clk);
+	if (ret) {
+		dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret);
+		return;
+	}
+
+	if (dsi->slave) {
+		if (vop_id)
+			val = pdata->dsi0_en_bit |
+			      (pdata->dsi0_en_bit << 16) |
+			      pdata->dsi1_en_bit |
+			      (pdata->dsi1_en_bit << 16);
+		else
+			val = (pdata->dsi0_en_bit << 16) |
+			      (pdata->dsi1_en_bit << 16);
+
+		regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
+
+		if (pdata->grf_dsi0_mode_reg)
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
+				     pdata->grf_dsi0_mode);
+		if (pdata->grf_dsi1_mode_reg1)
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
+				     pdata->grf_dsi1_mode);
+		if (pdata->grf_dsi1_mode_reg2)
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2,
+				     RK3399_GRF_DSI1_MODE2);
+		if (pdata->grf_dsi1_mode_reg1)
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
+				     RK3399_GRF_DSI1_ENABLE);
+	} else {
+		if (vop_id)
+			val = pdata->dsi0_en_bit |
+			      (pdata->dsi0_en_bit << 16);
+		else
+			val = pdata->dsi0_en_bit << 16;
+
+		regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
+
+		if (pdata->grf_dsi0_mode_reg)
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
+				     pdata->grf_dsi0_mode);
+	}
+
+	clk_disable_unprepare(dsi->grf_clk);
+}
+
 static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
 				   struct drm_display_mode *mode)
 {
@@ -867,7 +968,14 @@ static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
 static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
 					    struct drm_display_mode *mode)
 {
-	dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay));
+	int pkt_size;
+
+	if (dsi->slave || dsi->master)
+		pkt_size = VID_PKT_SIZE(mode->hdisplay / 2 + 4);
+	else
+		pkt_size = VID_PKT_SIZE(mode->hdisplay);
+
+	dsi_write(dsi, DSI_VID_PKT_SIZE, pkt_size);
 }
 
 static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
@@ -971,24 +1079,24 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
 
 	dw_mipi_dsi_disable(dsi);
 	pm_runtime_put(dsi->dev);
+
+	if (dsi->slave) {
+		if (clk_prepare_enable(dsi->slave->pclk)) {
+			dev_err(dsi->slave->dev, "%s: Failed to enable pclk\n", __func__);
+			return;
+		}
+		dw_mipi_dsi_disable(dsi->slave);
+		pm_runtime_put(dsi->slave->dev);
+		clk_disable_unprepare(dsi->slave->pclk);
+	}
+
 	clk_disable_unprepare(dsi->pclk);
 	dsi->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
-static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
+static void dw_mipi_dsi_enable(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode)
 {
-	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
-	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
-	const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata;
-	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, mode);
-	if (ret < 0)
-		return;
-
-	if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
+	if (dw_mipi_dsi_get_lane_bps(dsi, mode) < 0)
 		return;
 
 	if (clk_prepare_enable(dsi->pclk)) {
@@ -1009,43 +1117,42 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
 	dw_mipi_dsi_dphy_interface_config(dsi);
 	dw_mipi_dsi_clear_err(dsi);
 
-	/*
-	 * For the RK3399, the clk of grf must be enabled before writing grf
-	 * register. And for RK3288 or other soc, this grf_clk must be NULL,
-	 * the clk_prepare_enable return true directly.
-	 */
-	ret = clk_prepare_enable(dsi->grf_clk);
-	if (ret) {
-		dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret);
-		return;
-	}
-
-	if (pdata->grf_dsi0_mode_reg)
-		regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
-			     pdata->grf_dsi0_mode);
-
 	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);
+}
+
+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 = &encoder->crtc->state->adjusted_mode;
+	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
+
+	if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
+		return;
+
+	rockchip_dsi_grf_config(dsi, mux);
+	dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG");
+
+	dw_mipi_dsi_enable(dsi, mode);
+	if (dsi->slave)
+		dw_mipi_dsi_enable(dsi->slave, 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);
+	if (dsi->slave)
+		dw_mipi_dsi_set_mode(dsi->slave, DW_MIPI_DSI_VID_MODE);
+
 	drm_panel_enable(dsi->panel);
 
 	clk_disable_unprepare(dsi->pclk);
+	if (dsi->slave)
+		clk_disable_unprepare(dsi->slave->pclk);
 
-	if (mux)
-		val = pdata->dsi0_en_bit | (pdata->dsi0_en_bit << 16);
-	else
-		val = pdata->dsi0_en_bit << 16;
-
-	regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
-	dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG");
 	dsi->dpms_mode = DRM_MODE_DPMS_ON;
-
-	clk_disable_unprepare(dsi->grf_clk);
 }
 
 static int
@@ -1073,6 +1180,9 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
 
 	s->output_type = DRM_MODE_CONNECTOR_DSI;
 
+	if (dsi->slave)
+		s->output_flags = ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL;
+
 	return 0;
 }
 
@@ -1178,6 +1288,12 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi)
 	.grf_switch_reg = RK3399_GRF_SOC_CON20,
 	.grf_dsi0_mode = RK3399_GRF_DSI_MODE,
 	.grf_dsi0_mode_reg = RK3399_GRF_SOC_CON22,
+	.grf_dsi1_mode = RK3399_GRF_DSI1_MODE1,
+	.grf_dsi1_mode_reg1 = RK3399_GRF_SOC_CON23,
+	.dsi1_basedir = RK3399_TXRX_BASEDIR,
+	.dsi1_masterslavez = RK3399_TXRX_MASTERSLAVEZ,
+	.dsi1_enableclk = RK3399_TXRX_ENABLECLK,
+	.grf_dsi1_mode_reg2 = RK3399_GRF_SOC_CON24,
 	.flags = DW_MIPI_NEEDS_PHY_CFG_CLK | DW_MIPI_NEEDS_GRF_CLK,
 	.max_data_lanes = 4,
 };
@@ -1194,17 +1310,106 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi)
 };
 MODULE_DEVICE_TABLE(of, dw_mipi_dsi_dt_ids);
 
+
+static int rockchip_dsi_dual_channel_probe(struct dw_mipi_dsi *dsi)
+{
+	struct device_node *np;
+	struct platform_device *secondary;
+
+	np = of_parse_phandle(dsi->dev->of_node, "rockchip,dual-channel", 0);
+	if (np) {
+		secondary = of_find_device_by_node(np);
+		dsi->slave = platform_get_drvdata(secondary);
+		of_node_put(np);
+
+		if (!dsi->slave)
+			return -EPROBE_DEFER;
+
+		dsi->slave->master = dsi;
+	}
+
+	return 0;
+}
+
 static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
 			    void *data)
 {
+	struct drm_device *drm = data;
+	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = rockchip_dsi_dual_channel_probe(dsi);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(dsi->pllref_clk);
+	if (ret) {
+		dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);
+		return ret;
+	}
+
+	pm_runtime_enable(dev);
+
+	if (dsi->master)
+		return 0;
+
+	ret = dw_mipi_dsi_register(drm, dsi);
+	if (ret) {
+		dev_err(dev, "Failed to register mipi_dsi: %d\n", ret);
+		goto err_pllref;
+	}
+
+	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;
+	dsi->dsi_host.dev = dev;
+	ret = mipi_dsi_host_register(&dsi->dsi_host);
+	if (ret) {
+		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
+		goto err_cleanup;
+	}
+
+	if (!dsi->panel) {
+		ret = -EPROBE_DEFER;
+		goto err_mipi_dsi_host;
+	}
+
+	return 0;
+
+err_mipi_dsi_host:
+	mipi_dsi_host_unregister(&dsi->dsi_host);
+err_cleanup:
+	drm_encoder_cleanup(&dsi->encoder);
+	drm_connector_cleanup(&dsi->connector);
+err_pllref:
+	clk_disable_unprepare(dsi->pllref_clk);
+	return ret;
+}
+
+static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
+			       void *data)
+{
+	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
+
+	mipi_dsi_host_unregister(&dsi->dsi_host);
+	pm_runtime_disable(dev);
+	if (dsi->slave)
+		pm_runtime_disable(dsi->slave->dev);
+	clk_disable_unprepare(dsi->pllref_clk);
+}
+
+static const struct component_ops dw_mipi_dsi_ops = {
+	.bind	= dw_mipi_dsi_bind,
+	.unbind	= dw_mipi_dsi_unbind,
+};
+
+static int dw_mipi_dsi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
 	const struct of_device_id *of_id =
 			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;
+	struct reset_control *apb_rst;
 	int ret;
 
 	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
@@ -1214,6 +1419,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
 	dsi->dev = dev;
 	dsi->pdata = pdata;
 	dsi->dpms_mode = DRM_MODE_DPMS_OFF;
+	dev_set_drvdata(dev, dsi);
 
 	ret = rockchip_mipi_parse_dt(dsi);
 	if (ret)
@@ -1288,63 +1494,6 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
 		}
 	}
 
-	ret = clk_prepare_enable(dsi->pllref_clk);
-	if (ret) {
-		dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);
-		return ret;
-	}
-
-	ret = dw_mipi_dsi_register(drm, dsi);
-	if (ret) {
-		dev_err(dev, "Failed to register mipi_dsi: %d\n", ret);
-		goto err_pllref;
-	}
-
-	pm_runtime_enable(dev);
-
-	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;
-	dsi->dsi_host.dev = dev;
-	ret = mipi_dsi_host_register(&dsi->dsi_host);
-	if (ret) {
-		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
-		goto err_cleanup;
-	}
-
-	if (!dsi->panel) {
-		ret = -EPROBE_DEFER;
-		goto err_mipi_dsi_host;
-	}
-
-	dev_set_drvdata(dev, dsi);
-	return 0;
-
-err_mipi_dsi_host:
-	mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
-	drm_encoder_cleanup(&dsi->encoder);
-	drm_connector_cleanup(&dsi->connector);
-err_pllref:
-	clk_disable_unprepare(dsi->pllref_clk);
-	return ret;
-}
-
-static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
-			       void *data)
-{
-	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
-
-	mipi_dsi_host_unregister(&dsi->dsi_host);
-	pm_runtime_disable(dev);
-	clk_disable_unprepare(dsi->pllref_clk);
-}
-
-static const struct component_ops dw_mipi_dsi_ops = {
-	.bind	= dw_mipi_dsi_bind,
-	.unbind	= dw_mipi_dsi_unbind,
-};
-
-static int dw_mipi_dsi_probe(struct platform_device *pdev)
-{
 	return component_add(&pdev->dev, &dw_mipi_dsi_ops);
 }
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index c7e96b8..51ad1c2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -36,6 +36,7 @@ struct rockchip_crtc_state {
 	struct drm_crtc_state base;
 	int output_type;
 	int output_mode;
+	int output_flags;
 };
 #define to_rockchip_crtc_state(s) \
 		container_of(s, struct rockchip_crtc_state, base)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index bf9ed0e..cb40cdd 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -917,6 +917,8 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 	case DRM_MODE_CONNECTOR_DSI:
 		VOP_REG_SET(vop, output, mipi_pin_pol, pin_pol);
 		VOP_REG_SET(vop, output, mipi_en, 1);
+		VOP_REG_SET(vop, output, mipi_dual_channel_en,
+			!!(s->output_flags & ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL));
 		break;
 	case DRM_MODE_CONNECTOR_DisplayPort:
 		pin_pol &= ~BIT(DCLK_INVERT);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 56bbd2e..d184531 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -60,6 +60,7 @@ struct vop_output {
 	struct vop_reg edp_en;
 	struct vop_reg hdmi_en;
 	struct vop_reg mipi_en;
+	struct vop_reg mipi_dual_channel_en;
 	struct vop_reg rgb_en;
 };
 
@@ -212,6 +213,8 @@ struct vop_data {
 /* for use special outface */
 #define ROCKCHIP_OUT_MODE_AAAA	15
 
+#define ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL	BIT(0)
+
 enum alpha_mode {
 	ALPHA_STRAIGHT,
 	ALPHA_INVERSE,
-- 
1.9.1

[PATCH 3/7] dt-bindings: add the rockchip,dual-channel for dw-mipi-dsi

[Nickey Yang]

Configure dsi slave channel when driving a panel
which needs 2 DSI links.

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt       | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
index 6bb59ab..e13e1a3 100644
--- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
@@ -19,6 +19,8 @@ Optional properties:
 - power-domains: a phandle to mipi dsi power domain node.
 - resets: list of phandle + reset specifier pairs, as described in [3].
 - reset-names: string reset name, must be "apb".
+- rockchip,dual-channel: configure dsi slave channel when driving a panel
+  which needs 2 DSI links.
 
 [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
 [2] Documentation/devicetree/bindings/media/video-interfaces.txt
-- 
1.9.1

[PATCH 4/7] drm/rockchip/dsi: correct phy parameter setting

[Nickey Yang]

As MIPI PHY document show, icpctrl<3..0> and lpfctrl<5..0>
should depend on frequency,so fix it.

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 70 +++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 9265b7f..d5250e8 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -228,10 +228,10 @@
 #define VCO_IN_CAP_CON_HIGH	(0x2 << 1)
 #define REF_BIAS_CUR_SEL	BIT(0)
 
-#define CP_CURRENT_3MA		BIT(3)
+#define CP_CURRENT_SEL(val)	((val) & 0xf)
 #define CP_PROGRAM_EN		BIT(7)
 #define LPF_PROGRAM_EN		BIT(6)
-#define LPF_RESISTORS_20_KOHM	0
+#define LPF_RESISTORS_SEL(val)	((val) & 0x3f)
 
 #define HSFREQRANGE_SEL(val)	(((val) & 0x3f) << 1)
 
@@ -340,32 +340,44 @@ enum dw_mipi_dsi_mode {
 	DW_MIPI_DSI_VID_MODE,
 };
 
-struct dphy_pll_testdin_map {
+struct dphy_pll_parameter_map {
 	unsigned int max_mbps;
-	u8 testdin;
+	u8 hsfreqrange;
+	u8 icpctrl;
+	u8 lpfctrl;
 };
 
 /* The table is based on 27MHz DPHY pll reference clock. */
-static const struct dphy_pll_testdin_map dptdin_map[] = {
-	{  90, 0x00}, { 100, 0x10}, { 110, 0x20}, { 130, 0x01},
-	{ 140, 0x11}, { 150, 0x21}, { 170, 0x02}, { 180, 0x12},
-	{ 200, 0x22}, { 220, 0x03}, { 240, 0x13}, { 250, 0x23},
-	{ 270, 0x04}, { 300, 0x14}, { 330, 0x05}, { 360, 0x15},
-	{ 400, 0x25}, { 450, 0x06}, { 500, 0x16}, { 550, 0x07},
-	{ 600, 0x17}, { 650, 0x08}, { 700, 0x18}, { 750, 0x09},
-	{ 800, 0x19}, { 850, 0x29}, { 900, 0x39}, { 950, 0x0a},
-	{1000, 0x1a}, {1050, 0x2a}, {1100, 0x3a}, {1150, 0x0b},
-	{1200, 0x1b}, {1250, 0x2b}, {1300, 0x3b}, {1350, 0x0c},
-	{1400, 0x1c}, {1450, 0x2c}, {1500, 0x3c}
+static const struct dphy_pll_parameter_map dppa_map[] = {
+	{  90, 0x00, 0x1 ,0x02}, { 100, 0x10 ,0x1, 0x02},
+	{ 110, 0x20, 0x1, 0x02}, { 130, 0x01, 0x1, 0x01},
+	{ 140, 0x11, 0x1, 0x01}, { 150, 0x21, 0x1, 0x01},
+	{ 170, 0x02, 0x9, 0x02}, { 180, 0x12, 0x9, 0x02},
+	{ 200, 0x22, 0x9, 0x02}, { 220, 0x03, 0x2, 0x02},
+	{ 240, 0x13, 0x2, 0x02}, { 250, 0x23, 0x2, 0x02},
+	{ 270, 0x04, 0x9, 0x04}, { 300, 0x14, 0x9, 0x04},
+	{ 330, 0x05, 0x1, 0x01}, { 360, 0x15, 0x1, 0x01},
+	{ 400, 0x25, 0x1, 0x01}, { 450, 0x06, 0x6, 0x04},
+	{ 500, 0x16, 0x6, 0x04}, { 550, 0x07, 0x6, 0x08},
+	{ 600, 0x17, 0x6, 0x08}, { 650, 0x08, 0x6, 0x04},
+	{ 700, 0x18, 0x6, 0x04}, { 750, 0x09, 0x6, 0x04},
+	{ 800, 0x19, 0x6, 0x04}, { 850, 0x29, 0x6, 0x04},
+	{ 900, 0x39, 0x6, 0x04}, { 950, 0x0a, 0xb, 0x10},
+	{1000, 0x1a, 0xb, 0x10}, {1050, 0x2a, 0xb, 0x10},
+	{1100, 0x3a, 0xb, 0x10}, {1150, 0x0b, 0xb, 0x08},
+	{1200, 0x1b, 0xb, 0x08}, {1250, 0x2b, 0xb, 0x08},
+	{1300, 0x3b, 0xb, 0x08}, {1350, 0x0c, 0xb, 0x08},
+	{1400, 0x1c, 0xb, 0x08}, {1450, 0x2c, 0xb, 0x08},
+	{1500, 0x3c, 0xb, 0x08}
 };
 
-static int max_mbps_to_testdin(unsigned int max_mbps)
+static int max_mbps_to_parameter(unsigned int max_mbps)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(dptdin_map); i++)
-		if (dptdin_map[i].max_mbps > max_mbps)
-			return dptdin_map[i].testdin;
+	for (i = 0; i < ARRAY_SIZE(dppa_map); i++)
+		if (dppa_map[i].max_mbps > max_mbps)
+			return i;
 
 	return -EINVAL;
 }
@@ -447,16 +459,16 @@ static inline unsigned int ns2ui(struct dw_mipi_dsi *dsi, int ns)
 
 static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 {
-	int ret, testdin, vco, val;
+	int ret, i, vco, val;
 
 	vco = (dsi->lane_mbps < 200) ? 0 : (dsi->lane_mbps + 100) / 200;
 
-	testdin = max_mbps_to_testdin(dsi->lane_mbps);
-	if (testdin < 0) {
+	i = max_mbps_to_parameter(dsi->lane_mbps);
+	if (i < 0) {
 		dev_err(dsi->dev,
-			"failed to get testdin for %dmbps lane clock\n",
+			"failed to get parameter for %dmbps lane clock\n",
 			dsi->lane_mbps);
-		return testdin;
+		return i;
 	}
 
 	/* Start by clearing PHY state */
@@ -475,12 +487,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 					 VCO_IN_CAP_CON_LOW |
 					 REF_BIAS_CUR_SEL);
 
-	dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_3MA);
+	dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_SEL(dppa_map[i].icpctrl));
 	dw_mipi_dsi_phy_write(dsi, 0x12, CP_PROGRAM_EN | LPF_PROGRAM_EN |
-					 LPF_RESISTORS_20_KOHM);
-
-	dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin));
-
+					 LPF_RESISTORS_SEL(dppa_map[i].lpfctrl));
+	dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(dppa_map[i].hsfreqrange));
 	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);
@@ -547,7 +557,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 {
 	unsigned long mpclk, tmp;
 	unsigned int target_mbps = 1000;
-	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
+	unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps;
 	int bpp;
 	int lanes = dsi->lanes;
 	unsigned long best_freq = 0;
-- 
1.9.1

[PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock

[Nickey Yang]

clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index d79e9b3..6aa43fd 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1629,7 +1629,7 @@
 		compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi";
 		reg = <0x0 0xff960000 0x0 0x8000>;
 		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
+		clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI0>,
 			 <&cru SCLK_DPHY_TX0_CFG>;
 		clock-names = "ref", "pclk", "phy_cfg";
 		power-domains = <&power RK3399_PD_VIO>;
-- 
1.9.1

Re: [PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock

[Heiko Stübner]

Hi Nickey,

Am Montag, 18. September 2017, 17:05:37 CEST schrieb Nickey Yang:
> clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll

please try to be a bit more verbose in your commit messages :-) .

It looks to me, like this patch does not depend on the other ones and
I can just pick it directly. Correct?


Heiko

> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index d79e9b3..6aa43fd 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -1629,7 +1629,7 @@
>  		compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi";
>  		reg = <0x0 0xff960000 0x0 0x8000>;
>  		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH 0>;
> -		clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
> +		clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI0>,
>  			 <&cru SCLK_DPHY_TX0_CFG>;
>  		clock-names = "ref", "pclk", "phy_cfg";
>  		power-domains = <&power RK3399_PD_VIO>;

Re: [PATCH 3/7] dt-bindings: add the rockchip,dual-channel for dw-mipi-dsi

[Brian Norris]

Hi,

On Mon, Sep 18, 2017 at 05:05:35PM +0800, Nickey Yang wrote:
> Configure dsi slave channel when driving a panel
> which needs 2 DSI links.
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt       | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
> index 6bb59ab..e13e1a3 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
> @@ -19,6 +19,8 @@ Optional properties:
>  - power-domains: a phandle to mipi dsi power domain node.
>  - resets: list of phandle + reset specifier pairs, as described in [3].
>  - reset-names: string reset name, must be "apb".
> +- rockchip,dual-channel: configure dsi slave channel when driving a panel

The wording "configure dsi slave channel" doesn't really tell you
exactly what this *is*; in fact, it's more like a description of the SW
than the HW (DT is supposed to describe HW).

You probably want something like "phandle to a 2nd DSI channel; useful
as a slave channel when driving a panel which needs 2 DSI links".

Brian

> +  which needs 2 DSI links.
>  
>  [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>  [2] Documentation/devicetree/bindings/media/video-interfaces.txt
> -- 
> 1.9.1
> 
> 

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[Brian Norris]

Hi Nickey,

On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> This patch correct Feedback divider setting:
> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> 2、Due to the use of a "by 2 pre-scaler," the range of the
> feedback multiplication Feedback divider is limited to even
> division numbers, and Feedback divider must be greater than
> 12, less than 1000.
> 3、Make the previously configured Feedback divider(LSB)
> factors effective
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 9a20b9d..52698b7 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -228,7 +228,7 @@
>  #define LOW_PROGRAM_EN		0
>  #define HIGH_PROGRAM_EN		BIT(7)
>  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
>  #define PLL_LOOP_DIV_EN		BIT(5)
>  #define PLL_INPUT_DIV_EN	BIT(4)
>  
> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_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);
> @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(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;
> -	unsigned int m = 1, n = 1, target_mbps = 1000;
> +	unsigned long mpclk, tmp;
> +	unsigned int target_mbps = 1000;
>  	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
>  	int bpp;
> +	unsigned long best_freq = 0;
> +	unsigned long fvco_min, fvco_max, fin ,fout;
> +	unsigned int min_prediv, max_prediv;
> +	unsigned int _prediv, uninitialized_var(best_prediv);
> +	unsigned long _fbdiv, uninitialized_var(best_fbdiv);
> +	unsigned long min_delta = UINT_MAX;
>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  	if (bpp < 0) {
> @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  			dev_err(dsi->dev, "DPHY clock frequency is out of range\n");
>  	}
>  
> -	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
> -	tmp = pllref;
> -
> -	/*
> -	 * The limits on the PLL divisor are:
> -	 *
> -	 *	5MHz <= (pllref / n) <= 40MHz
> -	 *
> -	 * we walk over these values in descreasing order so that if we hit
> -	 * an exact match for target_mbps it is more likely that "m" will be
> -	 * even.
> -	 *
> -	 * TODO: ensure that "m" is even after this loop.
> -	 */
> -	for (i = pllref / 5; i > (pllref / 40); i--) {
> -		pre = pllref / i;
> -		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
> -			tmp = target_mbps % pre;
> -			n = i;
> -			m = target_mbps / pre;
> +	fin = clk_get_rate(dsi->pllref_clk);
> +	fout = target_mbps * USEC_PER_SEC;
> +
> +	/* constraint: 5Mhz < Fref / N < 40MHz */
> +	min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
> +	max_prediv = fin / (5 * USEC_PER_SEC);
> +
> +	/* constraint: 80MHz < Fvco < 1500Mhz */
> +	fvco_min = 80 * USEC_PER_SEC;
> +	fvco_max = 1500 * USEC_PER_SEC;
> +
> +	for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
> +		u32 delta;
> +		/* Fvco = Fref * M / N */
> +		tmp = fout * _prediv;
> +		do_div(tmp, fin);
> +		_fbdiv = tmp;
> +		/*
> +		 * Due to the use of a "by 2 pre-scaler," the range of the
> +		 * feedback multiplication value M is limited to even division
> +		 * numbers, and m must be greater than 12, less than 1000.
> +		 */
> +		if (_fbdiv < 12 || _fbdiv > 1000)
> +			continue;
> +
> +		if (_fbdiv % 2)
> +			++_fbdiv;
> +
> +		tmp = (u64)_fbdiv * fin;

Are you relying on this being able to handle >32-bit integers? They you
should declare 'tmp' as a 64-bit type; 'unsigned long' isn't guaranteed
on 32-bit architectures. Try 'u64'?

Brian

> +		do_div(tmp, _prediv);
> +		if (tmp < fvco_min || tmp > fvco_max)
> +			continue;
> +
> +		delta = abs(fout - tmp);
> +		if (delta < min_delta) {
> +			best_prediv = _prediv;
> +			best_fbdiv = _fbdiv;
> +			min_delta = delta;
> +			best_freq = tmp;
>  		}
> -		if (tmp == 0)
> -			break;
>  	}
>  
> -	dsi->lane_mbps = pllref / n * m;
> -	dsi->input_div = n;
> -	dsi->feedback_div = m;
> +	if (best_freq) {
> +		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
> +		dsi->input_div = best_prediv;
> +		dsi->feedback_div = best_fbdiv;
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.9.1
> 
> 

Re: [PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock

[Nickey Yang]

[-- Attachment #1.1: Type: text/plain, Size: 1333 bytes --]

Hi Heiko,


On 2017年09月18日 19:31, Heiko Stübner wrote:
> Hi Nickey,
>
> Am Montag, 18. September 2017, 17:05:37 CEST schrieb Nickey Yang:
>> clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll
> please try to be a bit more verbose in your commit messages :-) .
>
> It looks to me, like this patch does not depend on the other ones and
> I can just pick it directly. Correct?
Yes, this patch is independent,you can pick it directly.
>
> Heiko
>
>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index d79e9b3..6aa43fd 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> @@ -1629,7 +1629,7 @@
>>   		compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi";
>>   		reg = <0x0 0xff960000 0x0 0x8000>;
>>   		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH 0>;
>> -		clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
>> +		clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI0>,
>>   			 <&cru SCLK_DPHY_TX0_CFG>;
>>   		clock-names = "ref", "pclk", "phy_cfg";
>>   		power-domains = <&power RK3399_PD_VIO>;
>
>
>
>


[-- Attachment #1.2: Type: text/html, Size: 2816 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[Sean Paul]

On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> This patch correct Feedback divider setting:
> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> 2、Due to the use of a "by 2 pre-scaler," the range of the
> feedback multiplication Feedback divider is limited to even
> division numbers, and Feedback divider must be greater than
> 12, less than 1000.
> 3、Make the previously configured Feedback divider(LSB)
> factors effective
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 9a20b9d..52698b7 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -228,7 +228,7 @@
>  #define LOW_PROGRAM_EN		0
>  #define HIGH_PROGRAM_EN		BIT(7)
>  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
>  #define PLL_LOOP_DIV_EN		BIT(5)
>  #define PLL_INPUT_DIV_EN	BIT(4)
>  
> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);

You do the same write 2 lines down. Are both needed? It would be nice if the
register names were also defined, so this is easier to read.

>  	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);
> @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(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;
> -	unsigned int m = 1, n = 1, target_mbps = 1000;
> +	unsigned long mpclk, tmp;
> +	unsigned int target_mbps = 1000;
>  	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
>  	int bpp;
> +	unsigned long best_freq = 0;
> +	unsigned long fvco_min, fvco_max, fin ,fout;
> +	unsigned int min_prediv, max_prediv;
> +	unsigned int _prediv, uninitialized_var(best_prediv);
> +	unsigned long _fbdiv, uninitialized_var(best_fbdiv);
> +	unsigned long min_delta = UINT_MAX;

Once you explicitly type these, make sure you use the appropriate _MAX value
(ie: U64_MAX)

>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  	if (bpp < 0) {
> @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  			dev_err(dsi->dev, "DPHY clock frequency is out of range\n");
>  	}
>  
> -	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
> -	tmp = pllref;
> -
> -	/*
> -	 * The limits on the PLL divisor are:
> -	 *
> -	 *	5MHz <= (pllref / n) <= 40MHz
> -	 *
> -	 * we walk over these values in descreasing order so that if we hit
> -	 * an exact match for target_mbps it is more likely that "m" will be
> -	 * even.
> -	 *
> -	 * TODO: ensure that "m" is even after this loop.
> -	 */
> -	for (i = pllref / 5; i > (pllref / 40); i--) {
> -		pre = pllref / i;
> -		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
> -			tmp = target_mbps % pre;
> -			n = i;
> -			m = target_mbps / pre;
> +	fin = clk_get_rate(dsi->pllref_clk);
> +	fout = target_mbps * USEC_PER_SEC;
> +
> +	/* constraint: 5Mhz < Fref / N < 40MHz */
> +	min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
> +	max_prediv = fin / (5 * USEC_PER_SEC);
> +
> +	/* constraint: 80MHz < Fvco < 1500Mhz */
> +	fvco_min = 80 * USEC_PER_SEC;
> +	fvco_max = 1500 * USEC_PER_SEC;
> +
> +	for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
> +		u32 delta;
> +		/* Fvco = Fref * M / N */
> +		tmp = fout * _prediv;
> +		do_div(tmp, fin);
> +		_fbdiv = tmp;

Why use tmp at all? Can't you just use _fbdiv directly in do_div?

> +		/*
> +		 * Due to the use of a "by 2 pre-scaler," the range of the
> +		 * feedback multiplication value M is limited to even division
> +		 * numbers, and m must be greater than 12, less than 1000.
> +		 */
> +		if (_fbdiv < 12 || _fbdiv > 1000)
> +			continue;
> +
> +		if (_fbdiv % 2)
> +			++_fbdiv;

You can reduce this down to:
_fbdiv += _fbdiv % 2;

> +
> +		tmp = (u64)_fbdiv * fin;

I'll echo Brian's concerns on type mixing here. Please be explicit with size
when you declare your variables.

> +		do_div(tmp, _prediv);
> +		if (tmp < fvco_min || tmp > fvco_max)
> +			continue;
> +
> +		delta = abs(fout - tmp);
> +		if (delta < min_delta) {
> +			best_prediv = _prediv;
> +			best_fbdiv = _fbdiv;
> +			min_delta = delta;
> +			best_freq = tmp;
>  		}
> -		if (tmp == 0)
> -			break;
>  	}
>  
> -	dsi->lane_mbps = pllref / n * m;
> -	dsi->input_div = n;
> -	dsi->feedback_div = m;
> +	if (best_freq) {

Should you return an error or log something if best_freq is not found?

> +		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
> +		dsi->input_div = best_prediv;
> +		dsi->feedback_div = best_fbdiv;
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.9.1
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[Brian Norris]

Hi Sean,

On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> > This patch correct Feedback divider setting:
> > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> > 2、Due to the use of a "by 2 pre-scaler," the range of the
> > feedback multiplication Feedback divider is limited to even
> > division numbers, and Feedback divider must be greater than
> > 12, less than 1000.
> > 3、Make the previously configured Feedback divider(LSB)
> > factors effective
> > 
> > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> > ---
> >  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
> >  1 file changed, 54 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > index 9a20b9d..52698b7 100644
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > @@ -228,7 +228,7 @@
> >  #define LOW_PROGRAM_EN		0
> >  #define HIGH_PROGRAM_EN		BIT(7)
> >  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> > -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> > +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
> >  #define PLL_LOOP_DIV_EN		BIT(5)
> >  #define PLL_INPUT_DIV_EN	BIT(4)
> >  
> > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
> >  	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> 
> You do the same write 2 lines down. Are both needed? It would be nice if the
> register names were also defined, so this is easier to read.

If I'm reading correctly, I think this is what Nickey meant by:

"3、Make the previously configured Feedback divider(LSB)
factors effective"

. My reading of the databook is that this step finalizes the previous
two writes (to test code 0x17 and 0x18).

Given this was buggy (?) previously, it does seem like having some extra
language to document this could help. Register names (or "test codes",
per the docs?) could help, but additionally, maybe a few more comments.

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

[...]

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

Re: [PATCH 2/7] drm/rockchip/dsi: add dual mipi channel support

[Sean Paul]

On Mon, Sep 18, 2017 at 05:05:34PM +0800, Nickey Yang wrote:

Hi Nickey,
You're adding a substantial new feature to the driver. You should add a commit
message that describes what you're adding, why you're adding it, and how it is
done.

> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c      | 353 ++++++++++++++++++++--------
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   2 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   3 +
>  4 files changed, 257 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 52698b7..9265b7f 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -42,6 +42,17 @@
>  #define RK3399_GRF_SOC_CON22		0x6258
>  #define RK3399_GRF_DSI_MODE		0xffff0000
>  
> +/* disable turndisable, forcetxstopmode, forcerxmode, enable */
> +#define RK3399_GRF_SOC_CON23		0x625c
> +#define RK3399_GRF_DSI1_MODE1		0xffff0000
> +#define RK3399_GRF_DSI1_ENABLE		0x000f000f

Please break these out into separate defines so we know which bit is
responsible for turning on/off each feature.

> +/* disable basedir and enable clk*/
> +#define RK3399_GRF_SOC_CON24		0x6260
> +#define RK3399_TXRX_MASTERSLAVEZ	BIT(7)
> +#define RK3399_TXRX_ENABLECLK		BIT(6)
> +#define RK3399_TXRX_BASEDIR		BIT(5)
> +#define RK3399_GRF_DSI1_MODE2		0x00e00080

Same comment here.

> +
>  #define DSI_VERSION			0x00
>  #define DSI_PWR_UP			0x04
>  #define RESET				0
> @@ -282,6 +293,12 @@ struct dw_mipi_dsi_plat_data {
>  	u32 grf_switch_reg;
>  	u32 grf_dsi0_mode;
>  	u32 grf_dsi0_mode_reg;
> +	u32 grf_dsi1_mode;
> +	u32 grf_dsi1_mode_reg1;
> +	u32 dsi1_basedir;
> +	u32 dsi1_masterslavez;
> +	u32 dsi1_enableclk;
> +	u32 grf_dsi1_mode_reg2;
>  	unsigned int flags;
>  	unsigned int max_data_lanes;
>  };
> @@ -300,6 +317,12 @@ struct dw_mipi_dsi {
>  	struct clk *pclk;
>  	struct clk *phy_cfg_clk;
>  
> +	/* dual-channel */
> +	struct dw_mipi_dsi *master;
> +	struct dw_mipi_dsi *slave;
> +	struct device_node *panel_node;
> +	int id;
> +
>  	int dpms_mode;
>  	unsigned int lane_mbps; /* per lane */
>  	u32 channel;
> @@ -526,6 +549,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  	unsigned int target_mbps = 1000;
>  	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
>  	int bpp;
> +	int lanes = dsi->lanes;
>  	unsigned long best_freq = 0;
>  	unsigned long fvco_min, fvco_max, fin ,fout;
>  	unsigned int min_prediv, max_prediv;
> @@ -540,10 +564,13 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  		return bpp;
>  	}
>  
> +	if (dsi->slave || dsi->master)
> +		lanes = dsi->lanes * 2;
> +
>  	mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC);
>  	if (mpclk) {
>  		/* take 1 / 0.8, since mbps must big than bandwidth of RGB */
> -		tmp = mpclk * (bpp / dsi->lanes) * 10 / 8;
> +		tmp = mpclk * (bpp / lanes) * 10 / 8;
>  		if (tmp < max_mbps)
>  			target_mbps = tmp;
>  		else
> @@ -605,17 +632,26 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
>  				   struct mipi_dsi_device *device)
>  {
>  	struct dw_mipi_dsi *dsi = host_to_dsi(host);
> +	int lanes = dsi->slave ? device->lanes / 2 : device->lanes;
>  
> -	if (device->lanes > dsi->pdata->max_data_lanes) {
> +	if (lanes > dsi->pdata->max_data_lanes) {
>  		dev_err(dsi->dev, "the number of data lanes(%u) is too many\n",
> -			device->lanes);
> +			lanes);
>  		return -EINVAL;
>  	}
>  
> -	dsi->lanes = device->lanes;
> +	dsi->lanes = lanes;
>  	dsi->channel = device->channel;
>  	dsi->format = device->format;
>  	dsi->mode_flags = device->mode_flags;
> +
> +	if (dsi->slave) {
> +		dsi->slave->lanes = lanes;
> +		dsi->slave->channel = device->channel;
> +		dsi->slave->format = device->format;
> +		dsi->slave->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);
> @@ -745,15 +781,22 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>  	int ret;
>  
>  	dw_mipi_message_config(dsi, msg);
> +	if (dsi->slave)
> +		dw_mipi_message_config(dsi->slave, msg);
>  
>  	switch (msg->type) {
>  	case MIPI_DSI_DCS_SHORT_WRITE:
>  	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
>  	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
>  		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
> +		if (dsi->slave)
> +			ret = dw_mipi_dsi_dcs_short_write(dsi->slave, msg);
>  		break;
>  	case MIPI_DSI_DCS_LONG_WRITE:
>  		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
> +		if (dsi->slave)
> +			ret = dw_mipi_dsi_dcs_long_write(dsi->slave, msg);
>  		break;
>  	default:
>  		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
> @@ -827,6 +870,64 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
>  		  TX_ESC_CLK_DIVIDSION(esc_clk_division));
>  }
>  
> +static void rockchip_dsi_grf_config(struct dw_mipi_dsi *dsi, int vop_id)
> +{
> +	const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata;
> +	int val = 0;
> +	int ret;
> +
> +	/*
> +	 * For the RK3399, the clk of grf must be enabled before writing grf
> +	 * register. And for RK3288 or other soc, this grf_clk must be NULL,
> +	 * the clk_prepare_enable return true directly.
> +	 */
> +	ret = clk_prepare_enable(dsi->grf_clk);
> +	if (ret) {
> +		dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret);
> +		return;
> +	}
> +
> +	if (dsi->slave) {
> +		if (vop_id)
> +			val = pdata->dsi0_en_bit |
> +			      (pdata->dsi0_en_bit << 16) |
> +			      pdata->dsi1_en_bit |
> +			      (pdata->dsi1_en_bit << 16);
> +		else
> +			val = (pdata->dsi0_en_bit << 16) |
> +			      (pdata->dsi1_en_bit << 16);
> +
> +		regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
> +
> +		if (pdata->grf_dsi0_mode_reg)
> +			regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
> +				     pdata->grf_dsi0_mode);
> +		if (pdata->grf_dsi1_mode_reg1)
> +			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
> +				     pdata->grf_dsi1_mode);
> +		if (pdata->grf_dsi1_mode_reg2)
> +			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2,
> +				     RK3399_GRF_DSI1_MODE2);
> +		if (pdata->grf_dsi1_mode_reg1)
> +			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
> +				     RK3399_GRF_DSI1_ENABLE);
> +	} else {
> +		if (vop_id)
> +			val = pdata->dsi0_en_bit |
> +			      (pdata->dsi0_en_bit << 16);
> +		else
> +			val = pdata->dsi0_en_bit << 16;
> +
> +		regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
> +
> +		if (pdata->grf_dsi0_mode_reg)
> +			regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
> +				     pdata->grf_dsi0_mode);
> +	}

You have some duplication here, how about:

        val = pdata->dsi0_en_bit << 16;
        if (dsi->slave)
                val |= pdata->dsi1_en_bit << 16
        if (vop_id) {
                val |= pdata->dsi0_en_bit;
                if (dsi->slave)
                        val |= pdata->dsi1_en_bit;
        }
        regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);

        if (pdata->grf_dsi0_mode_reg)
                regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
                             pdata->grf_dsi0_mode);

        if (dsi->slave) {
                if (pdata->grf_dsi1_mode_reg1)
                        regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
                                     pdata->grf_dsi1_mode);
		if (pdata->grf_dsi1_mode_reg2)
			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2,
				     RK3399_GRF_DSI1_MODE2);
		if (pdata->grf_dsi1_mode_reg1)
			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
				     RK3399_GRF_DSI1_ENABLE);
        }

> +
> +	clk_disable_unprepare(dsi->grf_clk);
> +}
> +
>  static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
>  				   struct drm_display_mode *mode)
>  {
> @@ -867,7 +968,14 @@ static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
>  static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
>  					    struct drm_display_mode *mode)
>  {
> -	dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay));
> +	int pkt_size;
> +
> +	if (dsi->slave || dsi->master)
> +		pkt_size = VID_PKT_SIZE(mode->hdisplay / 2 + 4);

Why add 4? Please either add a comment, pull out into a well-named define, or
both :)

> +	else
> +		pkt_size = VID_PKT_SIZE(mode->hdisplay);
> +
> +	dsi_write(dsi, DSI_VID_PKT_SIZE, pkt_size);
>  }
>  
>  static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
> @@ -971,24 +1079,24 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
>  
>  	dw_mipi_dsi_disable(dsi);
>  	pm_runtime_put(dsi->dev);
> +
> +	if (dsi->slave) {
> +		if (clk_prepare_enable(dsi->slave->pclk)) {
> +			dev_err(dsi->slave->dev, "%s: Failed to enable pclk\n", __func__);

Rockchip driver has been moved to using the DRM_DEV_* log messages, please
change all instances of dev_* in the patch.

> +			return;

You still need to disable_unprepare dsi->pclk as well. Consider adding a label
'out' above the disable_unprepare below and jump to it from here.

> +		}
> +		dw_mipi_dsi_disable(dsi->slave);
> +		pm_runtime_put(dsi->slave->dev);
> +		clk_disable_unprepare(dsi->slave->pclk);
> +	}
> +
>  	clk_disable_unprepare(dsi->pclk);
>  	dsi->dpms_mode = DRM_MODE_DPMS_OFF;
>  }
>  
> -static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
> +static void dw_mipi_dsi_enable(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode)
>  {
> -	struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
> -	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
> -	const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata;
> -	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, mode);
> -	if (ret < 0)
> -		return;
> -
> -	if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
> +	if (dw_mipi_dsi_get_lane_bps(dsi, mode) < 0)

Can you please log an error with the return code here? Silently failing is bound
to cause frustration.

>  		return;
>  
>  	if (clk_prepare_enable(dsi->pclk)) {
> @@ -1009,43 +1117,42 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
>  	dw_mipi_dsi_dphy_interface_config(dsi);
>  	dw_mipi_dsi_clear_err(dsi);
>  
> -	/*
> -	 * For the RK3399, the clk of grf must be enabled before writing grf
> -	 * register. And for RK3288 or other soc, this grf_clk must be NULL,
> -	 * the clk_prepare_enable return true directly.
> -	 */
> -	ret = clk_prepare_enable(dsi->grf_clk);
> -	if (ret) {
> -		dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret);
> -		return;
> -	}
> -
> -	if (pdata->grf_dsi0_mode_reg)
> -		regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,
> -			     pdata->grf_dsi0_mode);
> -
>  	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);
> +}
> +
> +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 = &encoder->crtc->state->adjusted_mode;
> +	int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
> +
> +	if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
> +		return;
> +
> +	rockchip_dsi_grf_config(dsi, mux);
> +	dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG");
> +
> +	dw_mipi_dsi_enable(dsi, mode);
> +	if (dsi->slave)
> +		dw_mipi_dsi_enable(dsi->slave, 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);
> +	if (dsi->slave)
> +		dw_mipi_dsi_set_mode(dsi->slave, DW_MIPI_DSI_VID_MODE);
> +
>  	drm_panel_enable(dsi->panel);
>  
>  	clk_disable_unprepare(dsi->pclk);
> +	if (dsi->slave)
> +		clk_disable_unprepare(dsi->slave->pclk);
>  
> -	if (mux)
> -		val = pdata->dsi0_en_bit | (pdata->dsi0_en_bit << 16);
> -	else
> -		val = pdata->dsi0_en_bit << 16;
> -
> -	regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
> -	dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG");
>  	dsi->dpms_mode = DRM_MODE_DPMS_ON;
> -
> -	clk_disable_unprepare(dsi->grf_clk);
>  }
>  
>  static int
> @@ -1073,6 +1180,9 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
>  
>  	s->output_type = DRM_MODE_CONNECTOR_DSI;
>  
> +	if (dsi->slave)
> +		s->output_flags = ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL;
> +
>  	return 0;
>  }
>  
> @@ -1178,6 +1288,12 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi)
>  	.grf_switch_reg = RK3399_GRF_SOC_CON20,
>  	.grf_dsi0_mode = RK3399_GRF_DSI_MODE,
>  	.grf_dsi0_mode_reg = RK3399_GRF_SOC_CON22,
> +	.grf_dsi1_mode = RK3399_GRF_DSI1_MODE1,
> +	.grf_dsi1_mode_reg1 = RK3399_GRF_SOC_CON23,
> +	.dsi1_basedir = RK3399_TXRX_BASEDIR,
> +	.dsi1_masterslavez = RK3399_TXRX_MASTERSLAVEZ,
> +	.dsi1_enableclk = RK3399_TXRX_ENABLECLK,
> +	.grf_dsi1_mode_reg2 = RK3399_GRF_SOC_CON24,
>  	.flags = DW_MIPI_NEEDS_PHY_CFG_CLK | DW_MIPI_NEEDS_GRF_CLK,
>  	.max_data_lanes = 4,
>  };
> @@ -1194,17 +1310,106 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi)
>  };
>  MODULE_DEVICE_TABLE(of, dw_mipi_dsi_dt_ids);
>  
> +
> +static int rockchip_dsi_dual_channel_probe(struct dw_mipi_dsi *dsi)
> +{
> +	struct device_node *np;
> +	struct platform_device *secondary;
> +
> +	np = of_parse_phandle(dsi->dev->of_node, "rockchip,dual-channel", 0);
> +	if (np) {
> +		secondary = of_find_device_by_node(np);
> +		dsi->slave = platform_get_drvdata(secondary);
> +		of_node_put(np);
> +
> +		if (!dsi->slave)
> +			return -EPROBE_DEFER;
> +
> +		dsi->slave->master = dsi;
> +	}
> +
> +	return 0;
> +}
> +
>  static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
>  			    void *data)
>  {
> +	struct drm_device *drm = data;
> +	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = rockchip_dsi_dual_channel_probe(dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dsi->pllref_clk);
> +	if (ret) {
> +		dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);
> +		return ret;
> +	}
> +
> +	pm_runtime_enable(dev);
> +
> +	if (dsi->master)
> +		return 0;
> +
> +	ret = dw_mipi_dsi_register(drm, dsi);
> +	if (ret) {
> +		dev_err(dev, "Failed to register mipi_dsi: %d\n", ret);
> +		goto err_pllref;
> +	}
> +
> +	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;
> +	dsi->dsi_host.dev = dev;
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret) {
> +		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> +		goto err_cleanup;
> +	}
> +
> +	if (!dsi->panel) {
> +		ret = -EPROBE_DEFER;
> +		goto err_mipi_dsi_host;
> +	}
> +
> +	return 0;
> +
> +err_mipi_dsi_host:
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +err_cleanup:
> +	drm_encoder_cleanup(&dsi->encoder);
> +	drm_connector_cleanup(&dsi->connector);
> +err_pllref:

Do you need to disable pm runtime on dev?

> +	clk_disable_unprepare(dsi->pllref_clk);
> +	return ret;
> +}
> +
> +static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
> +			       void *data)
> +{
> +	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
> +
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	pm_runtime_disable(dev);
> +	if (dsi->slave)
> +		pm_runtime_disable(dsi->slave->dev);
> +	clk_disable_unprepare(dsi->pllref_clk);
> +}
> +
> +static const struct component_ops dw_mipi_dsi_ops = {
> +	.bind	= dw_mipi_dsi_bind,
> +	.unbind	= dw_mipi_dsi_unbind,
> +};
> +
> +static int dw_mipi_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
>  	const struct of_device_id *of_id =
>  			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;
> +	struct reset_control *apb_rst;
>  	int ret;
>  
>  	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> @@ -1214,6 +1419,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
>  	dsi->dev = dev;
>  	dsi->pdata = pdata;
>  	dsi->dpms_mode = DRM_MODE_DPMS_OFF;
> +	dev_set_drvdata(dev, dsi);
>  
>  	ret = rockchip_mipi_parse_dt(dsi);
>  	if (ret)
> @@ -1288,63 +1494,6 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
>  		}
>  	}
>  
> -	ret = clk_prepare_enable(dsi->pllref_clk);
> -	if (ret) {
> -		dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);
> -		return ret;
> -	}
> -
> -	ret = dw_mipi_dsi_register(drm, dsi);
> -	if (ret) {
> -		dev_err(dev, "Failed to register mipi_dsi: %d\n", ret);
> -		goto err_pllref;
> -	}
> -
> -	pm_runtime_enable(dev);
> -
> -	dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;
> -	dsi->dsi_host.dev = dev;
> -	ret = mipi_dsi_host_register(&dsi->dsi_host);
> -	if (ret) {
> -		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> -		goto err_cleanup;
> -	}
> -
> -	if (!dsi->panel) {
> -		ret = -EPROBE_DEFER;
> -		goto err_mipi_dsi_host;
> -	}
> -
> -	dev_set_drvdata(dev, dsi);
> -	return 0;
> -
> -err_mipi_dsi_host:
> -	mipi_dsi_host_unregister(&dsi->dsi_host);
> -err_cleanup:
> -	drm_encoder_cleanup(&dsi->encoder);
> -	drm_connector_cleanup(&dsi->connector);
> -err_pllref:
> -	clk_disable_unprepare(dsi->pllref_clk);
> -	return ret;
> -}
> -
> -static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
> -			       void *data)
> -{
> -	struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
> -
> -	mipi_dsi_host_unregister(&dsi->dsi_host);
> -	pm_runtime_disable(dev);
> -	clk_disable_unprepare(dsi->pllref_clk);
> -}
> -
> -static const struct component_ops dw_mipi_dsi_ops = {
> -	.bind	= dw_mipi_dsi_bind,
> -	.unbind	= dw_mipi_dsi_unbind,
> -};
> -
> -static int dw_mipi_dsi_probe(struct platform_device *pdev)
> -{
>  	return component_add(&pdev->dev, &dw_mipi_dsi_ops);
>  }
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index c7e96b8..51ad1c2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -36,6 +36,7 @@ struct rockchip_crtc_state {
>  	struct drm_crtc_state base;
>  	int output_type;
>  	int output_mode;
> +	int output_flags;
>  };
>  #define to_rockchip_crtc_state(s) \
>  		container_of(s, struct rockchip_crtc_state, base)
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index bf9ed0e..cb40cdd 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -917,6 +917,8 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
>  	case DRM_MODE_CONNECTOR_DSI:
>  		VOP_REG_SET(vop, output, mipi_pin_pol, pin_pol);
>  		VOP_REG_SET(vop, output, mipi_en, 1);
> +		VOP_REG_SET(vop, output, mipi_dual_channel_en,
> +			!!(s->output_flags & ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL));
>  		break;
>  	case DRM_MODE_CONNECTOR_DisplayPort:
>  		pin_pol &= ~BIT(DCLK_INVERT);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 56bbd2e..d184531 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -60,6 +60,7 @@ struct vop_output {
>  	struct vop_reg edp_en;
>  	struct vop_reg hdmi_en;
>  	struct vop_reg mipi_en;
> +	struct vop_reg mipi_dual_channel_en;
>  	struct vop_reg rgb_en;
>  };
>  
> @@ -212,6 +213,8 @@ struct vop_data {
>  /* for use special outface */
>  #define ROCKCHIP_OUT_MODE_AAAA	15
>  
> +#define ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL	BIT(0)
> +
>  enum alpha_mode {
>  	ALPHA_STRAIGHT,
>  	ALPHA_INVERSE,
> -- 
> 1.9.1
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[Sean Paul]

On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
> Hi Sean,
> 
> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
> > On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> > > This patch correct Feedback divider setting:
> > > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> > > 2、Due to the use of a "by 2 pre-scaler," the range of the
> > > feedback multiplication Feedback divider is limited to even
> > > division numbers, and Feedback divider must be greater than
> > > 12, less than 1000.
> > > 3、Make the previously configured Feedback divider(LSB)
> > > factors effective
> > > 
> > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> > > ---
> > >  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
> > >  1 file changed, 54 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > index 9a20b9d..52698b7 100644
> > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > @@ -228,7 +228,7 @@
> > >  #define LOW_PROGRAM_EN		0
> > >  #define HIGH_PROGRAM_EN		BIT(7)
> > >  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> > > -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> > > +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
> > >  #define PLL_LOOP_DIV_EN		BIT(5)
> > >  #define PLL_INPUT_DIV_EN	BIT(4)
> > >  
> > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
> > >  	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> > 
> > You do the same write 2 lines down. Are both needed? It would be nice if the
> > register names were also defined, so this is easier to read.
> 
> If I'm reading correctly, I think this is what Nickey meant by:
> 
> "3、Make the previously configured Feedback divider(LSB)
> factors effective"
> 
> . My reading of the databook is that this step finalizes the previous
> two writes (to test code 0x17 and 0x18).
> 
> Given this was buggy (?) previously, it does seem like having some extra
> language to document this could help. Register names (or "test codes",
> per the docs?) could help, but additionally, maybe a few more comments.
> 

Ah, yeah, thanks for the explanation. It's not clear that this latches the 
values above. I think register names and comments would be immensely helpful.

Sean

> > >  	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);
> 
> [...]
> 
> Brian

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

Re: [PATCH 4/7] drm/rockchip/dsi: correct phy parameter setting

[Sean Paul]

On Mon, Sep 18, 2017 at 05:05:36PM +0800, Nickey Yang wrote:
> As MIPI PHY document show, icpctrl<3..0> and lpfctrl<5..0>
> should depend on frequency,so fix it.
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 70 +++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 9265b7f..d5250e8 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -228,10 +228,10 @@
>  #define VCO_IN_CAP_CON_HIGH	(0x2 << 1)
>  #define REF_BIAS_CUR_SEL	BIT(0)
>  
> -#define CP_CURRENT_3MA		BIT(3)
> +#define CP_CURRENT_SEL(val)	((val) & 0xf)
>  #define CP_PROGRAM_EN		BIT(7)
>  #define LPF_PROGRAM_EN		BIT(6)
> -#define LPF_RESISTORS_20_KOHM	0
> +#define LPF_RESISTORS_SEL(val)	((val) & 0x3f)
>  
>  #define HSFREQRANGE_SEL(val)	(((val) & 0x3f) << 1)
>  
> @@ -340,32 +340,44 @@ enum dw_mipi_dsi_mode {
>  	DW_MIPI_DSI_VID_MODE,
>  };
>  
> -struct dphy_pll_testdin_map {
> +struct dphy_pll_parameter_map {
>  	unsigned int max_mbps;
> -	u8 testdin;
> +	u8 hsfreqrange;
> +	u8 icpctrl;
> +	u8 lpfctrl;
>  };
>  
>  /* The table is based on 27MHz DPHY pll reference clock. */
> -static const struct dphy_pll_testdin_map dptdin_map[] = {
> -	{  90, 0x00}, { 100, 0x10}, { 110, 0x20}, { 130, 0x01},
> -	{ 140, 0x11}, { 150, 0x21}, { 170, 0x02}, { 180, 0x12},
> -	{ 200, 0x22}, { 220, 0x03}, { 240, 0x13}, { 250, 0x23},
> -	{ 270, 0x04}, { 300, 0x14}, { 330, 0x05}, { 360, 0x15},
> -	{ 400, 0x25}, { 450, 0x06}, { 500, 0x16}, { 550, 0x07},
> -	{ 600, 0x17}, { 650, 0x08}, { 700, 0x18}, { 750, 0x09},
> -	{ 800, 0x19}, { 850, 0x29}, { 900, 0x39}, { 950, 0x0a},
> -	{1000, 0x1a}, {1050, 0x2a}, {1100, 0x3a}, {1150, 0x0b},
> -	{1200, 0x1b}, {1250, 0x2b}, {1300, 0x3b}, {1350, 0x0c},
> -	{1400, 0x1c}, {1450, 0x2c}, {1500, 0x3c}
> +static const struct dphy_pll_parameter_map dppa_map[] = {
> +	{  90, 0x00, 0x1 ,0x02}, { 100, 0x10 ,0x1, 0x02},

s/ ,/, /g

Switching icpctrl and lpfctrl values to hardcoded numbers obfuscates the value
they're being set to (ie: 3mA and 20kOhm). Can you please replace the hardcoded
values with #defines describing what the values mean?

> +	{ 110, 0x20, 0x1, 0x02}, { 130, 0x01, 0x1, 0x01},
> +	{ 140, 0x11, 0x1, 0x01}, { 150, 0x21, 0x1, 0x01},
> +	{ 170, 0x02, 0x9, 0x02}, { 180, 0x12, 0x9, 0x02},
> +	{ 200, 0x22, 0x9, 0x02}, { 220, 0x03, 0x2, 0x02},
> +	{ 240, 0x13, 0x2, 0x02}, { 250, 0x23, 0x2, 0x02},
> +	{ 270, 0x04, 0x9, 0x04}, { 300, 0x14, 0x9, 0x04},
> +	{ 330, 0x05, 0x1, 0x01}, { 360, 0x15, 0x1, 0x01},
> +	{ 400, 0x25, 0x1, 0x01}, { 450, 0x06, 0x6, 0x04},
> +	{ 500, 0x16, 0x6, 0x04}, { 550, 0x07, 0x6, 0x08},
> +	{ 600, 0x17, 0x6, 0x08}, { 650, 0x08, 0x6, 0x04},
> +	{ 700, 0x18, 0x6, 0x04}, { 750, 0x09, 0x6, 0x04},
> +	{ 800, 0x19, 0x6, 0x04}, { 850, 0x29, 0x6, 0x04},
> +	{ 900, 0x39, 0x6, 0x04}, { 950, 0x0a, 0xb, 0x10},
> +	{1000, 0x1a, 0xb, 0x10}, {1050, 0x2a, 0xb, 0x10},
> +	{1100, 0x3a, 0xb, 0x10}, {1150, 0x0b, 0xb, 0x08},
> +	{1200, 0x1b, 0xb, 0x08}, {1250, 0x2b, 0xb, 0x08},
> +	{1300, 0x3b, 0xb, 0x08}, {1350, 0x0c, 0xb, 0x08},
> +	{1400, 0x1c, 0xb, 0x08}, {1450, 0x2c, 0xb, 0x08},
> +	{1500, 0x3c, 0xb, 0x08}
>  };
>  
> -static int max_mbps_to_testdin(unsigned int max_mbps)
> +static int max_mbps_to_parameter(unsigned int max_mbps)
>  {
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(dptdin_map); i++)
> -		if (dptdin_map[i].max_mbps > max_mbps)
> -			return dptdin_map[i].testdin;
> +	for (i = 0; i < ARRAY_SIZE(dppa_map); i++)
> +		if (dppa_map[i].max_mbps > max_mbps)
> +			return i;
>  
>  	return -EINVAL;
>  }
> @@ -447,16 +459,16 @@ static inline unsigned int ns2ui(struct dw_mipi_dsi *dsi, int ns)
>  
>  static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  {
> -	int ret, testdin, vco, val;
> +	int ret, i, vco, val;
>  
>  	vco = (dsi->lane_mbps < 200) ? 0 : (dsi->lane_mbps + 100) / 200;
>  
> -	testdin = max_mbps_to_testdin(dsi->lane_mbps);
> -	if (testdin < 0) {
> +	i = max_mbps_to_parameter(dsi->lane_mbps);
> +	if (i < 0) {
>  		dev_err(dsi->dev,
> -			"failed to get testdin for %dmbps lane clock\n",
> +			"failed to get parameter for %dmbps lane clock\n",
>  			dsi->lane_mbps);
> -		return testdin;
> +		return i;
>  	}
>  
>  	/* Start by clearing PHY state */
> @@ -475,12 +487,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  					 VCO_IN_CAP_CON_LOW |
>  					 REF_BIAS_CUR_SEL);
>  
> -	dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_3MA);
> +	dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_SEL(dppa_map[i].icpctrl));
>  	dw_mipi_dsi_phy_write(dsi, 0x12, CP_PROGRAM_EN | LPF_PROGRAM_EN |
> -					 LPF_RESISTORS_20_KOHM);
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin));
> -
> +					 LPF_RESISTORS_SEL(dppa_map[i].lpfctrl));
> +	dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(dppa_map[i].hsfreqrange));
>  	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);
> @@ -547,7 +557,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  {
>  	unsigned long mpclk, tmp;
>  	unsigned int target_mbps = 1000;
> -	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
> +	unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps;
>  	int bpp;
>  	int lanes = dsi->lanes;
>  	unsigned long best_freq = 0;
> -- 
> 1.9.1
> 
> 

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

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[John Keeping]

On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
> > Hi Sean,
> > 
> > On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
> > > On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> > > > This patch correct Feedback divider setting:
> > > > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> > > > 2、Due to the use of a "by 2 pre-scaler," the range of the
> > > > feedback multiplication Feedback divider is limited to even
> > > > division numbers, and Feedback divider must be greater than
> > > > 12, less than 1000.
> > > > 3、Make the previously configured Feedback divider(LSB)
> > > > factors effective
> > > > 
> > > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> > > > ---
> > > >  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
> > > >  1 file changed, 54 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > index 9a20b9d..52698b7 100644
> > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > @@ -228,7 +228,7 @@
> > > >  #define LOW_PROGRAM_EN		0
> > > >  #define HIGH_PROGRAM_EN		BIT(7)
> > > >  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> > > > -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> > > > +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
> > > >  #define PLL_LOOP_DIV_EN		BIT(5)
> > > >  #define PLL_INPUT_DIV_EN	BIT(4)
> > > >  
> > > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
> > > >  	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> > > 
> > > You do the same write 2 lines down. Are both needed? It would be nice if the
> > > register names were also defined, so this is easier to read.
> > 
> > If I'm reading correctly, I think this is what Nickey meant by:
> > 
> > "3、Make the previously configured Feedback divider(LSB)
> > factors effective"
> > 
> > . My reading of the databook is that this step finalizes the previous
> > two writes (to test code 0x17 and 0x18).
> > 
> > Given this was buggy (?) previously, it does seem like having some extra
> > language to document this could help. Register names (or "test codes",
> > per the docs?) could help, but additionally, maybe a few more comments.
> > 
> 
> Ah, yeah, thanks for the explanation. It's not clear that this latches the 
> values above. I think register names and comments would be immensely helpful.

According to the databook, 0x19 controls whether the loop/input dividers
are derived from the hsfreqrange configuration or use the values in 0x17
and 0x18.  I can't see why writing the same value to this register
multiple times is necessary.

> > > >  	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);
> > 
> > [...]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock

[Heiko Stübner]

Am Montag, 18. September 2017, 17:05:37 CEST schrieb Nickey Yang:
> clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>

applied as fix for 4.14 after polishing the commit message a bit


Thanks
Heiko

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[hl]

On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote:
> On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
>> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
>>> Hi Sean,
>>>
>>> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
>>>> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
>>>>> This patch correct Feedback divider setting:
>>>>> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
>>>>> 2、Due to the use of a "by 2 pre-scaler," the range of the
>>>>> feedback multiplication Feedback divider is limited to even
>>>>> division numbers, and Feedback divider must be greater than
>>>>> 12, less than 1000.
>>>>> 3、Make the previously configured Feedback divider(LSB)
>>>>> factors effective
>>>>>
>>>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>>>>> ---
>>>>>   drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
>>>>>   1 file changed, 54 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>>>> index 9a20b9d..52698b7 100644
>>>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>>>> @@ -228,7 +228,7 @@
>>>>>   #define LOW_PROGRAM_EN		0
>>>>>   #define HIGH_PROGRAM_EN		BIT(7)
>>>>>   #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
>>>>> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
>>>>> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
>>>>>   #define PLL_LOOP_DIV_EN		BIT(5)
>>>>>   #define PLL_INPUT_DIV_EN	BIT(4)
>>>>>   
>>>>> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>>>>>   	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
>>>> You do the same write 2 lines down. Are both needed? It would be nice if the
>>>> register names were also defined, so this is easier to read.
>>> If I'm reading correctly, I think this is what Nickey meant by:
>>>
>>> "3、Make the previously configured Feedback divider(LSB)
>>> factors effective"
>>>
>>> . My reading of the databook is that this step finalizes the previous
>>> two writes (to test code 0x17 and 0x18).
>>>
>>> Given this was buggy (?) previously, it does seem like having some extra
>>> language to document this could help. Register names (or "test codes",
>>> per the docs?) could help, but additionally, maybe a few more comments.
>>>
>> Ah, yeah, thanks for the explanation. It's not clear that this latches the
>> values above. I think register names and comments would be immensely helpful.
> According to the databook, 0x19 controls whether the loop/input dividers
> are derived from the hsfreqrange configuration or use the values in 0x17
> and 0x18.  I can't see why writing the same value to this register
> multiple times is necessary.
According to databook, set 0x19 to 0x30 make the previously configured 
N(0x17) and M(0x18)
factors effective, 0x18 need to be setted by twice, since we need to set 
0x18 LSB and MSB separately,
As we test, after set 0x18 LSB, if we do not set 0x19 immediately to 
make the configrued effective,
when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get 
wrong pll frequency. Anyway,
I think should add some comment here to clear that.
>
>>>>>   	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);
>>> [...]
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[John Keeping]

On Wed, Sep 20, 2017 at 07:08:11PM +0800, hl wrote:
> 
> 
> On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote:
> > On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
> >> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
> >>> Hi Sean,
> >>>
> >>> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
> >>>> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> >>>>> This patch correct Feedback divider setting:
> >>>>> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> >>>>> 2、Due to the use of a "by 2 pre-scaler," the range of the
> >>>>> feedback multiplication Feedback divider is limited to even
> >>>>> division numbers, and Feedback divider must be greater than
> >>>>> 12, less than 1000.
> >>>>> 3、Make the previously configured Feedback divider(LSB)
> >>>>> factors effective
> >>>>>
> >>>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> >>>>> ---
> >>>>>   drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
> >>>>>   1 file changed, 54 insertions(+), 29 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> >>>>> index 9a20b9d..52698b7 100644
> >>>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> >>>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> >>>>> @@ -228,7 +228,7 @@
> >>>>>   #define LOW_PROGRAM_EN		0
> >>>>>   #define HIGH_PROGRAM_EN		BIT(7)
> >>>>>   #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> >>>>> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> >>>>> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
> >>>>>   #define PLL_LOOP_DIV_EN		BIT(5)
> >>>>>   #define PLL_INPUT_DIV_EN	BIT(4)
> >>>>>   
> >>>>> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
> >>>>>   	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> >>>> You do the same write 2 lines down. Are both needed? It would be nice if the
> >>>> register names were also defined, so this is easier to read.
> >>> If I'm reading correctly, I think this is what Nickey meant by:
> >>>
> >>> "3、Make the previously configured Feedback divider(LSB)
> >>> factors effective"
> >>>
> >>> . My reading of the databook is that this step finalizes the previous
> >>> two writes (to test code 0x17 and 0x18).
> >>>
> >>> Given this was buggy (?) previously, it does seem like having some extra
> >>> language to document this could help. Register names (or "test codes",
> >>> per the docs?) could help, but additionally, maybe a few more comments.
> >>>
> >> Ah, yeah, thanks for the explanation. It's not clear that this latches the
> >> values above. I think register names and comments would be immensely helpful.
> > According to the databook, 0x19 controls whether the loop/input dividers
> > are derived from the hsfreqrange configuration or use the values in 0x17
> > and 0x18.  I can't see why writing the same value to this register
> > multiple times is necessary.
> According to databook, set 0x19 to 0x30 make the previously configured 
> N(0x17) and M(0x18)
> factors effective, 0x18 need to be setted by twice, since we need to set 
> 0x18 LSB and MSB separately,
> As we test, after set 0x18 LSB, if we do not set 0x19 immediately to 
> make the configrued effective,
> when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get 
> wrong pll frequency. Anyway,
> I think should add some comment here to clear that.

That's surprising, the examples in sections 6.2.1 and 6.2.2 of the
databook I have both show the high and low parts of 0x18 being written
before 0x19 is set.

When reading 0x18 are you setting bit 7 in TESTDIN correctly in order to
select the correct bits of the feedback divider?

> >
> >>>>>   	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);
> >>> [...]
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[hl]

On Wednesday, September 20, 2017 08:07 PM, John Keeping wrote:
> On Wed, Sep 20, 2017 at 07:08:11PM +0800, hl wrote:
>>
>> On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote:
>>> On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
>>>> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
>>>>> Hi Sean,
>>>>>
>>>>> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
>>>>>> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
>>>>>>> This patch correct Feedback divider setting:
>>>>>>> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
>>>>>>> 2、Due to the use of a "by 2 pre-scaler," the range of the
>>>>>>> feedback multiplication Feedback divider is limited to even
>>>>>>> division numbers, and Feedback divider must be greater than
>>>>>>> 12, less than 1000.
>>>>>>> 3、Make the previously configured Feedback divider(LSB)
>>>>>>> factors effective
>>>>>>>
>>>>>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
>>>>>>>    1 file changed, 54 insertions(+), 29 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>>>>>> index 9a20b9d..52698b7 100644
>>>>>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>>>>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>>>>>> @@ -228,7 +228,7 @@
>>>>>>>    #define LOW_PROGRAM_EN		0
>>>>>>>    #define HIGH_PROGRAM_EN		BIT(7)
>>>>>>>    #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
>>>>>>> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
>>>>>>> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
>>>>>>>    #define PLL_LOOP_DIV_EN		BIT(5)
>>>>>>>    #define PLL_INPUT_DIV_EN	BIT(4)
>>>>>>>    
>>>>>>> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>>>>>>>    	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
>>>>>> You do the same write 2 lines down. Are both needed? It would be nice if the
>>>>>> register names were also defined, so this is easier to read.
>>>>> If I'm reading correctly, I think this is what Nickey meant by:
>>>>>
>>>>> "3、Make the previously configured Feedback divider(LSB)
>>>>> factors effective"
>>>>>
>>>>> . My reading of the databook is that this step finalizes the previous
>>>>> two writes (to test code 0x17 and 0x18).
>>>>>
>>>>> Given this was buggy (?) previously, it does seem like having some extra
>>>>> language to document this could help. Register names (or "test codes",
>>>>> per the docs?) could help, but additionally, maybe a few more comments.
>>>>>
>>>> Ah, yeah, thanks for the explanation. It's not clear that this latches the
>>>> values above. I think register names and comments would be immensely helpful.
>>> According to the databook, 0x19 controls whether the loop/input dividers
>>> are derived from the hsfreqrange configuration or use the values in 0x17
>>> and 0x18.  I can't see why writing the same value to this register
>>> multiple times is necessary.
>> According to databook, set 0x19 to 0x30 make the previously configured
>> N(0x17) and M(0x18)
>> factors effective, 0x18 need to be setted by twice, since we need to set
>> 0x18 LSB and MSB separately,
>> As we test, after set 0x18 LSB, if we do not set 0x19 immediately to
>> make the configrued effective,
>> when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get
>> wrong pll frequency. Anyway,
>> I think should add some comment here to clear that.
> That's surprising, the examples in sections 6.2.1 and 6.2.2 of the
> databook I have both show the high and low parts of 0x18 being written
> before 0x19 is set.
>
> When reading 0x18 are you setting bit 7 in TESTDIN correctly in order to
> select the correct bits of the feedback divider?
We scope the mipi lane clock, found it got wrong clock frequency use flow:
     set 0x17
     set 0x18 LSB
     set 0x18 MSB
     set 0x19 = 0x30

and we can get right mipi lane clock use flow:
     set 0x17
     set 0x18 LSB
     set 0x19 = 0x30
     set 0x18 MSB
     set 0x19 = 0x30

>
>>>>>>>    	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);
>>>>> [...]
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

[Matthias Kaehlcke]

Hi Nickey,

El Mon, Sep 18, 2017 at 05:05:33PM +0800 Nickey Yang ha dit:

> This patch correct Feedback divider setting:
> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> 2、Due to the use of a "by 2 pre-scaler," the range of the
> feedback multiplication Feedback divider is limited to even
> division numbers, and Feedback divider must be greater than
> 12, less than 1000.
> 3、Make the previously configured Feedback divider(LSB)
> factors effective
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 9a20b9d..52698b7 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -228,7 +228,7 @@
>  #define LOW_PROGRAM_EN		0
>  #define HIGH_PROGRAM_EN		BIT(7)
>  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
>  #define PLL_LOOP_DIV_EN		BIT(5)
>  #define PLL_INPUT_DIV_EN	BIT(4)
>  
> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  	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, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_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);
> @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(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;
> -	unsigned int m = 1, n = 1, target_mbps = 1000;
> +	unsigned long mpclk, tmp;
> +	unsigned int target_mbps = 1000;
>  	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
>  	int bpp;
> +	unsigned long best_freq = 0;
> +	unsigned long fvco_min, fvco_max, fin ,fout;

nit: should be 'fin, fout'.

> +	unsigned int min_prediv, max_prediv;
> +	unsigned int _prediv, uninitialized_var(best_prediv);
> +	unsigned long _fbdiv, uninitialized_var(best_fbdiv);
> +	unsigned long min_delta = UINT_MAX;
>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  	if (bpp < 0) {
> @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  			dev_err(dsi->dev, "DPHY clock frequency is out of range\n");
>  	}
>  
> -	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
> -	tmp = pllref;
> -
> -	/*
> -	 * The limits on the PLL divisor are:
> -	 *
> -	 *	5MHz <= (pllref / n) <= 40MHz
> -	 *
> -	 * we walk over these values in descreasing order so that if we hit
> -	 * an exact match for target_mbps it is more likely that "m" will be
> -	 * even.
> -	 *
> -	 * TODO: ensure that "m" is even after this loop.
> -	 */
> -	for (i = pllref / 5; i > (pllref / 40); i--) {
> -		pre = pllref / i;
> -		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
> -			tmp = target_mbps % pre;
> -			n = i;
> -			m = target_mbps / pre;
> +	fin = clk_get_rate(dsi->pllref_clk);
> +	fout = target_mbps * USEC_PER_SEC;
> +
> +	/* constraint: 5Mhz < Fref / N < 40MHz */

According to the previous comment above it should be '<=' instead of
'<'.

> +	min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
> +	max_prediv = fin / (5 * USEC_PER_SEC);
> +
> +	/* constraint: 80MHz < Fvco < 1500Mhz */

Same here.

> +	fvco_min = 80 * USEC_PER_SEC;
> +	fvco_max = 1500 * USEC_PER_SEC;
> +
> +	for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
> +		u32 delta;
> +		/* Fvco = Fref * M / N */
> +		tmp = fout * _prediv;
> +		do_div(tmp, fin);
> +		_fbdiv = tmp;
> +		/*
> +		 * Due to the use of a "by 2 pre-scaler," the range of the
> +		 * feedback multiplication value M is limited to even division
> +		 * numbers, and m must be greater than 12, less than 1000.
> +		 */

I didn't encounter the second part of the constraint in my version of
the databook, judging from the code below it seems the comment should
be "12 <= m <= 1000".

> +		if (_fbdiv < 12 || _fbdiv > 1000)
> +			continue;
> +
> +		if (_fbdiv % 2)
> +			++_fbdiv;
> +
> +		tmp = (u64)_fbdiv * fin;
> +		do_div(tmp, _prediv);
> +		if (tmp < fvco_min || tmp > fvco_max)
> +			continue;
> +
> +		delta = abs(fout - tmp);
> +		if (delta < min_delta) {
> +			best_prediv = _prediv;
> +			best_fbdiv = _fbdiv;
> +			min_delta = delta;
> +			best_freq = tmp;
>  		}
> -		if (tmp == 0)
> -			break;
>  	}
>  
> -	dsi->lane_mbps = pllref / n * m;
> -	dsi->input_div = n;
> -	dsi->feedback_div = m;
> +	if (best_freq) {
> +		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
> +		dsi->input_div = best_prediv;
> +		dsi->feedback_div = best_fbdiv;
> +	}
>  
>  	return 0;
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel