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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 02, 2021 at 05:52:28AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 28 January 2021 04:46
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <tejaskumarx.surendrakumar.upadhyay@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom
> > <hariom.pandey@intel.com>; Roper, Matthew D
> > <matthew.d.roper@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@intel.com>
> > > Signed-off-by: Tejas Upadhyay
> > > <tejaskumarx.surendrakumar.upadhyay@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+?

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx