[PATCH v2 0/7] drm/i915: Baytrail MIPI DSI support Updated

[PATCH v2 0/7] drm/i915: Baytrail MIPI DSI support Updated

[Shobhit Kumar]

Hi All - 
These patches enhance the current support for MIPI DSI for Baytrail. They
continue on the sub-encoder design and adds few more dev_ops to handle
sequence correctly. Major changes are -

1. DSI Clock calculation based on pixel clock
2. Add new dev_ops for better sequencing the enable/disable path
3. Parameterized the hardcoded DSI parameters. These also forms building
   block for the generic MIPI driver to come in future based on enhancements
   in VBT. All these parameters are initialized or computed in the sub-encoder
   driver. Some of them might look unneccesary for now.

 I am also aware of the drm_bridge support now comming in and will in future
 migrate from sub-encoder design to drm_bridge.

This DSI sequence has been validated with couple of test panels and is working now.
Still no sub-encoder driver is included and this support will be mostly be disabled
untill a panel sub-encoder driver is added. Proper detection or VBT is still pending.

v2: Mostly changes from review comments from Jani Nikula and Ville Syrjala
    - Split the parameters into new patch
    - Split the dsi_clk computation and m-n-p modification in separate patches
    - The DSI sequence refactoring has been splitted into multiple patches and also
      few code changes are not needed after reworking/relooking at them and have been
      removed
    - Backlight enabling has been removed as that depends on platform PMIC driver which
      is not yet there in upstream kernel. Will be added later.
    - Other general code cleanup as suggested
    - drm/i915: Use FLISDSI interface for band gap reset - has no changes and is included 
      for completeness of the patch set

Regards
Shobhit

Shobhit Kumar (7):
  drm/i915: Add more dev ops for MIPI sub encoder
  drm/i915: Use FLISDSI interface for band gap reset
  drm/i915: Compute dsi_clk from pixel clock
  drm/i915: Try harder to get best m,n,p values with minimal error
  drm/i915: Reorganize the DSI enable/disable sequence
  drm/i915: Remove redundant DSI PLL enabling
  drm/i915: Parametrize the dphy and other spec specific parameters

 drivers/gpu/drm/i915/i915_drv.h       |   13 +++
 drivers/gpu/drm/i915/i915_reg.h       |    1 +
 drivers/gpu/drm/i915/intel_dsi.c      |  187 ++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_dsi.h      |   21 ++++
 drivers/gpu/drm/i915/intel_dsi_pll.c  |   72 ++++++++++---
 drivers/gpu/drm/i915/intel_sideband.c |   14 +++
 6 files changed, 209 insertions(+), 99 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/7] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar]

Some panels require one time programming if they do not contain their
own eeprom for basic register initialization. The sequence is

Panel Reset --> Send OTP --> Enable Pixel Stream --> Enable the panel

v2: Based on review comments from Jani and Ville
    - Updated the commit message with more details
    - Move the new parameters out of this patch

Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c |    9 ++++++++-
 drivers/gpu/drm/i915/intel_dsi.h |    5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index d257b09..61267e2 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
+	if (intel_dsi->dev.dev_ops->panel_reset)
+		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
+
 	temp = I915_READ(MIPI_DEVICE_READY(pipe));
 	if ((temp & DEVICE_READY) == 0) {
 		temp &= ~ULPS_STATE_MASK;
@@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
 	}
 
+	if (intel_dsi->dev.dev_ops->send_otp_cmds)
+		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
+
 	if (is_cmd_mode(intel_dsi))
 		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
 
@@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 		POSTING_READ(MIPI_PORT_CTRL(pipe));
 	}
 
-	intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
+	if (intel_dsi->dev.dev_ops->enable)
+		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
 }
 
 static void intel_dsi_disable(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index c7765f3..14509d6 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -39,6 +39,11 @@ struct intel_dsi_device {
 struct intel_dsi_dev_ops {
 	bool (*init)(struct intel_dsi_device *dsi);
 
+	void (*panel_reset)(struct intel_dsi_device *dsi);
+
+	/* one time programmable commands if needed */
+	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
+
 	/* This callback must be able to assume DSI commands can be sent */
 	void (*enable)(struct intel_dsi_device *dsi);
 
-- 
1.7.9.5

[PATCH v2 2/7] drm/i915: Use FLISDSI interface for band gap reset

[Shobhit Kumar]

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Reviewed-by: Jani Nikula<jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    2 ++
 drivers/gpu/drm/i915/i915_reg.h       |    1 +
 drivers/gpu/drm/i915/intel_dsi.c      |   47 ++++++---------------------------
 drivers/gpu/drm/i915/intel_sideband.c |   14 ++++++++++
 4 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b12d942..a2bbff9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2408,6 +2408,8 @@ u32 intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
 		   enum intel_sbi_destination destination);
 void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
 		     enum intel_sbi_destination destination);
+u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
+void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
 
 int vlv_gpu_freq(int ddr_freq, int val);
 int vlv_freq_opcode(int ddr_freq, int val);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3f303ba..6236926 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -356,6 +356,7 @@
 #define   IOSF_PORT_CCK				0x14
 #define   IOSF_PORT_CCU				0xA9
 #define   IOSF_PORT_GPS_CORE			0x48
+#define   IOSF_PORT_FLISDSI			0x1B
 #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
 #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 61267e2..8dc9a38 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -37,49 +37,18 @@
 static const struct intel_dsi_device intel_dsi_devices[] = {
 };
 
-
-static void vlv_cck_modify(struct drm_i915_private *dev_priv, u32 reg, u32 val,
-			   u32 mask)
-{
-	u32 tmp = vlv_cck_read(dev_priv, reg);
-	tmp &= ~mask;
-	tmp |= val;
-	vlv_cck_write(dev_priv, reg, tmp);
-}
-
-static void band_gap_wa(struct drm_i915_private *dev_priv)
+static void band_gap_reset(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->dpio_lock);
 
-	/* Enable bandgap fix in GOP driver */
-	vlv_cck_modify(dev_priv, 0x6D, 0x00010000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6E, 0x00010000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6F, 0x00010000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x00, 0x00008000, 0x00008000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x00, 0x00000000, 0x00008000);
-	msleep(20);
-
-	/* Turn Display Trunk on */
-	vlv_cck_modify(dev_priv, 0x6B, 0x00020000, 0x00030000);
-	msleep(20);
-
-	vlv_cck_modify(dev_priv, 0x6C, 0x00020000, 0x00030000);
-	msleep(20);
-
-	vlv_cck_modify(dev_priv, 0x6D, 0x00020000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6E, 0x00020000, 0x00030000);
-	msleep(20);
-	vlv_cck_modify(dev_priv, 0x6F, 0x00020000, 0x00030000);
+	vlv_flisdsi_write(dev_priv, 0x08, 0x0001);
+	vlv_flisdsi_write(dev_priv, 0x0F, 0x0005);
+	vlv_flisdsi_write(dev_priv, 0x0F, 0x0025);
+	udelay(150);
+	vlv_flisdsi_write(dev_priv, 0x0F, 0x0000);
+	vlv_flisdsi_write(dev_priv, 0x08, 0x0000);
 
 	mutex_unlock(&dev_priv->dpio_lock);
-
-	/* Need huge delay, otherwise clock is not stable */
-	msleep(100);
 }
 
 static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
@@ -363,7 +332,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 	vlv_enable_dsi_pll(intel_encoder);
 
 	/* XXX: Location of the call */
-	band_gap_wa(dev_priv);
+	band_gap_reset(dev_priv);
 
 	/* escape clock divider, 20MHz, shared for A and C. device ready must be
 	 * off when doing this! txclkesc? */
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 9944d81..f15e57e 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -242,3 +242,17 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
 		return;
 	}
 }
+
+u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg)
+{
+	u32 val = 0;
+	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
+					DPIO_OPCODE_REG_READ, reg, &val);
+	return val;
+}
+
+void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
+{
+	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
+					DPIO_OPCODE_REG_WRITE, reg, &val);
+}
-- 
1.7.9.5

[PATCH v2 3/7] drm/i915: Compute dsi_clk from pixel clock

[Shobhit Kumar]

Pixel clock based calculation is recommended in the MIPI host controller
documentation

v2: Based on review comments from Jani and Ville
    - Use dsi_clk in KHz rather than converting in Hz and back to MHz
    - RR formula is retained though not used but return dsi_clk in KHz now
    - Moved the m-n-p changes into a separate patch
    - Removed the parameter check for intel_dsi->dsi_clock_freq. This will be
      bought back in if needed when appropriate panel drivers are done

Signed-off-by: Vijayakumar Balakrishnan <vijayakumar.balakrishnan@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_pll.c |   46 +++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
index 44279b2..9f3e6b0 100644
--- a/drivers/gpu/drm/i915/intel_dsi_pll.c
+++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
@@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
 	71, 35							/* 91 - 92 */
 };
 
+#ifdef DSI_CLK_FROM_RR
+
 static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 			  int pixel_format, int video_mode_format,
 			  int lane_count, bool eotp)
@@ -121,7 +123,7 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 
 	/* the dsi clock is divided by 2 in the hardware to get dsi ddr clock */
 	dsi_bit_clock_hz = bytes_per_x_frames_x_lanes * 8;
-	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
+	dsi_clk = dsi_bit_clock_hz / 1000;
 
 	if (eotp && video_mode_format == VIDEO_MODE_BURST)
 		dsi_clk *= 2;
@@ -129,6 +131,38 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 	return dsi_clk;
 }
 
+#else
+
+/* Get DSI clock from pixel clock */
+static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
+			  int pixel_format, int lane_count)
+{
+	u32 dsi_clk_khz;
+	u32 bpp;
+
+	switch (pixel_format) {
+	default:
+	case VID_MODE_FORMAT_RGB888:
+	case VID_MODE_FORMAT_RGB666_LOOSE:
+		bpp = 24;
+		break;
+	case VID_MODE_FORMAT_RGB666:
+		bpp = 18;
+		break;
+	case VID_MODE_FORMAT_RGB565:
+		bpp = 16;
+		break;
+	}
+
+	/* DSI data rate = pixel clock * bits per pixel / lane count
+	   pixel clock is converted from KHz to Hz */
+	dsi_clk_khz = DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
+
+	return dsi_clk_khz;
+}
+
+#endif
+
 #ifdef MNP_FROM_TABLE
 
 struct dsi_clock_table {
@@ -200,13 +234,14 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
 	u32 calc_p;
 	u32 m_seed;
 
-	if (dsi_clk < 300 || dsi_clk > 1150) {
+	/* dsi_clk is expected in KHZ */
+	if (dsi_clk < 300000 || dsi_clk > 1150000) {
 		DRM_ERROR("DSI CLK Out of Range\n");
 		return -ECHRNG;
 	}
 
 	ref_clk = 25000;
-	target_dsi_clk = dsi_clk * 1000;
+	target_dsi_clk = dsi_clk;
 	error = 0xFFFFFFFF;
 	calc_m = 0;
 	calc_p = 0;
@@ -251,9 +286,8 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
 	struct dsi_mnp dsi_mnp;
 	u32 dsi_clk;
 
-	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
-				 intel_dsi->video_mode_format,
-				 intel_dsi->lane_count, !intel_dsi->eot_disable);
+	dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
+						intel_dsi->lane_count);
 
 	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
 	if (ret) {
-- 
1.7.9.5

[PATCH v2 4/7] drm/i915: Try harder to get best m, n, p values with minimal error

[Shobhit Kumar]

Basically check for both +ive and -ive deviation from target clock and
pick the one with minimal error. If we get a direct match, break from
loop to acheive some optimization.

Signed-off-by: Vijayakumar Balakrishnan <vijayakumar.balakrishnan@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_pll.c |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
index 9f3e6b0..16bc6b2 100644
--- a/drivers/gpu/drm/i915/intel_dsi_pll.c
+++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
@@ -243,22 +243,32 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
 	ref_clk = 25000;
 	target_dsi_clk = dsi_clk;
 	error = 0xFFFFFFFF;
+	tmp_error = 0xFFFFFFFF;
 	calc_m = 0;
 	calc_p = 0;
 
 	for (m = 62; m <= 92; m++) {
 		for (p = 2; p <= 6; p++) {
-
+			/* Find the optimal m and p divisors
+			with minimal error +/- the required clock */
 			calc_dsi_clk = (m * ref_clk) / p;
-			if (calc_dsi_clk >= target_dsi_clk) {
-				tmp_error = calc_dsi_clk - target_dsi_clk;
-				if (tmp_error < error) {
-					error = tmp_error;
-					calc_m = m;
-					calc_p = p;
-				}
+			if (calc_dsi_clk == target_dsi_clk) {
+				calc_m = m;
+				calc_p = p;
+				error = 0;
+				break;
+			} else
+				tmp_error = abs(target_dsi_clk - calc_dsi_clk);
+
+			if (tmp_error < error) {
+				error = tmp_error;
+				calc_m = m;
+				calc_p = p;
 			}
 		}
+
+		if (error == 0)
+			break;
 	}
 
 	m_seed = lfsr_converts[calc_m - 62];
-- 
1.7.9.5

[PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Shobhit Kumar]

Basically ULPS handling during enable/disable has been moved to
pre_enable and post_disable phases. PLL and panel power disable
also has been moved to post_disable phase. The ULPS entry/exit
sequneces as suggested by HW team is as follows -

During enable time -
set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY

And during disable time to flush all FIFOs -
set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP

Also during disbale sequnece sub-encoder disable is moved to the end
after port is disabled.

v2: Based on comments from Ville
    - Detailed epxlaination in the commit messgae
    - Moved parameter changes out into another patch
    - Backlight enabling will be a new patch

Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
 drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_dsi.h |    2 +
 3 files changed, 91 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2bbff9..55c16cb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
 #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
 #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
 
+#define I915_WRITE_BITS(reg, val, mask) \
+do { \
+	u32 tmp, data; \
+	tmp = I915_READ((reg)); \
+	tmp &= ~(mask); \
+	data = (val) & (mask); \
+	data = data | tmp; \
+	I915_WRITE((reg), data); \
+} while(0)
+
+
 /* "Broadcast RGB" property */
 #define INTEL_BROADCAST_RGB_AUTO 0
 #define INTEL_BROADCAST_RGB_FULL 1
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 8dc9a38..9e67f78 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
 	vlv_enable_dsi_pll(encoder);
 }
 
-static void intel_dsi_pre_enable(struct intel_encoder *encoder)
-{
-	DRM_DEBUG_KMS("\n");
-}
-
-static void intel_dsi_enable(struct intel_encoder *encoder)
+void intel_dsi_device_ready(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	int pipe = intel_crtc->pipe;
-	u32 temp;
 
 	DRM_DEBUG_KMS("\n");
 
 	if (intel_dsi->dev.dev_ops->panel_reset)
 		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
 
-	temp = I915_READ(MIPI_DEVICE_READY(pipe));
-	if ((temp & DEVICE_READY) == 0) {
-		temp &= ~ULPS_STATE_MASK;
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
-	} else if (temp & ULPS_STATE_MASK) {
-		temp &= ~ULPS_STATE_MASK;
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
-		/*
-		 * We need to ensure that there is a minimum of 1 ms time
-		 * available before clearing the UPLS exit state.
-		 */
-		msleep(2);
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
-	}
+	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
+	usleep_range(1000, 1500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
+			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
+			DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
+			DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
+			DEVICE_READY | ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
 
 	if (intel_dsi->dev.dev_ops->send_otp_cmds)
 		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
 
+}
+static void intel_dsi_pre_enable(struct intel_encoder *encoder)
+{
+	DRM_DEBUG_KMS("\n");
+
+	/* put device in ready state */
+	intel_dsi_device_ready(encoder);
+}
+
+static void intel_dsi_enable(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	int pipe = intel_crtc->pipe;
+	u32 temp;
+
+	DRM_DEBUG_KMS("\n");
+
 	if (is_cmd_mode(intel_dsi))
 		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
-
-	if (is_vid_mode(intel_dsi)) {
+	else {
 		msleep(20); /* XXX */
 		dpi_send_cmd(intel_dsi, TURN_ON);
 		msleep(100);
@@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 static void intel_dsi_disable(struct intel_encoder *encoder)
 {
-	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	int pipe = intel_crtc->pipe;
@@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
-	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
-
 	if (is_vid_mode(intel_dsi)) {
 		dpi_send_cmd(intel_dsi, SHUTDOWN);
 		msleep(10);
@@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 		msleep(2);
 	}
 
-	temp = I915_READ(MIPI_DEVICE_READY(pipe));
-	if (temp & DEVICE_READY) {
-		temp &= ~DEVICE_READY;
-		temp &= ~ULPS_STATE_MASK;
-		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
-	}
+	/* if disable packets are sent before sending shutdown packet then in
+	 * some next enable sequence send turn on packet error is observed */
+	if (intel_dsi->dev.dev_ops->disable)
+		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
 }
 
-static void intel_dsi_post_disable(struct intel_encoder *encoder)
+void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
 {
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	int pipe = intel_crtc->pipe;
+
 	DRM_DEBUG_KMS("\n");
 
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
+							ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
+							ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
+							ULPS_STATE_MASK);
+	usleep_range(2000, 2500);
+
+	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
+	usleep_range(1000, 1500);
+
+	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
+					== 0x00000), 30))
+		DRM_ERROR("DSI LP not going Low\n");
+
+	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
+	usleep_range(2000, 2500);
+
 	vlv_disable_dsi_pll(encoder);
+
+	if (intel_dsi->dev.dev_ops->disable_panel_power)
+		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
+}
+static void intel_dsi_post_disable(struct intel_encoder *encoder)
+{
+	DRM_DEBUG_KMS("\n");
+	intel_dsi_clear_device_ready(encoder);
 }
 
 static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 14509d6..387dfe1 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -41,6 +41,8 @@ struct intel_dsi_dev_ops {
 
 	void (*panel_reset)(struct intel_dsi_device *dsi);
 
+	void (*disable_panel_power)(struct intel_dsi_device *dsi);
+
 	/* one time programmable commands if needed */
 	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
 
-- 
1.7.9.5

[PATCH v2 6/7] drm/i915: Remove redundant DSI PLL enabling

[Shobhit Kumar]

DSI PLL will get configured during crtc_enable using ->pre_pll_enable
and no need to do in ->mode_set

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 9e67f78..4dccb4b 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -373,9 +373,6 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 
 	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
 
-	/* Update the DSI PLL */
-	vlv_enable_dsi_pll(intel_encoder);
-
 	/* XXX: Location of the call */
 	band_gap_reset(dev_priv);
 
-- 
1.7.9.5

[PATCH v2 7/7] drm/i915: Parametrize the dphy and other spec specific parameters

[Shobhit Kumar]

The values of these parameters will be different for differnet panel
based on dsi rate, lane count, etc. Remove the hardcodings and make
these as parameters whch will be initialized in panel specific
sub-encoder implementaion.

This will also form groundwork for planned generic panel sub-encoder
implemntation based on VBT design enhancments to support multiple panels

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c |   27 +++++++++++++--------------
 drivers/gpu/drm/i915/intel_dsi.h |   14 ++++++++++++++
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 4dccb4b..e9fde76 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -160,6 +160,7 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 		/* assert ip_tg_enable signal */
 		temp = I915_READ(MIPI_PORT_CTRL(pipe));
+		temp = temp | intel_dsi->port_bits;
 		I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
 		POSTING_READ(MIPI_PORT_CTRL(pipe));
 	}
@@ -391,11 +392,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 	I915_WRITE(MIPI_INTR_STAT(pipe), 0xffffffff);
 	I915_WRITE(MIPI_INTR_EN(pipe), 0xffffffff);
 
-	I915_WRITE(MIPI_DPHY_PARAM(pipe),
-		   0x3c << EXIT_ZERO_COUNT_SHIFT |
-		   0x1f << TRAIL_COUNT_SHIFT |
-		   0xc5 << CLK_ZERO_COUNT_SHIFT |
-		   0x1f << PREPARE_COUNT_SHIFT);
+	I915_WRITE(MIPI_DPHY_PARAM(pipe), intel_dsi->dphy_reg);
 
 	I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
 		   adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT |
@@ -443,9 +440,9 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 				       adjusted_mode->htotal,
 				       bpp, intel_dsi->lane_count) + 1);
 	}
-	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), 8309); /* max */
-	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), 0x14); /* max */
-	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), 0xffff); /* max */
+	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), intel_dsi->lp_rx_timeout);
+	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), intel_dsi->turn_arnd_val);
+	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), intel_dsi->rst_timer_val);
 
 	/* dphy stuff */
 
@@ -460,29 +457,31 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 	 *
 	 * XXX: write MIPI_STOP_STATE_STALL?
 	 */
-	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), 0x46);
+	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe),
+						intel_dsi->hs_to_lp_count);
 
 	/* XXX: low power clock equivalence in terms of byte clock. the number
 	 * of byte clocks occupied in one low power clock. based on txbyteclkhs
 	 * and txclkesc. txclkesc time / txbyteclk time * (105 +
 	 * MIPI_STOP_STATE_STALL) / 105.???
 	 */
-	I915_WRITE(MIPI_LP_BYTECLK(pipe), 4);
+	I915_WRITE(MIPI_LP_BYTECLK(pipe), intel_dsi->lp_byte_clk);
 
 	/* the bw essential for transmitting 16 long packets containing 252
 	 * bytes meant for dcs write memory command is programmed in this
 	 * register in terms of byte clocks. based on dsi transfer rate and the
 	 * number of lanes configured the time taken to transmit 16 long packets
 	 * in a dsi stream varies. */
-	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), 0x820);
+	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), intel_dsi->bw_timer);
 
 	I915_WRITE(MIPI_CLK_LANE_SWITCH_TIME_CNT(pipe),
-		   0xa << LP_HS_SSW_CNT_SHIFT |
-		   0x14 << HS_LP_PWR_SW_CNT_SHIFT);
+		   intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT |
+		   intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT);
 
 	if (is_vid_mode(intel_dsi))
 		I915_WRITE(MIPI_VIDEO_MODE_FORMAT(pipe),
-			   intel_dsi->video_mode_format);
+				intel_dsi->video_frmt_cfg_bits |
+				intel_dsi->video_mode_format);
 }
 
 static enum drm_connector_status
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 387dfe1..b4a27ce 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -96,6 +96,20 @@ struct intel_dsi {
 
 	/* eot for MIPI_EOT_DISABLE register */
 	u32 eot_disable;
+
+	u32 port_bits;
+	u32 bw_timer;
+	u32 dphy_reg;
+	u32 video_frmt_cfg_bits;
+	u16 lp_byte_clk;
+
+	/* timeouts in byte clocks */
+	u16 lp_rx_timeout;
+	u16 turn_arnd_val;
+	u16 rst_timer_val;
+	u16 hs_to_lp_count;
+	u16 clk_lp_to_hs_count;
+	u16 clk_hs_to_lp_count;
 };
 
 static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
-- 
1.7.9.5

Re: [PATCH v2 0/7] drm/i915: Baytrail MIPI DSI support Updated

[Daniel Vetter]

On Sat, Nov 09, 2013 at 03:19:01PM +0530, Shobhit Kumar wrote:
> Hi All - 
> These patches enhance the current support for MIPI DSI for Baytrail. They
> continue on the sub-encoder design and adds few more dev_ops to handle
> sequence correctly. Major changes are -
> 
> 1. DSI Clock calculation based on pixel clock
> 2. Add new dev_ops for better sequencing the enable/disable path
> 3. Parameterized the hardcoded DSI parameters. These also forms building
>    block for the generic MIPI driver to come in future based on enhancements
>    in VBT. All these parameters are initialized or computed in the sub-encoder
>    driver. Some of them might look unneccesary for now.
> 
>  I am also aware of the drm_bridge support now comming in and will in future
>  migrate from sub-encoder design to drm_bridge.

Just a quick aside: Thierry Reding from nvidia is also working on a DSI
design for the tegra driver. Atm he seems to aim for a full-blown DSI bus
based on his drm_panel patches for getting the panel metadata out of an
ARM DT (we'd use VBT instead). Iirc there's no patches anywhere yet, but
maybe Thierry could share a git branch somewhere with the wip stuff?

Cc'ing Thierry and dri-devel in case a bigger discussion develops.

Cheers, Daniel

> 
> This DSI sequence has been validated with couple of test panels and is working now.
> Still no sub-encoder driver is included and this support will be mostly be disabled
> untill a panel sub-encoder driver is added. Proper detection or VBT is still pending.
> 
> v2: Mostly changes from review comments from Jani Nikula and Ville Syrjala
>     - Split the parameters into new patch
>     - Split the dsi_clk computation and m-n-p modification in separate patches
>     - The DSI sequence refactoring has been splitted into multiple patches and also
>       few code changes are not needed after reworking/relooking at them and have been
>       removed
>     - Backlight enabling has been removed as that depends on platform PMIC driver which
>       is not yet there in upstream kernel. Will be added later.
>     - Other general code cleanup as suggested
>     - drm/i915: Use FLISDSI interface for band gap reset - has no changes and is included 
>       for completeness of the patch set
> 
> Regards
> Shobhit
> 
> Shobhit Kumar (7):
>   drm/i915: Add more dev ops for MIPI sub encoder
>   drm/i915: Use FLISDSI interface for band gap reset
>   drm/i915: Compute dsi_clk from pixel clock
>   drm/i915: Try harder to get best m,n,p values with minimal error
>   drm/i915: Reorganize the DSI enable/disable sequence
>   drm/i915: Remove redundant DSI PLL enabling
>   drm/i915: Parametrize the dphy and other spec specific parameters
> 
>  drivers/gpu/drm/i915/i915_drv.h       |   13 +++
>  drivers/gpu/drm/i915/i915_reg.h       |    1 +
>  drivers/gpu/drm/i915/intel_dsi.c      |  187 ++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_dsi.h      |   21 ++++
>  drivers/gpu/drm/i915/intel_dsi_pll.c  |   72 ++++++++++---
>  drivers/gpu/drm/i915/intel_sideband.c |   14 +++
>  6 files changed, 209 insertions(+), 99 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v2 0/7] drm/i915: Baytrail MIPI DSI support Updated

[Thierry Reding]

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

On Sat, Nov 09, 2013 at 11:28:16AM +0100, Daniel Vetter wrote:
> On Sat, Nov 09, 2013 at 03:19:01PM +0530, Shobhit Kumar wrote:
> > Hi All - 
> > These patches enhance the current support for MIPI DSI for Baytrail. They
> > continue on the sub-encoder design and adds few more dev_ops to handle
> > sequence correctly. Major changes are -
> > 
> > 1. DSI Clock calculation based on pixel clock
> > 2. Add new dev_ops for better sequencing the enable/disable path
> > 3. Parameterized the hardcoded DSI parameters. These also forms building
> >    block for the generic MIPI driver to come in future based on enhancements
> >    in VBT. All these parameters are initialized or computed in the sub-encoder
> >    driver. Some of them might look unneccesary for now.
> > 
> >  I am also aware of the drm_bridge support now comming in and will in future
> >  migrate from sub-encoder design to drm_bridge.
> 
> Just a quick aside: Thierry Reding from nvidia is also working on a DSI
> design for the tegra driver. Atm he seems to aim for a full-blown DSI bus
> based on his drm_panel patches for getting the panel metadata out of an
> ARM DT (we'd use VBT instead). Iirc there's no patches anywhere yet, but
> maybe Thierry could share a git branch somewhere with the wip stuff?
> 
> Cc'ing Thierry and dri-devel in case a bigger discussion develops.

I've been cleaning up the patches and was going to post them today. The
implementation really isn't as "full-blown" as you make it sound =),
primarily because the DSI panel that I have doesn't support things such
as reading out the DDB, so I cannot test most of the functionality that
I planned to.

However I think introducing a DSI bus type is the right thing and it's
been suggested recently that we have too few bus types. Furthermore it
seems to be playing out rather nicely with the DRM panel work, so it
would be really nice if Intel could test-drive this within their driver
to see if it's good enough for their purposes as well.

Is everyone working on that subscribed to dri-devel or should I Cc the
intel-gfx mailing list (or someone in particular) when posting the
patches?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCH v2 0/7] drm/i915: Baytrail MIPI DSI support Updated

[Shobhit Kumar]

On 11/11/2013 02:20 PM, Thierry Reding wrote:
> On Sat, Nov 09, 2013 at 11:28:16AM +0100, Daniel Vetter wrote:
>> On Sat, Nov 09, 2013 at 03:19:01PM +0530, Shobhit Kumar wrote:
>>> Hi All -
>>> These patches enhance the current support for MIPI DSI for Baytrail. They
>>> continue on the sub-encoder design and adds few more dev_ops to handle
>>> sequence correctly. Major changes are -
>>>
>>> 1. DSI Clock calculation based on pixel clock
>>> 2. Add new dev_ops for better sequencing the enable/disable path
>>> 3. Parameterized the hardcoded DSI parameters. These also forms building
>>>     block for the generic MIPI driver to come in future based on enhancements
>>>     in VBT. All these parameters are initialized or computed in the sub-encoder
>>>     driver. Some of them might look unneccesary for now.
>>>
>>>   I am also aware of the drm_bridge support now comming in and will in future
>>>   migrate from sub-encoder design to drm_bridge.
>>
>> Just a quick aside: Thierry Reding from nvidia is also working on a DSI
>> design for the tegra driver. Atm he seems to aim for a full-blown DSI bus
>> based on his drm_panel patches for getting the panel metadata out of an
>> ARM DT (we'd use VBT instead). Iirc there's no patches anywhere yet, but
>> maybe Thierry could share a git branch somewhere with the wip stuff?
>>
>> Cc'ing Thierry and dri-devel in case a bigger discussion develops.
>
> I've been cleaning up the patches and was going to post them today. The
> implementation really isn't as "full-blown" as you make it sound =),
> primarily because the DSI panel that I have doesn't support things such
> as reading out the DDB, so I cannot test most of the functionality that
> I planned to.
>
> However I think introducing a DSI bus type is the right thing and it's
> been suggested recently that we have too few bus types. Furthermore it
> seems to be playing out rather nicely with the DRM panel work, so it
> would be really nice if Intel could test-drive this within their driver
> to see if it's good enough for their purposes as well.

Interesting. Would be nice to have a look.

>
> Is everyone working on that subscribed to dri-devel or should I Cc the
> intel-gfx mailing list (or someone in particular) when posting the
> patches?
>

Will keep an eye for your patches in dri-devel.

Regards
Shobhit

Re: [PATCH v2 4/7] drm/i915: Try harder to get best m, n, p values with minimal error

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Basically check for both +ive and -ive deviation from target clock and
> pick the one with minimal error. If we get a direct match, break from
> loop to acheive some optimization.
>
> Signed-off-by: Vijayakumar Balakrishnan <vijayakumar.balakrishnan@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 9f3e6b0..16bc6b2 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -243,22 +243,32 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	ref_clk = 25000;
>  	target_dsi_clk = dsi_clk;
>  	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
>  
>  	for (m = 62; m <= 92; m++) {
>  		for (p = 2; p <= 6; p++) {
> -
> +			/* Find the optimal m and p divisors
> +			with minimal error +/- the required clock */
>  			calc_dsi_clk = (m * ref_clk) / p;
> -			if (calc_dsi_clk >= target_dsi_clk) {
> -				tmp_error = calc_dsi_clk - target_dsi_clk;
> -				if (tmp_error < error) {
> -					error = tmp_error;
> -					calc_m = m;
> -					calc_p = p;
> -				}
> +			if (calc_dsi_clk == target_dsi_clk) {
> +				calc_m = m;
> +				calc_p = p;
> +				error = 0;
> +				break;
> +			} else
> +				tmp_error = abs(target_dsi_clk - calc_dsi_clk);

This is fragile, and only works because of the kernel's implementation
of abs(). The substraction should be done with signed values.

BR,
Jani.

> +
> +			if (tmp_error < error) {
> +				error = tmp_error;
> +				calc_m = m;
> +				calc_p = p;
>  			}
>  		}
> +
> +		if (error == 0)
> +			break;
>  	}
>  
>  	m_seed = lfsr_converts[calc_m - 62];
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 3/7] drm/i915: Compute dsi_clk from pixel clock

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Pixel clock based calculation is recommended in the MIPI host controller
> documentation
>
> v2: Based on review comments from Jani and Ville
>     - Use dsi_clk in KHz rather than converting in Hz and back to MHz
>     - RR formula is retained though not used but return dsi_clk in KHz now
>     - Moved the m-n-p changes into a separate patch
>     - Removed the parameter check for intel_dsi->dsi_clock_freq. This will be
>       bought back in if needed when appropriate panel drivers are done
>
> Signed-off-by: Vijayakumar Balakrishnan <vijayakumar.balakrishnan@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   46 +++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 44279b2..9f3e6b0 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>  	71, 35							/* 91 - 92 */
>  };
>  
> +#ifdef DSI_CLK_FROM_RR
> +
>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  			  int pixel_format, int video_mode_format,
>  			  int lane_count, bool eotp)
> @@ -121,7 +123,7 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  
>  	/* the dsi clock is divided by 2 in the hardware to get dsi ddr clock */
>  	dsi_bit_clock_hz = bytes_per_x_frames_x_lanes * 8;
> -	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
> +	dsi_clk = dsi_bit_clock_hz / 1000;
>  
>  	if (eotp && video_mode_format == VIDEO_MODE_BURST)
>  		dsi_clk *= 2;
> @@ -129,6 +131,38 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  	return dsi_clk;
>  }
>  
> +#else
> +
> +/* Get DSI clock from pixel clock */
> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
> +			  int pixel_format, int lane_count)
> +{
> +	u32 dsi_clk_khz;
> +	u32 bpp;
> +
> +	switch (pixel_format) {
> +	default:
> +	case VID_MODE_FORMAT_RGB888:
> +	case VID_MODE_FORMAT_RGB666_LOOSE:
> +		bpp = 24;
> +		break;
> +	case VID_MODE_FORMAT_RGB666:
> +		bpp = 18;
> +		break;
> +	case VID_MODE_FORMAT_RGB565:
> +		bpp = 16;
> +		break;
> +	}
> +
> +	/* DSI data rate = pixel clock * bits per pixel / lane count
> +	   pixel clock is converted from KHz to Hz */
> +	dsi_clk_khz = DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
> +
> +	return dsi_clk_khz;
> +}
> +
> +#endif
> +
>  #ifdef MNP_FROM_TABLE
>  
>  struct dsi_clock_table {
> @@ -200,13 +234,14 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	u32 calc_p;
>  	u32 m_seed;
>  
> -	if (dsi_clk < 300 || dsi_clk > 1150) {
> +	/* dsi_clk is expected in KHZ */
> +	if (dsi_clk < 300000 || dsi_clk > 1150000) {
>  		DRM_ERROR("DSI CLK Out of Range\n");
>  		return -ECHRNG;
>  	}
>  
>  	ref_clk = 25000;
> -	target_dsi_clk = dsi_clk * 1000;
> +	target_dsi_clk = dsi_clk;

The *other* dsi_calc_mnp() (with MNP_FROM_TABLE defined) remains
unchanged, and bitrots. Either /= 1000 the input there, or better, if
you don't see that the other function will be needed, just remove it.

BR,
Jani.


>  	error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
> @@ -251,9 +286,8 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>  	struct dsi_mnp dsi_mnp;
>  	u32 dsi_clk;
>  
> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> -				 intel_dsi->video_mode_format,
> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +	dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
> +						intel_dsi->lane_count);
>  
>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>  	if (ret) {
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 7/7] drm/i915: Parametrize the dphy and other spec specific parameters

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> The values of these parameters will be different for differnet panel
> based on dsi rate, lane count, etc. Remove the hardcodings and make
> these as parameters whch will be initialized in panel specific
> sub-encoder implementaion.
>
> This will also form groundwork for planned generic panel sub-encoder
> implemntation based on VBT design enhancments to support multiple panels
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c |   27 +++++++++++++--------------
>  drivers/gpu/drm/i915/intel_dsi.h |   14 ++++++++++++++
>  2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 4dccb4b..e9fde76 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -160,6 +160,7 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  		/* assert ip_tg_enable signal */
>  		temp = I915_READ(MIPI_PORT_CTRL(pipe));
> +		temp = temp | intel_dsi->port_bits;

Need to mask away port_bits before OR:ing?

>  		I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
>  		POSTING_READ(MIPI_PORT_CTRL(pipe));
>  	}
> @@ -391,11 +392,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  	I915_WRITE(MIPI_INTR_STAT(pipe), 0xffffffff);
>  	I915_WRITE(MIPI_INTR_EN(pipe), 0xffffffff);
>  
> -	I915_WRITE(MIPI_DPHY_PARAM(pipe),
> -		   0x3c << EXIT_ZERO_COUNT_SHIFT |
> -		   0x1f << TRAIL_COUNT_SHIFT |
> -		   0xc5 << CLK_ZERO_COUNT_SHIFT |
> -		   0x1f << PREPARE_COUNT_SHIFT);
> +	I915_WRITE(MIPI_DPHY_PARAM(pipe), intel_dsi->dphy_reg);
>  
>  	I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
>  		   adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT |
> @@ -443,9 +440,9 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  				       adjusted_mode->htotal,
>  				       bpp, intel_dsi->lane_count) + 1);
>  	}
> -	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), 8309); /* max */
> -	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), 0x14); /* max */
> -	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), 0xffff); /* max */
> +	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), intel_dsi->lp_rx_timeout);
> +	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), intel_dsi->turn_arnd_val);
> +	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), intel_dsi->rst_timer_val);
>  
>  	/* dphy stuff */
>  
> @@ -460,29 +457,31 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  	 *
>  	 * XXX: write MIPI_STOP_STATE_STALL?
>  	 */
> -	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), 0x46);
> +	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe),
> +						intel_dsi->hs_to_lp_count);
>  
>  	/* XXX: low power clock equivalence in terms of byte clock. the number
>  	 * of byte clocks occupied in one low power clock. based on txbyteclkhs
>  	 * and txclkesc. txclkesc time / txbyteclk time * (105 +
>  	 * MIPI_STOP_STATE_STALL) / 105.???
>  	 */
> -	I915_WRITE(MIPI_LP_BYTECLK(pipe), 4);
> +	I915_WRITE(MIPI_LP_BYTECLK(pipe), intel_dsi->lp_byte_clk);
>  
>  	/* the bw essential for transmitting 16 long packets containing 252
>  	 * bytes meant for dcs write memory command is programmed in this
>  	 * register in terms of byte clocks. based on dsi transfer rate and the
>  	 * number of lanes configured the time taken to transmit 16 long packets
>  	 * in a dsi stream varies. */
> -	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), 0x820);
> +	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), intel_dsi->bw_timer);
>  
>  	I915_WRITE(MIPI_CLK_LANE_SWITCH_TIME_CNT(pipe),
> -		   0xa << LP_HS_SSW_CNT_SHIFT |
> -		   0x14 << HS_LP_PWR_SW_CNT_SHIFT);
> +		   intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT |
> +		   intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT);
>  
>  	if (is_vid_mode(intel_dsi))
>  		I915_WRITE(MIPI_VIDEO_MODE_FORMAT(pipe),
> -			   intel_dsi->video_mode_format);
> +				intel_dsi->video_frmt_cfg_bits |
> +				intel_dsi->video_mode_format);
>  }
>  
>  static enum drm_connector_status
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 387dfe1..b4a27ce 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -96,6 +96,20 @@ struct intel_dsi {
>  
>  	/* eot for MIPI_EOT_DISABLE register */
>  	u32 eot_disable;
> +
> +	u32 port_bits;
> +	u32 bw_timer;
> +	u32 dphy_reg;
> +	u32 video_frmt_cfg_bits;
> +	u16 lp_byte_clk;
> +
> +	/* timeouts in byte clocks */
> +	u16 lp_rx_timeout;
> +	u16 turn_arnd_val;
> +	u16 rst_timer_val;
> +	u16 hs_to_lp_count;
> +	u16 clk_lp_to_hs_count;
> +	u16 clk_hs_to_lp_count;
>  };
>  
>  static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Basically ULPS handling during enable/disable has been moved to
> pre_enable and post_disable phases. PLL and panel power disable
> also has been moved to post_disable phase. The ULPS entry/exit
> sequneces as suggested by HW team is as follows -
>
> During enable time -
> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
>
> And during disable time to flush all FIFOs -
> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
>
> Also during disbale sequnece sub-encoder disable is moved to the end
> after port is disabled.
>
> v2: Based on comments from Ville
>     - Detailed epxlaination in the commit messgae
>     - Moved parameter changes out into another patch
>     - Backlight enabling will be a new patch
>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
>  drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_dsi.h |    2 +
>  3 files changed, 91 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2bbff9..55c16cb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
>  #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
>  #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
>  
> +#define I915_WRITE_BITS(reg, val, mask) \
> +do { \
> +	u32 tmp, data; \
> +	tmp = I915_READ((reg)); \
> +	tmp &= ~(mask); \
> +	data = (val) & (mask); \
> +	data = data | tmp; \
> +	I915_WRITE((reg), data); \
> +} while(0)

I would still prefer the explicit read, modify, and write in the code
instead of this, but it's a matter of taste I'll leave for Daniel to
call the shots on.

One reason for my dislike is how easy it will be to accidentally get the
val and mask parameters mixed up. I would instinctively put the mask
before the value (common convention of context before the rest), which
would be wrong here. I am not saying changing the order would make me
like this.

> +
> +
>  /* "Broadcast RGB" property */
>  #define INTEL_BROADCAST_RGB_AUTO 0
>  #define INTEL_BROADCAST_RGB_FULL 1
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 8dc9a38..9e67f78 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>  	vlv_enable_dsi_pll(encoder);
>  }
>  
> -static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> -{
> -	DRM_DEBUG_KMS("\n");
> -}
> -
> -static void intel_dsi_enable(struct intel_encoder *encoder)
> +void intel_dsi_device_ready(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int pipe = intel_crtc->pipe;
> -	u32 temp;
>  
>  	DRM_DEBUG_KMS("\n");
>  
>  	if (intel_dsi->dev.dev_ops->panel_reset)
>  		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>  
> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
> -	if ((temp & DEVICE_READY) == 0) {
> -		temp &= ~ULPS_STATE_MASK;
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
> -	} else if (temp & ULPS_STATE_MASK) {
> -		temp &= ~ULPS_STATE_MASK;
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
> -		/*
> -		 * We need to ensure that there is a minimum of 1 ms time
> -		 * available before clearing the UPLS exit state.
> -		 */
> -		msleep(2);
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
> -	}
> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
> +	usleep_range(1000, 1500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
> +			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
> +			DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
> +			DEVICE_READY | ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
> +			DEVICE_READY | ULPS_STATE_MASK);

It seems like an odd dance, but if that's what the hw folks say, I guess
we'll just have to take their word for it...

> +	usleep_range(2000, 2500);
>  
>  	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>  		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);

Maybe the hooks would better belong in intel_dsi_pre_enable, not
here. The panel_reset hook could be renamed pre_enable while at it.

>  
> +}
> +static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> +{
> +	DRM_DEBUG_KMS("\n");
> +
> +	/* put device in ready state */
> +	intel_dsi_device_ready(encoder);
> +}
> +
> +static void intel_dsi_enable(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	int pipe = intel_crtc->pipe;
> +	u32 temp;
> +
> +	DRM_DEBUG_KMS("\n");
> +
>  	if (is_cmd_mode(intel_dsi))
>  		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
> -
> -	if (is_vid_mode(intel_dsi)) {
> +	else {
>  		msleep(20); /* XXX */
>  		dpi_send_cmd(intel_dsi, TURN_ON);
>  		msleep(100);
> @@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  static void intel_dsi_disable(struct intel_encoder *encoder)
>  {
> -	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int pipe = intel_crtc->pipe;
> @@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> -	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
> -
>  	if (is_vid_mode(intel_dsi)) {
>  		dpi_send_cmd(intel_dsi, SHUTDOWN);
>  		msleep(10);
> @@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>  		msleep(2);
>  	}
>  
> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
> -	if (temp & DEVICE_READY) {
> -		temp &= ~DEVICE_READY;
> -		temp &= ~ULPS_STATE_MASK;
> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
> -	}
> +	/* if disable packets are sent before sending shutdown packet then in
> +	 * some next enable sequence send turn on packet error is observed */
> +	if (intel_dsi->dev.dev_ops->disable)
> +		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>  }
>  
> -static void intel_dsi_post_disable(struct intel_encoder *encoder)
> +void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	int pipe = intel_crtc->pipe;
> +
>  	DRM_DEBUG_KMS("\n");
>  
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
> +							ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
> +							ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
> +							ULPS_STATE_MASK);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
> +	usleep_range(1000, 1500);
> +
> +	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
> +					== 0x00000), 30))
> +		DRM_ERROR("DSI LP not going Low\n");

AFE_LATCHOUT?

> +
> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
> +	usleep_range(2000, 2500);
> +
>  	vlv_disable_dsi_pll(encoder);
> +
> +	if (intel_dsi->dev.dev_ops->disable_panel_power)
> +		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);

Here too, hook better suited at intel_dsi_post_disable. And call it
post_disable.

> +}
> +static void intel_dsi_post_disable(struct intel_encoder *encoder)
> +{
> +	DRM_DEBUG_KMS("\n");
> +	intel_dsi_clear_device_ready(encoder);
>  }
>  
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 14509d6..387dfe1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -41,6 +41,8 @@ struct intel_dsi_dev_ops {
>  
>  	void (*panel_reset)(struct intel_dsi_device *dsi);
>  
> +	void (*disable_panel_power)(struct intel_dsi_device *dsi);
> +
>  	/* one time programmable commands if needed */
>  	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 1/7] drm/i915: Add more dev ops for MIPI sub encoder

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Some panels require one time programming if they do not contain their
> own eeprom for basic register initialization. The sequence is
>
> Panel Reset --> Send OTP --> Enable Pixel Stream --> Enable the panel
>
> v2: Based on review comments from Jani and Ville
>     - Updated the commit message with more details
>     - Move the new parameters out of this patch
>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c |    9 ++++++++-
>  drivers/gpu/drm/i915/intel_dsi.h |    5 +++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index d257b09..61267e2 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (intel_dsi->dev.dev_ops->panel_reset)
> +		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
> +
>  	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>  	if ((temp & DEVICE_READY) == 0) {
>  		temp &= ~ULPS_STATE_MASK;
> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>  	}
>  
> +	if (intel_dsi->dev.dev_ops->send_otp_cmds)
> +		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
> +
>  	if (is_cmd_mode(intel_dsi))
>  		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>  
> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  		POSTING_READ(MIPI_PORT_CTRL(pipe));
>  	}
>  
> -	intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
> +	if (intel_dsi->dev.dev_ops->enable)
> +		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>  }
>  
>  static void intel_dsi_disable(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index c7765f3..14509d6 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -39,6 +39,11 @@ struct intel_dsi_device {
>  struct intel_dsi_dev_ops {
>  	bool (*init)(struct intel_dsi_device *dsi);
>  
> +	void (*panel_reset)(struct intel_dsi_device *dsi);

See comments to patch 5/7.

> +
> +	/* one time programmable commands if needed */
> +	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
> +
>  	/* This callback must be able to assume DSI commands can be sent */
>  	void (*enable)(struct intel_dsi_device *dsi);
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 3/7] drm/i915: Compute dsi_clk from pixel clock

[Jani Nikula]

On Fri, 15 Nov 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Pixel clock based calculation is recommended in the MIPI host controller
>> documentation
>>
>> v2: Based on review comments from Jani and Ville
>>     - Use dsi_clk in KHz rather than converting in Hz and back to MHz
>>     - RR formula is retained though not used but return dsi_clk in KHz now
>>     - Moved the m-n-p changes into a separate patch
>>     - Removed the parameter check for intel_dsi->dsi_clock_freq. This will be
>>       bought back in if needed when appropriate panel drivers are done
>>
>> Signed-off-by: Vijayakumar Balakrishnan <vijayakumar.balakrishnan@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_pll.c |   46 +++++++++++++++++++++++++++++-----
>>  1 file changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> index 44279b2..9f3e6b0 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>>  	71, 35							/* 91 - 92 */
>>  };
>>  
>> +#ifdef DSI_CLK_FROM_RR
>> +
>>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>  			  int pixel_format, int video_mode_format,
>>  			  int lane_count, bool eotp)
>> @@ -121,7 +123,7 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>  
>>  	/* the dsi clock is divided by 2 in the hardware to get dsi ddr clock */
>>  	dsi_bit_clock_hz = bytes_per_x_frames_x_lanes * 8;
>> -	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
>> +	dsi_clk = dsi_bit_clock_hz / 1000;
>>  
>>  	if (eotp && video_mode_format == VIDEO_MODE_BURST)
>>  		dsi_clk *= 2;
>> @@ -129,6 +131,38 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>  	return dsi_clk;
>>  }
>>  
>> +#else
>> +
>> +/* Get DSI clock from pixel clock */
>> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
>> +			  int pixel_format, int lane_count)
>> +{
>> +	u32 dsi_clk_khz;
>> +	u32 bpp;
>> +
>> +	switch (pixel_format) {
>> +	default:
>> +	case VID_MODE_FORMAT_RGB888:
>> +	case VID_MODE_FORMAT_RGB666_LOOSE:
>> +		bpp = 24;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB666:
>> +		bpp = 18;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB565:
>> +		bpp = 16;
>> +		break;
>> +	}
>> +
>> +	/* DSI data rate = pixel clock * bits per pixel / lane count
>> +	   pixel clock is converted from KHz to Hz */
>> +	dsi_clk_khz = DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
>> +
>> +	return dsi_clk_khz;
>> +}
>> +
>> +#endif
>> +
>>  #ifdef MNP_FROM_TABLE
>>  
>>  struct dsi_clock_table {
>> @@ -200,13 +234,14 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>>  	u32 calc_p;
>>  	u32 m_seed;
>>  
>> -	if (dsi_clk < 300 || dsi_clk > 1150) {
>> +	/* dsi_clk is expected in KHZ */
>> +	if (dsi_clk < 300000 || dsi_clk > 1150000) {
>>  		DRM_ERROR("DSI CLK Out of Range\n");
>>  		return -ECHRNG;
>>  	}
>>  
>>  	ref_clk = 25000;
>> -	target_dsi_clk = dsi_clk * 1000;
>> +	target_dsi_clk = dsi_clk;
>
> The *other* dsi_calc_mnp() (with MNP_FROM_TABLE defined) remains
> unchanged, and bitrots. Either /= 1000 the input there, or better, if
> you don't see that the other function will be needed, just remove it.

Other than that,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>




>
> BR,
> Jani.
>
>
>>  	error = 0xFFFFFFFF;
>>  	calc_m = 0;
>>  	calc_p = 0;
>> @@ -251,9 +286,8 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>>  	struct dsi_mnp dsi_mnp;
>>  	u32 dsi_clk;
>>  
>> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
>> -				 intel_dsi->video_mode_format,
>> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
>> +	dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
>> +						intel_dsi->lane_count);
>>  
>>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>>  	if (ret) {
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 6/7] drm/i915: Remove redundant DSI PLL enabling

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> DSI PLL will get configured during crtc_enable using ->pre_pll_enable
> and no need to do in ->mode_set

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c |    3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 9e67f78..4dccb4b 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -373,9 +373,6 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  
>  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>  
> -	/* Update the DSI PLL */
> -	vlv_enable_dsi_pll(intel_encoder);
> -
>  	/* XXX: Location of the call */
>  	band_gap_reset(dev_priv);
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 7/7] drm/i915: Parametrize the dphy and other spec specific parameters

[Jani Nikula]

On Fri, 15 Nov 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> The values of these parameters will be different for differnet panel
>> based on dsi rate, lane count, etc. Remove the hardcodings and make
>> these as parameters whch will be initialized in panel specific
>> sub-encoder implementaion.
>>
>> This will also form groundwork for planned generic panel sub-encoder
>> implemntation based on VBT design enhancments to support multiple panels
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c |   27 +++++++++++++--------------
>>  drivers/gpu/drm/i915/intel_dsi.h |   14 ++++++++++++++
>>  2 files changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 4dccb4b..e9fde76 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -160,6 +160,7 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>  
>>  		/* assert ip_tg_enable signal */
>>  		temp = I915_READ(MIPI_PORT_CTRL(pipe));
>> +		temp = temp | intel_dsi->port_bits;
>
> Need to mask away port_bits before OR:ing?

Other than that,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>




>
>>  		I915_WRITE(MIPI_PORT_CTRL(pipe), temp | DPI_ENABLE);
>>  		POSTING_READ(MIPI_PORT_CTRL(pipe));
>>  	}
>> @@ -391,11 +392,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>>  	I915_WRITE(MIPI_INTR_STAT(pipe), 0xffffffff);
>>  	I915_WRITE(MIPI_INTR_EN(pipe), 0xffffffff);
>>  
>> -	I915_WRITE(MIPI_DPHY_PARAM(pipe),
>> -		   0x3c << EXIT_ZERO_COUNT_SHIFT |
>> -		   0x1f << TRAIL_COUNT_SHIFT |
>> -		   0xc5 << CLK_ZERO_COUNT_SHIFT |
>> -		   0x1f << PREPARE_COUNT_SHIFT);
>> +	I915_WRITE(MIPI_DPHY_PARAM(pipe), intel_dsi->dphy_reg);
>>  
>>  	I915_WRITE(MIPI_DPI_RESOLUTION(pipe),
>>  		   adjusted_mode->vdisplay << VERTICAL_ADDRESS_SHIFT |
>> @@ -443,9 +440,9 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>>  				       adjusted_mode->htotal,
>>  				       bpp, intel_dsi->lane_count) + 1);
>>  	}
>> -	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), 8309); /* max */
>> -	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), 0x14); /* max */
>> -	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), 0xffff); /* max */
>> +	I915_WRITE(MIPI_LP_RX_TIMEOUT(pipe), intel_dsi->lp_rx_timeout);
>> +	I915_WRITE(MIPI_TURN_AROUND_TIMEOUT(pipe), intel_dsi->turn_arnd_val);
>> +	I915_WRITE(MIPI_DEVICE_RESET_TIMER(pipe), intel_dsi->rst_timer_val);
>>  
>>  	/* dphy stuff */
>>  
>> @@ -460,29 +457,31 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>>  	 *
>>  	 * XXX: write MIPI_STOP_STATE_STALL?
>>  	 */
>> -	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe), 0x46);
>> +	I915_WRITE(MIPI_HIGH_LOW_SWITCH_COUNT(pipe),
>> +						intel_dsi->hs_to_lp_count);
>>  
>>  	/* XXX: low power clock equivalence in terms of byte clock. the number
>>  	 * of byte clocks occupied in one low power clock. based on txbyteclkhs
>>  	 * and txclkesc. txclkesc time / txbyteclk time * (105 +
>>  	 * MIPI_STOP_STATE_STALL) / 105.???
>>  	 */
>> -	I915_WRITE(MIPI_LP_BYTECLK(pipe), 4);
>> +	I915_WRITE(MIPI_LP_BYTECLK(pipe), intel_dsi->lp_byte_clk);
>>  
>>  	/* the bw essential for transmitting 16 long packets containing 252
>>  	 * bytes meant for dcs write memory command is programmed in this
>>  	 * register in terms of byte clocks. based on dsi transfer rate and the
>>  	 * number of lanes configured the time taken to transmit 16 long packets
>>  	 * in a dsi stream varies. */
>> -	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), 0x820);
>> +	I915_WRITE(MIPI_DBI_BW_CTRL(pipe), intel_dsi->bw_timer);
>>  
>>  	I915_WRITE(MIPI_CLK_LANE_SWITCH_TIME_CNT(pipe),
>> -		   0xa << LP_HS_SSW_CNT_SHIFT |
>> -		   0x14 << HS_LP_PWR_SW_CNT_SHIFT);
>> +		   intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT |
>> +		   intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT);
>>  
>>  	if (is_vid_mode(intel_dsi))
>>  		I915_WRITE(MIPI_VIDEO_MODE_FORMAT(pipe),
>> -			   intel_dsi->video_mode_format);
>> +				intel_dsi->video_frmt_cfg_bits |
>> +				intel_dsi->video_mode_format);
>>  }
>>  
>>  static enum drm_connector_status
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index 387dfe1..b4a27ce 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -96,6 +96,20 @@ struct intel_dsi {
>>  
>>  	/* eot for MIPI_EOT_DISABLE register */
>>  	u32 eot_disable;
>> +
>> +	u32 port_bits;
>> +	u32 bw_timer;
>> +	u32 dphy_reg;
>> +	u32 video_frmt_cfg_bits;
>> +	u16 lp_byte_clk;
>> +
>> +	/* timeouts in byte clocks */
>> +	u16 lp_rx_timeout;
>> +	u16 turn_arnd_val;
>> +	u16 rst_timer_val;
>> +	u16 hs_to_lp_count;
>> +	u16 clk_lp_to_hs_count;
>> +	u16 clk_hs_to_lp_count;
>>  };
>>  
>>  static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Daniel Vetter]

On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote:
> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> > Basically ULPS handling during enable/disable has been moved to
> > pre_enable and post_disable phases. PLL and panel power disable
> > also has been moved to post_disable phase. The ULPS entry/exit
> > sequneces as suggested by HW team is as follows -
> >
> > During enable time -
> > set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
> >
> > And during disable time to flush all FIFOs -
> > set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
> >
> > Also during disbale sequnece sub-encoder disable is moved to the end
> > after port is disabled.
> >
> > v2: Based on comments from Ville
> >     - Detailed epxlaination in the commit messgae
> >     - Moved parameter changes out into another patch
> >     - Backlight enabling will be a new patch
> >
> > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
> >  drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_dsi.h |    2 +
> >  3 files changed, 91 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a2bbff9..55c16cb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
> >  #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
> >  #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
> >  
> > +#define I915_WRITE_BITS(reg, val, mask) \
> > +do { \
> > +	u32 tmp, data; \
> > +	tmp = I915_READ((reg)); \
> > +	tmp &= ~(mask); \
> > +	data = (val) & (mask); \
> > +	data = data | tmp; \
> > +	I915_WRITE((reg), data); \
> > +} while(0)
> 
> I would still prefer the explicit read, modify, and write in the code
> instead of this, but it's a matter of taste I'll leave for Daniel to
> call the shots on.

Yeah, this looks a bit funny. We could compute the tmp value once (where
the mask is mutliple times the same thing) and then just or in the right
bits.  That should make the I915_WRITE calls fit ont on line, too, which
helps readability.

Also we put POSTING_READs before any waits to ensure the write has
actually landed. It's mostly documentation.

And while I'm at it: We generally frown upon readbacks of register value
and prefer to just keep track of things in software well enough. The
reason for that is that register readbacks allows us too much flexibility
in adding subtile state-depencies. Which long-term makes the code a real
pain to maintain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 2/7] drm/i915: Use FLISDSI interface for band gap reset

[Jani Nikula]

On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Reviewed-by: Jani Nikula<jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    2 ++
>  drivers/gpu/drm/i915/i915_reg.h       |    1 +
>  drivers/gpu/drm/i915/intel_dsi.c      |   47 ++++++---------------------------
>  drivers/gpu/drm/i915/intel_sideband.c |   14 ++++++++++
>  4 files changed, 25 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b12d942..a2bbff9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2408,6 +2408,8 @@ u32 intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
>  		   enum intel_sbi_destination destination);
>  void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
>  		     enum intel_sbi_destination destination);
> +u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
> +void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);

This hunk has a (trivial) conflict now, needs a rebase.

Jani.

>  
>  int vlv_gpu_freq(int ddr_freq, int val);
>  int vlv_freq_opcode(int ddr_freq, int val);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3f303ba..6236926 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -356,6 +356,7 @@
>  #define   IOSF_PORT_CCK				0x14
>  #define   IOSF_PORT_CCU				0xA9
>  #define   IOSF_PORT_GPS_CORE			0x48
> +#define   IOSF_PORT_FLISDSI			0x1B
>  #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
>  #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 61267e2..8dc9a38 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -37,49 +37,18 @@
>  static const struct intel_dsi_device intel_dsi_devices[] = {
>  };
>  
> -
> -static void vlv_cck_modify(struct drm_i915_private *dev_priv, u32 reg, u32 val,
> -			   u32 mask)
> -{
> -	u32 tmp = vlv_cck_read(dev_priv, reg);
> -	tmp &= ~mask;
> -	tmp |= val;
> -	vlv_cck_write(dev_priv, reg, tmp);
> -}
> -
> -static void band_gap_wa(struct drm_i915_private *dev_priv)
> +static void band_gap_reset(struct drm_i915_private *dev_priv)
>  {
>  	mutex_lock(&dev_priv->dpio_lock);
>  
> -	/* Enable bandgap fix in GOP driver */
> -	vlv_cck_modify(dev_priv, 0x6D, 0x00010000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6E, 0x00010000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6F, 0x00010000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x00, 0x00008000, 0x00008000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x00, 0x00000000, 0x00008000);
> -	msleep(20);
> -
> -	/* Turn Display Trunk on */
> -	vlv_cck_modify(dev_priv, 0x6B, 0x00020000, 0x00030000);
> -	msleep(20);
> -
> -	vlv_cck_modify(dev_priv, 0x6C, 0x00020000, 0x00030000);
> -	msleep(20);
> -
> -	vlv_cck_modify(dev_priv, 0x6D, 0x00020000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6E, 0x00020000, 0x00030000);
> -	msleep(20);
> -	vlv_cck_modify(dev_priv, 0x6F, 0x00020000, 0x00030000);
> +	vlv_flisdsi_write(dev_priv, 0x08, 0x0001);
> +	vlv_flisdsi_write(dev_priv, 0x0F, 0x0005);
> +	vlv_flisdsi_write(dev_priv, 0x0F, 0x0025);
> +	udelay(150);
> +	vlv_flisdsi_write(dev_priv, 0x0F, 0x0000);
> +	vlv_flisdsi_write(dev_priv, 0x08, 0x0000);
>  
>  	mutex_unlock(&dev_priv->dpio_lock);
> -
> -	/* Need huge delay, otherwise clock is not stable */
> -	msleep(100);
>  }
>  
>  static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
> @@ -363,7 +332,7 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  	vlv_enable_dsi_pll(intel_encoder);
>  
>  	/* XXX: Location of the call */
> -	band_gap_wa(dev_priv);
> +	band_gap_reset(dev_priv);
>  
>  	/* escape clock divider, 20MHz, shared for A and C. device ready must be
>  	 * off when doing this! txclkesc? */
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 9944d81..f15e57e 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -242,3 +242,17 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
>  		return;
>  	}
>  }
> +
> +u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg)
> +{
> +	u32 val = 0;
> +	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
> +					DPIO_OPCODE_REG_READ, reg, &val);
> +	return val;
> +}
> +
> +void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
> +{
> +	vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_FLISDSI,
> +					DPIO_OPCODE_REG_WRITE, reg, &val);
> +}
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Shobhit Kumar]

On Friday 15 November 2013 02:25 PM, Daniel Vetter wrote:
> On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote:
>> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>> Basically ULPS handling during enable/disable has been moved to
>>> pre_enable and post_disable phases. PLL and panel power disable
>>> also has been moved to post_disable phase. The ULPS entry/exit
>>> sequneces as suggested by HW team is as follows -
>>>
>>> During enable time -
>>> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
>>>
>>> And during disable time to flush all FIFOs -
>>> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
>>>
>>> Also during disbale sequnece sub-encoder disable is moved to the end
>>> after port is disabled.
>>>
>>> v2: Based on comments from Ville
>>>      - Detailed epxlaination in the commit messgae
>>>      - Moved parameter changes out into another patch
>>>      - Backlight enabling will be a new patch
>>>
>>> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
>>>   drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
>>>   drivers/gpu/drm/i915/intel_dsi.h |    2 +
>>>   3 files changed, 91 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index a2bbff9..55c16cb 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
>>>   #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
>>>   #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
>>>
>>> +#define I915_WRITE_BITS(reg, val, mask) \
>>> +do { \
>>> +	u32 tmp, data; \
>>> +	tmp = I915_READ((reg)); \
>>> +	tmp &= ~(mask); \
>>> +	data = (val) & (mask); \
>>> +	data = data | tmp; \
>>> +	I915_WRITE((reg), data); \
>>> +} while(0)
>>
>> I would still prefer the explicit read, modify, and write in the code
>> instead of this, but it's a matter of taste I'll leave for Daniel to
>> call the shots on.
>
> Yeah, this looks a bit funny. We could compute the tmp value once (where
> the mask is mutliple times the same thing) and then just or in the right
> bits.  That should make the I915_WRITE calls fit ont on line, too, which
> helps readability.
>
> Also we put POSTING_READs before any waits to ensure the write has
> actually landed. It's mostly documentation.
>
> And while I'm at it: We generally frown upon readbacks of register value
> and prefer to just keep track of things in software well enough. The
> reason for that is that register readbacks allows us too much flexibility
> in adding subtile state-depencies. Which long-term makes the code a real
> pain to maintain.

Ok. Will work on updating the patch accordingly and take care of other 
comments as well in next patch set update soon.

Regards
Shobhit

Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Shobhit Kumar]

On Wednesday 20 November 2013 07:09 AM, Shobhit Kumar wrote:
> On Friday 15 November 2013 02:25 PM, Daniel Vetter wrote:
>> On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote:
>>> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>>> Basically ULPS handling during enable/disable has been moved to
>>>> pre_enable and post_disable phases. PLL and panel power disable
>>>> also has been moved to post_disable phase. The ULPS entry/exit
>>>> sequneces as suggested by HW team is as follows -
>>>>
>>>> During enable time -
>>>> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
>>>>
>>>> And during disable time to flush all FIFOs -
>>>> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
>>>>
>>>> Also during disbale sequnece sub-encoder disable is moved to the end
>>>> after port is disabled.
>>>>
>>>> v2: Based on comments from Ville
>>>>      - Detailed epxlaination in the commit messgae
>>>>      - Moved parameter changes out into another patch
>>>>      - Backlight enabling will be a new patch
>>>>
>>>> Signed-off-by: Yogesh Mohan Marimuthu
>>>> <yogesh.mohan.marimuthu@intel.com>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
>>>>   drivers/gpu/drm/i915/intel_dsi.c |  111
>>>> ++++++++++++++++++++++++++------------
>>>>   drivers/gpu/drm/i915/intel_dsi.h |    2 +
>>>>   3 files changed, 91 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index a2bbff9..55c16cb 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
>>>>   #define POSTING_READ(reg)    (void)I915_READ_NOTRACE(reg)
>>>>   #define POSTING_READ16(reg)    (void)I915_READ16_NOTRACE(reg)
>>>>
>>>> +#define I915_WRITE_BITS(reg, val, mask) \
>>>> +do { \
>>>> +    u32 tmp, data; \
>>>> +    tmp = I915_READ((reg)); \
>>>> +    tmp &= ~(mask); \
>>>> +    data = (val) & (mask); \
>>>> +    data = data | tmp; \
>>>> +    I915_WRITE((reg), data); \
>>>> +} while(0)
>>>
>>> I would still prefer the explicit read, modify, and write in the code
>>> instead of this, but it's a matter of taste I'll leave for Daniel to
>>> call the shots on.
>>
>> Yeah, this looks a bit funny. We could compute the tmp value once (where
>> the mask is mutliple times the same thing) and then just or in the right
>> bits.  That should make the I915_WRITE calls fit ont on line, too, which
>> helps readability.
>>
>> Also we put POSTING_READs before any waits to ensure the write has
>> actually landed. It's mostly documentation.
>>
>> And while I'm at it: We generally frown upon readbacks of register value
>> and prefer to just keep track of things in software well enough. The
>> reason for that is that register readbacks allows us too much flexibility
>> in adding subtile state-depencies. Which long-term makes the code a real
>> pain to maintain.
>
> Ok. Will work on updating the patch accordingly and take care of other
> comments as well in next patch set update soon.

Sorry took me more time than I anticipated to rework on this due to 
other critical stuff. Looking at the code and doing more testing I now 
confirmed that there is no READ/Modify/WRITE needed for ULPS and hence I 
will convert I915_WRITE_BITS to normal I915_WRITE. Will be sending 
updated patches on Monday

Regards
Shobhit

Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Shobhit Kumar]

On Friday 15 November 2013 01:57 PM, Jani Nikula wrote:
> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Basically ULPS handling during enable/disable has been moved to
>> pre_enable and post_disable phases. PLL and panel power disable
>> also has been moved to post_disable phase. The ULPS entry/exit
>> sequneces as suggested by HW team is as follows -
>>
>> During enable time -
>> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
>>
>> And during disable time to flush all FIFOs -
>> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
>>
>> Also during disbale sequnece sub-encoder disable is moved to the end
>> after port is disabled.
>>
>> v2: Based on comments from Ville
>>      - Detailed epxlaination in the commit messgae
>>      - Moved parameter changes out into another patch
>>      - Backlight enabling will be a new patch
>>
>> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
>>   drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_dsi.h |    2 +
>>   3 files changed, 91 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a2bbff9..55c16cb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
>>   #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
>>   #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
>>
>> +#define I915_WRITE_BITS(reg, val, mask) \
>> +do { \
>> +	u32 tmp, data; \
>> +	tmp = I915_READ((reg)); \
>> +	tmp &= ~(mask); \
>> +	data = (val) & (mask); \
>> +	data = data | tmp; \
>> +	I915_WRITE((reg), data); \
>> +} while(0)
>
> I would still prefer the explicit read, modify, and write in the code
> instead of this, but it's a matter of taste I'll leave for Daniel to
> call the shots on.
>
> One reason for my dislike is how easy it will be to accidentally get the
> val and mask parameters mixed up. I would instinctively put the mask
> before the value (common convention of context before the rest), which
> would be wrong here. I am not saying changing the order would make me
> like this.

As mentioned in another mail, this is not required and I will remove it

>
>> +
>> +
>>   /* "Broadcast RGB" property */
>>   #define INTEL_BROADCAST_RGB_AUTO 0
>>   #define INTEL_BROADCAST_RGB_FULL 1
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 8dc9a38..9e67f78 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>>   	vlv_enable_dsi_pll(encoder);
>>   }
>>
>> -static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> -{
>> -	DRM_DEBUG_KMS("\n");
>> -}
>> -
>> -static void intel_dsi_enable(struct intel_encoder *encoder)
>> +void intel_dsi_device_ready(struct intel_encoder *encoder)
>>   {
>>   	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int pipe = intel_crtc->pipe;
>> -	u32 temp;
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>>   	if (intel_dsi->dev.dev_ops->panel_reset)
>>   		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>>
>> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> -	if ((temp & DEVICE_READY) == 0) {
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
>> -	} else if (temp & ULPS_STATE_MASK) {
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
>> -		/*
>> -		 * We need to ensure that there is a minimum of 1 ms time
>> -		 * available before clearing the UPLS exit state.
>> -		 */
>> -		msleep(2);
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> -	}
>> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
>> +	usleep_range(1000, 1500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
>> +			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>
> It seems like an odd dance, but if that's what the hw folks say, I guess
> we'll just have to take their word for it...

Yeah, but I reconfirmed from HW team and this is also now updated in 
documents

>
>> +	usleep_range(2000, 2500);
>>
>>   	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>>   		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>
> Maybe the hooks would better belong in intel_dsi_pre_enable, not
> here. The panel_reset hook could be renamed pre_enable while at it.
>

Ok, moved the hooks into pre_enable, but I am not too sure on changing 
the names as the current name suggests what exactly we are doing with 
the panel. If you agree I will leave that bit out but do other changes.

>>
>> +}
>> +static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> +{
>> +	DRM_DEBUG_KMS("\n");
>> +
>> +	/* put device in ready state */
>> +	intel_dsi_device_ready(encoder);
>> +}
>> +
>> +static void intel_dsi_enable(struct intel_encoder *encoder)
>> +{
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> +	int pipe = intel_crtc->pipe;
>> +	u32 temp;
>> +
>> +	DRM_DEBUG_KMS("\n");
>> +
>>   	if (is_cmd_mode(intel_dsi))
>>   		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>> -
>> -	if (is_vid_mode(intel_dsi)) {
>> +	else {
>>   		msleep(20); /* XXX */
>>   		dpi_send_cmd(intel_dsi, TURN_ON);
>>   		msleep(100);
>> @@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>
>>   static void intel_dsi_disable(struct intel_encoder *encoder)
>>   {
>> -	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int pipe = intel_crtc->pipe;
>> @@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> -	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>> -
>>   	if (is_vid_mode(intel_dsi)) {
>>   		dpi_send_cmd(intel_dsi, SHUTDOWN);
>>   		msleep(10);
>> @@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>>   		msleep(2);
>>   	}
>>
>> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> -	if (temp & DEVICE_READY) {
>> -		temp &= ~DEVICE_READY;
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> -	}
>> +	/* if disable packets are sent before sending shutdown packet then in
>> +	 * some next enable sequence send turn on packet error is observed */
>> +	if (intel_dsi->dev.dev_ops->disable)
>> +		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>>   }
>>
>> -static void intel_dsi_post_disable(struct intel_encoder *encoder)
>> +void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>>   {
>> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> +	int pipe = intel_crtc->pipe;
>> +
>>   	DRM_DEBUG_KMS("\n");
>>
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
>> +	usleep_range(1000, 1500);
>> +
>> +	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
>> +					== 0x00000), 30))
>> +		DRM_ERROR("DSI LP not going Low\n");
>
> AFE_LATCHOUT?

Yeah, fixed it. Rap on me hardcodings :)

>
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
>> +	usleep_range(2000, 2500);
>> +
>>   	vlv_disable_dsi_pll(encoder);
>> +
>> +	if (intel_dsi->dev.dev_ops->disable_panel_power)
>> +		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
>
> Here too, hook better suited at intel_dsi_post_disable. And call it
> post_disable.

Will move the hook but not too sure on changing the name as above

You can expect updated patches by Monday. Sorry it took me some time to 
get back to this.

Regards
Shobiht

Re: [PATCH v2 0/7] drm/i915: Baytrail MIPI DSI support Updated

[Daniel Vetter]

Re-add intel-gfx for real ...

On Wed, Dec 11, 2013 at 3:25 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> Re-adding intel-gfx for the testing discussion.
>
> On Wed, Dec 11, 2013 at 3:00 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>>>> btw for command mode support I'd really like to see an automated
>>>> testcase. Ville has done a crc based testcase for fbc and it greatly
>>>> helped in hunting down issues and corner cases. We plan to do
>>>> something similar for edp psr (using the mandatory sync crcs). Is
>>>> there anything like that for command mode dsi so that we can check
>>>> that the manual update all works correctly? Preferably something in
>>>> the sink, but if we have some way to do crcs on the source that might
>>>> also be useful.
>>>
>>>
>>> I think support for that is going to be panel specific if it exists at
>>> all. Some panels support reading back the framebuffer, but reading is
>>> low power mode only i.e. too slow to be useful. I've also seen a pentile
>>> amoled display return the internal pentile representation of the frame,
>>> not what was written to the buffer.
>>>
>>
>> Agree. I will see what can be done. BTW command mode code is there but we
>> have not got it working yet :). Another thing we are starting now is Dual
>> link. I will keep testing in mind.
>
> If we don't have any means to check the panel's framebuffer I think we
> should look into source-based CRCs if possible. If that again also
> doesn't work for command mode display I guess we're left with indirect
> testing by sharing the overall invalidation logic with PSR. But that
> only really works if both psr and dsi command-mode use the same
> sw-based invalidation logic, as soon as we use some of the hw support
> (like we do for psr on hsw) the testing doesn't translate to DSI cmd
> mode panels any more.
>
> So some good ideas for how we could better validate the cmd mode logic
> would be awesome, I think atm we only have sub-par options. And ime
> such fb invalidation like for fbc/psr is really tricky, if we need to
> rely on manual testing then that pretty much means it'll always be
> broken a bit. Or regress really often without us noticing :(
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch