Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1733778 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting Nickey Yang <nickey.yang@rock-chips.com> - 2017-09-18 11:10 +0200
    [PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock Nickey Yang <nickey.yang@rock-chips.com> - 2017-09-18 11:10 +0200
      Re: [PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock Heiko Stübner <heiko@sntech.de> - 2017-09-18 13:40 +0200
      Re: [PATCH 5/7] arm64: dts: rockchip: rk3399: Correct MIPI DPHY PLL clock Heiko Stübner <heiko@sntech.de> - 2017-09-20 12:30 +0200
    [PATCH 2/7] drm/rockchip/dsi: add dual mipi channel support Nickey Yang <nickey.yang@rock-chips.com> - 2017-09-18 11:10 +0200
      Re: [PATCH 2/7] drm/rockchip/dsi: add dual mipi channel support Sean Paul <seanpaul@chromium.org> - 2017-09-19 22:30 +0200
    Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting Brian Norris <briannorris@chromium.org> - 2017-09-19 01:30 +0200
    Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting Sean Paul <seanpaul@chromium.org> - 2017-09-19 20:10 +0200
      Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting Brian Norris <briannorris@chromium.org> - 2017-09-19 20:20 +0200
        Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting Sean Paul <seanpaul@chromium.org> - 2017-09-19 22:30 +0200
          Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting John Keeping <john@keeping.me.uk> - 2017-09-20 12:20 +0200
            Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting hl <hl@rock-chips.com> - 2017-09-20 13:10 +0200
              Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting John Keeping <john@keeping.me.uk> - 2017-09-20 14:10 +0200
    Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting Matthias Kaehlcke <mka@chromium.org> - 2017-09-23 01:00 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1734540

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

[toc] | [prev] | [next] | [standalone]

#1735140

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

[toc] | [prev] | [next] | [standalone]

#1735142

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

[toc] | [prev] | [next] | [standalone]

#1735241

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

[toc] | [prev] | [next] | [standalone]

#1735691

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

[toc] | [prev] | [next] | [standalone]

#1735719

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

[toc] | [prev] | [next] | [standalone]

#1735742

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

[toc] | [prev] | [next] | [standalone]

#1737918

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web