[Intel-gfx] [PATCH v5 02/13] drm/i915/icl: DSI vswing programming sequence

[Madhav Chauhan]

On 9/12/2018 12:20 AM, Jani Nikula wrote:
> On Tue, 10 Jul 2018, Madhav Chauhan <madhav.chauhan at intel.com> wrote:
>> This patch setup voltage swing before enabling
>> combo PHY DDI (shared with DSI).
>> Note that DSI voltage swing programming is for
>> high speed data buffers. HW automatically handles
>> the voltage swing for the low power data buffers.
>>
>> v2: Rebase
>>
>> Signed-off-by: Madhav Chauhan <madhav.chauhan at intel.com>
>> ---
>>   drivers/gpu/drm/i915/icl_dsi.c | 114 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 114 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
>> index a571339..dc16c1f 100644
>> --- a/drivers/gpu/drm/i915/icl_dsi.c
>> +++ b/drivers/gpu/drm/i915/icl_dsi.c
>> @@ -27,6 +27,65 @@
>>   
>>   #include "intel_dsi.h"
>>   
>> +static void dsi_program_swing_and_deemphasis(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;
>> +
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +
>> +		/* Bspec: set scaling mode to 0x6 */
> Today bspec says 2. Also, please don't duplicate the value in the
> comment.

Right..thanks for catching :)

>
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
>> +		tmp |= SCALING_MODE_SEL(6);
>> +		I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
> Like Ville said, adding a blank line between each read-modify-write
> group helps readability. Perhaps add /* DW5 */ etc. comments to group
> the, eh, groups.

Ok.

>
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
>> +		tmp |= SCALING_MODE_SEL(6);
>> +		I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
>> +		tmp |= TAP2_DISABLE | TAP3_DISABLE;
>> +		I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
>> +		tmp |= TAP2_DISABLE | TAP3_DISABLE;
>> +		I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
> Are you missing RTERM_SELECT?

Looks this was not earlier and added recently. Will program in next version.

>
> Why do you do two read-modify-writes (RMW) on both GRP and AUX, instead
> of doing all the changes at once?

Do you mean for  tmp |= TAP2_DISABLE | TAP3_DISABLE ??  If yes, because 
GRP and AUX
might contain different values and need to read them explicitly.

>
> The RMW doesn't actually clear the fields before changing them, just ORs
> more stuff on top of them, and cursor program or coeff polarity might
> contain garbage (at least in theory). The same below.

Yeah, we need to reset those bits using MASK and then do 'OR'.
Or are you suggesting something else??

>
>> +
>> +		/*
>> +		 * swing and scaling values are taken from DSI
>> +		 * table under vswing programming sequence for
>> +		 * combo phy ddi in BSPEC.
>> +		 * program swing values
>> +		 */
> Please reflow the comment.

Ok.

>
>> +		tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
>> +		tmp |= SWING_SEL_UPPER(0x2);
>> +		tmp |= SWING_SEL_LOWER(0x2);
> This would benefit from
>
> +#define   SWING_SEL_MASK		(SWING_SEL_UPPER_MASK | SWING_SEL_LOWER_MASK)
> +#define   SWING_SEL(x)			(SWING_SEL_UPPER(x) | SWING_SEL_LOWER(x))
>
> in i915_reg.h. But I can look the other way and fix it myself later...
>
>> +		tmp |= RCOMP_SCALAR(0x98);
>> +		I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
>> +		tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
>> +		tmp |= SWING_SEL_UPPER(0x2);
>> +		tmp |= SWING_SEL_LOWER(0x2);
>> +		tmp |= RCOMP_SCALAR(0x98);
>> +		I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
>> +
>> +		/* program scaling values */
>> +		tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
>> +		tmp |= POST_CURSOR_1(0x0);
>> +		tmp |= POST_CURSOR_2(0x0);
>> +		tmp |= CURSOR_COEFF(0x18);
> 0x3f?

Yes, now its changed to 0x3f.

>
> Again, you need to zero the fields before ORin the new values into them.

Agree.

>
>> +		I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
>> +
>> +		for (lane = 0; lane <= 3; lane++) {
>> +			/* Bspec: must not use GRP register for write */
> I'll take your word for it, although I've missed such a requirement.

:-)

>
>> +			tmp = I915_READ(ICL_PORT_TX_DW4_LN(port, lane));
>> +			tmp |= POST_CURSOR_1(0x0);
>> +			tmp |= POST_CURSOR_2(0x0);
>> +			tmp |= CURSOR_COEFF(0x18);
> 0x3f?

Yes.

>
>> +			I915_WRITE(ICL_PORT_TX_DW4_LN(port, lane), tmp);
>> +		}
>> +	}
>> +}
>> +
>>   static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> @@ -140,6 +199,58 @@ static void gen11_dsi_config_phy_lanes_sequence(struct intel_encoder *encoder)
>>   	}
>>   }
>>   
>> +static void gen11_dsi_voltage_swing_program_seq(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);
>> +	u32 tmp;
>> +	enum port port;
>> +
> The step numbering below has changed in bspec. Please update. Maybe drop
> the numbering, and use just the headings.

Ok.

Regards,
Madhav

>
> Otherwise, the bits here look ok.
>
> BR,
> Jani.
>
>> +	/* Step C.1:clear common keeper enable bit */
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		tmp = I915_READ(ICL_PORT_PCS_DW1_LN0(port));
>> +		tmp &= ~COMMON_KEEPER_EN;
>> +		I915_WRITE(ICL_PORT_PCS_DW1_GRP(port), tmp);
>> +		tmp = I915_READ(ICL_PORT_PCS_DW1_AUX(port));
>> +		tmp &= ~COMMON_KEEPER_EN;
>> +		I915_WRITE(ICL_PORT_PCS_DW1_AUX(port), tmp);
>> +	}
>> +
>> +	/*
>> +	 * Step C.3: Set SUS Clock Config bitfield to 11b
>> +	 * Note: Step C.2 (loadgen select program) is done
>> +	 * as part of lane phy sequence configuration
>> +	 */
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		tmp = I915_READ(ICL_PORT_CL_DW5(port));
>> +		tmp |= SUS_CLOCK_CONFIG;
>> +		I915_WRITE(ICL_PORT_CL_DW5(port), tmp);
>> +	}
>> +
>> +	/* Step C.4: Clear training enable to change swing values */
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
>> +		tmp &= ~TX_TRAINING_EN;
>> +		I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
>> +		tmp &= ~TX_TRAINING_EN;
>> +		I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
>> +	}
>> +
>> +	/* Step C.5: Program swing and de-emphasis */
>> +	dsi_program_swing_and_deemphasis(encoder);
>> +
>> +	/* Step: C.6: Set training enable to trigger update */
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
>> +		tmp |= TX_TRAINING_EN;
>> +		I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
>> +		tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
>> +		tmp |= TX_TRAINING_EN;
>> +		I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
>> +	}
>> +}
>> +
>>   static void gen11_dsi_enable_port_and_phy(struct intel_encoder *encoder)
>>   {
>>   	/* step 4a: power up all lanes of the DDI used by DSI */
>> @@ -147,6 +258,9 @@ static void gen11_dsi_enable_port_and_phy(struct intel_encoder *encoder)
>>   
>>   	/* step 4b: configure lane sequencing of the Combo-PHY transmitters */
>>   	gen11_dsi_config_phy_lanes_sequence(encoder);
>> +
>> +	/* step 4c: configure voltage swing and skew */
>> +	gen11_dsi_voltage_swing_program_seq(encoder);
>>   }
>>   
>>   static void __attribute__((unused))