From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915/vlv: assert and de-assert sideband reset on resume Date: Fri, 11 Apr 2014 21:06:31 +0300 Message-ID: <20140411180631.GH18465@intel.com> References: <1397235616-25925-1-git-send-email-jbarnes@virtuousgeek.org> <20140411171632.GM9262@phenom.ffwll.local> <20140411103419.53192616@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id BF0B86E415 for ; Fri, 11 Apr 2014 11:06:38 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140411103419.53192616@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Apr 11, 2014 at 10:34:19AM -0700, Jesse Barnes wrote: > On Fri, 11 Apr 2014 19:16:32 +0200 > Daniel Vetter 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, wi= th > > > 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 f= or > > > low level platform code like this. > > > = > > > Signed-off-by: Jesse Barnes > > > --- > > > 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/i= ntel_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(s= truct 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 =3D power_well->data; > > > u32 mask; > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i9= 15/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 d= rm_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 =3D dev->dev_private; > > > @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_devi= ce *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 =3D { > > > + .data =3D 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=E4l=E4 Intel OTC