[Intel-gfx] [PATCH v5 01/13] drm/i915/icl: Configure lane sequencing of combo phy transmitter

[Madhav Chauhan]

On 9/11/2018 11:16 PM, Jani Nikula wrote:
> On Fri, 27 Jul 2018, "Chauhan, Madhav" <madhav.chauhan at intel.com> wrote:
>>> -----Original Message-----
>>> From: Chauhan, Madhav
>>> Sent: Friday, July 20, 2018 12:06 AM
>>> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Cc: intel-gfx at lists.freedesktop.org; Nikula, Jani <jani.nikula at intel.com>;
>>> Zanoni, Paulo R <paulo.r.zanoni at intel.com>; Vivi, Rodrigo
>>> <rodrigo.vivi at intel.com>
>>> Subject: RE: [Intel-gfx] [PATCH v5 01/13] drm/i915/icl: Configure lane
>>> sequencing of combo phy transmitter
>>>
>>>> -----Original Message-----
>>>> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>>>> Sent: Thursday, July 19, 2018 9:42 PM
>>>> To: Chauhan, Madhav <madhav.chauhan at intel.com>
>>>> Cc: intel-gfx at lists.freedesktop.org; Nikula, Jani
>>>> <jani.nikula at intel.com>; Zanoni, Paulo R <paulo.r.zanoni at intel.com>;
>>>> Vivi, Rodrigo <rodrigo.vivi at intel.com>
>>>> Subject: Re: [Intel-gfx] [PATCH v5 01/13] drm/i915/icl: Configure lane
>>>> sequencing of combo phy transmitter
>>>>
>>>> On Tue, Jul 10, 2018 at 03:10:02PM +0530, Madhav Chauhan wrote:
>>>>> This patch set the loadgen select and latency optimization for aux
>>>>> and transmit lanes of combo phy transmitters. It will be used for
>>>>> MIPI DSI HS operations.
>>> Thanks for reviewing DSI patches.
>>>
>>>>> v2: Rebase
>>>>>
>>>>> Signed-off-by: Madhav Chauhan <madhav.chauhan at intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/icl_dsi.c | 38
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/icl_dsi.c
>>>>> b/drivers/gpu/drm/i915/icl_dsi.c index 13830e4..a571339 100644
>>>>> --- a/drivers/gpu/drm/i915/icl_dsi.c
>>>>> +++ b/drivers/gpu/drm/i915/icl_dsi.c
>>>>> @@ -105,10 +105,48 @@ static void gen11_dsi_power_up_lanes(struct
>>>> intel_encoder *encoder)
>>>>>   	}
>>>>>   }
>>>>>
>>>>> +static void gen11_dsi_config_phy_lanes_sequence(struct
>>>>> +intel_encoder
>>>>> +*encoder) {
>>>>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>>>> +	enum port port;
>>>>> +	u32 tmp;
>>>>> +	int lane;
>>>> tmp/lane could be moved to into the loops.
>> Was it due to intel_dsi->ports have no port assigned and
>> loop for_each_dsi_port() will not proceed further??
>> If that's the case, these encoder enable/disable function should be called
>> Only when dsi_init is success and then, intel_dsi->ports have some valid port value.
>>
>> Please clarify.
> Ville's comments are purely about style and readability.
>
>> Regards,
>> Madhav
>>
>>>> Same in other patches.
>>> Agree, make sense.
>> Just to understand
>>>>> +
>>>>> +	/* Step 4b(i) set loadgen select for transmit and aux lanes */
>>>>> +	for_each_dsi_port(port, intel_dsi->ports) {
>>>>> +		tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
>>>>> +		tmp &= ~LOADGEN_SELECT;
>>>>> +		I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
>>>>> +		for (lane = 0; lane <= 3; lane++) {
>>>>> +			tmp = I915_READ(ICL_PORT_TX_DW4_LN(port,
>>>> lane));
>>>>> +			tmp &= ~LOADGEN_SELECT;
>>>>> +			if (lane != 2)
>>>>> +				tmp |= LOADGEN_SELECT;
>>>>> +			I915_WRITE(ICL_PORT_TX_DW4_LN(port, lane),
>>>> tmp);
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	/* Step 4b(ii) set latency optimization for transmit and aux lanes */
>>>>> +	for_each_dsi_port(port, intel_dsi->ports) {
>>>>> +		tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
>>>>> +		tmp &= ~FRC_LATENCY_OPTIM_MASK;
>>>>> +		tmp |= FRC_LATENCY_OPTIM_VAL(0x5);
>>>>> +		I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
>>>>> +		tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
>>>>> +		tmp &= ~FRC_LATENCY_OPTIM_MASK;
>>>>> +		tmp |= FRC_LATENCY_OPTIM_VAL(0x5);
>>>>> +		I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
> The "read something, modify, write something else" pattern always gives
> me the creeps. But I guess reading _GRP is not an option?
>
> Anyway, for the actual content,
>
> Reviewed-by: Jani Nikula <jani.nikula at intel.com>

Thanks for the review. Yes, we need to read *only* LN0 and if same 
value, then write through GRP
register.

Regards,
Madhav

>
>>>>> +	}
>>>> An empty line here and there would make this a bit more legible.
>>>>
>>>> Same in other patches.
>>> Ok.  Thought this will be additional line, multiple Places in code use this :)
>>>
>>> Regards,
>>> Madhav
>>>
>>>>> +}
>>>>> +
>>>>>   static void gen11_dsi_enable_port_and_phy(struct intel_encoder
>>>>> *encoder)  {
>>>>>   	/* step 4a: power up all lanes of the DDI used by DSI */
>>>>>   	gen11_dsi_power_up_lanes(encoder);
>>>>> +
>>>>> +	/* step 4b: configure lane sequencing of the Combo-PHY
>>>>> +transmitters
>>>> */
>>>>> +	gen11_dsi_config_phy_lanes_sequence(encoder);
>>>>>   }
>>>>>
>>>>>   static void __attribute__((unused))
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> --
>>>> Ville Syrjälä
>>>> Intel
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx