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

From: "Singh, Gaurav K" <gaurav.k.singh@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>
Cc: Shobhit Kumar <shobhit.kumar@intel.com>
Subject: Re: [PATCH 3/9] drm/i915: Add support for port	enable/disable for dual link configuration
Date: Mon, 01 Dec 2014 23:35:41 +0530	[thread overview]
Message-ID: <547CADF5.1070105@intel.com> (raw)
In-Reply-To: <87wq6ba291.fsf@intel.com>

On 12/1/2014 7:41 PM, Jani Nikula wrote:
> On Mon, 01 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> 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.
> Okay, so it's becoming redundant for checking whether we are using dual
> link or not, but it still has it's place for indicating which dual link
> mode to use.
>
> BR,
> Jani.
Hi Jani,
I preferred if (intel_dsi->dual_link) than if (intel_dsi->ports ==((1 << 
PORT_A) | (1 << PORT_C))), as it looks more cleaner than the second one.

Regarding port_bits, in the old code for single link DSI panels, this 
variable was always 0, and it did not had any use. But with dual link 
configuration, there are few more register bits which we need to 
configure, since the function name was intel_dsi_port_enable, i thought 
of setting the corresponding mipi port_bits variable in this function 
itself.

With regards,
Gaurav

>
>> 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