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

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

On Mon, 01 Dec 2014, "Singh, Gaurav K" <gaurav.k.singh@intel.com> wrote:
> 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.

I was thinking we could add a helper for that, but let's leave it as it
is now.

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

Here's my thinking:

If port_bits is (or will be) used in places other than
intel_dsi_port_enable, I think it needs to be set beforehand, because
otherwise the other users will depend on intel_dsi_port_enable having
been called.

On the other hand, if port_bits is not (and will not be) used in other
places, intel_dsi_port_enable has all the information to set
MIPI_PORT_CTRL, and the whole intel_dsi->port_bits field can be removed.

BR,
Jani.



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

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