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