[Intel-gfx] [PATCH] drm/i915/cnl: WA Display #1178 to fix some type C dongles

[Lucas De Marchi]

On Thu, Nov 23, 2017 at 7:21 AM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Wed, Nov 22, 2017 at 10:55:14AM -0800, Lucas De Marchi wrote:
>> WA Display #1178 is meant to fix Aux channel voltage swing too low with
>> some type C dongles. Although it is for type C, HW engineers reported
>> that it can be applied to all external ports even if they are not going
>> to type C.
>>
>> For CNL we apply the workaround every time Aux B, C and D are powering
>> up since they will lose the configuration when powered down.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Arthur J Runyan <arthur.j.runyan at intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>
>> Since this is a workaround I think it would be desirable not to be
>> so intrusive. The simplest thing to do is to add the
>> IS_CANNONLAKE() and workaround as done here.
>>
>> An alternative that may be more elegant (but also more intrusive) is to
>> declare a new ops for CNL for AUX B/C/D. Let me know what you think.
>>
>> For the type-C dongles that I have here it worked both with and without
>> this patch, so bear in mind I couldn't actually reproduce the problem.
>>
>>  drivers/gpu/drm/i915/i915_reg.h         | 11 +++++++++++
>>  drivers/gpu/drm/i915/intel_runtime_pm.c |  9 +++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 96c80fa0fcac..32064605f82d 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8332,6 +8332,17 @@ enum skl_power_gate {
>>  #define  SKL_PW_TO_PG(pw)                    ((pw) - SKL_DISP_PW_1 + SKL_PG1)
>>  #define  SKL_FUSE_PG_DIST_STATUS(pg)         (1 << (27 - (pg)))
>>
>> +#define _CNL_AUX_REG_IDX(pw)         ((pw - 1) >> 4)
>> +#define _CNL_AUX_ANAOVRD1_B          0x162250
>> +#define _CNL_AUX_ANAOVRD1_C          0x162210
>> +#define _CNL_AUX_ANAOVRD1_D          0x1622D0
>> +#define CNL_AUX_ANAOVRD1(pw)         _MMIO(_PICK(_CNL_AUX_REG_IDX(pw), \
>> +                                                 _CNL_AUX_ANAOVRD1_B, \
>> +                                                 _CNL_AUX_ANAOVRD1_C, \
>> +                                                 _CNL_AUX_ANAOVRD1_D))
>> +#define   CNL_AUX_ANAOVRD1_ENABLE    (1<<16)
>> +#define   CNL_AUX_ANAOVRD1_LDO_BYPASS        (1<<23)
>
> I can't actually find these registers in bspec. How do you come up with
> the names and stuff?
>
> Based on the offset they look like PHY registers to me, so probably
> should be placed somewhere around the existing PHY registers.
>
>> +
>>  /* Per-pipe DDI Function Control */
>>  #define _TRANS_DDI_FUNC_CTL_A                0x60400
>>  #define _TRANS_DDI_FUNC_CTL_B                0x61400
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 8315499452dc..9bf200e4885d 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -388,6 +388,15 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
>>       I915_WRITE(HSW_PWR_WELL_CTL_DRIVER(id), val | HSW_PWR_WELL_CTL_REQ(id));
>>       hsw_wait_for_power_well_enable(dev_priv, power_well);
>>
>> +     /* WA Display #1178 */
>
> Pls stick to a consistent w/a comment style.

Are you suggesting to change to "/* Display Wa #1178 */"? It seems the
most common style in the
codebase, although others are used.

- "Wa Display #number" is used in my other pending patch that was
based on first version by Rodrigo and
has 1 occurence
- "Display WA #number" has  13 occurrences
- "Display Wa #number" has 1 occurrence
- "WaName" has several occurrences and is by far the most common,
although I don't think all Wa have names
like these

Should I send a fix to these as well?

Thanks
Lucas De Marchi


>
>> +     if (IS_CANNONLAKE(dev_priv) &&
>> +         (id == CNL_DISP_PW_AUX_B || id == CNL_DISP_PW_AUX_C ||
>> +          id == CNL_DISP_PW_AUX_D)) {
>> +             val = I915_READ(CNL_AUX_ANAOVRD1(id));
>> +             val |= CNL_AUX_ANAOVRD1_ENABLE | CNL_AUX_ANAOVRD1_LDO_BYPASS;
>> +             I915_WRITE(CNL_AUX_ANAOVRD1(id), val);
>> +     }
>> +
>>       if (wait_fuses)
>>               gen9_wait_for_power_well_fuses(dev_priv, pg);
>>
>> --
>> 2.14.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Lucas De Marchi