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

Jani Nikula
2013-12-11T11:00:28Z

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