On Thu, 04 Dec 2014, Gaurav K Singh <gaurav.k.singh@xxxxxxxxx> wrote:
> We need to program both port registers during dual link disable path.
>
> v2: Address review comments by Jani
> - Used a for loop instead of do-while loop.
>
> v3: Used for_each_dsi_port macro instead of for loop
>
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@xxxxxxxxx>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_dsi.c | 67 ++++++++++++++++++++------------------
> 1 file changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 22b1570..5ae4015 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -281,9 +281,8 @@ static void intel_dsi_disable(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);
> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe);
> + enum port port;
> u32 temp;
>
> DRM_DEBUG_KMS("\n");
> @@ -295,23 +294,24 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
> msleep(2);
> }
>
> - /* Panel commands can be sent when clock is in LP11 */
> - I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
> -
> - temp = I915_READ(MIPI_CTRL(port));
> - temp &= ~ESCAPE_CLOCK_DIVIDER_MASK;
> - I915_WRITE(MIPI_CTRL(port), temp |
> - intel_dsi->escape_clk_div <<
> - ESCAPE_CLOCK_DIVIDER_SHIFT);
> + for_each_dsi_port(port, intel_dsi->ports) {
> + /* Panel commands can be sent when clock is in LP11 */
> + I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
>
> - I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
> + temp = I915_READ(MIPI_CTRL(port));
> + temp &= ~ESCAPE_CLOCK_DIVIDER_MASK;
> + I915_WRITE(MIPI_CTRL(port), temp |
> + intel_dsi->escape_clk_div <<
> + ESCAPE_CLOCK_DIVIDER_SHIFT);
>
> - temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
> - temp &= ~VID_MODE_FORMAT_MASK;
> - I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
> + I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
>
> - I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
> + temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
> + temp &= ~VID_MODE_FORMAT_MASK;
> + I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
>
> + I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
> + }
> /* 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)
> @@ -323,31 +323,36 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
> static 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);
> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe);
> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> + enum port port;
> u32 val;
>
> DRM_DEBUG_KMS("\n");
> + for_each_dsi_port(port, intel_dsi->ports) {
>
> - I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY | ULPS_STATE_ENTER);
> - usleep_range(2000, 2500);
> + I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> + ULPS_STATE_ENTER);
> + usleep_range(2000, 2500);
>
> - I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY | ULPS_STATE_EXIT);
> - usleep_range(2000, 2500);
> + I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> + ULPS_STATE_EXIT);
> + usleep_range(2000, 2500);
>
> - I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY | ULPS_STATE_ENTER);
> - usleep_range(2000, 2500);
> + I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
> + ULPS_STATE_ENTER);
> + usleep_range(2000, 2500);
>
> - if (wait_for(((I915_READ(MIPI_PORT_CTRL(port)) & AFE_LATCHOUT)
> - == 0x00000), 30))
> - DRM_ERROR("DSI LP not going Low\n");
> + if (wait_for(((I915_READ(MIPI_PORT_CTRL(PORT_A)) & AFE_LATCHOUT)
> + == 0x00000), 30))
> + DRM_ERROR("DSI LP not going Low\n");
I find it a bit odd that you do the sequence for each port, while this
bit is specific to port A. It deserves a comment at the very least.
But perhaps we need several for_each_dsi_port() loops here, doing some
things for all ports, then common things (like this one, and maybe group
sleeps into one too?), then again some things for all ports, etc.
I am really not sure what the right thing is, but this *feels* a bit
odd. Please explain at least.
In the end, I don't see anything particularly wrong with the patch, so
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> - val = I915_READ(MIPI_PORT_CTRL(port));
> - I915_WRITE(MIPI_PORT_CTRL(port), val & ~LP_OUTPUT_HOLD);
> - usleep_range(1000, 1500);
> + val = I915_READ(MIPI_PORT_CTRL(port));
> + I915_WRITE(MIPI_PORT_CTRL(port), val & ~LP_OUTPUT_HOLD);
> + usleep_range(1000, 1500);
>
> - I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
> - usleep_range(2000, 2500);
> + I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
> + usleep_range(2000, 2500);
> + }
>
> vlv_disable_dsi_pll(encoder);
> }
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
