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

Shobhit Kumar
2013-12-11T12:19:21Z

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