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

[Shobhit Kumar <shobhit.kumar@intel.com>]

On Wednesday 11 December 2013 04:32 PM, Jani Nikula wrote:
> 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>

Yeah sending the updated patch

Regards
Shobhit