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

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 10 Dec 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
>
> v3: Updated as per Jani's comments
>     - Removed the I915_WRITE_BITS as it is not needed
>     - Moved panel_reset and send_otp_cmds hooks to dsi_pre_enable
>     - Moved disable_panel_power hook to dsi_post_disable
>     - Replace hardcoding with AFE_LATCHOUT
>
> 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 | 110 +++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_dsi.h |   2 +
>  2 files changed, 79 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 1016e7b..01b9f3a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -101,46 +101,57 @@ 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)
> +void intel_dsi_device_ready(struct intel_encoder *encoder)

Should be static.

>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int pipe = intel_crtc->pipe;
> +	u32 val;
> +
>  	DRM_DEBUG_KMS("\n");
> -}
>  
> -static void intel_dsi_enable(struct intel_encoder *encoder)
> +	val = I915_READ(MIPI_PORT_CTRL(pipe));
> +	I915_WRITE(MIPI_PORT_CTRL(pipe), val | LP_OUTPUT_HOLD);
> +	usleep_range(1000, 1500);
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), DEVICE_READY | ULPS_STATE_EXIT);
> +	usleep_range(2000, 2500);
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), DEVICE_READY);
> +	usleep_range(2000, 2500);
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), 0x00);
> +	usleep_range(2000, 2500);
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), DEVICE_READY);
> +	usleep_range(2000, 2500);
> +}
> +static void intel_dsi_pre_enable(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);
> -	}
> +	/* put device in ready state */
> +	intel_dsi_device_ready(encoder);
>  
>  	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>  		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
> +}
> +
> +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 +168,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 +177,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,20 +189,54 @@ 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)

Should be static.

With these fixed,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int pipe = intel_crtc->pipe;
> +	u32 val;
> +
>  	DRM_DEBUG_KMS("\n");
>  
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT);
> +	usleep_range(2000, 2500);
> +
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER);
> +	usleep_range(2000, 2500);
> +
> +	val = I915_READ(MIPI_PORT_CTRL(pipe));
> +	I915_WRITE(MIPI_PORT_CTRL(pipe), val & ~LP_OUTPUT_HOLD);
> +	usleep_range(1000, 1500);
> +
> +	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & AFE_LATCHOUT)
> +					== 0x00000), 30))
> +		DRM_ERROR("DSI LP not going Low\n");
> +
> +	I915_WRITE(MIPI_DEVICE_READY(pipe), 0x00);
> +	usleep_range(2000, 2500);
> +
>  	vlv_disable_dsi_pll(encoder);
>  }
> +static void intel_dsi_post_disable(struct intel_encoder *encoder)
> +{
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	intel_dsi_clear_device_ready(encoder);
> +
> +	if (intel_dsi->dev.dev_ops->disable_panel_power)
> +		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
> +}
>  
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
>  				   enum pipe *pipe)
> 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.8.3.2
>

-- 
Jani Nikula, Intel Open Source Technology Center