[PATCH v3 1/6] drm/rockchip/dsi: Define and use macros for PHY register addresses

[PATCH v3 1/6] drm/rockchip/dsi: Define and use macros for PHY register addresses

[Nickey Yang]

Replace the hardcoded register address numerical values with macros to
clarify the code.

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

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index b15755b..95ce253 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -254,6 +254,28 @@
 #define DW_MIPI_NEEDS_PHY_CFG_CLK	BIT(0)
 #define DW_MIPI_NEEDS_GRF_CLK		BIT(1)
 
+#define PLL_BIAS_CUR_SEL_CAP_VCO_CONTROL	0x10
+#define PLL_CP_CONTROL_PLL_LOCK_BYPASS	0x11
+#define PLL_LPF_AND_CP_CONTROL	0x12
+#define PLL_INPUT_DIVIDER_RATIO	0x17
+#define PLL_LOOP_DIVIDER_RATIO	0x18
+#define PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL	0x19
+#define BANDGAP_AND_BIAS_CONTROL	0x20
+#define TERMINATION_RESISTER_CONTROL	0x21
+#define AFE_BIAS_BANDGAP_ANALOG_PROGRAMMABILITY	0x22
+#define HS_RX_CONTROL_OF_LANE_0	0x44
+#define HS_TX_CLOCK_LANE_REQUEST_STATE_TIME_CONTROL	0x60
+#define HS_TX_CLOCK_LANE_PREPARE_STATE_TIME_CONTROL	0x61
+#define HS_TX_CLOCK_LANE_HS_ZERO_STATE_TIME_CONTROL	0x62
+#define HS_TX_CLOCK_LANE_TRAIL_STATE_TIME_CONTROL	0x63
+#define HS_TX_CLOCK_LANE_EXIT_STATE_TIME_CONTROL	0x64
+#define HS_TX_CLOCK_LANE_POST_TIME_CONTROL	0x65
+#define HS_TX_DATA_LANE_REQUEST_STATE_TIME_CONTROL	0x70
+#define HS_TX_DATA_LANE_PREPARE_STATE_TIME_CONTROL	0x71
+#define HS_TX_DATA_LANE_HS_ZERO_STATE_TIME_CONTROL	0x72
+#define HS_TX_DATA_LANE_TRAIL_STATE_TIME_CONTROL	0x73
+#define HS_TX_DATA_LANE_EXIT_STATE_TIME_CONTROL	0x74
+
 enum {
 	BANDGAP_97_07,
 	BANDGAP_98_05,
@@ -447,53 +469,72 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 		return ret;
 	}
 
-	dw_mipi_dsi_phy_write(dsi, 0x10, BYPASS_VCO_RANGE |
-					 VCO_RANGE_CON_SEL(vco) |
-					 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, 0x12, CP_PROGRAM_EN | LPF_PROGRAM_EN |
-					 LPF_RESISTORS_20_KOHM);
-
-	dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin));
-
-	dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div));
-	dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) |
-					 LOW_PROGRAM_EN);
-	dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
-					 HIGH_PROGRAM_EN);
-	dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
-
-	dw_mipi_dsi_phy_write(dsi, 0x22, LOW_PROGRAM_EN |
-					 BIASEXTR_SEL(BIASEXTR_127_7));
-	dw_mipi_dsi_phy_write(dsi, 0x22, HIGH_PROGRAM_EN |
-					 BANDGAP_SEL(BANDGAP_96_10));
-
-	dw_mipi_dsi_phy_write(dsi, 0x20, POWER_CONTROL | INTERNAL_REG_CURRENT |
-					 BIAS_BLOCK_ON | BANDGAP_ON);
-
-	dw_mipi_dsi_phy_write(dsi, 0x21, TER_RESISTOR_LOW | TER_CAL_DONE |
-					 SETRD_MAX | TER_RESISTORS_ON);
-	dw_mipi_dsi_phy_write(dsi, 0x21, TER_RESISTOR_HIGH | LEVEL_SHIFTERS_ON |
-					 SETRD_MAX | POWER_MANAGE |
-					 TER_RESISTORS_ON);
-
-	dw_mipi_dsi_phy_write(dsi, 0x60, TLP_PROGRAM_EN | ns2bc(dsi, 500));
-	dw_mipi_dsi_phy_write(dsi, 0x61, THS_PRE_PROGRAM_EN | ns2ui(dsi, 40));
-	dw_mipi_dsi_phy_write(dsi, 0x62, THS_ZERO_PROGRAM_EN | ns2bc(dsi, 300));
-	dw_mipi_dsi_phy_write(dsi, 0x63, THS_PRE_PROGRAM_EN | ns2ui(dsi, 100));
-	dw_mipi_dsi_phy_write(dsi, 0x64, BIT(5) | ns2bc(dsi, 100));
-	dw_mipi_dsi_phy_write(dsi, 0x65, BIT(5) | (ns2bc(dsi, 60) + 7));
-
-	dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | ns2bc(dsi, 500));
-	dw_mipi_dsi_phy_write(dsi, 0x71,
+	dw_mipi_dsi_phy_write(dsi, PLL_BIAS_CUR_SEL_CAP_VCO_CONTROL,
+			      BYPASS_VCO_RANGE |
+			      VCO_RANGE_CON_SEL(vco) |
+			      VCO_IN_CAP_CON_LOW |
+			      REF_BIAS_CUR_SEL);
+
+	dw_mipi_dsi_phy_write(dsi, PLL_CP_CONTROL_PLL_LOCK_BYPASS,
+			      CP_CURRENT_3MA);
+	dw_mipi_dsi_phy_write(dsi, PLL_LPF_AND_CP_CONTROL,
+			      CP_PROGRAM_EN | LPF_PROGRAM_EN |
+			      LPF_RESISTORS_20_KOHM);
+
+	dw_mipi_dsi_phy_write(dsi, HS_RX_CONTROL_OF_LANE_0,
+			      HSFREQRANGE_SEL(testdin));
+
+	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_DIVIDER_RATIO,
+			      INPUT_DIVIDER(dsi->input_div));
+	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
+			      LOOP_DIV_LOW_SEL(dsi->feedback_div) |
+			      LOW_PROGRAM_EN);
+	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
+			      LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
+			      HIGH_PROGRAM_EN);
+	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL,
+			      PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
+
+	dw_mipi_dsi_phy_write(dsi, AFE_BIAS_BANDGAP_ANALOG_PROGRAMMABILITY,
+			      LOW_PROGRAM_EN | BIASEXTR_SEL(BIASEXTR_127_7));
+	dw_mipi_dsi_phy_write(dsi, AFE_BIAS_BANDGAP_ANALOG_PROGRAMMABILITY,
+			      HIGH_PROGRAM_EN | BANDGAP_SEL(BANDGAP_96_10));
+
+	dw_mipi_dsi_phy_write(dsi, BANDGAP_AND_BIAS_CONTROL,
+			      POWER_CONTROL | INTERNAL_REG_CURRENT |
+			      BIAS_BLOCK_ON | BANDGAP_ON);
+
+	dw_mipi_dsi_phy_write(dsi, TERMINATION_RESISTER_CONTROL,
+			      TER_RESISTOR_LOW | TER_CAL_DONE |
+			      SETRD_MAX | TER_RESISTORS_ON);
+	dw_mipi_dsi_phy_write(dsi, TERMINATION_RESISTER_CONTROL,
+			      TER_RESISTOR_HIGH | LEVEL_SHIFTERS_ON |
+			      SETRD_MAX | POWER_MANAGE |
+			      TER_RESISTORS_ON);
+
+	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_REQUEST_STATE_TIME_CONTROL,
+			      TLP_PROGRAM_EN | ns2bc(dsi, 500));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_PREPARE_STATE_TIME_CONTROL,
+			      THS_PRE_PROGRAM_EN | ns2ui(dsi, 40));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_HS_ZERO_STATE_TIME_CONTROL,
+			      THS_ZERO_PROGRAM_EN | ns2bc(dsi, 300));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_TRAIL_STATE_TIME_CONTROL,
+			      THS_PRE_PROGRAM_EN | ns2ui(dsi, 100));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_EXIT_STATE_TIME_CONTROL,
+			      BIT(5) | ns2bc(dsi, 100));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_POST_TIME_CONTROL,
+			      BIT(5) | (ns2bc(dsi, 60) + 7));
+
+	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_REQUEST_STATE_TIME_CONTROL,
+			      TLP_PROGRAM_EN | ns2bc(dsi, 500));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_PREPARE_STATE_TIME_CONTROL,
 			      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 50) + 5));
-	dw_mipi_dsi_phy_write(dsi, 0x72,
+	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_HS_ZERO_STATE_TIME_CONTROL,
 			      THS_ZERO_PROGRAM_EN | (ns2bc(dsi, 140) + 2));
-	dw_mipi_dsi_phy_write(dsi, 0x73,
+	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_TRAIL_STATE_TIME_CONTROL,
 			      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 60) + 8));
-	dw_mipi_dsi_phy_write(dsi, 0x74, BIT(5) | ns2bc(dsi, 100));
+	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_EXIT_STATE_TIME_CONTROL,
+			      BIT(5) | ns2bc(dsi, 100));
 
 	dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
 				     PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
-- 
1.9.1

[PATCH v3 2/6] 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.

Reviewed-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 98 ++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 95ce253..09e7bfe 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -217,10 +217,21 @@
 #define VCO_IN_CAP_CON_HIGH	(0x2 << 1)
 #define REF_BIAS_CUR_SEL	BIT(0)
 
-#define CP_CURRENT_3MA		BIT(3)
+#define CP_CURRENT_1_5UA	0x1
+#define CP_CURRENT_4_5UA	0x2
+#define CP_CURRENT_7_5UA	0x6
+#define CP_CURRENT_6UA	0x9
+#define CP_CURRENT_12UA	0xb
+#define CP_CURRENT_SEL(val)	((val) & 0xf)
 #define CP_PROGRAM_EN		BIT(7)
+
+#define LPF_RESISTORS_15_5KOHM	0x1
+#define LPF_RESISTORS_13KOHM	0x2
+#define LPF_RESISTORS_11_5KOHM	0x4
+#define LPF_RESISTORS_10_5KOHM	0x8
+#define LPF_RESISTORS_8KOHM	0x10
 #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)
 
@@ -339,32 +350,63 @@ 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[] = {
+	{  89, 0x00, CP_CURRENT_1_5UA, LPF_RESISTORS_13KOHM},
+	{  99, 0x10, CP_CURRENT_1_5UA, LPF_RESISTORS_13KOHM},
+	{ 109, 0x20, CP_CURRENT_1_5UA, LPF_RESISTORS_13KOHM},
+	{ 129, 0x01, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
+	{ 139, 0x11, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
+	{ 149, 0x21, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
+	{ 169, 0x02, CP_CURRENT_6UA, LPF_RESISTORS_13KOHM},
+	{ 179, 0x12, CP_CURRENT_6UA, LPF_RESISTORS_13KOHM},
+	{ 199, 0x22, CP_CURRENT_6UA, LPF_RESISTORS_13KOHM},
+	{ 219, 0x03, CP_CURRENT_4_5UA, LPF_RESISTORS_13KOHM},
+	{ 239, 0x13, CP_CURRENT_4_5UA, LPF_RESISTORS_13KOHM},
+	{ 249, 0x23, CP_CURRENT_4_5UA, LPF_RESISTORS_13KOHM},
+	{ 269, 0x04, CP_CURRENT_6UA, LPF_RESISTORS_11_5KOHM},
+	{ 299, 0x14, CP_CURRENT_6UA, LPF_RESISTORS_11_5KOHM},
+	{ 329, 0x05, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
+	{ 359, 0x15, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
+	{ 399, 0x25, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
+	{ 449, 0x06, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 499, 0x16, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 549, 0x07, CP_CURRENT_7_5UA, LPF_RESISTORS_10_5KOHM},
+	{ 599, 0x17, CP_CURRENT_7_5UA, LPF_RESISTORS_10_5KOHM},
+	{ 649, 0x08, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 699, 0x18, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 749, 0x09, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 799, 0x19, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 849, 0x29, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 899, 0x39, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
+	{ 949, 0x0a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
+	{ 999, 0x1a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
+	{1049, 0x2a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
+	{1099, 0x3a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
+	{1149, 0x0b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1199, 0x1b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1249, 0x2b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1299, 0x3b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1349, 0x0c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1399, 0x1c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1449, 0x2c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
+	{1500, 0x3c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM}
 };
 
-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;
 }
@@ -446,16 +488,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) {
 		DRM_DEV_ERROR(dsi->dev,
-			      "failed to get testdin for %dmbps lane clock\n",
+			      "failed to get parameter for %dmbps clock\n",
 			      dsi->lane_mbps);
-		return testdin;
+		return i;
 	}
 
 	/* Start by clearing PHY state */
@@ -476,13 +518,13 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 			      REF_BIAS_CUR_SEL);
 
 	dw_mipi_dsi_phy_write(dsi, PLL_CP_CONTROL_PLL_LOCK_BYPASS,
-			      CP_CURRENT_3MA);
+			      CP_CURRENT_SEL(dppa_map[i].icpctrl));
 	dw_mipi_dsi_phy_write(dsi, PLL_LPF_AND_CP_CONTROL,
 			      CP_PROGRAM_EN | LPF_PROGRAM_EN |
-			      LPF_RESISTORS_20_KOHM);
+			      LPF_RESISTORS_SEL(dppa_map[i].lpfctrl));
 
 	dw_mipi_dsi_phy_write(dsi, HS_RX_CONTROL_OF_LANE_0,
-			      HSFREQRANGE_SEL(testdin));
+			      HSFREQRANGE_SEL(dppa_map[i].hsfreqrange));
 
 	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_DIVIDER_RATIO,
 			      INPUT_DIVIDER(dsi->input_div));
@@ -565,7 +607,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 	unsigned int i, pre;
 	unsigned long mpclk, pllref, tmp;
 	unsigned int m = 1, n = 1, 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;
 
 	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
-- 
1.9.1

Re: [PATCH v3 2/6] drm/rockchip/dsi: correct phy parameter setting

[Sean Paul]

On Wed, Oct 25, 2017 at 11:50:59AM +0800, Nickey Yang wrote:
> As MIPI PHY document show, icpctrl<3..0> and lpfctrl<5..0>
> should depend on frequency,so fix it.
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---

In future, please list the differences between versions. It makes reviewing much
easier.

Thanks,

Sean

>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 98 ++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 95ce253..09e7bfe 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -217,10 +217,21 @@
>  #define VCO_IN_CAP_CON_HIGH	(0x2 << 1)
>  #define REF_BIAS_CUR_SEL	BIT(0)
>  
> -#define CP_CURRENT_3MA		BIT(3)
> +#define CP_CURRENT_1_5UA	0x1
> +#define CP_CURRENT_4_5UA	0x2
> +#define CP_CURRENT_7_5UA	0x6
> +#define CP_CURRENT_6UA	0x9
> +#define CP_CURRENT_12UA	0xb
> +#define CP_CURRENT_SEL(val)	((val) & 0xf)
>  #define CP_PROGRAM_EN		BIT(7)
> +
> +#define LPF_RESISTORS_15_5KOHM	0x1
> +#define LPF_RESISTORS_13KOHM	0x2
> +#define LPF_RESISTORS_11_5KOHM	0x4
> +#define LPF_RESISTORS_10_5KOHM	0x8
> +#define LPF_RESISTORS_8KOHM	0x10
>  #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)
>  
> @@ -339,32 +350,63 @@ 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[] = {
> +	{  89, 0x00, CP_CURRENT_1_5UA, LPF_RESISTORS_13KOHM},
> +	{  99, 0x10, CP_CURRENT_1_5UA, LPF_RESISTORS_13KOHM},
> +	{ 109, 0x20, CP_CURRENT_1_5UA, LPF_RESISTORS_13KOHM},
> +	{ 129, 0x01, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
> +	{ 139, 0x11, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
> +	{ 149, 0x21, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
> +	{ 169, 0x02, CP_CURRENT_6UA, LPF_RESISTORS_13KOHM},
> +	{ 179, 0x12, CP_CURRENT_6UA, LPF_RESISTORS_13KOHM},
> +	{ 199, 0x22, CP_CURRENT_6UA, LPF_RESISTORS_13KOHM},
> +	{ 219, 0x03, CP_CURRENT_4_5UA, LPF_RESISTORS_13KOHM},
> +	{ 239, 0x13, CP_CURRENT_4_5UA, LPF_RESISTORS_13KOHM},
> +	{ 249, 0x23, CP_CURRENT_4_5UA, LPF_RESISTORS_13KOHM},
> +	{ 269, 0x04, CP_CURRENT_6UA, LPF_RESISTORS_11_5KOHM},
> +	{ 299, 0x14, CP_CURRENT_6UA, LPF_RESISTORS_11_5KOHM},
> +	{ 329, 0x05, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
> +	{ 359, 0x15, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
> +	{ 399, 0x25, CP_CURRENT_1_5UA, LPF_RESISTORS_15_5KOHM},
> +	{ 449, 0x06, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 499, 0x16, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 549, 0x07, CP_CURRENT_7_5UA, LPF_RESISTORS_10_5KOHM},
> +	{ 599, 0x17, CP_CURRENT_7_5UA, LPF_RESISTORS_10_5KOHM},
> +	{ 649, 0x08, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 699, 0x18, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 749, 0x09, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 799, 0x19, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 849, 0x29, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 899, 0x39, CP_CURRENT_7_5UA, LPF_RESISTORS_11_5KOHM},
> +	{ 949, 0x0a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
> +	{ 999, 0x1a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
> +	{1049, 0x2a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
> +	{1099, 0x3a, CP_CURRENT_12UA, LPF_RESISTORS_8KOHM},
> +	{1149, 0x0b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1199, 0x1b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1249, 0x2b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1299, 0x3b, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1349, 0x0c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1399, 0x1c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1449, 0x2c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM},
> +	{1500, 0x3c, CP_CURRENT_12UA, LPF_RESISTORS_10_5KOHM}
>  };
>  
> -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;
>  }
> @@ -446,16 +488,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) {
>  		DRM_DEV_ERROR(dsi->dev,
> -			      "failed to get testdin for %dmbps lane clock\n",
> +			      "failed to get parameter for %dmbps clock\n",
>  			      dsi->lane_mbps);
> -		return testdin;
> +		return i;
>  	}
>  
>  	/* Start by clearing PHY state */
> @@ -476,13 +518,13 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  			      REF_BIAS_CUR_SEL);
>  
>  	dw_mipi_dsi_phy_write(dsi, PLL_CP_CONTROL_PLL_LOCK_BYPASS,
> -			      CP_CURRENT_3MA);
> +			      CP_CURRENT_SEL(dppa_map[i].icpctrl));
>  	dw_mipi_dsi_phy_write(dsi, PLL_LPF_AND_CP_CONTROL,
>  			      CP_PROGRAM_EN | LPF_PROGRAM_EN |
> -			      LPF_RESISTORS_20_KOHM);
> +			      LPF_RESISTORS_SEL(dppa_map[i].lpfctrl));
>  
>  	dw_mipi_dsi_phy_write(dsi, HS_RX_CONTROL_OF_LANE_0,
> -			      HSFREQRANGE_SEL(testdin));
> +			      HSFREQRANGE_SEL(dppa_map[i].hsfreqrange));
>  
>  	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_DIVIDER_RATIO,
>  			      INPUT_DIVIDER(dsi->input_div));
> @@ -565,7 +607,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  	unsigned int i, pre;
>  	unsigned long mpclk, pllref, tmp;
>  	unsigned int m = 1, n = 1, 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;
>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -- 
> 1.9.1
> 

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

[PATCH v3 3/6] 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 | 93 ++++++++++++++++++++++------------
 1 file changed, 62 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 09e7bfe..589b420 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -239,7 +239,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)
 
@@ -531,6 +531,14 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
 	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
 			      LOOP_DIV_LOW_SEL(dsi->feedback_div) |
 			      LOW_PROGRAM_EN);
+	/*
+	 * we need set PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL immediately
+	 * to make the configrued LSB effective according to IP simulation
+	 * and lab test results.
+	 * Only in this way can we get correct mipi phy pll frequency.
+	 */
+	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL,
+			      PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
 	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
 			      LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
 			      HIGH_PROGRAM_EN);
@@ -604,11 +612,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 = dppa_map[ARRAY_SIZE(dppa_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 = ULONG_MAX;
 
 	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
 	if (bpp < 0) {
@@ -629,35 +642,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 				      "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++) {
+		u64 tmp;
+		u32 delta;
+		/* Fvco = Fref * M / N */
+		tmp = (u64)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;
+
+		_fbdiv += _fbdiv % 2;
+
+		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;
+	} else
+		DRM_DEV_ERROR(dsi->dev, "Can not find best_freq for DPHY\n");
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Sean Paul]

On Wed, Oct 25, 2017 at 11:51:00AM +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>
> ---

You don't list the changes between this version and the previous ones, so I looked
at the feedback from the last time. Archit asked a question about moving to
dw-mipi-dsi, and Kristian asked you to split the patch for each line item in the
above list. I know you split out the register definitions, but an explanation
about why you didn't split the rest would be helpful.

In future, list the changes between patches and Cc people who have given you
review on your previous versions (I've cc'd Archit and Kristian).

Sean


>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 93 ++++++++++++++++++++++------------
>  1 file changed, 62 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 09e7bfe..589b420 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -239,7 +239,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)
>  
> @@ -531,6 +531,14 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
>  			      LOOP_DIV_LOW_SEL(dsi->feedback_div) |
>  			      LOW_PROGRAM_EN);
> +	/*
> +	 * we need set PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL immediately
> +	 * to make the configrued LSB effective according to IP simulation
> +	 * and lab test results.
> +	 * Only in this way can we get correct mipi phy pll frequency.
> +	 */
> +	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL,
> +			      PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
>  	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
>  			      LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
>  			      HIGH_PROGRAM_EN);
> @@ -604,11 +612,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 = dppa_map[ARRAY_SIZE(dppa_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 = ULONG_MAX;
>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  	if (bpp < 0) {
> @@ -629,35 +642,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  				      "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++) {
> +		u64 tmp;
> +		u32 delta;
> +		/* Fvco = Fref * M / N */
> +		tmp = (u64)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;
> +
> +		_fbdiv += _fbdiv % 2;
> +
> +		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;
> +	} else
> +		DRM_DEV_ERROR(dsi->dev, "Can not find best_freq for DPHY\n");
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 

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

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Brian Norris]

On Wed, Oct 25, 2017 at 03:57:19AM -0400, Sean Paul wrote:
> Archit asked a question about moving to
> dw-mipi-dsi

That question made me think though: this approach seems backwards. It
seems like someone did copy/paste/fork, and then we're asking the
authors of the original driver to un-fork? It seems like this should
happen the other way around -- those trying to support a new incarnation
should have looked to try to abstract the original driver for their
uses first.

IIUC, that's exactly what Rockchip did for much of their Analogix eDP
code -- they reworked the Exynos DP driver to split common Analogix code
from any Exynos-specific bits.

And actually, the current stuff in
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c is completely unused. It
exports some functions, but I see no users of it. Is that intended? Is
somebody already working on refactoring existing Rockchip code to use
this?

Brian

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Archit Taneja]

Hi,

On 10/26/2017 06:39 AM, Brian Norris wrote:
> On Wed, Oct 25, 2017 at 03:57:19AM -0400, Sean Paul wrote:
>> Archit asked a question about moving to
>> dw-mipi-dsi
> 
> That question made me think though: this approach seems backwards. It
> seems like someone did copy/paste/fork, and then we're asking the
> authors of the original driver to un-fork? It seems like this should
> happen the other way around -- those trying to support a new incarnation
> should have looked to try to abstract the original driver for their
> uses first.

Yes, ST wanted to replicate rockchip's version of the mipi DSI driver and
put it in their folder. If they did that, their KMS driver would have been
the third driver to implement a third instance of the DW DSI controller driver.
Hisilicon and Rockchip being the other 2.

It was either that or attempt at a common DSI DW bridge driver. I suggested
the latter.

The ST guys have abstracted out the PHY pieces, which they knew varied between
rockchip and ST. Ideally, they should have also tried to create a RFC patch to
make the rockchip driver use the bridge too. But they didn't do that, and
the rockchip or hisilicon people were interested in even looking at it,
even after I CC'ed them.

> 
> IIUC, that's exactly what Rockchip did for much of their Analogix eDP
> code -- they reworked the Exynos DP driver to split common Analogix code
> from any Exynos-specific bits.

I get that. I had hoped either ST or Rockchip guys would have done the similar
thing, but no one volunteered.

> 
> And actually, the current stuff in
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c is completely unused. It
> exports some functions, but I see no users of it. Is that intended? Is

The ST kms driver uses it:

drivers/gpu/drm/stm/dw_mipi_dsi-stm.c

> somebody already working on refactoring existing Rockchip code to use
> this?

I don't know. If rockchip isn't interested in doing it, we can check with
Philippe from ST if he can try creating a RFC that converts the rockchip
driver to use the dw-mipi-dsi driver.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Philippe CORNU]

Hi,

On 10/26/2017 06:13 AM, Archit Taneja wrote:
> Hi,
> 
> On 10/26/2017 06:39 AM, Brian Norris wrote:
>> On Wed, Oct 25, 2017 at 03:57:19AM -0400, Sean Paul wrote:
>>> Archit asked a question about moving to
>>> dw-mipi-dsi
>>
>> That question made me think though: this approach seems backwards. It
>> seems like someone did copy/paste/fork, and then we're asking the
>> authors of the original driver to un-fork? It seems like this should
>> happen the other way around -- those trying to support a new incarnation
>> should have looked to try to abstract the original driver for their
>> uses first.
> 
> Yes, ST wanted to replicate rockchip's version of the mipi DSI driver and
> put it in their folder. If they did that, their KMS driver would have been
> the third driver to implement a third instance of the DW DSI controller 
> driver.
> Hisilicon and Rockchip being the other 2.
> 
> It was either that or attempt at a common DSI DW bridge driver. I suggested
> the latter.
> 
> The ST guys have abstracted out the PHY pieces, which they knew varied 
> between
> rockchip and ST. Ideally, they should have also tried to create a RFC 
> patch to
> make the rockchip driver use the bridge too. But they didn't do that, and
> the rockchip or hisilicon people were interested in even looking at it,
> even after I CC'ed them.
> 
>>
>> IIUC, that's exactly what Rockchip did for much of their Analogix eDP
>> code -- they reworked the Exynos DP driver to split common Analogix code
>> from any Exynos-specific bits.
> 
> I get that. I had hoped either ST or Rockchip guys would have done the 
> similar
> thing, but no one volunteered.
> 
>>
>> And actually, the current stuff in
>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c is completely unused. It
>> exports some functions, but I see no users of it. Is that intended? Is
> 
> The ST kms driver uses it:
> 
> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> 

I confirm STM32 chipsets use the Synopsys dw dsi bridge driver.

I plan to improve this bridge driver by adding new features (see todos + 
dsi read, command mode with bta & gpio...).

For the first commit, I did my best to keep the source code as close as 
possible to the Rockchip version, in order to ease the port for Rockchip 
guys.

>> somebody already working on refactoring existing Rockchip code to use
>> this?
> 
> I don't know. If rockchip isn't interested in doing it, we can check with
> Philippe from ST if he can try creating a RFC that converts the rockchip
> driver to use the dw-mipi-dsi driver.

I am not really interested in doing this port for Rockchip (or Hisilicon 
or i.MX...) but happy to help anyone that wants to use the dw-mipi-dsi 
bridge driver :)

Many thanks,
Philippe :)

> 
> Thanks,
> Archit
> 

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Brian Norris]

(correction zyw's email ".comg" is not a TLD!)

Hi,

On Thu, Oct 26, 2017 at 09:44:14AM +0000, Philippe CORNU wrote:
> On 10/26/2017 06:13 AM, Archit Taneja wrote:
> > On 10/26/2017 06:39 AM, Brian Norris wrote:
> >> On Wed, Oct 25, 2017 at 03:57:19AM -0400, Sean Paul wrote:
> >>> Archit asked a question about moving to
> >>> dw-mipi-dsi
> >>
> >> That question made me think though: this approach seems backwards. It
> >> seems like someone did copy/paste/fork, and then we're asking the
> >> authors of the original driver to un-fork? It seems like this should
> >> happen the other way around -- those trying to support a new incarnation
> >> should have looked to try to abstract the original driver for their
> >> uses first.
> > 
> > Yes, ST wanted to replicate rockchip's version of the mipi DSI driver and
> > put it in their folder. If they did that, their KMS driver would have been
> > the third driver to implement a third instance of the DW DSI controller 
> > driver.
> > Hisilicon and Rockchip being the other 2.

I hadn't noticed Hisilicon's version. That's an unfortunate start :(

> > It was either that or attempt at a common DSI DW bridge driver. I suggested
> > the latter.
> > 
> > The ST guys have abstracted out the PHY pieces, which they knew varied 
> > between
> > rockchip and ST. Ideally, they should have also tried to create a RFC 
> > patch to
> > make the rockchip driver use the bridge too. But they didn't do that, and
> > the rockchip or hisilicon people were interested in even looking at it,
> > even after I CC'ed them.

I see. At least the current code from Philippe isn't that big, and it is
indeed fairly similar.

But I still think the way to get cooperation upstream is to enforce it;
so there was a 3rd option to the above -- don't merge *any* new driver
without posting at least an attempt to unify the existing drivers.

> >> IIUC, that's exactly what Rockchip did for much of their Analogix eDP
> >> code -- they reworked the Exynos DP driver to split common Analogix code
> >> from any Exynos-specific bits.
> > 
> > I get that. I had hoped either ST or Rockchip guys would have done the 
> > similar
> > thing, but no one volunteered.

:(

Nickey, can you confirm that you or someone on your team will look at
utilizing the common driver? Please reply to this email thread before
sending another version of this series.

> >> And actually, the current stuff in
> >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c is completely unused. It
> >> exports some functions, but I see no users of it. Is that intended? Is
> > 
> > The ST kms driver uses it:
> > 
> > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> > 
> 
> I confirm STM32 chipsets use the Synopsys dw dsi bridge driver.
> 
> I plan to improve this bridge driver by adding new features (see todos + 
> dsi read, command mode with bta & gpio...).
> 
> For the first commit, I did my best to keep the source code as close as 
> possible to the Rockchip version, in order to ease the port for Rockchip 
> guys.

That's nice to see, even if it still isn't ideal.

> >> somebody already working on refactoring existing Rockchip code to use
> >> this?
> > 
> > I don't know. If rockchip isn't interested in doing it, we can check with
> > Philippe from ST if he can try creating a RFC that converts the rockchip
> > driver to use the dw-mipi-dsi driver.
> 
> I am not really interested in doing this port for Rockchip (or Hisilicon 
> or i.MX...) but happy to help anyone that wants to use the dw-mipi-dsi 
> bridge driver :)

Well, see my comments above. Your "interest" is obviously in merging
code to support your own IP, but the community can *make* it your
interest by requiring you do the unification before your code goes
upstream. Apparently that's not the policy here though.

Brian

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Brian Norris]

On Thu, Oct 26, 2017 at 09:44:14AM +0000, Philippe CORNU wrote:
> On 10/26/2017 06:13 AM, Archit Taneja wrote:
> > On 10/26/2017 06:39 AM, Brian Norris wrote:
> >> somebody already working on refactoring existing Rockchip code to use
> >> this?
> > 
> > I don't know. If rockchip isn't interested in doing it, we can check with
> > Philippe from ST if he can try creating a RFC that converts the rockchip
> > driver to use the dw-mipi-dsi driver.
> 
> I am not really interested in doing this port for Rockchip (or Hisilicon 
> or i.MX...) but happy to help anyone that wants to use the dw-mipi-dsi 
> bridge driver :)

Ugh, this stuff is worse than I expected. I'm helping Rockchip along
with getting this rewritten, but there are some hiccups.

Among other things, the bridge driver is assuming it can set the
device's drvdata itself. This works because the STM MIPI driver is
simple, but the Rockchip one registers stuff via component_add(), and so
it *needs* to handle drvdata between probe() and bind()...but then the
"common" bridge driver is going to clobber it (dev_set_drvdata()).

Along the way, I'm noticing that the STM driver just steps around this
at times by referencing a static (!!) instance of its priv_data. See:

static int dw_mipi_dsi_stm_remove(struct platform_device *pdev)
{
	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
...


I might rewrite this, but it's not fun to have to fix somebody else's
fork for them...

Brian

Re: [PATCH v3 3/6] drm/rockchip/dsi: correct Feedback divider setting

[Brian Norris]

(Dropping Mark, whose Rockchip address is dead; and fixing Chris's email again)

On Mon, Nov 27, 2017 at 4:29 PM, Brian Norris <briannorris@chromium.org> wrote:
> On Thu, Oct 26, 2017 at 09:44:14AM +0000, Philippe CORNU wrote:
>> On 10/26/2017 06:13 AM, Archit Taneja wrote:
>> > On 10/26/2017 06:39 AM, Brian Norris wrote:
>> >> somebody already working on refactoring existing Rockchip code to use
>> >> this?
>> >
>> > I don't know. If rockchip isn't interested in doing it, we can check with
>> > Philippe from ST if he can try creating a RFC that converts the rockchip
>> > driver to use the dw-mipi-dsi driver.
>>
>> I am not really interested in doing this port for Rockchip (or Hisilicon
>> or i.MX...) but happy to help anyone that wants to use the dw-mipi-dsi
>> bridge driver :)
>
> Ugh, this stuff is worse than I expected. I'm helping Rockchip along
> with getting this rewritten, but there are some hiccups.
>
> Among other things, the bridge driver is assuming it can set the
> device's drvdata itself. This works because the STM MIPI driver is
> simple, but the Rockchip one registers stuff via component_add(), and so
> it *needs* to handle drvdata between probe() and bind()...but then the
> "common" bridge driver is going to clobber it (dev_set_drvdata()).
>
> Along the way, I'm noticing that the STM driver just steps around this
> at times by referencing a static (!!) instance of its priv_data. See:
>
> static int dw_mipi_dsi_stm_remove(struct platform_device *pdev)
> {
>         struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
> ...
>
>
> I might rewrite this, but it's not fun to have to fix somebody else's
> fork for them...
>
> Brian

[PATCH v3 4/6] drm/rockchip/dsi: add dual mipi channel support

[Nickey Yang]

This patch add dual mipi channel support:
1.add definition of dsi1 register and grf operation.
2.dsi0 and dsi1 will work in master and slave mode
when driving dual mipi panel.

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c      | 377 ++++++++++++++++++++--------
 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 +
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   1 +
 5 files changed, 279 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 589b420..25e7b77 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -39,8 +39,42 @@
 #define RK3399_DSI1_SEL_VOP_LIT		BIT(4)
 
 /* disable turnrequest, turndisable, forcetxstopmode, forcerxmode */
-#define RK3399_GRF_SOC_CON22		0x6258
-#define RK3399_GRF_DSI_MODE		0xffff0000
+#define RK3399_GRF_SOC_CON22			0x6258
+#define DPHY_TX0_TURNREQUEST_ENABLE		(0xf << 12)
+#define DPHY_TX0_TURNREQUEST_SET		(DPHY_TX0_TURNREQUEST_ENABLE << 16)
+#define DPHY_TX0_TURNDISABLE_ENABLE		(0xf << 8)
+#define DPHY_TX0_TURNDISABLE_SET		(DPHY_TX0_TURNDISABLE_ENABLE << 16)
+#define DPHY_TX0_FORCETXSTOPMODE_ENABLE		(0xf << 4)
+#define DPHY_TX0_FORCETXSTOPMODE_SET		(DPHY_TX0_FORCETXSTOPMODE_ENABLE << 16)
+#define DPHY_TX0_FORCETRXMODE_ENABLE		0xf
+#define DPHY_TX0_FORCETRXMODE_SET		(DPHY_TX0_FORCETRXMODE_ENABLE << 16)
+#define RK3399_GRF_DSI_MODE			(DPHY_TX0_TURNREQUEST_SET | \
+						 DPHY_TX0_TURNDISABLE_SET | \
+						 DPHY_TX0_FORCETXSTOPMODE_SET | \
+						 DPHY_TX0_FORCETRXMODE_SET)
+
+
+/* disable turndisable, forcetxstopmode, forcerxmode, enable */
+#define RK3399_GRF_SOC_CON23			0x625c
+#define DPHY_TX1RX1_TURNDISABLE_ENABLE		(0xf << 12)
+#define DPHY_TX1RX1_TURNDISABLE_SET		(DPHY_TX1RX1_TURNDISABLE_ENABLE << 16)
+#define DPHY_TX1RX1_FORCETXSTOPMODE_ENABLE	(0xf << 8)
+#define DPHY_TX1RX1_FORCETXSTOPMODE_SET		(DPHY_TX1RX1_FORCETXSTOPMODE_ENABLE << 16)
+#define DPHY_TX1RX1_FORCERXMODE_ENABLE		(0xf << 4)
+#define DPHY_TX1RX1_FORCERXMODE_SET		(DPHY_TX1RX1_FORCERXMODE_ENABLE << 16)
+#define DPHY_TX1RX1_ENABLE_ENABLE		0xf
+#define DPHY_TX1RX1_ENABLE_SET			(DPHY_TX1RX1_ENABLE_ENABLE << 16)
+#define RK3399_GRF_DSI1_MODE			(DPHY_TX1RX1_TURNDISABLE_SET | \
+						 DPHY_TX1RX1_FORCETXSTOPMODE_SET | \
+						 DPHY_TX1RX1_FORCERXMODE_SET | \
+						 DPHY_TX1RX1_ENABLE_SET)
+#define RK3399_GRF_DSI1_ENABLE			((DPHY_TX1RX1_ENABLE_SET | \
+						  DPHY_TX1RX1_ENABLE_ENABLE))
+
+#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 DSI_VERSION			0x00
 #define DSI_PWR_UP			0x04
@@ -315,6 +349,13 @@ 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_enable;
+	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;
 };
@@ -333,6 +374,10 @@ struct dw_mipi_dsi {
 	struct clk *pclk;
 	struct clk *phy_cfg_clk;
 
+	/* dual-channel */
+	struct dw_mipi_dsi *master;
+	struct dw_mipi_dsi *slave;
+
 	int dpms_mode;
 	unsigned int lane_mbps; /* per lane */
 	u32 channel;
@@ -617,6 +662,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 	unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps;
 	int bpp;
 	unsigned long best_freq = 0;
+	int lanes = dsi->lanes;
 	unsigned long fvco_min, fvco_max, fin, fout;
 	unsigned int min_prediv, max_prediv;
 	unsigned int _prediv, uninitialized_var(best_prediv);
@@ -631,10 +677,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
@@ -683,12 +732,14 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 			best_freq = tmp;
 		}
 	}
+
 	if (best_freq) {
 		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
 		dsi->input_div = best_prediv;
 		dsi->feedback_div = best_fbdiv;
 	} else
 		DRM_DEV_ERROR(dsi->dev, "Can not find best_freq for DPHY\n");
+
 	return 0;
 }
 
@@ -696,18 +747,25 @@ 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) {
 		DRM_DEV_ERROR(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);
@@ -840,15 +898,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:
 		DRM_DEV_ERROR(dsi->dev, "unsupported message type 0x%02x\n",
@@ -922,6 +987,56 @@ 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) {
+		DRM_DEV_ERROR(dsi->dev, "Failed to enable grf_clk: %d\n", ret);
+		return;
+	}
+
+	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) {
+			val = pdata->dsi1_masterslavez |
+			      (pdata->dsi1_masterslavez << 16) |
+			      (pdata->dsi1_basedir << 16);
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2,
+				     val);
+		}
+		if (pdata->grf_dsi1_mode_reg1)
+			regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,
+				     pdata->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)
 {
@@ -962,7 +1077,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);
+	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)
@@ -1067,24 +1189,27 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
 	dw_mipi_dsi_disable(dsi);
 	pm_runtime_put(dsi->dev);
 	clk_disable_unprepare(dsi->pclk);
+	if (dsi->slave) {
+		if (clk_prepare_enable(dsi->slave->pclk)) {
+			DRM_DEV_ERROR(dsi->slave->dev,
+				      "Failed to enable pclk\n");
+			return;
+		}
+		dw_mipi_dsi_disable(dsi->slave);
+		pm_runtime_put(dsi->slave->dev);
+		clk_disable_unprepare(dsi->slave->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) {
+		DRM_DEV_ERROR(dsi->dev, "Failed to get lane bps\n");
 		return;
+	}
 
 	if (clk_prepare_enable(dsi->pclk)) {
 		DRM_DEV_ERROR(dsi->dev, "Failed to enable pclk\n");
@@ -1104,44 +1229,43 @@ 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) {
-		DRM_DEV_ERROR(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);
+	DRM_DEV_DEBUG(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))
 		DRM_DEV_ERROR(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);
-	DRM_DEV_DEBUG(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
@@ -1169,6 +1293,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;
 }
 
@@ -1274,6 +1401,13 @@ 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_MODE,
+	.grf_dsi1_enable = RK3399_GRF_DSI1_ENABLE,
+	.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,
 };
@@ -1290,17 +1424,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) {
+		DRM_DEV_ERROR(dev, "Failed to enable pllref_clk: %d\n", ret);
+		return ret;
+	}
+
+	pm_runtime_enable(dev);
+
+	if (dsi->master)
+		return 0;
+
+	ret = dw_mipi_dsi_register(drm, dsi);
+	if (ret) {
+		DRM_DEV_ERROR(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) {
+		DRM_DEV_ERROR(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:
+	pm_runtime_disable(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);
@@ -1310,6 +1533,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)
@@ -1387,63 +1611,6 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,
 		}
 	}
 
-	ret = clk_prepare_enable(dsi->pllref_clk);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "Failed to enable pllref_clk\n");
-		return ret;
-	}
-
-	ret = dw_mipi_dsi_register(drm, dsi);
-	if (ret) {
-		DRM_DEV_ERROR(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) {
-		DRM_DEV_ERROR(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 498dfbc..f20fba2 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 19128b4..bb98bfc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -918,6 +918,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,
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index 4a39049..1e0c07b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -392,6 +392,7 @@
 	.hdmi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 13),
 	.edp_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 14),
 	.mipi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 15),
+	.mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3),
 };
 
 static const struct vop_data rk3399_vop_big = {
-- 
1.9.1

Re: [PATCH v3 4/6] drm/rockchip/dsi: add dual mipi channel support

[Sean Paul]

On Wed, Oct 25, 2017 at 11:51:01AM +0800, Nickey Yang wrote:
> This patch add dual mipi channel support:
> 1.add definition of dsi1 register and grf operation.
> 2.dsi0 and dsi1 will work in master and slave mode
> when driving dual mipi panel.
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---

In the last revision, I asked you to provide changelog between revisions,
*please* do this. You received review on the last version and a bunch of the
feedback hasn't been taken into account with no explanation as to why. Please go
back to the last version, read the reviews that people were generous enough to
give and either fix the code or explain why you're not.

>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c      | 377 ++++++++++++++++++++--------
>  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 +
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   1 +
>  5 files changed, 279 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 589b420..25e7b77 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c

<snip />

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

Archit asked you not to do this in the previous review.

> +
> +		dsi->slave->master = dsi;
> +	}
> +
> +	return 0;
> +}

<snip />

> -- 
> 1.9.1
> 

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

Re: [PATCH v3 4/6] drm/rockchip/dsi: add dual mipi channel support

[Archit Taneja]

On 10/25/2017 01:34 PM, Sean Paul wrote:
> On Wed, Oct 25, 2017 at 11:51:01AM +0800, Nickey Yang wrote:
>> This patch add dual mipi channel support:
>> 1.add definition of dsi1 register and grf operation.
>> 2.dsi0 and dsi1 will work in master and slave mode
>> when driving dual mipi panel.
>>
>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>> ---
> 
> In the last revision, I asked you to provide changelog between revisions,
> *please* do this. You received review on the last version and a bunch of the
> feedback hasn't been taken into account with no explanation as to why. Please go
> back to the last version, read the reviews that people were generous enough to
> give and either fix the code or explain why you're not.
> 
>>   drivers/gpu/drm/rockchip/dw-mipi-dsi.c      | 377 ++++++++++++++++++++--------
>>   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 +
>>   drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   1 +
>>   5 files changed, 279 insertions(+), 105 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>> index 589b420..25e7b77 100644
>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> 
> <snip />
> 
>> +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;
> 
> Archit asked you not to do this in the previous review.

I've replied with some more details to the patch which adds this in the DT
binding doc.

Thanks,
Archit

> 
>> +
>> +		dsi->slave->master = dsi;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> <snip />
> 
>> -- 
>> 1.9.1
>>
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 5/6] 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..a2bea22 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: phandle to a 2nd DSI channel, useful as a 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

Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi

[Archit Taneja]

On 10/25/2017 09:21 AM, 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..a2bea22 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: phandle to a 2nd DSI channel, useful as a slave
> +channel when driving a panel which needs 2 DSI links.
The example below is how dual DSI bindings could look like. Let me know what
you think of it.

If both DSI outputs drive the same device (i.e, point to the same panel DT
node), then I think it's reasonable enough to assume that the DSIs are
operating in a 'dual-channel' mode. That being said, we still need DT to
describe which of the DSIs generates the clock for both the channels. This
is done with the 'clock-master' DT binding.

Thanks,
Archit

mipi_dsi: mipi@ff960000 {
	...
	...

	clock-master;	/* implies that this DSI instance drivers the clock
			 * for both the DSIs.
			 */

	ports {
		mipi_in: port {
			...
			...
		};

		/* add extra output ports for both DSIs */
		mipi_out: port {
			mipi_panel_out: endpoint {
				remote-endpoint = <&panel_in_channel0>;
			};
		};
	};

	panel {
		...
		...
		/*
		 * panel node can describe its input ports, if both the DSIs output
		 * ports are connected to the same device (i.e, the same DSI panel),
		 * we can assume that the DSIs need to operate in dual DSI mode
		 */
		ports {
			...
			port@0 {
				panel_in_channel0: endpoint {
					remote-endpoint = <&mipi_panel_out>;
				};
			};

			port@1 {
				panel_in_channel1: endpoint {
					remote-endpoint = <&mipi1_panel_out>;
				};
	
			};
		};
	};
};


mipi_dsi1: mipi@ff968000 {
	...
	...

	ports {
		mipi1_in: port {
			...
			...
		};

		mipi1_out: port {
			mipi1_panel_out: endpoint {
				remote-endpoint = <&panel_in_channel1>;
			};
		};
	};
};

>   
>   [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi

[Nickey Yang]

Hi Archit,


On 2017年10月26日 12:53, Archit Taneja wrote:
>
>
> On 10/25/2017 09:21 AM, 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..a2bea22 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: phandle to a 2nd DSI channel, useful as a 
>> slave
>> +channel when driving a panel which needs 2 DSI links.
> The example below is how dual DSI bindings could look like. Let me 
> know what
> you think of it.
>
> If both DSI outputs drive the same device (i.e, point to the same 
> panel DT
> node), then I think it's reasonable enough to assume that the DSIs are
> operating in a 'dual-channel' mode. That being said, we still need DT to
> describe which of the DSIs generates the clock for both the channels. 
> This
> is done with the 'clock-master' DT binding.
>
> Thanks,
> Archit
>
> mipi_dsi: mipi@ff960000 {
>     ...
>     ...
>
>     clock-master;    /* implies that this DSI instance drivers the clock
>              * for both the DSIs.
>              */
>
>     ports {
>         mipi_in: port {
>             ...
>             ...
>         };
>
>         /* add extra output ports for both DSIs */
>         mipi_out: port {
>             mipi_panel_out: endpoint {
>                 remote-endpoint = <&panel_in_channel0>;
>             };
>         };
>     };
>
>     panel {
>         ...
>         ...
>         /*
>          * panel node can describe its input ports, if both the DSIs 
> output
>          * ports are connected to the same device (i.e, the same DSI 
> panel),
>          * we can assume that the DSIs need to operate in dual DSI mode
>          */
>         ports {
>             ...
>             port@0 {
>                 panel_in_channel0: endpoint {
>                     remote-endpoint = <&mipi_panel_out>;
>                 };
>             };
>
>             port@1 {
>                 panel_in_channel1: endpoint {
>                     remote-endpoint = <&mipi1_panel_out>;
>                 };
>
>             };
>         };
>     };
> };
>
>
> mipi_dsi1: mipi@ff968000 {
>     ...
>     ...
>
>     ports {
>         mipi1_in: port {
>             ...
>             ...
>         };
>
>         mipi1_out: port {
>             mipi1_panel_out: endpoint {
>                 remote-endpoint = <&panel_in_channel1>;
>             };
>         };
>     };
> };
>
I try to follow as you suggested,use

mipi_dsi: mipi@ff960000 {
     ...
     ...
     clock-master;    /* implies that this DSI instance drivers the clock
              * for both the DSIs.
              */
     ports {
         mipi_in: port {
             ...
             ...
         };
         /* add extra output ports for both DSIs */
         mipi_out: port {
             mipi_panel_out: endpoint {
                 remote-endpoint = <&panel_in_channel0>;
             };
         };
     };
     panel {
         ...
         ...
         /*
          * panel node can describe its input ports, if both the DSIs output
          * ports are connected to the same device (i.e, the same DSI 
panel),
          * we can assume that the DSIs need to operate in dual DSI mode
          */
         ports {
             ...
             port@0 {
                 panel_in_channel0: endpoint {
                     remote-endpoint = <&mipi_panel_out>;
                 };
             };
             port@1 {
                 panel_in_channel1: endpoint {
                     remote-endpoint = <&mipi1_panel_out>;
                 };

             };
         };
     };
};

mipi_dsi1: mipi@ff968000 {
     ...
     ...
     ports {
         mipi1_in: port {
             ...
             ...
         };
         mipi1_out: port {
             mipi1_panel_out: endpoint {
                 remote-endpoint = <&panel_in_channel1>;
             };
         };
     };
}

But it seems we can not use of_drm_find_panel(like below)

/*
         port = of_graph_get_port_by_id(dev->of_node, 1);
         if (port) {
                 endpoint = of_get_child_by_name(port, "endpoint");
                 of_node_put(port);
                 if (!endpoint) {
                         dev_err(dev, "no output endpoint found\n");
                         return -EINVAL;
                 }
                 panel_node = of_graph_get_remote_port_parent(endpoint);
                 of_node_put(endpoint);
                 if (!panel_node) {
                         dev_err(dev, "no output node found\n");
                         return -EINVAL;
                 }
                 panel = of_drm_find_panel(panel_node);
                 of_node_put(panel_node);
                 if (!panel)
                         return -EPROBE_DEFER;
         }
*/
to get DSI1 outputs,because of_drm_find_panel need compare

if (panel->dev->of_node == np)

in dsi_panel driver innolux->base.dev = &innolux->link->dev;
dsi->dev

struct innolux_panel {
         struct drm_panel base;
         struct mipi_dsi_device *link;
};
It means one panel can only be found in his dsi node,(like dsi0 above).

I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
(drivers/gpu/drm/tergra/dsi.c) method.


Thanks,
Nickey

>>     [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt
>>
>

Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi

[Archit Taneja]

On 11/30/2017 11:02 PM, Nickey Yang wrote:
> Hi Archit,
> 
> 
> On 2017年10月26日 12:53, Archit Taneja wrote:
>>
>>
>> On 10/25/2017 09:21 AM, 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..a2bea22 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: phandle to a 2nd DSI channel, useful as a slave
>>> +channel when driving a panel which needs 2 DSI links.
>> The example below is how dual DSI bindings could look like. Let me know what
>> you think of it.
>>
>> If both DSI outputs drive the same device (i.e, point to the same panel DT
>> node), then I think it's reasonable enough to assume that the DSIs are
>> operating in a 'dual-channel' mode. That being said, we still need DT to
>> describe which of the DSIs generates the clock for both the channels. This
>> is done with the 'clock-master' DT binding.
>>
>> Thanks,
>> Archit
>>
>> mipi_dsi: mipi@ff960000 {
>>     ...
>>     ...
>>
>>     clock-master;    /* implies that this DSI instance drivers the clock
>>              * for both the DSIs.
>>              */
>>
>>     ports {
>>         mipi_in: port {
>>             ...
>>             ...
>>         };
>>
>>         /* add extra output ports for both DSIs */
>>         mipi_out: port {
>>             mipi_panel_out: endpoint {
>>                 remote-endpoint = <&panel_in_channel0>;
>>             };
>>         };
>>     };
>>
>>     panel {
>>         ...
>>         ...
>>         /*
>>          * panel node can describe its input ports, if both the DSIs output
>>          * ports are connected to the same device (i.e, the same DSI panel),
>>          * we can assume that the DSIs need to operate in dual DSI mode
>>          */
>>         ports {
>>             ...
>>             port@0 {
>>                 panel_in_channel0: endpoint {
>>                     remote-endpoint = <&mipi_panel_out>;
>>                 };
>>             };
>>
>>             port@1 {
>>                 panel_in_channel1: endpoint {
>>                     remote-endpoint = <&mipi1_panel_out>;
>>                 };
>>
>>             };
>>         };
>>     };
>> };
>>
>>
>> mipi_dsi1: mipi@ff968000 {
>>     ...
>>     ...
>>
>>     ports {
>>         mipi1_in: port {
>>             ...
>>             ...
>>         };
>>
>>         mipi1_out: port {
>>             mipi1_panel_out: endpoint {
>>                 remote-endpoint = <&panel_in_channel1>;
>>             };
>>         };
>>     };
>> };
>>
> I try to follow as you suggested,use
> 
> mipi_dsi: mipi@ff960000 {
>      ...
>      ...
>      clock-master;    /* implies that this DSI instance drivers the clock
>               * for both the DSIs.
>               */
>      ports {
>          mipi_in: port {
>              ...
>              ...
>          };
>          /* add extra output ports for both DSIs */
>          mipi_out: port {
>              mipi_panel_out: endpoint {
>                  remote-endpoint = <&panel_in_channel0>;
>              };
>          };
>      };
>      panel {
>          ...
>          ...
>          /*
>           * panel node can describe its input ports, if both the DSIs output
>           * ports are connected to the same device (i.e, the same DSI panel),
>           * we can assume that the DSIs need to operate in dual DSI mode
>           */
>          ports {
>              ...
>              port@0 {
>                  panel_in_channel0: endpoint {
>                      remote-endpoint = <&mipi_panel_out>;
>                  };
>              };
>              port@1 {
>                  panel_in_channel1: endpoint {
>                      remote-endpoint = <&mipi1_panel_out>;
>                  };
> 
>              };
>          };
>      };
> };
> 
> mipi_dsi1: mipi@ff968000 {
>      ...
>      ...
>      ports {
>          mipi1_in: port {
>              ...
>              ...
>          };
>          mipi1_out: port {
>              mipi1_panel_out: endpoint {
>                  remote-endpoint = <&panel_in_channel1>;
>              };
>          };
>      };
> }
> 
> But it seems we can not use of_drm_find_panel(like below)
> 
> /*
>          port = of_graph_get_port_by_id(dev->of_node, 1);
>          if (port) {
>                  endpoint = of_get_child_by_name(port, "endpoint");
>                  of_node_put(port);
>                  if (!endpoint) {
>                          dev_err(dev, "no output endpoint found\n");
>                          return -EINVAL;
>                  }
>                  panel_node = of_graph_get_remote_port_parent(endpoint);
>                  of_node_put(endpoint);
>                  if (!panel_node) {
>                          dev_err(dev, "no output node found\n");
>                          return -EINVAL;
>                  }
>                  panel = of_drm_find_panel(panel_node);
>                  of_node_put(panel_node);
>                  if (!panel)
>                          return -EPROBE_DEFER;
>          }
> */
> to get DSI1 outputs,because of_drm_find_panel need compare
> 
> if (panel->dev->of_node == np)
> 
> in dsi_panel driver innolux->base.dev = &innolux->link->dev;
> dsi->dev

Yes, we should only have 1 drm_panel in the global panel list.
Shouldn't it be possible to modify the dsi driver such that dsi1
doesn't care whether it has a drm_panel for it or not, if we are
in dual dsi mode?

I imagine a sequence like this:

1. dsi0 probes, parses the of-graph, finds the panel and saves its device
node.

2. dsi1 probes, parses the of-graph, find the panel's device node
   - dsi1 checks if it is the same as the panel attached to dsi0.
   - If so, it just takes the drm_panel pointer from dsi0.
   - If not, it tries a of_drm_find_panel() on the panel's device node.

A dual DSI panel driver would also be a bit different. It will be a
mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
the of-graph helpers, we would get the device node of dsi1 using
of_find_mipi_dsi_host_by_node(), and create another DSI device using
mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
both the dsi devices.

> 
> struct innolux_panel {
>          struct drm_panel base;
>          struct mipi_dsi_device *link;
> };
> It means one panel can only be found in his dsi node,(like dsi0 above).
> 
> I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
> (drivers/gpu/drm/tergra/dsi.c) method.

This method will add a new binding similar to "nvidia,ganged-mode", which
is something we don't want to do.

Archit

> 
> 
> Thanks,
> Nickey
> 
>>>     [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt
>>>
>>
> 
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi

[Brian Norris]

Hi Archit,

I'm a relative n00b here, but I'm trying to follow along and I have some
questions:

On Fri, Dec 01, 2017 at 06:29:04PM +0530, Archit Taneja wrote:
> On 11/30/2017 11:02 PM, Nickey Yang wrote:
> >I try to follow as you suggested,use
> >
> >mipi_dsi: mipi@ff960000 {
> >     ...
> >     ...
> >     clock-master;    /* implies that this DSI instance drivers the clock
> >              * for both the DSIs.
> >              */
> >     ports {
> >         mipi_in: port {
> >             ...
> >             ...
> >         };
> >         /* add extra output ports for both DSIs */
> >         mipi_out: port {
> >             mipi_panel_out: endpoint {
> >                 remote-endpoint = <&panel_in_channel0>;
> >             };
> >         };
> >     };
> >     panel {
> >         ...
> >         ...
> >         /*
> >          * panel node can describe its input ports, if both the DSIs output
> >          * ports are connected to the same device (i.e, the same DSI panel),
> >          * we can assume that the DSIs need to operate in dual DSI mode
> >          */
> >         ports {
> >             ...
> >             port@0 {
> >                 panel_in_channel0: endpoint {
> >                     remote-endpoint = <&mipi_panel_out>;
> >                 };
> >             };
> >             port@1 {
> >                 panel_in_channel1: endpoint {
> >                     remote-endpoint = <&mipi1_panel_out>;
> >                 };
> >
> >             };
> >         };
> >     };
> >};
> >
> >mipi_dsi1: mipi@ff968000 {
> >     ...
> >     ...
> >     ports {
> >         mipi1_in: port {
> >             ...
> >             ...
> >         };
> >         mipi1_out: port {
> >             mipi1_panel_out: endpoint {
> >                 remote-endpoint = <&panel_in_channel1>;
> >             };
> >         };
> >     };
> >}
> >
> >But it seems we can not use of_drm_find_panel(like below)
> >
> >/*
> >         port = of_graph_get_port_by_id(dev->of_node, 1);
> >         if (port) {
> >                 endpoint = of_get_child_by_name(port, "endpoint");
> >                 of_node_put(port);
> >                 if (!endpoint) {
> >                         dev_err(dev, "no output endpoint found\n");
> >                         return -EINVAL;
> >                 }
> >                 panel_node = of_graph_get_remote_port_parent(endpoint);
> >                 of_node_put(endpoint);
> >                 if (!panel_node) {
> >                         dev_err(dev, "no output node found\n");
> >                         return -EINVAL;
> >                 }
> >                 panel = of_drm_find_panel(panel_node);
> >                 of_node_put(panel_node);
> >                 if (!panel)
> >                         return -EPROBE_DEFER;
> >         }
> >*/
> >to get DSI1 outputs,because of_drm_find_panel need compare
> >
> >if (panel->dev->of_node == np)
> >
> >in dsi_panel driver innolux->base.dev = &innolux->link->dev;
> >dsi->dev
> 
> Yes, we should only have 1 drm_panel in the global panel list.
> Shouldn't it be possible to modify the dsi driver such that dsi1
> doesn't care whether it has a drm_panel for it or not, if we are
> in dual dsi mode?
> 
> I imagine a sequence like this:
> 
> 1. dsi0 probes, parses the of-graph, finds the panel and saves its device
> node.

Does this mean we depend on probe order? Or would we be able to
-EPROBE_DEFER or similar if dsi1 binds first?

> 2. dsi1 probes, parses the of-graph, find the panel's device node
>   - dsi1 checks if it is the same as the panel attached to dsi0.
>   - If so, it just takes the drm_panel pointer from dsi0.
>   - If not, it tries a of_drm_find_panel() on the panel's device node.

So, that all means we'd need a new variant of
drm_of_find_panel_or_bridge() for "dual" drivers like this? Or else
open-code this logic in dw-mipi-dsi.c?

> A dual DSI panel driver would also be a bit different. It will be a
> mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
> the of-graph helpers, we would get the device node of dsi1 using
> of_find_mipi_dsi_host_by_node(), and create another DSI device using
> mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
> both the dsi devices.

That seems...interesting. I guess that sounds like it could work, but
someone would have to play with that a bit more.

I assume one wouldn't want to do all this in every dual DSI driver that
needs this, right?

> >struct innolux_panel {
> >         struct drm_panel base;
> >         struct mipi_dsi_device *link;
> >};
> >It means one panel can only be found in his dsi node,(like dsi0 above).
> >
> >I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
> >(drivers/gpu/drm/tergra/dsi.c) method.
> 
> This method will add a new binding similar to "nvidia,ganged-mode", which
> is something we don't want to do.

It's unfortunate we have the anti-pattern already merged :(

Brian

Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi

[Archit Taneja]

On 12/05/2017 06:49 AM, Brian Norris wrote:
> Hi Archit,
> 
> I'm a relative n00b here, but I'm trying to follow along and I have some
> questions:
> 
> On Fri, Dec 01, 2017 at 06:29:04PM +0530, Archit Taneja wrote:
>> On 11/30/2017 11:02 PM, Nickey Yang wrote:
>>> I try to follow as you suggested,use
>>>
>>> mipi_dsi: mipi@ff960000 {
>>>      ...
>>>      ...
>>>      clock-master;    /* implies that this DSI instance drivers the clock
>>>               * for both the DSIs.
>>>               */
>>>      ports {
>>>          mipi_in: port {
>>>              ...
>>>              ...
>>>          };
>>>          /* add extra output ports for both DSIs */
>>>          mipi_out: port {
>>>              mipi_panel_out: endpoint {
>>>                  remote-endpoint = <&panel_in_channel0>;
>>>              };
>>>          };
>>>      };
>>>      panel {
>>>          ...
>>>          ...
>>>          /*
>>>           * panel node can describe its input ports, if both the DSIs output
>>>           * ports are connected to the same device (i.e, the same DSI panel),
>>>           * we can assume that the DSIs need to operate in dual DSI mode
>>>           */
>>>          ports {
>>>              ...
>>>              port@0 {
>>>                  panel_in_channel0: endpoint {
>>>                      remote-endpoint = <&mipi_panel_out>;
>>>                  };
>>>              };
>>>              port@1 {
>>>                  panel_in_channel1: endpoint {
>>>                      remote-endpoint = <&mipi1_panel_out>;
>>>                  };
>>>
>>>              };
>>>          };
>>>      };
>>> };
>>>
>>> mipi_dsi1: mipi@ff968000 {
>>>      ...
>>>      ...
>>>      ports {
>>>          mipi1_in: port {
>>>              ...
>>>              ...
>>>          };
>>>          mipi1_out: port {
>>>              mipi1_panel_out: endpoint {
>>>                  remote-endpoint = <&panel_in_channel1>;
>>>              };
>>>          };
>>>      };
>>> }
>>>
>>> But it seems we can not use of_drm_find_panel(like below)
>>>
>>> /*
>>>          port = of_graph_get_port_by_id(dev->of_node, 1);
>>>          if (port) {
>>>                  endpoint = of_get_child_by_name(port, "endpoint");
>>>                  of_node_put(port);
>>>                  if (!endpoint) {
>>>                          dev_err(dev, "no output endpoint found\n");
>>>                          return -EINVAL;
>>>                  }
>>>                  panel_node = of_graph_get_remote_port_parent(endpoint);
>>>                  of_node_put(endpoint);
>>>                  if (!panel_node) {
>>>                          dev_err(dev, "no output node found\n");
>>>                          return -EINVAL;
>>>                  }
>>>                  panel = of_drm_find_panel(panel_node);
>>>                  of_node_put(panel_node);
>>>                  if (!panel)
>>>                          return -EPROBE_DEFER;
>>>          }
>>> */
>>> to get DSI1 outputs,because of_drm_find_panel need compare
>>>
>>> if (panel->dev->of_node == np)
>>>
>>> in dsi_panel driver innolux->base.dev = &innolux->link->dev;
>>> dsi->dev
>>
>> Yes, we should only have 1 drm_panel in the global panel list.
>> Shouldn't it be possible to modify the dsi driver such that dsi1
>> doesn't care whether it has a drm_panel for it or not, if we are
>> in dual dsi mode?
>>
>> I imagine a sequence like this:
>>
>> 1. dsi0 probes, parses the of-graph, finds the panel and saves its device
>> node.
> 
> Does this mean we depend on probe order? Or would we be able to
> -EPROBE_DEFER or similar if dsi1 binds first?

I don't think the probe order should matter. However, I also don't know what
challenges it might bring up once we actually try to implement it. I can see
the the driver's of-graph parsing code getting a bit complicated. The first dsi
instance that probes/binds (say dsi1) should peek into the panels other ports
and see if it is the slave DSI instance in a dual DSI set-up. If so, it could
defer until DSI0 first probes and registers the panel.

Btw, full disclosure, I work on the drm/msm driver, and the code uses a binding
called "qcom,dual-dsi-mode" done by someone in the past, but thankfully it isn't
used in any dts file. I plan to remove these and use the bindings I've suggested
here.

Also, the bindings I've shared above are more a proof of concept, and based on
how dual DSI is implemented on the MSM chipsets. If the HW requires special
properties while operating in Dual DSI mode, then it might be okay to have
additional bindings. However, it seems strange to have a DT prop that says
"operate in dual DSI mode" if it can be inferred from the port connections.

I'll post a RFC explaining the bindings and copy all the people with kms drivers
that support DSI. Maybe we'd come up with a better consensus.

> 
>> 2. dsi1 probes, parses the of-graph, find the panel's device node
>>    - dsi1 checks if it is the same as the panel attached to dsi0.
>>    - If so, it just takes the drm_panel pointer from dsi0.
>>    - If not, it tries a of_drm_find_panel() on the panel's device node.
> 
> So, that all means we'd need a new variant of
> drm_of_find_panel_or_bridge() for "dual" drivers like this? Or else
> open-code this logic in dw-mipi-dsi.c?

Yeah. It would be nice to have these in helpers.

> 
>> A dual DSI panel driver would also be a bit different. It will be a
>> mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
>> the of-graph helpers, we would get the device node of dsi1 using
>> of_find_mipi_dsi_host_by_node(), and create another DSI device using
>> mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
>> both the dsi devices.
> 
> That seems...interesting. I guess that sounds like it could work, but
> someone would have to play with that a bit more.
> 
> I assume one wouldn't want to do all this in every dual DSI driver that
> needs this, right?

Yes. I agree.

> 
>>> struct innolux_panel {
>>>          struct drm_panel base;
>>>          struct mipi_dsi_device *link;
>>> };
>>> It means one panel can only be found in his dsi node,(like dsi0 above).
>>>
>>> I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
>>> (drivers/gpu/drm/tergra/dsi.c) method.
>>
>> This method will add a new binding similar to "nvidia,ganged-mode", which
>> is something we don't want to do.

Btw, we already have a dual DSI panel driver, which has a special phandle called
"link2":

Documentation/devicetree/bindings/display/panel/sharp,lq101r1sx01.txt

I don't think we should continue using a prop like link2, it seems like something
that should be replaced by of-graph usage.

Thanks,
Archit

> 
> It's unfortunate we have the anti-pattern already merged :(
> 
> Brian
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 6/6] arm64: dts: rockchip: add mipi_dsi1 support for rk3399

[Nickey Yang]

This patch adds the mipi_dsi1 related needed information.
e.g.: interrupts, grf, clocks, ports and so on.

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

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index a65f7f7..48e2695 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1515,6 +1515,11 @@
 				reg = <2>;
 				remote-endpoint = <&hdmi_in_vopl>;
 			};
+
+			vopl_out_mipi1: endpoint@3 {
+				reg = <3>;
+				remote-endpoint = <&mipi1_in_vopl>;
+			};
 		};
 	};
 
@@ -1562,6 +1567,11 @@
 				reg = <2>;
 				remote-endpoint = <&hdmi_in_vopb>;
 			};
+
+			vopb_out_mipi1: endpoint@3 {
+				reg = <3>;
+				remote-endpoint = <&mipi1_in_vopb>;
+			};
 		};
 	};
 
@@ -1657,6 +1667,35 @@
 		};
 	};
 
+	mipi_dsi1: mipi@ff968000 {
+		compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi";
+		reg = <0x0 0xff968000 0x0 0x8000>;
+		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI1>,
+			 <&cru SCLK_DPHY_TX1RX1_CFG>, <&cru PCLK_VIO_GRF>;
+		clock-names = "ref", "pclk", "phy_cfg", "grf";
+		power-domains = <&power RK3399_PD_VIO>;
+		rockchip,grf = <&grf>;
+		status = "disabled";
+
+		ports {
+			mipi1_in: port {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				mipi1_in_vopb: endpoint@0 {
+					reg = <0>;
+					remote-endpoint = <&vopb_out_mipi1>;
+				};
+
+				mipi1_in_vopl: endpoint@1 {
+					reg = <1>;
+					remote-endpoint = <&vopl_out_mipi1>;
+				};
+			};
+		};
+	};
+
 	edp: edp@ff970000 {
 		compatible = "rockchip,rk3399-edp";
 		reg = <0x0 0xff970000 0x0 0x8000>;
-- 
1.9.1

Re: [PATCH v3 1/6] drm/rockchip/dsi: Define and use macros for PHY register addresses

[Sean Paul]

On Wed, Oct 25, 2017 at 11:50:58AM +0800, Nickey Yang wrote:
> Replace the hardcoded register address numerical values with macros to
> clarify the code.
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 129 ++++++++++++++++++++++-----------
>  1 file changed, 85 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index b15755b..95ce253 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -254,6 +254,28 @@
>  #define DW_MIPI_NEEDS_PHY_CFG_CLK	BIT(0)
>  #define DW_MIPI_NEEDS_GRF_CLK		BIT(1)
>  
> +#define PLL_BIAS_CUR_SEL_CAP_VCO_CONTROL	0x10
> +#define PLL_CP_CONTROL_PLL_LOCK_BYPASS	0x11
> +#define PLL_LPF_AND_CP_CONTROL	0x12
> +#define PLL_INPUT_DIVIDER_RATIO	0x17
> +#define PLL_LOOP_DIVIDER_RATIO	0x18
> +#define PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL	0x19
> +#define BANDGAP_AND_BIAS_CONTROL	0x20
> +#define TERMINATION_RESISTER_CONTROL	0x21
> +#define AFE_BIAS_BANDGAP_ANALOG_PROGRAMMABILITY	0x22
> +#define HS_RX_CONTROL_OF_LANE_0	0x44
> +#define HS_TX_CLOCK_LANE_REQUEST_STATE_TIME_CONTROL	0x60
> +#define HS_TX_CLOCK_LANE_PREPARE_STATE_TIME_CONTROL	0x61
> +#define HS_TX_CLOCK_LANE_HS_ZERO_STATE_TIME_CONTROL	0x62
> +#define HS_TX_CLOCK_LANE_TRAIL_STATE_TIME_CONTROL	0x63
> +#define HS_TX_CLOCK_LANE_EXIT_STATE_TIME_CONTROL	0x64
> +#define HS_TX_CLOCK_LANE_POST_TIME_CONTROL	0x65
> +#define HS_TX_DATA_LANE_REQUEST_STATE_TIME_CONTROL	0x70
> +#define HS_TX_DATA_LANE_PREPARE_STATE_TIME_CONTROL	0x71
> +#define HS_TX_DATA_LANE_HS_ZERO_STATE_TIME_CONTROL	0x72
> +#define HS_TX_DATA_LANE_TRAIL_STATE_TIME_CONTROL	0x73
> +#define HS_TX_DATA_LANE_EXIT_STATE_TIME_CONTROL	0x74
> +
>  enum {
>  	BANDGAP_97_07,
>  	BANDGAP_98_05,
> @@ -447,53 +469,72 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  		return ret;
>  	}
>  
> -	dw_mipi_dsi_phy_write(dsi, 0x10, BYPASS_VCO_RANGE |
> -					 VCO_RANGE_CON_SEL(vco) |
> -					 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, 0x12, CP_PROGRAM_EN | LPF_PROGRAM_EN |
> -					 LPF_RESISTORS_20_KOHM);
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin));
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div));
> -	dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) |
> -					 LOW_PROGRAM_EN);
> -	dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
> -					 HIGH_PROGRAM_EN);
> -	dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x22, LOW_PROGRAM_EN |
> -					 BIASEXTR_SEL(BIASEXTR_127_7));
> -	dw_mipi_dsi_phy_write(dsi, 0x22, HIGH_PROGRAM_EN |
> -					 BANDGAP_SEL(BANDGAP_96_10));
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x20, POWER_CONTROL | INTERNAL_REG_CURRENT |
> -					 BIAS_BLOCK_ON | BANDGAP_ON);
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x21, TER_RESISTOR_LOW | TER_CAL_DONE |
> -					 SETRD_MAX | TER_RESISTORS_ON);
> -	dw_mipi_dsi_phy_write(dsi, 0x21, TER_RESISTOR_HIGH | LEVEL_SHIFTERS_ON |
> -					 SETRD_MAX | POWER_MANAGE |
> -					 TER_RESISTORS_ON);
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x60, TLP_PROGRAM_EN | ns2bc(dsi, 500));
> -	dw_mipi_dsi_phy_write(dsi, 0x61, THS_PRE_PROGRAM_EN | ns2ui(dsi, 40));
> -	dw_mipi_dsi_phy_write(dsi, 0x62, THS_ZERO_PROGRAM_EN | ns2bc(dsi, 300));
> -	dw_mipi_dsi_phy_write(dsi, 0x63, THS_PRE_PROGRAM_EN | ns2ui(dsi, 100));
> -	dw_mipi_dsi_phy_write(dsi, 0x64, BIT(5) | ns2bc(dsi, 100));
> -	dw_mipi_dsi_phy_write(dsi, 0x65, BIT(5) | (ns2bc(dsi, 60) + 7));
> -
> -	dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | ns2bc(dsi, 500));
> -	dw_mipi_dsi_phy_write(dsi, 0x71,
> +	dw_mipi_dsi_phy_write(dsi, PLL_BIAS_CUR_SEL_CAP_VCO_CONTROL,
> +			      BYPASS_VCO_RANGE |
> +			      VCO_RANGE_CON_SEL(vco) |
> +			      VCO_IN_CAP_CON_LOW |
> +			      REF_BIAS_CUR_SEL);
> +
> +	dw_mipi_dsi_phy_write(dsi, PLL_CP_CONTROL_PLL_LOCK_BYPASS,
> +			      CP_CURRENT_3MA);
> +	dw_mipi_dsi_phy_write(dsi, PLL_LPF_AND_CP_CONTROL,
> +			      CP_PROGRAM_EN | LPF_PROGRAM_EN |
> +			      LPF_RESISTORS_20_KOHM);
> +
> +	dw_mipi_dsi_phy_write(dsi, HS_RX_CONTROL_OF_LANE_0,
> +			      HSFREQRANGE_SEL(testdin));
> +
> +	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_DIVIDER_RATIO,
> +			      INPUT_DIVIDER(dsi->input_div));
> +	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
> +			      LOOP_DIV_LOW_SEL(dsi->feedback_div) |
> +			      LOW_PROGRAM_EN);
> +	dw_mipi_dsi_phy_write(dsi, PLL_LOOP_DIVIDER_RATIO,
> +			      LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
> +			      HIGH_PROGRAM_EN);
> +	dw_mipi_dsi_phy_write(dsi, PLL_INPUT_AND_LOOP_DIVIDER_RATIOS_CONTROL,
> +			      PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> +
> +	dw_mipi_dsi_phy_write(dsi, AFE_BIAS_BANDGAP_ANALOG_PROGRAMMABILITY,
> +			      LOW_PROGRAM_EN | BIASEXTR_SEL(BIASEXTR_127_7));
> +	dw_mipi_dsi_phy_write(dsi, AFE_BIAS_BANDGAP_ANALOG_PROGRAMMABILITY,
> +			      HIGH_PROGRAM_EN | BANDGAP_SEL(BANDGAP_96_10));
> +
> +	dw_mipi_dsi_phy_write(dsi, BANDGAP_AND_BIAS_CONTROL,
> +			      POWER_CONTROL | INTERNAL_REG_CURRENT |
> +			      BIAS_BLOCK_ON | BANDGAP_ON);
> +
> +	dw_mipi_dsi_phy_write(dsi, TERMINATION_RESISTER_CONTROL,
> +			      TER_RESISTOR_LOW | TER_CAL_DONE |
> +			      SETRD_MAX | TER_RESISTORS_ON);
> +	dw_mipi_dsi_phy_write(dsi, TERMINATION_RESISTER_CONTROL,
> +			      TER_RESISTOR_HIGH | LEVEL_SHIFTERS_ON |
> +			      SETRD_MAX | POWER_MANAGE |
> +			      TER_RESISTORS_ON);
> +
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_REQUEST_STATE_TIME_CONTROL,
> +			      TLP_PROGRAM_EN | ns2bc(dsi, 500));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_PREPARE_STATE_TIME_CONTROL,
> +			      THS_PRE_PROGRAM_EN | ns2ui(dsi, 40));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_HS_ZERO_STATE_TIME_CONTROL,
> +			      THS_ZERO_PROGRAM_EN | ns2bc(dsi, 300));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_TRAIL_STATE_TIME_CONTROL,
> +			      THS_PRE_PROGRAM_EN | ns2ui(dsi, 100));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_EXIT_STATE_TIME_CONTROL,
> +			      BIT(5) | ns2bc(dsi, 100));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_CLOCK_LANE_POST_TIME_CONTROL,
> +			      BIT(5) | (ns2bc(dsi, 60) + 7));
> +
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_REQUEST_STATE_TIME_CONTROL,
> +			      TLP_PROGRAM_EN | ns2bc(dsi, 500));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_PREPARE_STATE_TIME_CONTROL,
>  			      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 50) + 5));
> -	dw_mipi_dsi_phy_write(dsi, 0x72,
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_HS_ZERO_STATE_TIME_CONTROL,
>  			      THS_ZERO_PROGRAM_EN | (ns2bc(dsi, 140) + 2));
> -	dw_mipi_dsi_phy_write(dsi, 0x73,
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_TRAIL_STATE_TIME_CONTROL,
>  			      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 60) + 8));
> -	dw_mipi_dsi_phy_write(dsi, 0x74, BIT(5) | ns2bc(dsi, 100));
> +	dw_mipi_dsi_phy_write(dsi, HS_TX_DATA_LANE_EXIT_STATE_TIME_CONTROL,
> +			      BIT(5) | ns2bc(dsi, 100));
>  
>  	dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
>  				     PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
> -- 
> 1.9.1
> 

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