Re: [PATCH 3/9] drm/i915: Add support for port enable/disable for dual link configuration

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sat, 29 Nov 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote:
> For Dual Link MIPI Panels, both Port A and Port C should be enabled
> during the MIPI encoder enabling sequence. Similarly, during the
> disabling sequence, both ports needs to be disabled.
>
> v2: Used for_each_dsi_port macro instead of for loop
>
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h            |    1 +
>  drivers/gpu/drm/i915/intel_dsi.c           |   39 +++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_dsi.h           |    1 +
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |    7 +++++
>  4 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc03fac..c981f5d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6664,6 +6664,7 @@ enum punit_power_well {
>  #define  DPI_ENABLE					(1 << 31) /* A + C */
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT		27
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK		(0xf << 27)
> +#define  DUAL_LINK_MODE_SHIFT				26
>  #define  DUAL_LINK_MODE_MASK				(1 << 26)
>  #define  DUAL_LINK_MODE_FRONT_BACK			(0 << 26)
>  #define  DUAL_LINK_MODE_PIXEL_ALTERNATIVE		(1 << 26)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 693736b..1163a5b 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -108,28 +108,43 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder)
>  	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;
>  
> -	/* assert ip_tg_enable signal */
> -	temp = I915_READ(MIPI_PORT_CTRL(port)) & ~LANE_CONFIGURATION_MASK;
> -	temp = temp | intel_dsi->port_bits;
> -	I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE);
> -	POSTING_READ(MIPI_PORT_CTRL(port));
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		temp = I915_READ(MIPI_PORT_CTRL(port));
> +
> +		if (intel_dsi->dual_link) {
> +			if (port == PORT_A)
> +				intel_dsi->port_bits |= intel_crtc->pipe ?
> +					LANE_CONFIGURATION_DUAL_LINK_B :
> +					LANE_CONFIGURATION_DUAL_LINK_A;
> +			else
> +				intel_dsi->port_bits = 0;

It feels wrong to clobber intel_dsi->port_bits here; the old code didn't
do that either. I think you should either set port_bits somewhere else
(or remove it altogether), and then modify temp depending on port.

Side note, it seems to me intel_dsi->dual_link is becoming redundant, as
intel_dsi->ports will have more than one bit set in the dual link
case. This can be a future cleanup though.

BR,
Jani.

> +		} else
> +			temp &= ~LANE_CONFIGURATION_MASK;
> +
> +		/* assert ip_tg_enable signal */
> +		temp = temp | intel_dsi->port_bits;
> +		I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE);
> +		POSTING_READ(MIPI_PORT_CTRL(port));
> +	}
>  }
>  
>  static void intel_dsi_port_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);
> -	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 temp;
>  
> -	/* de-assert ip_tg_enable signal */
> -	temp = I915_READ(MIPI_PORT_CTRL(port));
> -	I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE);
> -	POSTING_READ(MIPI_PORT_CTRL(port));
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		/* de-assert ip_tg_enable signal */
> +		temp = I915_READ(MIPI_PORT_CTRL(port));
> +		I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE);
> +		POSTING_READ(MIPI_PORT_CTRL(port));
> +	}
>  }
>  
>  static void intel_dsi_device_ready(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 7f5d028..f2cc2fc 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -104,6 +104,7 @@ struct intel_dsi {
>  	u8 clock_stop;
>  
>  	u8 escape_clk_div;
> +	u8 dual_link;
>  	u32 port_bits;
>  	u32 bw_timer;
>  	u32 dphy_reg;
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 612592f..7f1ba58 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -287,6 +287,13 @@ static bool generic_init(struct intel_dsi_device *dsi)
>  	intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0;
>  	intel_dsi->lane_count = mipi_config->lane_cnt + 1;
>  	intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
> +	intel_dsi->dual_link = mipi_config->dual_link;
> +
> +	if (intel_dsi->dual_link) {
> +		intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C));
> +		intel_dsi->port_bits = (intel_dsi->dual_link - 1)
> +					<< DUAL_LINK_MODE_SHIFT;
> +	}
>  
>  	if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666)
>  		bits_per_pixel = 18;
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx