[PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v2

[PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v2

[Jesse Barnes]

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 boot or
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.

v2: extract simpler set_power_well function for use in reset_dpio (Imre)
    move to reset_dpio (Daniel & Ville)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   12 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
 drivers/gpu/drm/i915/intel_pm.c      |   13 ++++++++++---
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index df58afc..4a2b70e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1509,6 +1509,18 @@ static void intel_reset_dpio(struct drm_device *dev)
 
 	} else {
 		/*
+		 * 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);
+
+		/*
 		 * From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
 		 *  6.	De-assert cmn_reset/side_reset. Same as VLV X0.
 		 *   a.	GUnit 0x2110 bit[0] set to 1 (def 0)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0ef2777..feb6165 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -966,7 +966,8 @@ 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 b59e8ab..8f7dbb9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5724,10 +5724,9 @@ 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,
+			  enum punit_power_well power_well_id, bool enable)
 {
-	enum punit_power_well power_well_id = power_well->data;
 	u32 mask;
 	u32 state;
 	u32 ctrl;
@@ -5760,6 +5759,14 @@ 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)
 {
-- 
1.7.9.5

[PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Jesse Barnes]

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 boot or
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.

v2: extract simpler set_power_well function for use in reset_dpio (Imre)
    move to reset_dpio (Daniel & Ville)
v3: don't reset if DPIO reset is already de-asserted (Imre)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_pm.c      | 13 ++++++++++---
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index df58afc..bdb4624 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1509,6 +1509,25 @@ static void intel_reset_dpio(struct drm_device *dev)
 
 	} 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);
+
+		/*
 		 * From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
 		 *  6.	De-assert cmn_reset/side_reset. Same as VLV X0.
 		 *   a.	GUnit 0x2110 bit[0] set to 1 (def 0)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0ef2777..feb6165 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -966,7 +966,8 @@ 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 b59e8ab..8f7dbb9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5724,10 +5724,9 @@ 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,
+			  enum punit_power_well power_well_id, bool enable)
 {
-	enum punit_power_well power_well_id = power_well->data;
 	u32 mask;
 	u32 state;
 	u32 ctrl;
@@ -5760,6 +5759,14 @@ 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)
 {
-- 
1.8.4.2

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v2

[Jesse Barnes]

On Mon, 19 May 2014 16:16:37 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> 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 boot or
> 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.
> 
> v2: extract simpler set_power_well function for use in reset_dpio (Imre)
>     move to reset_dpio (Daniel & Ville)

Summarizing the IRC discussion because we're terrible about doing that.

Imre was concerned that this new unconditional reset would prevent
fastboot from working, and I think he's right.

I added a check for the DPIO reset de-assertion for that case, and it
seems to do the right thing on my BYT here (skipping the reset on boot
since the BIOS already did it, and doing it on resume since the BIOS
hadn't).

Patch is in the next mail.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Ville Syrjälä]

On Tue, May 20, 2014 at 11:09:03AM -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 boot or
> 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.
> 
> v2: extract simpler set_power_well function for use in reset_dpio (Imre)
>     move to reset_dpio (Daniel & Ville)
> v3: don't reset if DPIO reset is already de-asserted (Imre)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
>  drivers/gpu/drm/i915/intel_pm.c      | 13 ++++++++++---
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index df58afc..bdb4624 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1509,6 +1509,25 @@ static void intel_reset_dpio(struct drm_device *dev)
>  
>  	} 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);

Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
with this. We should definitely rip that out regardless.

Another thing I'm wodering is did the BIOS/hw really power on the
common lane, or did we do that outselves? If the latter, then I wonder
if we simply do that too early. Or more precisely do we need to make
sure the cri clock and/or refclock are enabled before we power on the
common lane?

And third is that do we need to enable the TX wells before the CMN
well? Currently we do the opposite which could also explain why this
CMN well toggle fixes things.

And finally we should make sure the cmnreset assert/deassert happens
corecctly wrt. to the power well transitions.


So I'd really like to see the following measures taken:

- kill power well hack from intel_uncore_sanitize()

- move the DPLL register setup from intel_reset_dpio() into
  vlv_set_power_well() like so:

 vlv_set_power_well()
 {
+	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && enable) {
+		I915_WRITE(DPLL, ...);
+		POSTING_READ(DPLL);
+		udelay(1); /* >10ns for cmnreset, >0ns for sidereset */
+	}

	<really enable the power well>
 }

- reorder vlv_power_wells[] to bring up TX wells before CMN.
  Although maybe the DPLL stuff should then be done already before
  the TX wells are brought up? But maybe not.

- move the cmnreset assert/deassert into the power well code like so:

 vlv_set_power_well()
 {
+	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && !enable)
+		assert cmnreset

	<do stuff>

+	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && enable) 
+		deassert cmnreset
 }


If those measures aren't enough, then I think this patch would be OK.
But right now I have this nagging feeling that we may have shot
ourselves in the foot and we're about to pile workarounds on top of our
own bug.

> +
> +		/*
>  		 * From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
>  		 *  6.	De-assert cmn_reset/side_reset. Same as VLV X0.
>  		 *   a.	GUnit 0x2110 bit[0] set to 1 (def 0)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0ef2777..feb6165 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -966,7 +966,8 @@ 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 b59e8ab..8f7dbb9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5724,10 +5724,9 @@ 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,
> +			  enum punit_power_well power_well_id, bool enable)
>  {
> -	enum punit_power_well power_well_id = power_well->data;
>  	u32 mask;
>  	u32 state;
>  	u32 ctrl;
> @@ -5760,6 +5759,14 @@ 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)
>  {
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Jesse Barnes]

On Wed, 21 May 2014 20:54:03 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Tue, May 20, 2014 at 11:09:03AM -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 boot or
> > 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.
> > 
> > v2: extract simpler set_power_well function for use in reset_dpio (Imre)
> >     move to reset_dpio (Daniel & Ville)
> > v3: don't reset if DPIO reset is already de-asserted (Imre)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> >  drivers/gpu/drm/i915/intel_pm.c      | 13 ++++++++++---
> >  3 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index df58afc..bdb4624 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1509,6 +1509,25 @@ static void intel_reset_dpio(struct drm_device *dev)
> >  
> >  	} 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);
> 
> Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> with this. We should definitely rip that out regardless.
> 
> Another thing I'm wodering is did the BIOS/hw really power on the
> common lane, or did we do that outselves? If the latter, then I wonder
> if we simply do that too early. Or more precisely do we need to make
> sure the cri clock and/or refclock are enabled before we power on the
> common lane?
> 
> And third is that do we need to enable the TX wells before the CMN
> well? Currently we do the opposite which could also explain why this
> CMN well toggle fixes things.
> 
> And finally we should make sure the cmnreset assert/deassert happens
> corecctly wrt. to the power well transitions.
> 
> 
> So I'd really like to see the following measures taken:
> 
> - kill power well hack from intel_uncore_sanitize()
> 
> - move the DPLL register setup from intel_reset_dpio() into
>   vlv_set_power_well() like so:
> 
>  vlv_set_power_well()
>  {
> +	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && enable) {
> +		I915_WRITE(DPLL, ...);
> +		POSTING_READ(DPLL);
> +		udelay(1); /* >10ns for cmnreset, >0ns for sidereset */
> +	}
> 
> 	<really enable the power well>
>  }
> 
> - reorder vlv_power_wells[] to bring up TX wells before CMN.
>   Although maybe the DPLL stuff should then be done already before
>   the TX wells are brought up? But maybe not.
> 
> - move the cmnreset assert/deassert into the power well code like so:
> 
>  vlv_set_power_well()
>  {
> +	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && !enable)
> +		assert cmnreset
> 
> 	<do stuff>
> 
> +	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && enable) 
> +		deassert cmnreset
>  }
> 
> 
> If those measures aren't enough, then I think this patch would be OK.
> But right now I have this nagging feeling that we may have shot
> ourselves in the foot and we're about to pile workarounds on top of our
> own bug.
> 

I worry about this too, but on the flip side we only have the
incomplete doc to look at, and these bugs can be really hard to find
and test fixes for (this reset one in particular bit only some
machines on only some boots for example).

So going with something untested like the above may make more sense,
but may also give us a false sense of security wrt this issue.

Can you ping the PHY guys with the above and see what they have to
say?  Maybe in the context of the updated programming doc stuff I sent
out?  Ideally we could get firm answers, update the docs, then update
the code.

In the meantime, upstream will be broken without a tested fix on these
machines.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Jesse Barnes]

And to answer more specifically...

On Wed, 21 May 2014 20:54:03 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> > +		__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);
> 
> Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> with this. We should definitely rip that out regardless.

Yeah we can rip that out.  That's just an ungate, and it assumes the
BIOS has already done the reset toggle for us.

> Another thing I'm wodering is did the BIOS/hw really power on the
> common lane, or did we do that outselves? If the latter, then I wonder
> if we simply do that too early. Or more precisely do we need to make
> sure the cri clock and/or refclock are enabled before we power on the
> common lane?

Depends on the platform.  It looks like the right thing happens at boot
time on most machines (i.e. the BIOS does a full toggle which causes a
reset), but on suspend/resume it's up to us.  And of course on machines
without video init at boot time, we need to do it ourselves as well.

I don't think the cri or refclk is required at this point, at least not
if the docs we have are correct (the PHY reset is supposed to be the
very first thing).

> And third is that do we need to enable the TX wells before the CMN
> well? Currently we do the opposite which could also explain why this
> CMN well toggle fixes things.

I don't think that matters, but we should ask the PHY guys.  The lack
of symmetry between the gate and ungate bothers me too.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Ville Syrjälä]

On Wed, May 21, 2014 at 11:11:15AM -0700, Jesse Barnes wrote:
> And to answer more specifically...
> 
> On Wed, 21 May 2014 20:54:03 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > > +		__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);
> > 
> > Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> > with this. We should definitely rip that out regardless.
> 
> Yeah we can rip that out.  That's just an ungate, and it assumes the
> BIOS has already done the reset toggle for us.

Well I'm assumign the system may boot/resume with the wells powered
down. And we definitely don't have the cri/ref clocks set up at this
point. So if they're needed to complete the resets the PHY may get stuck
or something. Also it just ungates everything at once, if the wells need
some ordering in bringup we have just messed things up here.

> 
> > Another thing I'm wodering is did the BIOS/hw really power on the
> > common lane, or did we do that outselves? If the latter, then I wonder
> > if we simply do that too early. Or more precisely do we need to make
> > sure the cri clock and/or refclock are enabled before we power on the
> > common lane?
> 
> Depends on the platform.  It looks like the right thing happens at boot
> time on most machines (i.e. the BIOS does a full toggle which causes a
> reset), but on suspend/resume it's up to us.  And of course on machines
> without video init at boot time, we need to do it ourselves as well.
> 
> I don't think the cri or refclk is required at this point, at least not
> if the docs we have are correct (the PHY reset is supposed to be the
> very first thing).

The timing diagrams do show some kind of relationship between the
cri/ref clocks and the reset signals getting deasserted.

> 
> > And third is that do we need to enable the TX wells before the CMN
> > well? Currently we do the opposite which could also explain why this
> > CMN well toggle fixes things.
> 
> I don't think that matters, but we should ask the PHY guys.  The lack
> of symmetry between the gate and ungate bothers me too.

My theory here is that the cmn and/or side reset needs to propagate to
the data lanes to propery initialize them. If the lanes are powered down
when the reset occurs things go bad.

And looks like the "Dynamic Power Gating" section in the phy cspec
pretty much says as much. That is you need to fully reset the entire
phy if you want to ungate even one of the blocks.

So based on that I think pretty much everything I said is needed to make
stuff work correctly. Also I think currently we power gate the TX blocks
whenever the port gets disabled. If we want to keep doing that, we need
to adjust valleyview_modeset_global_resources() to fully reset the phy
whenever previously power down wells need to be brought up. The
alternative would be to model the entire PHY as a single power well,
which would definitely be the simpler option.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Imre Deak]

On Wed, 2014-05-21 at 21:43 +0300, Ville Syrjälä wrote:
> On Wed, May 21, 2014 at 11:11:15AM -0700, Jesse Barnes wrote:
> > And to answer more specifically...
> > 
> > On Wed, 21 May 2014 20:54:03 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > 
> > > > +		__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);
> > > 
> > > Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> > > with this. We should definitely rip that out regardless.
> > 
> > Yeah we can rip that out.  That's just an ungate, and it assumes the
> > BIOS has already done the reset toggle for us.
> 
> Well I'm assumign the system may boot/resume with the wells powered
> down. And we definitely don't have the cri/ref clocks set up at this
> point. So if they're needed to complete the resets the PHY may get stuck
> or something. Also it just ungates everything at once, if the wells need
> some ordering in bringup we have just messed things up here.

I agree, there doesn't seem to be anything that needs those power wells
until the power domain init code runs. I can send a patch for this after
some testing.
 
> > > Another thing I'm wodering is did the BIOS/hw really power on the
> > > common lane, or did we do that outselves? If the latter, then I wonder
> > > if we simply do that too early. Or more precisely do we need to make
> > > sure the cri clock and/or refclock are enabled before we power on the
> > > common lane?
> > 
> > Depends on the platform.  It looks like the right thing happens at boot
> > time on most machines (i.e. the BIOS does a full toggle which causes a
> > reset), but on suspend/resume it's up to us.  And of course on machines
> > without video init at boot time, we need to do it ourselves as well.
> > 
> > I don't think the cri or refclk is required at this point, at least not
> > if the docs we have are correct (the PHY reset is supposed to be the
> > very first thing).
> 
> The timing diagrams do show some kind of relationship between the
> cri/ref clocks and the reset signals getting deasserted.

Under "4.3 Clock Signal List" there is a hint about this saying that the
cri and pll1 ref clocks are needed for the init time calibration of the
whole PHY. The calibration is started by CMN reset, which should occur
after all the needed lanes are powered on. But this matches what we do
already now, except that toggling of the CMN reset signal is not
guaranteed if it was already deasserted by the BIOS. This would be fixed
by Jesse's v1 patch (but not v2).

> > > And third is that do we need to enable the TX wells before the CMN
> > > well? Currently we do the opposite which could also explain why this
> > > CMN well toggle fixes things.
> > 
> > I don't think that matters, but we should ask the PHY guys.  The lack
> > of symmetry between the gate and ungate bothers me too.
> 
> My theory here is that the cmn and/or side reset needs to propagate to
> the data lanes to propery initialize them. If the lanes are powered down
> when the reset occurs things go bad.

I agree that the CMN reset should happen after all the lanes are powered
on, but this is what we do already now, except guaranteeing that it
toggles. It's not clear what triggers the side reset signal, I assume
Punit deasserts it after powering on the CMN lane. Also not clear if
this really needs to happen after powering on the TX lanes. 

> And looks like the "Dynamic Power Gating" section in the phy cspec
> pretty much says as much. That is you need to fully reset the entire
> phy if you want to ungate even one of the blocks.

What does full reset mean? In my opinion toggling the CMN reset line is
enough, but I guess you suggest that we also need to power off/on the
CMN lane first. It would be good if Jesse could try if toggling CMN
reset alone is enough to get rid of the problem he saw.

> So based on that I think pretty much everything I said is needed to make
> stuff work correctly.

I'd be also happy to get some clarification from the HW people about our
assumptions, but your changes look sensible anyway, since handling the
CMN reset and the cri/ref clocks do belong logically to the power well
code.

> Also I think currently we power gate the TX blocks
> whenever the port gets disabled. If we want to keep doing that, we need
> to adjust valleyview_modeset_global_resources() to fully reset the phy
> whenever previously power down wells need to be brought up. The
> alternative would be to model the entire PHY as a single power well,
> which would definitely be the simpler option.

Yes this is correct and it needs fixing. I took it for granted that we
can turn on/off parts of the PHY independently from the rest of it, but
now it looks like it's not possible on VLV. (As I understand it's
possible on CHV.) I planned to come up with some fix after figuring out
how things would work with fastboot, where based on this constraint we
also have to avoid disabling/enabling any PHY lanes during booting.
Currently we enable all the lanes at that time. I guess we could just
leave the PHY configuration as-is until we read out the HW configuration
and then leave the PHY config unchanged if there are active DP/HDMI/VGA
outputs and disable the whole PHY otherwise.

--Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Ville Syrjälä]

On Thu, May 22, 2014 at 09:51:22AM +0300, Imre Deak wrote:
> On Wed, 2014-05-21 at 21:43 +0300, Ville Syrjälä wrote:
> > On Wed, May 21, 2014 at 11:11:15AM -0700, Jesse Barnes wrote:
> > > And to answer more specifically...
> > > 
> > > On Wed, 21 May 2014 20:54:03 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > 
> > > > > +		__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);
> > > > 
> > > > Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> > > > with this. We should definitely rip that out regardless.
> > > 
> > > Yeah we can rip that out.  That's just an ungate, and it assumes the
> > > BIOS has already done the reset toggle for us.
> > 
> > Well I'm assumign the system may boot/resume with the wells powered
> > down. And we definitely don't have the cri/ref clocks set up at this
> > point. So if they're needed to complete the resets the PHY may get stuck
> > or something. Also it just ungates everything at once, if the wells need
> > some ordering in bringup we have just messed things up here.
> 
> I agree, there doesn't seem to be anything that needs those power wells
> until the power domain init code runs. I can send a patch for this after
> some testing.
>  
> > > > Another thing I'm wodering is did the BIOS/hw really power on the
> > > > common lane, or did we do that outselves? If the latter, then I wonder
> > > > if we simply do that too early. Or more precisely do we need to make
> > > > sure the cri clock and/or refclock are enabled before we power on the
> > > > common lane?
> > > 
> > > Depends on the platform.  It looks like the right thing happens at boot
> > > time on most machines (i.e. the BIOS does a full toggle which causes a
> > > reset), but on suspend/resume it's up to us.  And of course on machines
> > > without video init at boot time, we need to do it ourselves as well.
> > > 
> > > I don't think the cri or refclk is required at this point, at least not
> > > if the docs we have are correct (the PHY reset is supposed to be the
> > > very first thing).
> > 
> > The timing diagrams do show some kind of relationship between the
> > cri/ref clocks and the reset signals getting deasserted.
> 
> Under "4.3 Clock Signal List" there is a hint about this saying that the
> cri and pll1 ref clocks are needed for the init time calibration of the
> whole PHY. The calibration is started by CMN reset, which should occur
> after all the needed lanes are powered on. But this matches what we do
> already now, except that toggling of the CMN reset signal is not
> guaranteed if it was already deasserted by the BIOS. This would be fixed
> by Jesse's v1 patch (but not v2).

My main issue with Jesse's patch is that it may fix the resume time
problem. But things should still fail in the exact same way if we
power gate the TX lanes and then try to bring them back. So it should
fail by simply disabling and re-enabling one port while keeping the
common lane powered up eg. by having the other port enabled all the
time.

And we don't assert cmnreset when powergating the cmnlane, so that
too may mess up the cmn lane power up since cmnreset is now
deasserted before sidereset. So even if we power down the common
lane as well, things may go bad.

> 
> > > > And third is that do we need to enable the TX wells before the CMN
> > > > well? Currently we do the opposite which could also explain why this
> > > > CMN well toggle fixes things.
> > > 
> > > I don't think that matters, but we should ask the PHY guys.  The lack
> > > of symmetry between the gate and ungate bothers me too.
> > 
> > My theory here is that the cmn and/or side reset needs to propagate to
> > the data lanes to propery initialize them. If the lanes are powered down
> > when the reset occurs things go bad.
> 
> I agree that the CMN reset should happen after all the lanes are powered
> on, but this is what we do already now, except guaranteeing that it
> toggles. It's not clear what triggers the side reset signal, I assume
> Punit deasserts it after powering on the CMN lane. Also not clear if
> this really needs to happen after powering on the TX lanes. 
> 
> > And looks like the "Dynamic Power Gating" section in the phy cspec
> > pretty much says as much. That is you need to fully reset the entire
> > phy if you want to ungate even one of the blocks.
> 
> What does full reset mean? In my opinion toggling the CMN reset line is
> enough, but I guess you suggest that we also need to power off/on the
> CMN lane first. It would be good if Jesse could try if toggling CMN
> reset alone is enough to get rid of the problem he saw.

IIRC the docs said sidereset needs to be reasserted too. And AFAICS the
only way to do that is by toggling the power to the common lane.

> 
> > So based on that I think pretty much everything I said is needed to make
> > stuff work correctly.
> 
> I'd be also happy to get some clarification from the HW people about our
> assumptions, but your changes look sensible anyway, since handling the
> CMN reset and the cri/ref clocks do belong logically to the power well
> code.
> 
> > Also I think currently we power gate the TX blocks
> > whenever the port gets disabled. If we want to keep doing that, we need
> > to adjust valleyview_modeset_global_resources() to fully reset the phy
> > whenever previously power down wells need to be brought up. The
> > alternative would be to model the entire PHY as a single power well,
> > which would definitely be the simpler option.
> 
> Yes this is correct and it needs fixing. I took it for granted that we
> can turn on/off parts of the PHY independently from the rest of it, but
> now it looks like it's not possible on VLV. (As I understand it's
> possible on CHV.) I planned to come up with some fix after figuring out
> how things would work with fastboot, where based on this constraint we
> also have to avoid disabling/enabling any PHY lanes during booting.
> Currently we enable all the lanes at that time. I guess we could just
> leave the PHY configuration as-is until we read out the HW configuration
> and then leave the PHY config unchanged if there are active DP/HDMI/VGA
> outputs and disable the whole PHY otherwise.
> 
> --Imre

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Jesse Barnes]

On Thu, 22 May 2014 09:51:22 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Wed, 2014-05-21 at 21:43 +0300, Ville Syrjälä wrote:
> > On Wed, May 21, 2014 at 11:11:15AM -0700, Jesse Barnes wrote:
> > > And to answer more specifically...
> > > 
> > > On Wed, 21 May 2014 20:54:03 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > 
> > > > > +		__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);
> > > > 
> > > > Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> > > > with this. We should definitely rip that out regardless.
> > > 
> > > Yeah we can rip that out.  That's just an ungate, and it assumes the
> > > BIOS has already done the reset toggle for us.
> > 
> > Well I'm assumign the system may boot/resume with the wells powered
> > down. And we definitely don't have the cri/ref clocks set up at this
> > point. So if they're needed to complete the resets the PHY may get stuck
> > or something. Also it just ungates everything at once, if the wells need
> > some ordering in bringup we have just messed things up here.
> 
> I agree, there doesn't seem to be anything that needs those power wells
> until the power domain init code runs. I can send a patch for this after
> some testing.
>  
> > > > Another thing I'm wodering is did the BIOS/hw really power on the
> > > > common lane, or did we do that outselves? If the latter, then I wonder
> > > > if we simply do that too early. Or more precisely do we need to make
> > > > sure the cri clock and/or refclock are enabled before we power on the
> > > > common lane?
> > > 
> > > Depends on the platform.  It looks like the right thing happens at boot
> > > time on most machines (i.e. the BIOS does a full toggle which causes a
> > > reset), but on suspend/resume it's up to us.  And of course on machines
> > > without video init at boot time, we need to do it ourselves as well.
> > > 
> > > I don't think the cri or refclk is required at this point, at least not
> > > if the docs we have are correct (the PHY reset is supposed to be the
> > > very first thing).
> > 
> > The timing diagrams do show some kind of relationship between the
> > cri/ref clocks and the reset signals getting deasserted.
> 
> Under "4.3 Clock Signal List" there is a hint about this saying that the
> cri and pll1 ref clocks are needed for the init time calibration of the
> whole PHY. The calibration is started by CMN reset, which should occur
> after all the needed lanes are powered on. But this matches what we do
> already now, except that toggling of the CMN reset signal is not
> guaranteed if it was already deasserted by the BIOS. This would be fixed
> by Jesse's v1 patch (but not v2).

I guess the earlier sequence docs aren't clear on this, but it makes
sense.

> What does full reset mean? In my opinion toggling the CMN reset line is
> enough, but I guess you suggest that we also need to power off/on the
> CMN lane first. It would be good if Jesse could try if toggling CMN
> reset alone is enough to get rid of the problem he saw.

Well, that's the problem.  I don't have access to the failing machines
anymore, and we only ever had one that was reliably failing, so this is
a really tough one to verify.  The only one that was actually tested
was the one I sent out at the very beginning.

> > Also I think currently we power gate the TX blocks
> > whenever the port gets disabled. If we want to keep doing that, we need
> > to adjust valleyview_modeset_global_resources() to fully reset the phy
> > whenever previously power down wells need to be brought up. The
> > alternative would be to model the entire PHY as a single power well,
> > which would definitely be the simpler option.
> 
> Yes this is correct and it needs fixing. I took it for granted that we
> can turn on/off parts of the PHY independently from the rest of it, but
> now it looks like it's not possible on VLV. (As I understand it's
> possible on CHV.) I planned to come up with some fix after figuring out
> how things would work with fastboot, where based on this constraint we
> also have to avoid disabling/enabling any PHY lanes during booting.
> Currently we enable all the lanes at that time. I guess we could just
> leave the PHY configuration as-is until we read out the HW configuration
> and then leave the PHY config unchanged if there are active DP/HDMI/VGA
> outputs and disable the whole PHY otherwise.

Yeah if the display is already active we should assume we don't have to
perform a full reset.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Jesse Barnes]

On Wed, 21 May 2014 20:54:03 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> +	if (power_well_id == PUNIT_POWER_WELL_DPIO_CMN_BC && enable) 
> +		deassert cmnreset
>  }
> 
> 
> If those measures aren't enough, then I think this patch would be OK.
> But right now I have this nagging feeling that we may have shot
> ourselves in the foot and we're about to pile workarounds on top of our
> own bug.

I'll add these changes, with the additon that we need to do an actual
toggle at boot and init.  Simply asserting in suspend or gate and then
de-asserting isn't enough to re-calibrate, since we tried that on the
Chromebooks across suspend/resume and it wasn't enough to get things
going again after the chip had lost power.

Across simple power well transitions it ought to be enough though, and
that's easy enough to verify in testing.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx