[Intel-gfx] [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence
Jani Nikula
jani.nikula at intel.com
Fri Nov 15 09:27:25 CET 2013
On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar at 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 at intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list