On Wed, 12 Sep 2018, Madhav Chauhan <madhav.chauhan@xxxxxxxxx> wrote:
> On 9/12/2018 12:20 AM, Jani Nikula wrote:
>> On Tue, 10 Jul 2018, Madhav Chauhan <madhav.chauhan@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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.
No, I mean first you RMW scaling mode for GRP and AUX, then you do
TAP2/3 disable for GRP and AUX. Why not scaling mode *and* TAP2/3
disable in one go, for GRP and AUX separately of course.
>
>>
>> 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'.
Yes.
> Or are you suggesting something else??
No, that's just it.
BR,
Jani.
>
>>
>>> +
>>> + /*
>>> + * 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))
>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
