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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel