[Intel-gfx] [PATCH] drm/i915/gen9bc: Handle TGP PCH during suspend/resume

[Surendrakumar Upadhyay, TejaskumarX]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: 02 February 2021 12:42
> To: Surendrakumar Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Pandey, Hariom
> <hariom.pandey at intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/gen9bc: Handle TGP PCH during
> suspend/resume
> 
> On Tue, Feb 02, 2021 at 08:31:48AM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 02, 2021 at 05:52:28AM +0000, Surendrakumar Upadhyay,
> TejaskumarX wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Sent: 28 January 2021 04:46
> > > > To: Surendrakumar Upadhyay, TejaskumarX
> > > > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > > Cc: intel-gfx at lists.freedesktop.org; Pandey, Hariom
> > > > <hariom.pandey at intel.com>; Roper, Matthew D
> > > > <matthew.d.roper at intel.com>
> > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gen9bc: Handle TGP PCH
> > > > during suspend/resume
> > > >
> > > > On Wed, Jan 27, 2021 at 03:38:30PM +0530, Tejas Upadhyay wrote:
> > > > > For Legacy S3 suspend/resume GEN9 BC needs to enable and setup
> > > > > TGP PCH.
> > > > >
> > > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > > Signed-off-by: Tejas Upadhyay
> > > > > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_irq.c | 36
> > > > > ++++++++++++++++++++++++---------
> > > > >  1 file changed, 27 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > b/drivers/gpu/drm/i915/i915_irq.c index
> > > > > a31980f69120..6dcefc3e24ac
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -3026,8 +3026,20 @@ static void gen8_irq_reset(struct
> > > > drm_i915_private *dev_priv)
> > > > >  	GEN3_IRQ_RESET(uncore, GEN8_DE_MISC_);
> > > > >  	GEN3_IRQ_RESET(uncore, GEN8_PCU_);
> > > > >
> > > > > -	if (HAS_PCH_SPLIT(dev_priv))
> > > > > +	if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > > > > +		GEN3_IRQ_RESET(uncore, SDE);
> > > > > +	else if (HAS_PCH_SPLIT(dev_priv))
> > > > >  		ibx_irq_reset(dev_priv);
> > > > > +
> > > > > +	/* Wa_14010685332:cnp/cmp,tgp,adp */
> > > > > +	if (INTEL_PCH_TYPE(dev_priv) == PCH_CNP ||
> > > > > +	    (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP &&
> > > > > +	    INTEL_PCH_TYPE(dev_priv) < PCH_DG1)) {
> > > > > +		intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > > +				 SBCLK_RUN_REFCLK_DIS,
> > > > SBCLK_RUN_REFCLK_DIS);
> > > > > +		intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > > +				 SBCLK_RUN_REFCLK_DIS, 0);
> > > > > +	}
> > > >
> > > > Time to refactor instead of copypasta.
> > > Do you expect below? :
> > >
> > > If ((INTEL_PCH_TYPE(dev_priv) == PCH_TGP) {
> > > 	intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > 				 SBCLK_RUN_REFCLK_DIS,
> > > SBCLK_RUN_REFCLK_DIS);
> > > 		intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > 				 SBCLK_RUN_REFCLK_DIS, 0);
> > > }
> >
> > I expect a new function instead of copy pasting this whole thing into
> > multiple places.
> >
> > That said even the current code doesn't make any sense to me.
> > Take for instance this part:
> >         if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > 	                GEN3_IRQ_RESET(uncore, SDE); What is that PCH type
> > check doing there? What weird PCH type are we supposed to have that
> > doesn't need this?
> >
> > Also the Wa_14010685332 part looks a bit odd. Is it correct that icp
> > doesn't need that, but cnp and tgp both do somehow? Can we even have
> > cnp on icl+?
> 
> Hmm. Looking at it a bit more, that w/a seems to have something to do with
> suspend/resume, so seems rather misplaced in irq_reset(). Should probably
> just move the whole thing into a more appropriate place.
> 
GEN11+ needs these checks in irq_reset(). Please check irq_reset for GEN11. Now that customer like dell are expecting TGP PCH with gen9bc platforms, I have done similar PCH checking in irq_reset() for gen9bc. You mean these checks are at wrong place for GEN11 irq_reset() as well? Or you want one function doing these checks and calling it everywhere!
> --
> Ville Syrjälä
> Intel