| Message ID | 20171122185514.27167-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Arthur J Runyan <arthur.j.runyan@intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@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. > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Nov 23, 2017 at 7:21 AM, Ville Syrjälä <ville.syrjala@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@intel.com> >> Cc: Arthur J Runyan <arthur.j.runyan@intel.com> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 27, 2017 at 03:14:21PM -0800, Lucas De Marchi wrote: > On Thu, Nov 23, 2017 at 7:21 AM, Ville Syrjälä > <ville.syrjala@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@intel.com> > >> Cc: Arthur J Runyan <arthur.j.runyan@intel.com> > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@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 I guess we should go for this one then. Also we should add the :platform tags to these as well. > - "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 These are generally the ones which have an entry in the w/a db. I think we should keep using them as well, when they exist. > > Should I send a fix to these as well? Please do.
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) + /* 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 */ + 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);
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@intel.com> Cc: Arthur J Runyan <arthur.j.runyan@intel.com> Signed-off-by: Lucas De Marchi <lucas.demarchi@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(+)