On Thu, Dec 12, 2019 at 03:56:38PM -0800, Lucas De Marchi wrote:
> On Thu, Dec 12, 2019 at 03:51:21PM -0800, Matt Roper wrote:
> > Now that the combo PHY aux power well handlers are used exclusively on
> > Icelake, we can drop a bunch of the extra tests.
> >
> > v2: Don't try to use intel_uncore_rmw for register updates yet; there's
> > pending display uncore patches that need to land first. (Lucas)
> >
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > .../drm/i915/display/intel_display_power.c | 25 ++++++++-----------
> > 1 file changed, 10 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 52f2332e0ab8..a0669dc15540 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -418,7 +418,9 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> > int pw_idx = power_well->desc->hsw.idx;
> > enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> > u32 val;
> > - int wa_idx_max;
> > +
> > + WARN_ON(!IS_ICELAKE(dev_priv));
> > + WARN_ON(intel_phy_is_combo(dev_priv, phy));
>
> did you mean !intel_phy_is_combo()?
Woops, yes! I'll send a followup shortly.
>
> I'm not sure I actually like the warns, we could just live without them.
I feel like power wells are an area where we tend to do a lot of
copy/paste/modify when defining a new platform because the tables are so
large and a lot of the entries are similar or stay the same. I want to
make sure we don't cargo cult the ICL-specific function onto future
platforms by accident. It's easy to miss those mistakes in code review
because it's hooking up a combo phy-specific table which *sounds* like
the right thing to do on the surface if you don't realize the relevant
functions have ICL-specific handling that we don't want to propagate
forward.
Granted, people are more likely to copy TGL than ICL at this point, but
I figured the extra assertions wouldn't hurt.
>
> We should also remove _TGL_AUX_ANAOVRD1_C either in this commit or in
> the previous one.
Yeah, good point. I'll do that too.
Matt
>
> Lucas De Marchi
>
> >
> > val = I915_READ(regs->driver);
> > I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
> > @@ -430,19 +432,11 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> >
> > hsw_wait_for_power_well_enable(dev_priv, power_well);
> >
> > - /* Display WA #1178: icl, tgl */
> > - if (IS_TIGERLAKE(dev_priv))
> > - wa_idx_max = ICL_PW_CTL_IDX_AUX_C;
> > - else
> > - wa_idx_max = ICL_PW_CTL_IDX_AUX_B;
> > -
> > - if (!IS_ELKHARTLAKE(dev_priv) &&
> > - pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= wa_idx_max &&
> > - !intel_bios_is_port_edp(dev_priv, (enum port)phy)) {
> > + if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
> > + !intel_bios_is_port_edp(dev_priv, (enum port)phy))
> > val = I915_READ(ICL_AUX_ANAOVRD1(pw_idx));
> > val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
> > I915_WRITE(ICL_AUX_ANAOVRD1(pw_idx), val);
> > - }
> > }
> >
> > static void
> > @@ -454,10 +448,11 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> > enum phy phy = ICL_AUX_PW_TO_PHY(pw_idx);
> > u32 val;
> >
> > - if (INTEL_GEN(dev_priv) < 12) {
> > - val = I915_READ(ICL_PORT_CL_DW12(phy));
> > - I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
> > - }
> > + WARN_ON(!IS_ICELAKE(dev_priv));
> > + WARN_ON(intel_phy_is_combo(dev_priv, phy));
> > +
> > + val = I915_READ(ICL_PORT_CL_DW12(phy));
> > + I915_WRITE(ICL_PORT_CL_DW12(phy), val & ~ICL_LANE_ENABLE_AUX);
> >
> > val = I915_READ(regs->driver);
> > I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> > --
> > 2.23.0
> >
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
