Re: [PATCH 10/11] drm/i915: Move VLV cmnlane workaround to intel_power_domains_init_hw()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 25, 2014 at 12:03:01PM -0700, Jesse Barnes wrote:
> On Fri, 13 Jun 2014 13:37:56 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Now that the CMNRESET deassert is part of the cmnlane power well,
> > intel_reset_dpio() is called too late to make any difference. We've
> > deasserted CMNRESET by that time, and so the off+on toggle w/a will
> > never kick in.
> > 
> > Move the workaround to intel_power_domains_init_hw() where it gets
> > called before we enable the init power domain.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 23 -------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
> >  drivers/gpu/drm/i915/intel_pm.c      | 67 ++++++++++++++++++++++++++++++------
> >  3 files changed, 58 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 33cc213..bcd32322 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1508,9 +1508,6 @@ static void intel_reset_dpio(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > -	if (!IS_VALLEYVIEW(dev))
> > -		return;
> > -
> >  	if (IS_CHERRYVIEW(dev)) {
> >  		enum dpio_phy phy;
> >  		u32 val;
> > @@ -1532,26 +1529,6 @@ static void intel_reset_dpio(struct drm_device *dev)
> >  			I915_WRITE(DISPLAY_PHY_CONTROL,
> >  				PHY_COM_LANE_RESET_DEASSERT(phy, val));
> >  		}
> > -
> > -	} else {
> > -		/*
> > -		 * If DPIO has already been reset, e.g. by BIOS, just skip all
> > -		 * this.
> > -		 */
> > -		if (I915_READ(DPIO_CTL) & DPIO_CMNRST)
> > -			return;
> > -
> > -		/*
> > -		 * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx:
> > -		 * Need to assert and de-assert PHY SB reset by gating the
> > -		 * common lane power, then un-gating it.
> > -		 * Simply ungating isn't enough to reset the PHY enough to get
> > -		 * ports and lanes running.
> > -		 */
> > -		__vlv_set_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC,
> > -				     false);
> > -		__vlv_set_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC,
> > -				     true);
> >  	}
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 5740be0..e565906 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -970,8 +970,7 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> >  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
> >  void ilk_wm_get_hw_state(struct drm_device *dev);
> > -void __vlv_set_power_well(struct drm_i915_private *dev_priv,
> > -			  enum punit_power_well power_well_id, bool enable);
> > +
> >  
> >  /* intel_sdvo.c */
> >  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index e9a8565..d8e20d3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5962,9 +5962,10 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> >  	return true;
> >  }
> >  
> > -void __vlv_set_power_well(struct drm_i915_private *dev_priv,
> > -			  enum punit_power_well power_well_id, bool enable)
> > +static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > +			       struct i915_power_well *power_well, bool enable)
> >  {
> > +	enum punit_power_well power_well_id = power_well->data;
> >  	u32 mask;
> >  	u32 state;
> >  	u32 ctrl;
> > @@ -5997,14 +5998,6 @@ out:
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> >  }
> >  
> > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > -			       struct i915_power_well *power_well, bool enable)
> > -{
> > -	enum punit_power_well power_well_id = power_well->data;
> > -
> > -	__vlv_set_power_well(dev_priv, power_well_id, enable);
> > -}
> > -
> >  static void vlv_power_well_sync_hw(struct drm_i915_private *dev_priv,
> >  				   struct i915_power_well *power_well)
> >  {
> > @@ -6435,6 +6428,21 @@ static struct i915_power_well vlv_power_wells[] = {
> >  	},
> >  };
> >  
> > +static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv,
> > +						 enum punit_power_well power_well_id)
> > +{
> > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +	struct i915_power_well *power_well;
> > +	int i;
> > +
> > +	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> > +		if (power_well->data == power_well_id)
> > +			return power_well;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  #define set_power_wells(power_domains, __power_wells) ({		\
> >  	(power_domains)->power_wells = (__power_wells);			\
> >  	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
> > @@ -6482,11 +6490,50 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
> >  	mutex_unlock(&power_domains->lock);
> >  }
> >  
> > +static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
> > +{
> > +	struct i915_power_well *cmn =
> > +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
> > +	struct i915_power_well *disp2d =
> > +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DISP2D);
> > +
> > +	/* nothing to do if common lane is already off */
> > +	if (!cmn->ops->is_enabled(dev_priv, cmn))
> > +		return;
> > +
> > +	/* If the display might be already active skip this */
> > +	if (disp2d->ops->is_enabled(dev_priv, disp2d) &&
> > +	    I915_READ(DPIO_CTL) & DPIO_CMNRST)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("toggling display PHY side reset\n");
> > +
> > +	/* cmnlane needs DPLL registers */
> > +	disp2d->ops->enable(dev_priv, disp2d);
> > +
> > +	/*
> > +	 * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx:
> > +	 * Need to assert and de-assert PHY SB reset by gating the
> > +	 * common lane power, then un-gating it.
> > +	 * Simply ungating isn't enough to reset the PHY enough to get
> > +	 * ports and lanes running.
> > +	 */
> > +	cmn->ops->disable(dev_priv, cmn);
> > +}
> > +
> >  void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
> >  {
> > +	struct drm_device *dev = dev_priv->dev;
> >  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >  
> >  	power_domains->initializing = true;
> > +
> > +	if (IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > +		mutex_lock(&power_domains->lock);
> > +		vlv_cmnlane_wa(dev_priv);
> > +		mutex_unlock(&power_domains->lock);
> > +	}
> > +
> >  	/* For now, we need the power well to be always enabled. */
> >  	intel_display_set_init_power(dev_priv, true);
> >  	intel_power_domains_resume(dev_priv);
> 
> I kind of preferred the low level function that just took a well ID
> directly since the idea of looking something we already know seems
> silly (and it should probably be a separate patch anyway).

Yeah the lookup part is a bit silly. But it avoids leaking the
implementation details of the relevant power well ops into vlv_cmnlane_wa().
The disp2d and cmnlane enable/disable hooks do other things besides just
frobbing the punit register, and we should always follow the proper
sequence.

> Also I'm not
> sure I would describe this as a W/A; the phy needs to get reset
> somehow...

I'd call it a hardware or firmware bug if it manages to power things up
in the wrong order and we have to step in to fix it. So calling it a w/a
seems reasonable to me. I can rename though if desired.

> 
> But with or without those changes, we need this.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC