drm/i915/vlv: assert and de-assert sideband reset on resume

Commit Message

Jesse Barnes April 11, 2014, 5 p.m. UTC

This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
that it resets the whole common lane section of the PHY.  This is
required on machines where the BIOS doesn't do this for us on resume to
properly re-calibrate and get the PHY ready to transmit data.

Without this patch, such machines won't resume correctly much of the time,
with the symptom being a 'port ready' timeout and/or a link training
failure.

I'm open to better suggestions on how to do the power well toggle, with
the existing code it looks like I'd have to walk through a bunch of
power domains looking for a match, then call a generic function which
will warn.  I'd prefer to just expose the specific domains directly for
low level platform code like this.

Signed-off-by: Jesse Barnes <[e-mail]>
---
 drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
 drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

Comments

Daniel Vetter April 11, 2014, 5:16 p.m. UTC | #1

On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> that it resets the whole common lane section of the PHY.  This is
> required on machines where the BIOS doesn't do this for us on resume to
> properly re-calibrate and get the PHY ready to transmit data.
> 
> Without this patch, such machines won't resume correctly much of the time,
> with the symptom being a 'port ready' timeout and/or a link training
> failure.
> 
> I'm open to better suggestions on how to do the power well toggle, with
> the existing code it looks like I'd have to walk through a bunch of
> power domains looking for a match, then call a generic function which
> will warn.  I'd prefer to just expose the specific domains directly for
> low level platform code like this.
> 
> Signed-off-by: Jesse Barnes <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
>  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fa00185..3afd0bc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
>  	return true;
>  }
>  
> -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> -			       struct i915_power_well *power_well, bool enable)
> +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;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2a72bab..f1abd2d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> +			struct i915_power_well *power_well, bool enable);
> +
>  void intel_uncore_early_sanitize(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
>  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
>  	}
>  
> +	/*
> +	 * 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.
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +		struct i915_power_well cmn_well = {
> +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> +		};
> +
> +		vlv_set_power_well(dev_priv, &cmn_well, false);
> +		vlv_set_power_well(dev_priv, &cmn_well, true);
> +	}

Relationship with intel_reset_dpio? Should we move this bit of code over
there? I'm lost in this maze of kick-me-harder patches for byt dpio ...
-Daniel

> +
>  	/* clear out old GT FIFO errors */
>  	if (IS_GEN6(dev) || IS_GEN7(dev))
>  		__raw_i915_write32(dev_priv, GTFIFODBG,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak April 11, 2014, 5:23 p.m. UTC | #2

On Fri, 2014-04-11 at 10:00 -0700, Jesse Barnes wrote:
> This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> that it resets the whole common lane section of the PHY.  This is
> required on machines where the BIOS doesn't do this for us on resume to
> properly re-calibrate and get the PHY ready to transmit data.
> 
> Without this patch, such machines won't resume correctly much of the time,
> with the symptom being a 'port ready' timeout and/or a link training
> failure.
> 
> I'm open to better suggestions on how to do the power well toggle, with
> the existing code it looks like I'd have to walk through a bunch of
> power domains looking for a match, then call a generic function which
> will warn.  I'd prefer to just expose the specific domains directly for
> low level platform code like this.

The power_well->sync_hw() handler looks like a good place for such
things. It will get called from intel_power_domains_init_hw(), which is
later than then the uncore sanitize functions, but then again if it's
really needed that early then intel_power_domains_init_hw() should be
moved earlier too..

--Imre

> 
> Signed-off-by: Jesse Barnes <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
>  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fa00185..3afd0bc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
>  	return true;
>  }
>  
> -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> -			       struct i915_power_well *power_well, bool enable)
> +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;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2a72bab..f1abd2d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> +			struct i915_power_well *power_well, bool enable);
> +
>  void intel_uncore_early_sanitize(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
>  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
>  	}
>  
> +	/*
> +	 * 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.
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +		struct i915_power_well cmn_well = {
> +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> +		};
> +
> +		vlv_set_power_well(dev_priv, &cmn_well, false);
> +		vlv_set_power_well(dev_priv, &cmn_well, true);
> +	}
> +
>  	/* clear out old GT FIFO errors */
>  	if (IS_GEN6(dev) || IS_GEN7(dev))
>  		__raw_i915_write32(dev_priv, GTFIFODBG,

Ville Syrjälä April 11, 2014, 5:26 p.m. UTC | #3

On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> that it resets the whole common lane section of the PHY.  This is
> required on machines where the BIOS doesn't do this for us on resume to
> properly re-calibrate and get the PHY ready to transmit data.
> 
> Without this patch, such machines won't resume correctly much of the time,
> with the symptom being a 'port ready' timeout and/or a link training
> failure.
> 
> I'm open to better suggestions on how to do the power well toggle, with
> the existing code it looks like I'd have to walk through a bunch of
> power domains looking for a match, then call a generic function which
> will warn.  I'd prefer to just expose the specific domains directly for
> low level platform code like this.
> 
> Signed-off-by: Jesse Barnes <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
>  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fa00185..3afd0bc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
>  	return true;
>  }
>  
> -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> -			       struct i915_power_well *power_well, bool enable)
> +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;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2a72bab..f1abd2d 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> +			struct i915_power_well *power_well, bool enable);
> +
>  void intel_uncore_early_sanitize(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
>  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
>  	}
>  
> +	/*
> +	 * 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.
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +		struct i915_power_well cmn_well = {
> +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> +		};
> +
> +		vlv_set_power_well(dev_priv, &cmn_well, false);
> +		vlv_set_power_well(dev_priv, &cmn_well, true);
> +	}

Stick this into intel_reset_dpio() instead?

And what about fastboot and whatnot? Should we check if the display is
already up and running somehow before we go and kill it with this?

> +
>  	/* clear out old GT FIFO errors */
>  	if (IS_GEN6(dev) || IS_GEN7(dev))
>  		__raw_i915_write32(dev_priv, GTFIFODBG,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jesse Barnes April 11, 2014, 5:34 p.m. UTC | #4

On Fri, 11 Apr 2014 19:16:32 +0200
Daniel Vetter <[e-mail]> wrote:

> On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > that it resets the whole common lane section of the PHY.  This is
> > required on machines where the BIOS doesn't do this for us on resume to
> > properly re-calibrate and get the PHY ready to transmit data.
> > 
> > Without this patch, such machines won't resume correctly much of the time,
> > with the symptom being a 'port ready' timeout and/or a link training
> > failure.
> > 
> > I'm open to better suggestions on how to do the power well toggle, with
> > the existing code it looks like I'd have to walk through a bunch of
> > power domains looking for a match, then call a generic function which
> > will warn.  I'd prefer to just expose the specific domains directly for
> > low level platform code like this.
> > 
> > Signed-off-by: Jesse Barnes <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index fa00185..3afd0bc 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> >  	return true;
> >  }
> >  
> > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > -			       struct i915_power_well *power_well, bool enable)
> > +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;
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 2a72bab..f1abd2d 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >  
> > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > +			struct i915_power_well *power_well, bool enable);
> > +
> >  void intel_uncore_early_sanitize(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
> >  	}
> >  
> > +	/*
> > +	 * 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.
> > +	 */
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		struct i915_power_well cmn_well = {
> > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > +		};
> > +
> > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > +	}
> 
> Relationship with intel_reset_dpio? Should we move this bit of code over
> there? I'm lost in this maze of kick-me-harder patches for byt dpio ...

That happens too late.  This will clobber register state, whereas the
DPIO reset just resets the interface between the phy and the display.

Jesse Barnes April 11, 2014, 5:35 p.m. UTC | #5

On Fri, 11 Apr 2014 20:26:24 +0300
Ville Syrjälä <[e-mail]> wrote:

> On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > that it resets the whole common lane section of the PHY.  This is
> > required on machines where the BIOS doesn't do this for us on resume to
> > properly re-calibrate and get the PHY ready to transmit data.
> > 
> > Without this patch, such machines won't resume correctly much of the time,
> > with the symptom being a 'port ready' timeout and/or a link training
> > failure.
> > 
> > I'm open to better suggestions on how to do the power well toggle, with
> > the existing code it looks like I'd have to walk through a bunch of
> > power domains looking for a match, then call a generic function which
> > will warn.  I'd prefer to just expose the specific domains directly for
> > low level platform code like this.
> > 
> > Signed-off-by: Jesse Barnes <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index fa00185..3afd0bc 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> >  	return true;
> >  }
> >  
> > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > -			       struct i915_power_well *power_well, bool enable)
> > +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;
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 2a72bab..f1abd2d 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >  
> > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > +			struct i915_power_well *power_well, bool enable);
> > +
> >  void intel_uncore_early_sanitize(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
> >  	}
> >  
> > +	/*
> > +	 * 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.
> > +	 */
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		struct i915_power_well cmn_well = {
> > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > +		};
> > +
> > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > +	}
> 
> Stick this into intel_reset_dpio() instead?
> 
> And what about fastboot and whatnot? Should we check if the display is
> already up and running somehow before we go and kill it with this?

reset_dpio is too late.

But yes, this generally doesn't need to happen on the boot path (well
maybe on some platforms) so we should do something conditional there or
we'll lose all the display state.

Ville Syrjälä April 11, 2014, 6:06 p.m. UTC | #6

On Fri, Apr 11, 2014 at 10:34:19AM -0700, Jesse Barnes wrote:
> On Fri, 11 Apr 2014 19:16:32 +0200
> Daniel Vetter <[e-mail]> wrote:
> 
> > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > > that it resets the whole common lane section of the PHY.  This is
> > > required on machines where the BIOS doesn't do this for us on resume to
> > > properly re-calibrate and get the PHY ready to transmit data.
> > > 
> > > Without this patch, such machines won't resume correctly much of the time,
> > > with the symptom being a 'port ready' timeout and/or a link training
> > > failure.
> > > 
> > > I'm open to better suggestions on how to do the power well toggle, with
> > > the existing code it looks like I'd have to walk through a bunch of
> > > power domains looking for a match, then call a generic function which
> > > will warn.  I'd prefer to just expose the specific domains directly for
> > > low level platform code like this.
> > > 
> > > Signed-off-by: Jesse Barnes <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index fa00185..3afd0bc 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > >  	return true;
> > >  }
> > >  
> > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > -			       struct i915_power_well *power_well, bool enable)
> > > +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;
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 2a72bab..f1abd2d 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > >  }
> > >  
> > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > +			struct i915_power_well *power_well, bool enable);
> > > +
> > >  void intel_uncore_early_sanitize(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
> > >  	}
> > >  
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > > +	if (IS_VALLEYVIEW(dev)) {
> > > +		struct i915_power_well cmn_well = {
> > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > > +		};
> > > +
> > > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > > +	}
> > 
> > Relationship with intel_reset_dpio? Should we move this bit of code over
> > there? I'm lost in this maze of kick-me-harder patches for byt dpio ...
> 
> That happens too late.  This will clobber register state, whereas the
> DPIO reset just resets the interface between the phy and the display.

As a clarification to the cmnreset thing, we never actually assert
that signal, we just deassert it. The idea being that it should be
asserted by default when things get powered on. But I wonder if we
should assert it before suspending anyway.

Oh and I think if we power gate the cmnlane we would need to
assert/deassert cmnreset around it. In some CHV doc I see a note
that side reset must be deasserted before cmnreset. The timing
diagrams in VLV docs seem to have that order as well. So unless
there's some internal logic which hold cmnreset asserted for the
required time, we should do it by hand.


Oh and there's another intersting looking note:

"NOTE1 : Common lane reset must not be de-asserted until REFCLK to PLL is
 enabled by i_pll*refclkbufen and the clock is running and stable"

I guess we managed to follow that by accident since we enable the
refclock for DPLLB for the hotplug workaround. But perhaps we should
enable the refclk for all PLLs just to be sure.

Ville Syrjälä April 11, 2014, 6:10 p.m. UTC | #7

On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote:
> On Fri, 11 Apr 2014 20:26:24 +0300
> Ville Syrjälä <[e-mail]> wrote:
> 
> > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > > that it resets the whole common lane section of the PHY.  This is
> > > required on machines where the BIOS doesn't do this for us on resume to
> > > properly re-calibrate and get the PHY ready to transmit data.
> > > 
> > > Without this patch, such machines won't resume correctly much of the time,
> > > with the symptom being a 'port ready' timeout and/or a link training
> > > failure.
> > > 
> > > I'm open to better suggestions on how to do the power well toggle, with
> > > the existing code it looks like I'd have to walk through a bunch of
> > > power domains looking for a match, then call a generic function which
> > > will warn.  I'd prefer to just expose the specific domains directly for
> > > low level platform code like this.
> > > 
> > > Signed-off-by: Jesse Barnes <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index fa00185..3afd0bc 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > >  	return true;
> > >  }
> > >  
> > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > -			       struct i915_power_well *power_well, bool enable)
> > > +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;
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 2a72bab..f1abd2d 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > >  }
> > >  
> > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > +			struct i915_power_well *power_well, bool enable);
> > > +
> > >  void intel_uncore_early_sanitize(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
> > >  	}
> > >  
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > > +	if (IS_VALLEYVIEW(dev)) {
> > > +		struct i915_power_well cmn_well = {
> > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > > +		};
> > > +
> > > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > > +	}
> > 
> > Stick this into intel_reset_dpio() instead?
> > 
> > And what about fastboot and whatnot? Should we check if the display is
> > already up and running somehow before we go and kill it with this?
> 
> reset_dpio is too late.

How come? We shouldn't touch the PHY before it. So either reset_dpio is
in the wrong place for some reason, or something else gets called too
soon.

Jesse Barnes April 11, 2014, 6:15 p.m. UTC | #8

On Fri, 11 Apr 2014 21:10:21 +0300
Ville Syrjälä <[e-mail]> wrote:

> On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote:
> > On Fri, 11 Apr 2014 20:26:24 +0300
> > Ville Syrjälä <[e-mail]> wrote:
> > 
> > > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > > > that it resets the whole common lane section of the PHY.  This is
> > > > required on machines where the BIOS doesn't do this for us on resume to
> > > > properly re-calibrate and get the PHY ready to transmit data.
> > > > 
> > > > Without this patch, such machines won't resume correctly much of the time,
> > > > with the symptom being a 'port ready' timeout and/or a link training
> > > > failure.
> > > > 
> > > > I'm open to better suggestions on how to do the power well toggle, with
> > > > the existing code it looks like I'd have to walk through a bunch of
> > > > power domains looking for a match, then call a generic function which
> > > > will warn.  I'd prefer to just expose the specific domains directly for
> > > > low level platform code like this.
> > > > 
> > > > Signed-off-by: Jesse Barnes <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> > > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> > > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index fa00185..3afd0bc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > > >  	return true;
> > > >  }
> > > >  
> > > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > -			       struct i915_power_well *power_well, bool enable)
> > > > +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;
> > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > > index 2a72bab..f1abd2d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> > > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > > >  }
> > > >  
> > > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > +			struct i915_power_well *power_well, bool enable);
> > > > +
> > > >  void intel_uncore_early_sanitize(struct drm_device *dev)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * 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.
> > > > +	 */
> > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > +		struct i915_power_well cmn_well = {
> > > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > > > +		};
> > > > +
> > > > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > > > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > > > +	}
> > > 
> > > Stick this into intel_reset_dpio() instead?
> > > 
> > > And what about fastboot and whatnot? Should we check if the display is
> > > already up and running somehow before we go and kill it with this?
> > 
> > reset_dpio is too late.
> 
> How come? We shouldn't touch the PHY before it. So either reset_dpio is
> in the wrong place for some reason, or something else gets called too
> soon.

Oh actually I haven't tested with just the common reset, it may be ok
to put that into the DPIO init function.  My earlier patch was toggling
all the wells, including display, which would obviously clobber things.

Jesse Barnes April 11, 2014, 6:18 p.m. UTC | #9

On Fri, 11 Apr 2014 21:06:31 +0300
Ville Syrjälä <[e-mail]> wrote:

> On Fri, Apr 11, 2014 at 10:34:19AM -0700, Jesse Barnes wrote:
> > On Fri, 11 Apr 2014 19:16:32 +0200
> > Daniel Vetter <[e-mail]> wrote:
> > 
> > > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > > > that it resets the whole common lane section of the PHY.  This is
> > > > required on machines where the BIOS doesn't do this for us on resume to
> > > > properly re-calibrate and get the PHY ready to transmit data.
> > > > 
> > > > Without this patch, such machines won't resume correctly much of the time,
> > > > with the symptom being a 'port ready' timeout and/or a link training
> > > > failure.
> > > > 
> > > > I'm open to better suggestions on how to do the power well toggle, with
> > > > the existing code it looks like I'd have to walk through a bunch of
> > > > power domains looking for a match, then call a generic function which
> > > > will warn.  I'd prefer to just expose the specific domains directly for
> > > > low level platform code like this.
> > > > 
> > > > Signed-off-by: Jesse Barnes <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> > > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> > > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index fa00185..3afd0bc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > > >  	return true;
> > > >  }
> > > >  
> > > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > -			       struct i915_power_well *power_well, bool enable)
> > > > +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;
> > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > > index 2a72bab..f1abd2d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> > > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > > >  }
> > > >  
> > > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > +			struct i915_power_well *power_well, bool enable);
> > > > +
> > > >  void intel_uncore_early_sanitize(struct drm_device *dev)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * 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.
> > > > +	 */
> > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > +		struct i915_power_well cmn_well = {
> > > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > > > +		};
> > > > +
> > > > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > > > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > > > +	}
> > > 
> > > Relationship with intel_reset_dpio? Should we move this bit of code over
> > > there? I'm lost in this maze of kick-me-harder patches for byt dpio ...
> > 
> > That happens too late.  This will clobber register state, whereas the
> > DPIO reset just resets the interface between the phy and the display.
> 
> As a clarification to the cmnreset thing, we never actually assert
> that signal, we just deassert it. The idea being that it should be
> asserted by default when things get powered on. But I wonder if we
> should assert it before suspending anyway.

And maybe do a write of 0 then 1 on resume too.  That's what Windows
does afaik.

> Oh and I think if we power gate the cmnlane we would need to
> assert/deassert cmnreset around it. In some CHV doc I see a note
> that side reset must be deasserted before cmnreset. The timing
> diagrams in VLV docs seem to have that order as well. So unless
> there's some internal logic which hold cmnreset asserted for the
> required time, we should do it by hand.

Yeah would be good to do that to be on the safe side.

> Oh and there's another intersting looking note:
> 
> "NOTE1 : Common lane reset must not be de-asserted until REFCLK to PLL is
>  enabled by i_pll*refclkbufen and the clock is running and stable"
> 
> I guess we managed to follow that by accident since we enable the
> refclock for DPLLB for the hotplug workaround. But perhaps we should
> enable the refclk for all PLLs just to be sure.

Yeah that shouldn't hurt.  In talking with the PHY guys, the cmnreset
de-assert will cause the PHY to re-calibrate, and any PLL settings
won't take effect until that's complete.  They will however be latched
& pended into the display, so it's safe to write them before, but
they'll take longer to lock the first time.

Vijay Purushothaman April 15, 2014, 11:39 a.m. UTC | #10

> -----Original Message-----

> From: Intel-gfx [mailto:[e-mail]] On Behalf Of

> Jesse Barnes

> Sent: Friday, April 11, 2014 11:46 PM

> To: Ville Syrjälä

> Cc: [e-mail]

> Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband

> reset on resume

> 

> On Fri, 11 Apr 2014 21:10:21 +0300

> Ville Syrjälä <[e-mail]> wrote:

> 

> > On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote:

> > > On Fri, 11 Apr 2014 20:26:24 +0300

> > > Ville Syrjälä <[e-mail]> wrote:

> > >

> > > > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:

> > > > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except

> > > > > that it resets the whole common lane section of the PHY.  This is

> > > > > required on machines where the BIOS doesn't do this for us on resume to

> > > > > properly re-calibrate and get the PHY ready to transmit data.

> > > > >

> > > > > Without this patch, such machines won't resume correctly much of the

> time,

> > > > > with the symptom being a 'port ready' timeout and/or a link training

> > > > > failure.

> > > > >

> > > > > I'm open to better suggestions on how to do the power well toggle, with

> > > > > the existing code it looks like I'd have to walk through a bunch of

> > > > > power domains looking for a match, then call a generic function which

> > > > > will warn.  I'd prefer to just expose the specific domains directly for

> > > > > low level platform code like this.

> > > > >

> > > > > Signed-off-by: Jesse Barnes <[e-mail]>

> > > > > ---

> > > > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--

> > > > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++

> > > > >  2 files changed, 21 insertions(+), 2 deletions(-)

> > > > >

> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c

> b/drivers/gpu/drm/i915/intel_pm.c

> > > > > index fa00185..3afd0bc 100644

> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c

> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c

> > > > > @@ -5454,8 +5454,8 @@ static bool

> i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,

> > > > >  	return true;

> > > > >  }

> > > > >

> > > > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,

> > > > > -			       struct i915_power_well *power_well, bool

> enable)

> > > > > +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;

> > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c

> b/drivers/gpu/drm/i915/intel_uncore.c

> > > > > index 2a72bab..f1abd2d 100644

> > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c

> > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c

> > > > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct

> drm_device *dev, bool restore)

> > > > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);

> > > > >  }

> > > > >

> > > > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,

> > > > > +			struct i915_power_well *power_well, bool

> enable);

> > > > > +

> > > > >  void intel_uncore_early_sanitize(struct drm_device *dev)

> > > > >  {

> > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;

> > > > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct

> drm_device *dev)

> > > > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv-

> >ellc_size);

> > > > >  	}

> > > > >

> > > > > +	/*

> > > > > +	 * 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.

> > > > > +	 */

> > > > > +	if (IS_VALLEYVIEW(dev)) {

> > > > > +		struct i915_power_well cmn_well = {

> > > > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC

> > > > > +		};

> > > > > +

> > > > > +		vlv_set_power_well(dev_priv, &cmn_well, false);

> > > > > +		vlv_set_power_well(dev_priv, &cmn_well, true);

> > > > > +	}

> > > >

> > > > Stick this into intel_reset_dpio() instead?

> > > >

> > > > And what about fastboot and whatnot? Should we check if the display is

> > > > already up and running somehow before we go and kill it with this?

> > >

> > > reset_dpio is too late.

> >

> > How come? We shouldn't touch the PHY before it. So either reset_dpio is

> > in the wrong place for some reason, or something else gets called too

> > soon.

> 

> Oh actually I haven't tested with just the common reset, it may be ok

> to put that into the DPIO init function.  My earlier patch was toggling

> all the wells, including display, which would obviously clobber things.

> 


Following is my understanding after talking to PHY & windows teams..

The exact sequence to follow during power gating (as part of the suspend sequence):
- Power gate display controller & poll for the operation to complete
- Power gate DPIO RX / TX lanes & poll for the operation to complete
- Power gate DPIO common lanes & poll for the operation to complete

The power ungating sequence 
- Power ungate DPIO TX lanes & poll for the operation to complete
- Power ungate DPIO common lanes & poll for the operation to complete
- Power ungate display controller & poll for the operation to complete

Thanks,
Vijay

Ville Syrjälä April 15, 2014, 1:01 p.m. UTC | #11

On Tue, Apr 15, 2014 at 11:39:41AM +0000, Purushothaman, Vijay A wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:[e-mail]] On Behalf Of
> > Jesse Barnes
> > Sent: Friday, April 11, 2014 11:46 PM
> > To: Ville Syrjälä
> > Cc: [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband
> > reset on resume
> > 
> > On Fri, 11 Apr 2014 21:10:21 +0300
> > Ville Syrjälä <[e-mail]> wrote:
> > 
> > > On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote:
> > > > On Fri, 11 Apr 2014 20:26:24 +0300
> > > > Ville Syrjälä <[e-mail]> wrote:
> > > >
> > > > > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > > > > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > > > > > that it resets the whole common lane section of the PHY.  This is
> > > > > > required on machines where the BIOS doesn't do this for us on resume to
> > > > > > properly re-calibrate and get the PHY ready to transmit data.
> > > > > >
> > > > > > Without this patch, such machines won't resume correctly much of the
> > time,
> > > > > > with the symptom being a 'port ready' timeout and/or a link training
> > > > > > failure.
> > > > > >
> > > > > > I'm open to better suggestions on how to do the power well toggle, with
> > > > > > the existing code it looks like I'd have to walk through a bunch of
> > > > > > power domains looking for a match, then call a generic function which
> > > > > > will warn.  I'd prefer to just expose the specific domains directly for
> > > > > > low level platform code like this.
> > > > > >
> > > > > > Signed-off-by: Jesse Barnes <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> > > > > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> > > > > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index fa00185..3afd0bc 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -5454,8 +5454,8 @@ static bool
> > i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > > > > >  	return true;
> > > > > >  }
> > > > > >
> > > > > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > > > -			       struct i915_power_well *power_well, bool
> > enable)
> > > > > > +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;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > index 2a72bab..f1abd2d 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct
> > drm_device *dev, bool restore)
> > > > > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > > > > >  }
> > > > > >
> > > > > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > > > +			struct i915_power_well *power_well, bool
> > enable);
> > > > > > +
> > > > > >  void intel_uncore_early_sanitize(struct drm_device *dev)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct
> > drm_device *dev)
> > > > > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv-
> > >ellc_size);
> > > > > >  	}
> > > > > >
> > > > > > +	/*
> > > > > > +	 * 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.
> > > > > > +	 */
> > > > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > > > +		struct i915_power_well cmn_well = {
> > > > > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > > > > > +		};
> > > > > > +
> > > > > > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > > > > > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > > > > > +	}
> > > > >
> > > > > Stick this into intel_reset_dpio() instead?
> > > > >
> > > > > And what about fastboot and whatnot? Should we check if the display is
> > > > > already up and running somehow before we go and kill it with this?
> > > >
> > > > reset_dpio is too late.
> > >
> > > How come? We shouldn't touch the PHY before it. So either reset_dpio is
> > > in the wrong place for some reason, or something else gets called too
> > > soon.
> > 
> > Oh actually I haven't tested with just the common reset, it may be ok
> > to put that into the DPIO init function.  My earlier patch was toggling
> > all the wells, including display, which would obviously clobber things.
> > 
> 
> Following is my understanding after talking to PHY & windows teams..
> 
> The exact sequence to follow during power gating (as part of the suspend sequence):
> - Power gate display controller & poll for the operation to complete
> - Power gate DPIO RX / TX lanes & poll for the operation to complete
> - Power gate DPIO common lanes & poll for the operation to complete
> 
> The power ungating sequence 
> - Power ungate DPIO TX lanes & poll for the operation to complete
> - Power ungate DPIO common lanes & poll for the operation to complete
> - Power ungate display controller & poll for the operation to complete

The suggested order of power gating the display controller before the
PHY seems rather strange to me. The display controller controls several
input signals (cmnreset, ref clk enable, etc.) into the PHY, so what
will happen to those inputs when the display controller is power gated?
The sequence diagrams in the PHY docs show that cmnreset should be
deasserted after the side reset. But if the display controller is still
power gated when side reset is removed during PHY power up, what happens?

Intuitively the opposite order of first power gating then PHY and then
the display controller would make more sense to me. That order would
also allow us to power gate just the PHY but the leave the display
controller powered on (eg. if only DSI displays are active).

And if we were to power gate just the PHY but not the display controller,
what should we do with cmnreset when we power gate the common lanes?
Do we need to assert cmnreset before we power down the common lanes, and
deassert it again after the common lanes have been powered on? Or is
this use case simply not supported?

Imre Deak April 15, 2014, 1:04 p.m. UTC | #12

On Tue, 2014-04-15 at 11:39 +0000, Purushothaman, Vijay A wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:[e-mail]] On Behalf Of
> > Jesse Barnes
> > Sent: Friday, April 11, 2014 11:46 PM
> > To: Ville Syrjälä
> > Cc: [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband
> > reset on resume
> > 
> > On Fri, 11 Apr 2014 21:10:21 +0300
> > Ville Syrjälä <[e-mail]> wrote:
> > 
> > > On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote:
> > > > On Fri, 11 Apr 2014 20:26:24 +0300
> > > > Ville Syrjälä <[e-mail]> wrote:
> > > >
> > > > > On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote:
> > > > > > This is a bit like the CMN reset de-assert we do in DPIO_CTL, except
> > > > > > that it resets the whole common lane section of the PHY.  This is
> > > > > > required on machines where the BIOS doesn't do this for us on resume to
> > > > > > properly re-calibrate and get the PHY ready to transmit data.
> > > > > >
> > > > > > Without this patch, such machines won't resume correctly much of the
> > time,
> > > > > > with the symptom being a 'port ready' timeout and/or a link training
> > > > > > failure.
> > > > > >
> > > > > > I'm open to better suggestions on how to do the power well toggle, with
> > > > > > the existing code it looks like I'd have to walk through a bunch of
> > > > > > power domains looking for a match, then call a generic function which
> > > > > > will warn.  I'd prefer to just expose the specific domains directly for
> > > > > > low level platform code like this.
> > > > > >
> > > > > > Signed-off-by: Jesse Barnes <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_pm.c     |    4 ++--
> > > > > >  drivers/gpu/drm/i915/intel_uncore.c |   19 +++++++++++++++++++
> > > > > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index fa00185..3afd0bc 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -5454,8 +5454,8 @@ static bool
> > i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > > > > >  	return true;
> > > > > >  }
> > > > > >
> > > > > > -static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > > > -			       struct i915_power_well *power_well, bool
> > enable)
> > > > > > +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;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > index 2a72bab..f1abd2d 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct
> > drm_device *dev, bool restore)
> > > > > >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > > > > >  }
> > > > > >
> > > > > > +void vlv_set_power_well(struct drm_i915_private *dev_priv,
> > > > > > +			struct i915_power_well *power_well, bool
> > enable);
> > > > > > +
> > > > > >  void intel_uncore_early_sanitize(struct drm_device *dev)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct
> > drm_device *dev)
> > > > > >  		DRM_INFO("Found %zuMB of eLLC\n", dev_priv-
> > >ellc_size);
> > > > > >  	}
> > > > > >
> > > > > > +	/*
> > > > > > +	 * 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.
> > > > > > +	 */
> > > > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > > > +		struct i915_power_well cmn_well = {
> > > > > > +			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
> > > > > > +		};
> > > > > > +
> > > > > > +		vlv_set_power_well(dev_priv, &cmn_well, false);
> > > > > > +		vlv_set_power_well(dev_priv, &cmn_well, true);
> > > > > > +	}
> > > > >
> > > > > Stick this into intel_reset_dpio() instead?
> > > > >
> > > > > And what about fastboot and whatnot? Should we check if the display is
> > > > > already up and running somehow before we go and kill it with this?
> > > >
> > > > reset_dpio is too late.
> > >
> > > How come? We shouldn't touch the PHY before it. So either reset_dpio is
> > > in the wrong place for some reason, or something else gets called too
> > > soon.
> > 
> > Oh actually I haven't tested with just the common reset, it may be ok
> > to put that into the DPIO init function.  My earlier patch was toggling
> > all the wells, including display, which would obviously clobber things.
> > 
> 
> Following is my understanding after talking to PHY & windows teams..
> 
> The exact sequence to follow during power gating (as part of the suspend sequence):
> - Power gate display controller & poll for the operation to complete
> - Power gate DPIO RX / TX lanes & poll for the operation to complete
> - Power gate DPIO common lanes & poll for the operation to complete
> 
> The power ungating sequence 
> - Power ungate DPIO TX lanes & poll for the operation to complete
> - Power ungate DPIO common lanes & poll for the operation to complete
> - Power ungate display controller & poll for the operation to complete

Does this mean that we need the DPIO common and TX lanes up for all
display configurations? It doesn't seem logical. At the moment VGA works
without the TX lanes being down and I think MIPI should work with both
common and TX lanes down, (but I haven't tested MIPI).

--Imre

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fa00185..3afd0bc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5454,8 +5454,8 @@  static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
 	return true;
 }
 
-static void vlv_set_power_well(struct drm_i915_private *dev_priv,
-			       struct i915_power_well *power_well, bool enable)
+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;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a72bab..f1abd2d 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -363,6 +363,9 @@  static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
+void vlv_set_power_well(struct drm_i915_private *dev_priv,
+			struct i915_power_well *power_well, bool enable);
+
 void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -381,6 +384,22 @@  void intel_uncore_early_sanitize(struct drm_device *dev)
 		DRM_INFO("Found %zuMB of eLLC\n", dev_priv->ellc_size);
 	}
 
+	/*
+	 * 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.
+	 */
+	if (IS_VALLEYVIEW(dev)) {
+		struct i915_power_well cmn_well = {
+			.data = PUNIT_POWER_WELL_DPIO_CMN_BC
+		};
+
+		vlv_set_power_well(dev_priv, &cmn_well, false);
+		vlv_set_power_well(dev_priv, &cmn_well, true);
+	}
+
 	/* clear out old GT FIFO errors */
 	if (IS_GEN6(dev) || IS_GEN7(dev))
 		__raw_i915_write32(dev_priv, GTFIFODBG,